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

fix: text editor in public share and token related FPs #59

Merged
merged 10 commits into from
Feb 19, 2024

Conversation

EsadCetiner
Copy link
Member

This PR mostly fixes issues with session tokens that are only an issue at PL-2 and above, but there is a fix for using the markdown test editor in public shares.
The tests for some of these rules were very large(This PR would've been above 700 lines, but now it's about 300), so I refactored them by using a | to test against multiple URIs with the same payload, but for whatever reason the nginx regression tests are failing while the apache tests are passing.

@theseion
Copy link
Contributor

I'll check it out tomorrow.

@theseion
Copy link
Contributor

This will create an illegal request:

uri: |
  uri1
  uri2

This is the same issue you had with the regular expressions: the vertical bar | in YAML means "verbatim string", not "list". The request generated from the above would look like this:

PUT /apps/text/session/create
/apps/text/public/session/create
 HTTP/1.1
Accept: text/xml,application/xml,application/xhtml+xml,text/html;q=0.9,text/plain;q=0.8,image/png,*/*;q=0.5
Connection: close
Content-Length: 0
Host: localhost
User-Agent: OWASP CRS

As you can see, the URIs are used as the request URI verbatim, including line breaks.

There is currently no way to run the same test against multiple URIs (would be great though). You could use multiple stages per test but that only makes a small difference and isn't what stages are designed for. For now, you don't have any other option than to duplicate tests for different URIs.

@theseion
Copy link
Contributor

I've created a feature request in go-ftw: coreruleset/go-ftw#256

@EsadCetiner
Copy link
Member Author

@theseion Thanks for the explanation, guess we'll have to live with it for now until go-ftw adds support for what I'm trying to do.

@theseion theseion merged commit 00b687d into coreruleset:main Feb 19, 2024
3 checks passed
@EsadCetiner EsadCetiner deleted the text-editor-fixes branch February 19, 2024 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants