Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ascanrulesAlpha: Improve rule performance by not calculating Levenshtein distance above the threshold #3329

Merged

Conversation

pf-msi
Copy link
Contributor

@pf-msi pf-msi commented Nov 19, 2021

Fixes partially zaproxy/zaproxy#6655.

Issue zaproxy/zaproxy#6655 describes three problems. This PR should fix first of them, regarding performance of WebCacheDeceptionScanRule. As suggested in the issue comments, there's no point in calculating exact Levenshtein distance after reaching specified threshold.

Signed-off-by: pf-msi piotr.furman@motorolasolutions.com

@thc202
Copy link
Member

thc202 commented Nov 19, 2021

Tweaked the pull request description to not actually fix the issue if this is merged.

@kingthorin
Copy link
Member

The CHANGELOG.md should be updated. The LGTM failure can be addressed by rebasing current.

Thanks for tackling this.

@@ -138,6 +146,20 @@ void shouldReturnExpectedMappings() {
is(equalTo(CommonAlertTag.OWASP_2017_A06_SEC_MISCONFIG.getValue())));
}

@Test
@Timeout(value=3, unit=TimeUnit.SECONDS)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I didn't know this was a thing.

Copy link
Member

@kingthorin kingthorin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, just needs a change note.

@thc202
Copy link
Member

thc202 commented Nov 19, 2021

The LGTM failure is caused by use of Java 11 APIs.

@thc202 thc202 changed the title Improve rule performance by not calculating Levenshtein distance above the threshold ascanrulesAlpha: Improve rule performance by not calculating Levenshtein distance above the threshold Nov 19, 2021
@kingthorin
Copy link
Member

Oh oops, I just noticed the branch was behind 😉

@pf-msi
Copy link
Contributor Author

pf-msi commented Nov 19, 2021

Ok, there was no targetCompatibility in gradle config, I must have missed that ZAP is still on JDK 8. I'll remove this String.repeat call.

@thc202
Copy link
Member

thc202 commented Nov 19, 2021

That's defined in

java {
// Compile with Java 8 when building ZAP releases.
if (System.getenv("ZAP_RELEASE") != null) {
toolchain {
languageVersion.set(JavaLanguageVersion.of(8))
}
} else {
sourceCompatibility = JavaVersion.VERSION_1_8
targetCompatibility = JavaVersion.VERSION_1_8
}
}

@pf-msi
Copy link
Contributor Author

pf-msi commented Nov 19, 2021

Thanks, there is indeed. Now I wonder why it has built successfully locally in my environment, and did not complain about it :)

@thc202
Copy link
Member

thc202 commented Nov 19, 2021

How did you build? Command line or IDE?

@pf-msi
Copy link
Contributor Author

pf-msi commented Nov 19, 2021

Fixed Java API and formatting issues.

@kingthorin
Copy link
Member

I can squash this or it can be squashed at merge if it’s good to go.

@thc202
Copy link
Member

thc202 commented Dec 27, 2021

IMO we should fix the issue rather than add workarounds (e.g. #2892). I might take a look at that the following days.

@kingthorin
Copy link
Member

Okay.

@thc202 thc202 removed their request for review February 6, 2022 10:25
…e the threshold

Signed-off-by: pf-msi <piotr.furman@motorolasolutions.com>
@thc202 thc202 force-pushed the issue-6655-webcachedeception-performance branch from 412c445 to 75e1597 Compare February 10, 2022 15:03
@thc202
Copy link
Member

thc202 commented Feb 10, 2022

Rebased to move the changelog entry to unreleased version.

@kingthorin
Copy link
Member

Thank you both!

@thc202
Copy link
Member

thc202 commented Feb 10, 2022

While longer term it would be better to replace the algorithm having this change is still better than keep hanging the scan rule.

@kingthorin
Copy link
Member

I think a number of these can probably use ComparableResponse once it's ready.

@kingthorin kingthorin merged commit 0838a47 into zaproxy:main Feb 10, 2022
@pf-msi pf-msi deleted the issue-6655-webcachedeception-performance branch February 10, 2022 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants