exim: Use har-reader #5250

kingthorin · 2024-01-26T22:17:13Z

Overview

CHANGELOG > Added change note.
build file > Updated/added dependencies.
HarImporter > Re-worked to use har-reader lib.
HarUtils > Extensively added to to facilitate use of the new lib.
MenuImportHar > Re-worked and simplified to take advantage of the new utils, importer, and lib.
HarImporterUnitTest > Tweaked to accommodate HarImporter changes.

Related Issues

NA

Checklist

addOns/exim/src/main/java/org/zaproxy/addon/exim/har/HarImporter.java

kingthorin · 2024-01-27T00:57:18Z

addOns/exim/src/main/java/org/zaproxy/addon/exim/har/HarImporter.java

+        if ((name.equalsIgnoreCase(HttpFieldsNames.CACHE_CONTROL)
+                        || name.equalsIgnoreCase(HttpFieldsNames.SET_COOKIE))
+                && (value.contains("\n") || value.contains("\r"))) {
+            i.setValue(i.getValue().replaceAll("[\r\n]", ", "));


Should I just do this in all headers? I have no idea why the samples are like this, but these headers were where I encountered the issue 🤷‍♂️

kingthorin · 2024-01-27T00:59:07Z

addOns/exim/src/main/java/org/zaproxy/addon/exim/har/HarUtils.java

+ * @see <a href="http://www.softwareishard.com/blog/har-12-spec/">HTTP Archive 1.2</a>
+ * @see HttpMessage
+ */
+public final class HarUtils {


This is largely an updated version of what's in core. ~85% of the code is duplicated.

Is this one going to replace the core class to export?

In the long run I guess. It depends what we do with the "search" extension usage?

Once we move that out of core it can use "this" one too, I'm asking to know if more code is needed for this one. We should add a comment that custom fields are not yet migrated to this class (or add them now? I assume the new lib supports them).

I hadn't really handled any of the export related functionality. We might have to use jackson for that, the lib used here is very read centric.

Almost all code is for export functionality (starting in createZapHarLog() and following methods). It looks like it has setAdditionalField for custom fields. We would have to use Jackson (or rely on it anyway).

kingthorin · 2024-04-02T15:22:37Z

Removed the inner classes, still need to tweak the HTTP Version handling.

kingthorin · 2024-04-15T23:45:58Z

I've adjusted the version handling now. I believe this now has all the previously discussed changes.

addOns/commonlib/src/main/java/org/zaproxy/addon/commonlib/ui/ProgressPaneListener.java

addOns/exim/exim.gradle.kts

addOns/exim/src/main/java/org/zaproxy/addon/exim/har/HarImporter.java

thc202 · 2024-05-21T14:33:24Z

I'd do a final review later in the day.

addOns/exim/src/main/java/org/zaproxy/addon/exim/har/HarImporter.java

addOns/exim/src/main/java/org/zaproxy/addon/exim/har/MenuImportHar.java

thc202 · 2024-05-21T20:31:00Z

addOns/exim/src/main/java/org/zaproxy/addon/exim/har/HarImporter.java

-        List<HttpMessage> messages =
-                HarImporter.getHttpMessages(new HarFileReader().readHarFile(file));
-        int count = 1;
+    private HarLog preProcessHarLog(HarLog log) {


Would be great to have tests that cover these preprocessing checks.

I think I mentioned this in an email or the Friday notes at some point. I'd like to add all the HAR files we've collected along the way and use those for tests if there's no objection. None of them are terribly huge, and it's just text so it compresses well (for git operations etc).

We should still have specific tests for these, as it's easier to see what input tests what.

I'm fine with both.

Better to verify the messages are imported with the expected content (that's the most important).

That's fair, just gotta get the tests behaving well first.

kingthorin · 2024-05-22T00:54:41Z

I think I got all the comments, just need to add tests. Please lemme know if I've missed something.

addOns/exim/CHANGELOG.md

addOns/exim/src/main/java/org/zaproxy/addon/exim/har/HarImporter.java

kingthorin · 2024-05-23T01:07:12Z

I started to add tests. There were a number of challenges. I'll add some comments to cover those.

The tests will fail, they succeed individually but there seems to be some sort of concurrency or cleanup issue when run together.

addOns/exim/src/test/java/org/zaproxy/addon/exim/har/HarImporterUnitTest.java

thc202 · 2024-05-23T07:58:56Z

#5250 (comment)

The logger should be set up for each test not all of them, also better to call Configurator.reconfigure(getClass().getResource("/log4j2-test.properties").toURI()); to reset the configuration to previous state. (To be clear that does not fix the test failures, there are still some tests that need further changes, like setting up some more mocking and the messages.)

thc202 · 2024-05-23T10:39:26Z

I was not clear with #5250 (comment) I meant that the logger should be initialised in a BeforeEach rather than BeforeAll, sorry.

kingthorin · 2024-05-23T10:40:52Z

Okay I've updated a bunch of stuff. Things are a bit better. The log handling was ripped off from GraphQL tests. I had to put your reconfigure suggestion in each method because AfterAll is static and didn't like me using getClass (and I couldn't figure out an alternative to getClass).

Using mockMessages didn't seem to fix the console barf, the key is there so I don't get the issue....

Can you think of another place we've done mocking to handle persisting messages? I think I recall talking about this for another PR but I can't figure out what it was.

thc202 · 2024-05-23T10:51:33Z

The log reset should be done after each test.

Let me look.

SpiderTaskUnitTest

kingthorin · 2024-05-23T11:09:02Z

Getting closer... 🤷‍♂️

addOns/exim/src/main/java/org/zaproxy/addon/exim/har/HarImporter.java

addOns/exim/src/test/java/org/zaproxy/addon/exim/har/HarImporterUnitTest.java

thc202 · 2024-05-23T11:23:19Z

Change to extHistory = mock(ExtensionHistory.class);.

addOns/exim/src/test/java/org/zaproxy/addon/exim/har/HarImporterUnitTest.java

kingthorin · 2024-05-23T12:59:21Z

Latest changes bring me back to getResourcePath claiming to be null when using getHtml

Edit: Doing a gradle refresh to see if it helps/matters.

kingthorin · 2024-05-23T13:01:40Z

Wow it actually did help....and on a Linux VM 😕

kingthorin · 2024-05-23T13:05:27Z

Hrm missing something in cleanup I guess. UrlImporter test now failing 😢

thc202 · 2024-05-23T13:07:29Z

It's trying to send the requests :)

addOns/exim/src/test/java/org/zaproxy/addon/exim/har/HarImporterUnitTest.java

kingthorin · 2024-05-28T18:05:28Z

As discussed via slack, instead of dropping h3 requests they could probably be treated as http/2.

- CHANGELOG > Added change note. - build file > Updated/added dependencies. - HarImporter > Re-worked to use har-reader lib. - HarUtils > Extensively added to to facilitate use of the new lib. - MenuImportHar > Re-worked and simplified to take advantage of the new utils, importer, and lib. - HarImporterUnitTest > Tweaked to accommodate HarImporter changes. Signed-off-by: kingthorin <kingthorin@users.noreply.github.com>

kingthorin · 2024-06-03T01:23:20Z

I still have more testing to add but it's coming together

kingthorin commented Jan 27, 2024

View reviewed changes

kingthorin force-pushed the exim-har-reader branch 2 times, most recently from e6a80ec to 87db368 Compare January 29, 2024 13:32

kingthorin force-pushed the exim-har-reader branch from 87db368 to 729cc5c Compare April 2, 2024 15:19

kingthorin force-pushed the exim-har-reader branch from 729cc5c to 36aefe2 Compare April 15, 2024 23:43

thc202 reviewed May 21, 2024

View reviewed changes

kingthorin force-pushed the exim-har-reader branch from 36aefe2 to 6364ea0 Compare May 21, 2024 15:23

thc202 reviewed May 21, 2024

View reviewed changes

kingthorin force-pushed the exim-har-reader branch from 6364ea0 to ae150b5 Compare May 22, 2024 00:54

kingthorin commented May 22, 2024

View reviewed changes

addOns/exim/CHANGELOG.md Outdated Show resolved Hide resolved

addOns/exim/src/main/java/org/zaproxy/addon/exim/har/HarImporter.java Outdated Show resolved Hide resolved

kingthorin force-pushed the exim-har-reader branch 2 times, most recently from 092c58a to bd49ab6 Compare May 23, 2024 01:05

kingthorin commented May 23, 2024

View reviewed changes

addOns/exim/src/test/java/org/zaproxy/addon/exim/har/HarImporterUnitTest.java Outdated Show resolved Hide resolved

kingthorin commented May 23, 2024

View reviewed changes

addOns/exim/src/test/java/org/zaproxy/addon/exim/har/HarImporterUnitTest.java Outdated Show resolved Hide resolved

kingthorin force-pushed the exim-har-reader branch 2 times, most recently from 67a8efc to 98d875f Compare May 23, 2024 10:35

kingthorin force-pushed the exim-har-reader branch from 98d875f to 3a3f8fb Compare May 23, 2024 10:42

kingthorin force-pushed the exim-har-reader branch 2 times, most recently from 1961879 to 4279b38 Compare May 23, 2024 11:08

kingthorin force-pushed the exim-har-reader branch from 4279b38 to d3ef666 Compare May 23, 2024 11:11

thc202 reviewed May 23, 2024

View reviewed changes

addOns/exim/src/main/java/org/zaproxy/addon/exim/har/HarImporter.java Outdated Show resolved Hide resolved

addOns/exim/src/test/java/org/zaproxy/addon/exim/har/HarImporterUnitTest.java Outdated Show resolved Hide resolved

thc202 reviewed May 23, 2024

View reviewed changes

addOns/exim/src/test/java/org/zaproxy/addon/exim/har/HarImporterUnitTest.java Outdated Show resolved Hide resolved

kingthorin force-pushed the exim-har-reader branch from d3ef666 to 3bc7339 Compare May 23, 2024 12:59

thc202 reviewed May 23, 2024

View reviewed changes

addOns/exim/src/test/java/org/zaproxy/addon/exim/har/HarImporterUnitTest.java Outdated Show resolved Hide resolved

kingthorin force-pushed the exim-har-reader branch 8 times, most recently from 200e078 to 236471e Compare May 23, 2024 15:17

thc202 mentioned this pull request May 28, 2024

HAR import fails silently zaproxy/zaproxy#8500

Open

1 task

kingthorin force-pushed the exim-har-reader branch from 236471e to b0f5862 Compare May 28, 2024 13:30

kingthorin force-pushed the exim-har-reader branch 4 times, most recently from 48f7c92 to 6151e4e Compare May 30, 2024 13:29

kingthorin force-pushed the exim-har-reader branch from 6151e4e to 5a12576 Compare June 2, 2024 22:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

exim: Use har-reader #5250

exim: Use har-reader #5250

kingthorin commented Jan 26, 2024 •

edited

kingthorin Jan 27, 2024

kingthorin Jan 27, 2024

thc202 May 21, 2024

kingthorin May 21, 2024

thc202 May 21, 2024

kingthorin May 21, 2024

thc202 May 21, 2024

kingthorin commented Apr 2, 2024

kingthorin commented Apr 15, 2024

thc202 commented May 21, 2024

thc202 May 21, 2024

kingthorin May 21, 2024

thc202 May 21, 2024

kingthorin May 21, 2024

thc202 May 21, 2024

thc202 May 23, 2024

kingthorin May 23, 2024

kingthorin commented May 22, 2024

kingthorin commented May 23, 2024

thc202 commented May 23, 2024 •

edited

thc202 commented May 23, 2024

kingthorin commented May 23, 2024

thc202 commented May 23, 2024

kingthorin commented May 23, 2024

thc202 commented May 23, 2024

kingthorin commented May 23, 2024 •

edited

kingthorin commented May 23, 2024

kingthorin commented May 23, 2024

thc202 commented May 23, 2024

kingthorin commented May 28, 2024

kingthorin commented Jun 3, 2024

exim: Use har-reader #5250

Are you sure you want to change the base?

exim: Use har-reader #5250

Conversation

kingthorin commented Jan 26, 2024 • edited

Overview

Related Issues

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kingthorin commented Apr 2, 2024

kingthorin commented Apr 15, 2024

thc202 commented May 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kingthorin commented May 22, 2024

kingthorin commented May 23, 2024

thc202 commented May 23, 2024 • edited

thc202 commented May 23, 2024

kingthorin commented May 23, 2024

thc202 commented May 23, 2024

kingthorin commented May 23, 2024

thc202 commented May 23, 2024

kingthorin commented May 23, 2024 • edited

kingthorin commented May 23, 2024

kingthorin commented May 23, 2024

thc202 commented May 23, 2024

kingthorin commented May 28, 2024

kingthorin commented Jun 3, 2024

kingthorin commented Jan 26, 2024 •

edited

thc202 commented May 23, 2024 •

edited

kingthorin commented May 23, 2024 •

edited