feat: add ARGS_RAW, ARGS_GET_RAW, ARGS_POST_RAW, and ARGS_NAMES_RAW collections#1493
feat: add ARGS_RAW, ARGS_GET_RAW, ARGS_POST_RAW, and ARGS_NAMES_RAW collections#1493fzipi wants to merge 9 commits into
Conversation
…ollections Add raw (non-URL-decoded) argument collections that preserve wire-format values before any percent-decoding. This enables rules to safely apply t:urlDecodeUni without double-decoding, detect double URL encoding, and behave consistently across integrations. Closes #1491
There was a problem hiding this comment.
Pull request overview
This PR adds four new raw (non-URL-decoded) argument collections to Coraza: ARGS_RAW, ARGS_GET_RAW, ARGS_POST_RAW, and ARGS_NAMES_RAW. These collections preserve wire-format values before any percent-decoding, addressing a long-standing issue where applying transformations like t:urlDecodeUni to automatically-decoded variables results in double decoding. This creates both false positives and detection gaps depending on how rules are written. The new raw collections enable rule authors to safely apply URL decoding transformations exactly once, with consistent semantics across different Coraza integrations.
Changes:
- Added four new variable types (
ArgsRaw,ArgsGetRaw,ArgsPostRaw,ArgsNamesRaw) with complete registration in the variables system - Implemented
ParseQueryRawfunction that preserves percent-encoding and plus signs in query string parsing - Integrated raw collections into transaction processing for GET parameters and all body processors (urlencoded, multipart, JSON)
- Added comprehensive integration tests covering URL-encoded values, double encoding, encoded names, and plus sign preservation
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| types/variables/variables.go | Exports new raw collection constants to public API |
| internal/variables/variables.go | Defines new raw variable types with documentation |
| internal/variables/variablesmap.gen.go | Adds name mapping and selection support for raw variables |
| internal/url/url.go | Implements ParseQueryRaw function that skips URL decoding |
| internal/url/url_test.go | Tests for raw query parsing covering edge cases |
| internal/corazawaf/transaction.go | Integrates raw collections into transaction state and GET argument extraction |
| internal/bodyprocessors/urlencoded.go | Populates raw collections for URL-encoded POST bodies |
| internal/bodyprocessors/multipart.go | Handles multipart bodies (where raw == cooked) |
| internal/bodyprocessors/json.go | Handles JSON bodies (where raw == cooked) |
| experimental/plugins/plugintypes/transaction.go | Adds raw collection methods to plugin interface |
| internal/operators/validate_schema_test.go | Updates mock transaction with raw collection stubs |
| testing/engine/args_raw.go | Comprehensive integration tests for all raw collections |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1493 +/- ##
==========================================
- Coverage 86.37% 86.32% -0.06%
==========================================
Files 176 176
Lines 8810 8924 +114
==========================================
+ Hits 7610 7704 +94
- Misses 949 968 +19
- Partials 251 252 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Address PR review feedback: - Add ParseQueryBoth() that returns both decoded and raw maps in one pass, avoiding double parsing in ExtractGetArguments and the urlencoded body processor. - Add missing warning log in addGetRequestArgumentRaw when argument limit is exceeded, consistent with other Add*Argument methods.
- Add fast-path in queryUnescape: skip Builder allocation entirely when input contains no '%' or '+' (common case for parameter names and plain values). - Cache queryUnescape(key) result in ParseQueryBoth to avoid calling it twice per pair. - Pre-size maps in ParseQueryBoth based on separator count to reduce rehashing.
Add per-source raw name collections for symmetry with existing ArgsGetNames/ArgsPostNames, completing the raw variable set.
|
@copilot add more coverage to tests |
* Initial plan * test: add more coverage for raw argument collections Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com>
|
Let's replace this with lazy loading, the variable is stored by default as RAW and we lazy load it in standard ARGS variables. Otherwise there is a major performance regression |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 18 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
internal/corazawaf/transaction.go:1889
- Under the
coraza.rule.case_sensitive_args_keysbuild tag,argsGetRaw/argsPostRaware created as case-sensitive collections, but rule variable key normalization still treatsARGS_*_RAWvariables as case-insensitive (seecaseSensitiveVariableininternal/corazawaf/rule.go). This will cause keyed selections likeARGS_GET_RAW:Footo be lowercased and fail to match when raw keys preserve case. Add the new RAW variables to the same case-sensitivity handling as their cooked counterparts (including regex/key normalization paths).
v.serverPort = collections.NewSingle(variables.ServerPort)
v.highestSeverity = collections.NewSingle(variables.HighestSeverity)
v.statusLine = collections.NewSingle(variables.StatusLine)
v.duration = collections.NewSingle(variables.Duration)
v.resBodyError = collections.NewSingle(variables.ResBodyError)
v.resBodyErrorMsg = collections.NewSingle(variables.ResBodyErrorMsg)
v.resBodyProcessorError = collections.NewSingle(variables.ResBodyProcessorError)
v.resBodyProcessorErrorMsg = collections.NewSingle(variables.ResBodyProcessorErrorMsg)
v.filesSizes = collections.NewMap(variables.FilesSizes)
v.filesTmpContent = collections.NewMap(variables.FilesTmpContent)
v.multipartFilename = collections.NewMap(variables.MultipartFilename)
internal/corazawaf/transaction.go:728
- Comment grammar: "an url encoded" should be "a URL-encoded" (and consider capitalizing URL) to match the terminology used elsewhere in the codebase and docs.
tx.ruleRemoveByID[id] = struct{}{}
}
You can also share your feedback on Copilot code review. Take the survey.
I like this. Honestly, we can even just store everything as RAW, and then if you ask for ARGS it is just applying the |
what
Add raw (non-URL-decoded) argument collections that preserve wire-format values before any percent-decoding. This enables rules to safely apply
t:urlDecodeUniwithout double-decoding, detect double URL encoding, and behave consistently across integrations.New collections
ARGS_GET_RAWARGS_GETARGS_POST_RAWARGS_POSTARGS_RAWARGSARGS_GET_RAWandARGS_POST_RAWARGS_NAMES_RAWARGS_NAMESARGS_GET_NAMES_RAWARGS_GET_NAMESARGS_POST_NAMES_RAWARGS_POST_NAMESImplementation details
ParseQueryBoth()tointernal/urlthat returns both decoded and raw maps in a single pass (no double parsing)queryUnescape: skips allocation when input has no%or+(common case)ARGS_POST_RAWrefs
ARGS_RAW,ARGS_GET_RAW,ARGS_POST_RAW, andARGS_NAMES_RAWCollections #1491