ascanrules: detect Escaped XSS encodeURIComponent #6686

devplayer55221 · 2025-08-24T15:04:54Z

This is to enhance the detection capability of the active scanner for XSS.
Escaped XSS - /js/encodeURIComponent

Added a payload alert(1) in directAttack function
Found that the function was not getting called as different context was being detected.
So, added the code to call directAttack function in the closeTag context function.

github-actions · 2025-08-24T15:05:05Z

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

devplayer55221 · 2025-08-24T15:06:25Z

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

kingthorin · 2025-08-24T15:26:02Z

Should have unit tests for the new situation/coverage, as well as a chaneglog note.

kingthorin · 2025-08-24T15:30:01Z

Run ./gradlew :addOns:ascanrules:spotlessApply to fix these violations.

psiinon · 2025-08-24T15:43:22Z

Checkmarx One – Scan Summary & Details – 4b2b86f2-8aea-48a0-aecf-6aa8f4ad19b8

Great job! No new security vulnerabilities introduced in this pull request

Communicate with Checkmarx by submitting a PR comment with @Checkmarx followed by one of the supported commands. Learn about the supported commands here.

thc202 · 2025-08-24T16:17:42Z

Please, make sure to link to related issues: zaproxy/zaproxy#7122 and zaproxy/zaproxy#7148.

thc202 · 2025-08-24T16:22:49Z

Added a payload alert(1) in directAttack function

The payload is being added everywhere, see usages of GENERIC_SCRIPT_ALERT_LIST.

devplayer55221 · 2025-08-24T17:37:43Z

Run ./gradlew :addOns:ascanrules:spotlessApply to fix these violations.

I run this command. There were no errors. Build was successful.

devplayer55221 · 2025-08-24T17:38:30Z

Should have unit tests for the new situation/coverage, as well as a chaneglog note.

* https://github.com/zaproxy/zap-extensions/blob/main/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/CrossSiteScriptingScanRuleUnitTest.java

* https://github.com/zaproxy/zap-extensions/blob/main/addOns/ascanrules/CHANGELOG.md

I need to add code for the new unit test? I am not familiar with this.

devplayer55221 · 2025-08-24T17:39:27Z

Added a payload alert(1) in directAttack function

The payload is being added everywhere, see usages of GENERIC_SCRIPT_ALERT_LIST.

Where can I see the usage of GENERIC_SCRIPT_ALERT_LIST. Also can I create a new context for this type of case?

kingthorin · 2025-08-24T19:12:46Z

I run this command. There were no errors. Build was successful.

This applies the code formatting requirements. You can commit and push again.

I need to add code for the new unit test? I am not familiar with this

Yup there are lots of examples there already.

Where can I see the usage of GENERIC_SCRIPT_ALERT_LIST. Also can I create a new context for this type of case?

In the rule you're modifying.
Do you mean you also want to change the HTML context analyzer?

devplayer55221 · 2025-08-24T19:21:31Z

I run this command. There were no errors. Build was successful.

This applies the code formatting requirements. You can commit and push again.

I need to add code for the new unit test? I am not familiar with this

Yup there are lots of examples there already.

Where can I see the usage of GENERIC_SCRIPT_ALERT_LIST. Also can I create a new context for this type of case?

In the rule you're modifying. Do you mean you also want to change the HTML context analyzer?

Okay, I will try to write a unit test.
I have changed the code where I added the payload to GENERIC_SCRIPT_ALERT_LIST. Now I am using the payload directly in a new context inside the directAttack method.
Also added the modification in Changelog file.

kingthorin · 2025-08-24T19:54:33Z

I likely won't get a chance to check again until the morning, the change note should be in the Unreleased section (the version and date are set at release).

devplayer55221 · 2025-08-25T13:36:04Z

I have updated the changelog.
When I run ./gradlew test, its gives me error related to URL being deprecated. How to fix and test? Please guide.

thc202 · 2025-08-25T13:46:52Z

Use Java 17 as mentioned in https://www.zaproxy.org/docs/developer/quick-start-build/#preparation

devplayer55221 · 2025-08-25T14:02:14Z

Use Java 17 as mentioned in https://www.zaproxy.org/docs/developer/quick-start-build/#preparation

Java 17.0.12 should work right?
There is error regarding :addOns:selenium:generateZapAddOnManifest

thc202 · 2025-08-25T14:11:12Z

Yes, what command are you using?

devplayer55221 · 2025-08-25T14:17:50Z

Yes, what command are you using?

./gradlew test

This is the error:

thc202 · 2025-08-25T14:23:46Z

I'd suggest testing just the add-on you are changing, ./gradlew :aO:ascanrules:test (you can also just run the test for this specific scan rule).
That error is most likely related to OOME, you can increase Gradle's heap memory to address it.

devplayer55221 · 2025-08-25T14:31:15Z

I'd suggest testing just the add-on you are changing, ./gradlew :aO:ascanrules:test (you can also just run the test for this specific scan rule). That error is most likely related to OOME, you can increase Gradle's heap memory to address it.

Thanks! testing the specific rule worked. Although, the unit tests seem to fail even before I added my unit test. I checked with the original scan rule file, and there were no failed cases. So, it seems my update caused the unit cases to fail. I need to change the code to get all the test cases passed right?

thc202 · 2025-08-25T14:46:25Z

Depends on the tests, you need to check the assertions and see if the tests need to be updated (e.g. other attack being used) or the new code changes corrected (e.g. introduces FPs).

kingthorin · 2025-08-25T14:50:53Z

This is how you'd test just the one class from the CLI:

./gradlew :aO:ascanrules:test --tests org.zaproxy.zap.extension.ascanrules.CrossSiteScriptingScanRuleUnitTest

I can confirm on main it passes while on your branch (devplayer55221:escapedXSS/encodeURIComponent) 5 fail.

Edit: Two of them seem to be an unexpected attack value. Three of them seem to be false positives (alert raised where none is expected).

kingthorin · 2025-08-25T14:58:01Z

To address the two that are returning the unexpected attack you can simply move your new code below the contexts2 handling and before the return false.

devplayer55221 · 2025-08-25T15:29:07Z

This is how you'd test just the one class from the CLI:
./gradlew :aO:ascanrules:test --tests org.zaproxy.zap.extension.ascanrules.CrossSiteScriptingScanRuleUnitTest
I can confirm on main it passes while on your branch (devplayer55221:escapedXSS/encodeURIComponent) 5 fail.

Edit: Two of them seem to be an unexpected attack value. Three of them seem to be false positives (alert raised where none is expected).

Got it, Thanks!

devplayer55221 · 2025-08-25T15:33:41Z

To address the two that are returning the unexpected attack you can simply move your new code below the contexts2 handling and before the return false.

I tried this just now. The two unexpected attacks are not showing any more at the test command, but when I ran ZAP again, and ran Active Scanner on the [URL] (https://public-firing-range.appspot.com/escape/js/encodeURIComponent?q=a), it gave the alert but with a different payload, which is a false positive.

I haven't understood completely that how the code is analyzing if the payload actually triggered the alert. Can I get some help on this?

devplayer55221 · 2025-08-27T10:00:53Z

This one was not yet addressed #6686 (comment)

Okay, got it. I have done the spotlessApply, earlier something was missed I guess.

devplayer55221 · 2025-08-27T10:01:18Z

Maybe make the changelog comment more user facing/relevant? Maybe something like:

The Cross Site Scripting scan rule has been updated for additional coverage of JavaScript eval situations.

I modified the Changelog comment as suggested.

kingthorin · 2025-08-27T10:33:24Z

CrossSiteScriptingScanRuleUnitTest > shouldNotAlertXssInFilteredJsString() FAILED
    java.lang.AssertionError: 
    Expected: a collection with size <0>
         but: collection size was <1>
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
        at org.zaproxy.zap.extension.ascanrules.CrossSiteScriptingScanRuleUnitTest.shouldNotAlertXssInFilteredJsString(CrossSiteScriptingScanRuleUnitTest.java:2141)
CrossSiteScriptingScanRuleUnitTest > shouldNotReportXssInsideInputAndScriptWithGoodFiltering() FAILED
    java.lang.AssertionError: 
    Expected: <0>
         but: was <1>
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
        at org.zaproxy.zap.extension.ascanrules.CrossSiteScriptingScanRuleUnitTest.shouldNotReportXssInsideInputAndScriptWithGoodFiltering(CrossSiteScriptingScanRuleUnitTest.java:1693)
CrossSiteScriptingScanRuleUnitTest > shouldNotAlertXssInJsStringWithEncoding() FAILED
    java.lang.AssertionError: 
    Expected: <0>
         but: was <1>
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
        at org.zaproxy.zap.extension.ascanrules.CrossSiteScriptingScanRuleUnitTest.shouldNotAlertXssInJsStringWithEncoding(CrossSiteScriptingScanRuleUnitTest.java:2111)

devplayer55221 · 2025-08-27T11:39:09Z

To resolve this, I should focus on changing my scan rule, or changing these unit tests?

kingthorin · 2025-08-27T11:57:33Z

Well it depends why they're now failing. Given that no alert is raised at all it "seems" like it is something in the new changes that needs to be changed further.

kingthorin · 2025-08-27T12:08:58Z

Have you re-built the add-on and tested it against Firing Range?

devplayer55221 · 2025-08-27T12:13:34Z

Have you re-built the add-on and tested it against Firing Range?

Yes, I have checked it. It works okay. Checked it just now again.

devplayer55221 · 2025-08-27T12:38:58Z

Is there a way through which I see which functions are being triggered when the contexts are being checked in CrossSiteScriptingScanRule.java?

kingthorin · 2025-08-27T12:39:02Z

Sorry my earlier message was wrong, it should have said:
"Given that alerts are being raised where they previously weren't it "seems" like it is something in the new changes that needs to be changed/handled further."

I had a quick look at all three tests that are failing, it seems the new payload is always being assigned to a var which doesn't really represent an XSS, ex: <script>var a = "alert(1)"</script>

kingthorin · 2025-08-27T12:41:04Z

You can add print statements or use a debugger.

devplayer55221 · 2025-08-27T12:42:19Z

You can add print statements or use a debugger.

Okay, got it. And for that to print do I need to make some configuration change?
For printing the System.out.println in unit test file I had to use the --info flag.
Should I use the same flag with ./gradlew run

devplayer55221 · 2025-08-27T12:42:39Z

Sorry my earlier message was wrong, it should have said: "Given that alerts are being raised where they previously weren't it "seems" like it is something in the new changes that needs to be changed/handled further."

I had a quick look at all three tests that are failing, it seems the new payload is always being assigned to a var which doesn't really represent an XSS, ex: <script>var a = "alert(1)"</script>

Okay, I understand. Thanks

kingthorin · 2025-08-27T12:43:57Z

Using println you should just see it in the terminal/console. Are you using Eclipse as your IDE?

devplayer55221 · 2025-08-27T12:44:37Z

Using println you should just see it in the terminal/console. Are you using Eclipse as your IDE?

I am using VS Code, and executing the commands through terminal.

kingthorin · 2025-08-27T12:53:55Z

...just checked. It seems you do need to use --info, it's going to include a lot of output you weren't previously getting so make sure your print statements are really obvious or search for them in the terminal output.

devplayer55221 · 2025-08-27T13:02:18Z

Hi, I also just checked. I am able to get the System.out.println output 'without' the use of --info in ./gradlew run.
However, I am stuck with how do I identify if eval is being used to execute the input. Any help on this?

I will have to add a condition to check if eval is executing the input

kingthorin · 2025-08-27T13:28:09Z

Hmmm that might be easier said than done.
You could try with regex but there's a ton of things to consider: multiline, minified, deeper set of calls...
It might require something more like (ANTLR which is currently delayed due to other changes/priorities) or graal parser.

devplayer55221 · 2025-08-27T13:37:05Z

Hmmm that might be easier said than done. You could try with regex but there's a ton of things to consider: multiline, minified, deeper set of calls... It might require something more like (ANTLR which is currently delayed due to other changes/priorities) or graal parser.

Okay, I understand.
But I was thinking that are those unit tests that were failing, totally correct?
They are filtering the input value, but the payload alert(1) would not get filtered and will be detected by the scan rule as it will cause alert due to the new code change for the eval case.

kingthorin · 2025-08-27T14:49:44Z

If we knew that the variable the payload is assigned to is being executed in an eval.

devplayer55221 · 2025-08-27T15:03:23Z

I checked that isInTagWithSrc value is true for the context when using alert(1) as payload and the response contains <script> var a = "alert(1)" </script>
Is this expected?

psiinon · 2025-08-27T15:26:19Z

I checked that isInTagWithSrc value is true for the context when using alert(1) as payload and the response contains <script> var a = "alert(1)" </script> Is this expected?

Not sure, its been a while since I've looked at the code. But injecting into a string like that does not indicate a vulnerability. We need to be able to break out of the string and execute something we're specifying...

kingthorin · 2025-08-27T15:32:51Z

Or we'd have to know that something like eval(a) ends up happening.

devplayer55221 · 2025-08-27T15:54:31Z

Yeah got it. I have been debugging, and I will have to think of the logic again, it doesn't seem entirely correct.
Now I know how to debug and do unit testing, I will check further.

devplayer55221 · 2025-08-27T19:45:37Z

Are contexts interpreted differently when run by unit test and in the application?
For the performScriptAttack, I am getting different responses.

When I run active scanner in application: contexts2 is empty

When I run the ascanrules unit tests: contexts2 is not empty

The unit test function is 'shouldReportXssInSriptAttackInEval'. I could not figure out why is there a difference between active scanner and unit test for getting the value of 'contexts2'.

The next comment contains the performScriptAttack function I modified for debugging and getting the above outputs.

devplayer55221 · 2025-08-27T19:46:44Z

private boolean performScriptAttack(HtmlContext context, HttpMessage msg, String param) {
        System.out.println("******Inside performScriptAttack******");
        System.out.println("***** context *****");
        System.out.println("context -> " + context);
        System.out.println("context request -> " + context.getMsg().getRequestBody().toString());
        System.out.println("context response -> " + context.getMsg().getResponseBody().toString());
        System.out.println("msg request -> " + msg.getRequestBody());
        System.out.println("msg response -> " + msg.getResponseBody());
        System.out.println("param -> " + param);
        System.out.println("context getSurroundingQuote -> " + context.getSurroundingQuote());
        List<HtmlContext> contexts2 =
                performAttack(
                        msg,
                        param,
                        context.getSurroundingQuote()
                                + ";alert(1);"
                                + context.getSurroundingQuote(),
                        context,
                        0);
        System.out.println("contexts2 -> " + contexts2);
        if (contexts2 == null) {
            return false;
        }
        System.out.println("***** contexts2 *****");
        for (HtmlContext ctx : contexts2) {
            System.out.println("Response -> " + ctx.getMsg().getResponseBody().toString());
            if (context.getSurroundingQuote().isEmpty() || !ctx.getSurroundingQuote().isEmpty()) {
                // Yep, its vulnerable
                System.out.println("Vulnerability detected!");
                newAlert()
                        .setConfidence(Alert.CONFIDENCE_MEDIUM)
                        .setParam(param)
                        .setAttack(ctx.getTarget())
                        .setEvidence(ctx.getTarget())
                        .setMessage(ctx.getMsg())
                        .raise();
                return true;
            }
        }
        List<HtmlContext> contexts3 = performAttack(msg, param, GENERIC_ALERT, null, 0);
        if (contexts3 == null) {
            return false;
        }
        System.out.println("***** contexts3 *****");
        for (HtmlContext ctx : contexts3) {
            System.out.println("inTagWithSrc -> " + ctx.isInTagWithSrc());
            System.out.println("context request -> " + ctx.getMsg().getRequestBody().toString());
            System.out.println("context response -> " + ctx.getMsg().getResponseBody().toString());
        }
        System.out.println("msg -> " + msg);
        System.out.println("param -> " + param);
        if (contexts3 != null && !contexts3.isEmpty()) {
                if (processContexts(contexts3, param, GENERIC_ALERT, false)) {
                    return true;
                }
            }
        return false;
    }

psiinon · 2025-08-27T20:26:34Z

No, contexts should be handled the same in unit tests and in the app, otherwise the unit tests would be meaningless 😁

Added detection of Escaped XSS encodeURIComponent

a243ecb

This comment was marked as outdated.

Sign in to view

thc202 changed the title ~~Added detection of Escaped XSS encodeURIComponent~~ ascanrules: detect Escaped XSS encodeURIComponent Aug 24, 2025

Modified encodeURIComponent detection code

7a47dbf

Updated Changelog

c6635ca

devplayer55221 added 2 commits August 27, 2025 15:27

spotlessapply

354006b

Changelog modified.

0e321a5

Modified the unit test response HTML

800f3ae

kingthorin pushed a commit to kingthorin/cla that referenced this pull request Sep 18, 2025

@devplayer55221 has signed the CLA in zaproxy/zap-extensions#6686

a461982

Uh oh!

ascanrules: detect Escaped XSS encodeURIComponent #6686

Are you sure you want to change the base?

ascanrules: detect Escaped XSS encodeURIComponent #6686

Uh oh!

Conversation

devplayer55221 commented Aug 24, 2025

Uh oh!

github-actions bot commented Aug 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devplayer55221 commented Aug 24, 2025

Uh oh!

This comment was marked as outdated.

kingthorin commented Aug 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kingthorin commented Aug 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

psiinon commented Aug 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Great job! No new security vulnerabilities introduced in this pull request

Uh oh!

thc202 commented Aug 24, 2025

Uh oh!

thc202 commented Aug 24, 2025

Uh oh!

devplayer55221 commented Aug 24, 2025

Uh oh!

devplayer55221 commented Aug 24, 2025

Uh oh!

devplayer55221 commented Aug 24, 2025

Uh oh!

kingthorin commented Aug 24, 2025

Uh oh!

devplayer55221 commented Aug 24, 2025

Uh oh!

kingthorin commented Aug 24, 2025

Uh oh!

devplayer55221 commented Aug 25, 2025

Uh oh!

thc202 commented Aug 25, 2025

Uh oh!

devplayer55221 commented Aug 25, 2025

Uh oh!

thc202 commented Aug 25, 2025

Uh oh!

devplayer55221 commented Aug 25, 2025

Uh oh!

thc202 commented Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devplayer55221 commented Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thc202 commented Aug 25, 2025

Uh oh!

kingthorin commented Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kingthorin commented Aug 25, 2025

Uh oh!

devplayer55221 commented Aug 25, 2025

Uh oh!

devplayer55221 commented Aug 25, 2025

Uh oh!

devplayer55221 commented Aug 27, 2025

Uh oh!

devplayer55221 commented Aug 27, 2025

Uh oh!

kingthorin commented Aug 27, 2025

Uh oh!

devplayer55221 commented Aug 27, 2025 • edited by kingthorin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kingthorin commented Aug 27, 2025

Uh oh!

kingthorin commented Aug 27, 2025

Uh oh!

github-actions bot commented Aug 24, 2025 •

edited

Loading

kingthorin commented Aug 24, 2025 •

edited

Loading

kingthorin commented Aug 24, 2025 •

edited

Loading

psiinon commented Aug 24, 2025 •

edited

Loading

thc202 commented Aug 25, 2025 •

edited

Loading

devplayer55221 commented Aug 25, 2025 •

edited

Loading

kingthorin commented Aug 25, 2025 •

edited

Loading

devplayer55221 commented Aug 27, 2025 •

edited by kingthorin

Loading

kingthorin commented Aug 27, 2025 •

edited

Loading

devplayer55221 commented Aug 27, 2025 •

edited

Loading

kingthorin commented Aug 27, 2025 •

edited

Loading

devplayer55221 commented Aug 27, 2025 •

edited

Loading

devplayer55221 commented Aug 27, 2025 •

edited

Loading

devplayer55221 commented Aug 27, 2025 •

edited

Loading