pscanrulesBeta: fix isolation scan rule isPage200 check

[atezet]

Overview

Fix a bug in the site isolation scan rule.

Related Issues

zaproxy/zaproxy#8735

Checklist

• Update help
• Update changelog
• Run ./gradlew spotlessApply for code formatting
• Write tests
• Check code coverage
• Sign-off commits
• Squash commits
• Use a descriptive title

For more details, please refer to the developer rules and guidelines.

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[atezet]

It is my understanding that this check is intended to filter out any non-success pages. I also thought it would make sense that non-200 custom pages should be filtered out, but the HttpStatusCode.isSuccess(...) call prevents that from being possible. Removing that turned out to also not work, as the isPage200 only accepts a status code of EXACTLY 200 (so not, e.g., 204).

If this understanding is wrong, the failing test (shouldNotRaiseAlertGivenSiteIsNotIsolatedWhenFailureIdentifiedByCustomPage) can be removed. If this is right, the isPage200 check should probably be fixed s.t. the HttpStatusCode.isSuccess(...) call can be removed.

[atezet]

I have read the CLA Document and I hereby sign the CLA

[kingthorin]

As you noticed elsewhere this is unnecessary because a simple status code check is done first.

[atezet]

Removed this line

[atezet]

Made some more changes, adding back a different mock. This conversation is irrelevant now.

[kingthorin]

Same

[atezet]

Removed this whole test, as this was meant to trigger a path that never existed (see earlier comment)

[kingthorin]

Then set 404 instead of 200 🙂

[atezet]

I tried to replicate the Custom Pages example, but that didn't work in the old situation. I re-added the test, as this version using PassiveScanData.isSuccess actually works!

[kingthorin]

Removing that turned out to also not work, as the isPage200 only accepts a status code of EXACTLY 200 (so not, e.g., 204).

It's actually a lot more involved than that, but yes it ultimately falls back to purely 200 if the user hasn't set any custom pages and analyzer wasn't able to identify the 404 handling.

[thc202]

The correct is to call !isSuccess(msg).

[atezet]

So, I changed the flow from if !A return else do_something() to if A do_something(). This because A here was what is causing the bug; namely that if isPage200(...) the HttpMessage is skipped. So I believe this should either be:

if (!HttpStatusCode.isSuccess(msg.getResponseHeader().getStatusCode())
        && !getHelper().isPage200(msg)) {
        return;
}

rules.forEach(...);

or (what I wrote)

if (HttpStatusCode.isSuccess(msg.getResponseHeader().getStatusCode())
        || getHelper().isPage200(msg)) {
        rules.forEach(...);
}

That is; only scan pages that have status code 200-299 or are configured as non-standard error handling conditions as "page200" (see https://www.zaproxy.org/docs/desktop/start/features/custompages/). We could discuss whether this is what we want it to be, but at least we won't have so many false negatives as we have now.

[thc202]

Might be that the UI is not that clear but I highlighted the original code, which would be lines 91 to 92. I'm saying that the original code should have been (and should be) !isSuccess(msg). Note that isSuccess(HttpMessage) is other method than HttpStatusCode.isSuccess(HttpMessage), and isPage200 is already taken into account in the former.

[atezet]

Ah, thanks that makes sense. Shall I change my PR up s.t. it uses isSuccess(msg)? I think that this is exactly what I was aiming for all along

[atezet]

Changed it! Also fixed the tests by (re)adding mocks/stubs for isSuccess as it didn't seem to work otherwise

[psiinon]

Checkmarx One – Scan Summary & Details – 20b4a645-2aac-47c8-834b-f8fe8f947b65

No New Or Fixed Issues Found