Skip to content

feat: add JSON Schema validation via validateSchema operator #1343

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

Open
wants to merge 45 commits into
base: main
Choose a base branch
from

Conversation

cognitivegears
Copy link
Contributor

@cognitivegears cognitivegears commented Apr 5, 2025

Thank you for contributing to Coraza WAF, your effort is greatly appreciated
Before submitting check if what you want to add to coraza list meets quality standards before sending pull request. Thanks!

Make sure that you've checked the boxes below before you submit PR:

Thanks for your contribution ❤️

Fixes Issue #1279

This implements the validateSchema operator as per the existing modsecurity documentation, but currently only for JSON support. Since it didn't fit in the current structure of Coraza I didn't include it here, but I also have additional documentation and a test server for validation at: https://github.com/cognitivegears/coraza_validate_schema_extras

@cognitivegears cognitivegears requested a review from a team as a code owner April 5, 2025 04:28
Copy link

codecov bot commented Apr 5, 2025

Codecov Report

Attention: Patch coverage is 94.78261% with 6 lines in your changes missing coverage. Please review.

Project coverage is 84.51%. Comparing base (d0ca474) to head (f3b586f).

Files with missing lines Patch % Lines
internal/operators/validate_schema.go 93.40% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1343      +/-   ##
==========================================
+ Coverage   84.34%   84.51%   +0.16%     
==========================================
  Files         170      172       +2     
  Lines        9933    10046     +113     
==========================================
+ Hits         8378     8490     +112     
  Misses       1310     1310              
- Partials      245      246       +1     
Flag Coverage Δ
coraza.rule.case_sensitive_args_keys 84.47% <94.78%> (+0.16%) ⬆️
coraza.rule.multiphase_evaluation 84.17% <94.78%> (+0.16%) ⬆️
coraza.rule.no_regex_multiline 84.45% <94.78%> (+0.16%) ⬆️
default 84.51% <94.78%> (+0.16%) ⬆️
examples+ 16.15% <1.80%> (-0.18%) ⬇️
examples+coraza.rule.case_sensitive_args_keys 84.47% <94.78%> (+0.16%) ⬆️
examples+coraza.rule.multiphase_evaluation 84.01% <94.78%> (+0.17%) ⬆️
examples+coraza.rule.no_regex_multiline 84.37% <94.78%> (+0.16%) ⬆️
examples+memoize_builders 84.47% <94.59%> (+0.16%) ⬆️
examples+no_fs_access 81.88% <94.78%> (+0.19%) ⬆️
ftw 84.51% <94.78%> (+0.16%) ⬆️
memoize_builders 84.60% <94.59%> (+0.15%) ⬆️
no_fs_access 84.01% <94.78%> (+0.17%) ⬆️
tinygo 84.48% <94.78%> (+0.16%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cognitivegears
Copy link
Contributor Author

FYI I'll take a look at the code coverage change, looks like I need to add a few more tests.

I'm not sure what happened with the Tinygo test, I'll have to look into that one further.

In the meantime, would you mind taking a look at the overall approach and let me know if you have any questions or concerns / things you would like changed?

@jptosso
Copy link
Member

jptosso commented Apr 5, 2025

Thank you for your contribution!!
I'm concerned about XXE for XML, will have to look into the lib

@cognitivegears
Copy link
Contributor Author

Thank you for your contribution!! I'm concerned about XXE for XML, will have to look into the lib

Good callout. The way that libxml2 is initialized it does not look like it should resolve external entity references. However, I will try to add some tests to validate that specifically as well.

@cognitivegears
Copy link
Contributor Author

Thank you for your contribution!! I'm concerned about XXE for XML, will have to look into the lib

Good callout. The way that libxml2 is initialized it does not look like it should resolve external entity references. However, I will try to add some tests to validate that specifically as well.

Note - added some specific XXE validation testing as well.

@cognitivegears
Copy link
Contributor Author

Thanks so much for the feedback so far! I hope to have some more information later today or tomorrow.

@cognitivegears
Copy link
Contributor Author

If possible, can I please get an update on anything else needed on this PR? Otherwise if all is good can this be merged?

@jcchavezs
Copy link
Member

jcchavezs commented Apr 26, 2025 via email

@cognitivegears
Copy link
Contributor Author

I apologize for reaching out again, but if possible can I get an update on reviewing this PR? I'm hoping to release a feature on one of my projects that need this functionality and am hoping to put a bow on a couple of long-lived branches (here and my project.) Any help would be greatly appreciated.

@fzipi
Copy link
Member

fzipi commented Jun 17, 2025

ping @cognitivegears.

There are some small conflicts, that might be solved by rebasing.

@fzipi fzipi changed the title Adding JSON Schema validation via validateSchema operator feat: add JSON Schema validation via validateSchema operator Jun 17, 2025
@cognitivegears
Copy link
Contributor Author

ping @cognitivegears.

There are some small conflicts, that might be solved by rebasing.

Sorry it took a bit, there were a few changes beyond the rebase but they are resolved now.

@cognitivegears
Copy link
Contributor Author

All of the above code review comments should be completed now. If possible can I get a check?

@cognitivegears
Copy link
Contributor Author

@fzipi thanks also for updating the title

@jcchavezs
Copy link
Member

Did some rework in #1384

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.

5 participants