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

Add support for SecArgumentSeparator setting #1109

Open
fzipi opened this issue Jul 21, 2024 · 2 comments
Open

Add support for SecArgumentSeparator setting #1109

fzipi opened this issue Jul 21, 2024 · 2 comments
Assignees
Labels
documentation Improvements or additions to documentation

Comments

@fzipi
Copy link
Member

fzipi commented Jul 21, 2024

Summary

Do you want to engage with Coraza but don't know where to start? This is the perfect issue for you!

Let's add support for SecArgumentSeparator setting. This should be set in the coraza.conf-recommended, and the default is &.

👉 Where is this set? Easy, you can find the (hardcoded) value here (a):

values := urlutil.ParseQuery(b, '&')

I'm excited, this sounds awesome! What do I need to do?

  1. Add a new option in the WAF in
    type WAF struct {
    . You can call it ArgumentSeparator, and it will be a char probably 😄
  2. Add support for the SecArgumentSeparator directive. This is easy: take a look at the internal/seclang/directives.go file, and create a new func directiveSecArgumentSeparator(options *DirectiveOptions) error. It will be very similar to other functions, e.g. directiveSecWebAppID. You will need to store the value of the argument separator in the variable created in 1.
  3. Add the right documentation following the same pattern as other. The description of the directive can be borrowed from https://github.com/owasp-modsecurity/ModSecurity/wiki/Reference-Manual-(v2.x)-Configuration-Directives#secargumentseparator with reasonable improvements.
  4. Just inject the variable's value instead of the hardcoded value shown above in (a)
  5. Write a test proving that the directive works! Take a look for example at the tests here and add a simple test that proves the vale of the WAF option is changed when you update the value.
  6. Create a PR! 🚀

Motivation

  1. Bringing you to the awesome community we have in Coraza ❤️
  2. Enjoy coding and learning Go and WAFs!
  3. Fun. Because this is fun, isn't it? 😉
@fzipi fzipi added the good first issue Good for newcomers label Jul 21, 2024
@jptosso
Copy link
Member

jptosso commented Jul 21, 2024

I don't think we should implement this. I have never seen the ; separator for url encoded
First coraza experiments included this but I removed it as the url decode package changed and I had to port the changes across go versions
Also I haven't seen this option in commercial wafs

Do we have a valid use case for this ?

@fzipi
Copy link
Member Author

fzipi commented Aug 14, 2024

No, there is no real use case. This was mostly about documenting the process. But I think the exercise was valuable and will add to the website docs and close.

@fzipi fzipi self-assigned this Oct 6, 2024
@fzipi fzipi added documentation Improvements or additions to documentation and removed good first issue Good for newcomers labels Oct 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

2 participants