8000 fix(941390): add missing javascript `prompt` and `confirm` methods by Xhoenix · Pull Request #3395 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix(941390): add missing javascript prompt and confirm methods #3395

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Dec 14, 2023
Merged

fix(941390): add missing javascript prompt and confirm methods #3395

merged 11 commits into from
Dec 14, 2023

Conversation

Xhoenix
Copy link
Member
@Xhoenix Xhoenix commented Nov 29, 2023 •
edited by fzipi
Loading

Add missing Javascript interaction methods: prompt() and confirm().

Fixes 9KH-231120-1 and #3381.

Reference: https://javascript.info/alert-prompt-confirm

fzipi
fzipi requested changes Nov 29, 2023
View reviewed changes
Copy link
Member
@fzipi fzipi left a comment

Choose a reason for hiding this comment

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

Thanks for your PR.

@dune73
Copy link
Member
dune73 commented Nov 29, 2023

PR looks good to me with the exception of the missing double quote. Happy to merge afterwards.

@Xhoenix @fzipi
Update tests/regression/tests/REQUEST-941-APPLICATION-ATTACK-XSS/9413…
9e96e1e 
…90.yaml

Co-authored-by: Felipe Zipitría <3012076+fzipi@users.noreply.github.com>
dune73
dune73 previously approved these changes Nov 29, 2023
View reviewed changes
@dune73 dune73 requested a review from fzipi November 29, 2023 22:30
@fzipi
Copy link
Member
fzipi commented Dec 4, 2023

@Xhoenix Can you please add also confirm? And maybe adding a new test for it. Also, maybe adding a new reference to https://javascript.info/alert-prompt-confirm.

@fzipi fzipi changed the title fix: 9KH-231120-1 fix: add missing javascript prompt and confirm methods Dec 4, 2023
@fzipi fzipi changed the title fix: add missing javascript prompt and confirm methods fix(941390): add missing javascript prompt and confirm methods Dec 4, 2023
8000
@fzipi
Copy link
Member
fzipi commented Dec 4, 2023

@RedXanadu Is there a way to test these two word additions to the rule using your script? I'm afraid adding them will be way more FP prone 🤔

@fzipi fzipi mentioned this pull request Dec 4, 2023
Closed
@dune73 dune73 added the v4 Should go into release v4 label Dec 12, 2023
theMiddleBlue
theMiddleBlue previously approved these changes Dec 12, 2023
View reviewed changes
Copy link
Contributor
@theMiddleBlue theMiddleBlue left a comment

Choose a reason for hiding this comment

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

LGTM

@dune73
Copy link
Member
dune73 commented Dec 12, 2023

Waiting for a comment from @RedXanadu. Ready to merge afterwards.

@RedXanadu
Copy link
Member

I'll run before and after numbers and compare…

@RedXanadu
Copy link
Member
RedXanadu commented Dec 13, 2023 •
edited
Loading

The original rule caused no false positives in the natural language testing.

The new regular expression does not cause any false positives with the set of 10,000 English test sentences. I even tried the corpus of 1,000,000 sentences just to be safe, and again there were 0 false positives caused by this new regex.

Inevitably, there will be natural language false positives with this eventually. It's easy to create some, e.g.:

  • Be prompt (to every meeting!)
  • I can confirm (with regret) that the coffee machine is broken.

But it's easy to work backwards from a regex and create false positive payloads… Certainly, nothing triggered in the news article test sentences 🙂

RedXanadu
RedXanadu requested changes Dec 13, 2023
View reviewed changes
Copy link
Member
@RedXanadu RedXanadu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@Xhoenix A question about the change to rules/unix-shell.data: is that a commit that fixes / belongs to a different issue/PR? I believe that file relates to the 93216x rules.

@dune73
Copy link
Member
dune73 commented Dec 14, 2023

That pyversions and py3versions had me puzzled too. I suggest we keep PRs separate.

So the idea would be to remove that change from this change and open a separate PR for it. It's a welcome addition to unix-shell.data.

fzipi
fzipi reviewed Dec 14, 2023
View reviewed changes
Copy link
Member
@fzipi fzipi left a comment

Choose a reason for hiding this comment

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

One last small change, so we keep tests clean.

RedXanadu
RedXanadu previously approved these changes Dec 14, 2023
View reviewed changes
@dune73
Copy link
Member
dune73 commented Dec 14, 2023

Cool. Let's get the HTTP methods into the URI and we're ready to merge.

@Xhoenix @fzipi
Update tests/regression/tests/REQUEST-941-APPLICATION-ATTACK-XSS/9413…
8f192bb 
…90.yaml

Co-authored-by: Felipe Zipitría <3012076+fzipi@users.noreply.github.com>
Xhoenix and others added 2 commits December 14, 2023 18:26
@Xhoenix @fzipi
Update tests/regression/tests/REQUEST-941-APPLICATION-ATTACK-XSS/9413…
3ef2d6c 
…90.yaml

Co-authored-by: Felipe Zipitría <3012076+fzipi@users.noreply.github.com>
@Xhoenix
Merge branch 'v4.0/dev' into v4.0/dev
d8167f9
@fzipi fzipi merged commit b93da4f into coreruleset:v4.0/dev Dec 14, 2023
@github-actions github-actions bot mentioned this pull request Dec 15, 2023
Merged
@theseion theseion mentioned this pull request Dec 17, 2023
Closed
@dune73 dune73 mentioned this pull request Jan 8, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fzipi fzipi fzipi approved these changes

@dune73 dune73 dune73 left review comments

@theMiddleBlue theMiddleBlue theMiddleBlue left review comments

@RedXanadu RedXanadu RedXanadu left review comments

Assignees
No one assigned
Labels
v4 Should go into release v4
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Xhoenix @dune73 @fzipi @RedXanadu @theMiddleBlue
307C
0