URLENCODED_ERROR is never set for the condition it documents; the decoder cannot detect it

[fzipi]

Summary

URLENCODED_ERROR is declared and documented, but Coraza never sets it for the condition it documents, and cannot: the query/body decoder is deliberately non-strict and reports no error. The one place it is set today is a different condition entirely.

What the variable is supposed to mean

From internal/variables/variables.go, inherited from ModSecurity:

This variable is created when an invalid URL encoding is encountered during the parsing of a query string (on every request) or during the parsing of an application/x-www-form-urlencoded request body (only on the requests that use the URLENCODED request body processor).

So: an invalid percent-encoding, in the query string or an urlencoded body.

What actually happens

The decoder never detects one. internal/url/url.go:

// queryUnescape is a non-strict version of net/url.QueryUnescape.
func queryUnescape(input string) string {

It returns no error, and malformed escapes are written through verbatim:

hi, ok := hexDigitToByte(input[i+1])
if !ok {
    res.WriteByte(ci)
    continue
}

Observed on main:

a=%41   ->  "A"        valid
a=%ZZ   ->  "%ZZ"      invalid hex, passed through
a=%A    ->  "%A"       truncated escape, passed through
a=100%  ->  "100%"     trailing percent, passed through
a=%%41  ->  "%A"       stray percent, then decoded

No error is produced in any of these cases, so nothing can set the variable.

The one place it is set is a different condition. internal/corazawaf/transaction.go:837:

parsedURL, err := url.ParseRequestURI(uri)
if err != nil {
    tx.variables.urlencodedError.Set(err.Error())

That fires when the URI structure is unparseable — raw control bytes, for example — not when a percent-encoding is invalid. Those are different failures. The commented-out block immediately below it even names a different variable for that case:

/*
    tx.Variables.VARIABLE_URI_PARSE_ERROR.Set("1")

So the variable is set for the wrong reason and never for the documented one.

Why it matters

A rule author reading the docs would reasonably write a rule on URLENCODED_ERROR expecting to catch malformed encoding, and get something else. It is also a genuine blind spot: Coraza currently cannot tell a rule that decoding was ambiguous, and "the WAF and the backend may have decoded this differently" is exactly the kind of disagreement that hides an evasion. A backend that rejects %ZZ, or decodes it differently from us, is a parser-mismatch surface with no signal attached.

CRS does not consume the variable either — coreruleset/coreruleset#482, "This flag is currently ignored by CRS", closed without a rule ever being added. That is a consequence rather than a cause: there is little point writing a rule against a flag that is never set for its documented meaning.

Related: this is the same shape as #1687, where MULTIPART_* variables are declared but never populated.

Proposed

1. Have the decoder report whether any escape was malformed. The natural shape is an additional return from doParseQuery / ParseQuery, leaving the decoding behaviour itself unchanged — Coraza should keep mirroring what backends accept rather than rejecting input, so this is a signal, not a policy change.
2. Set URLENCODED_ERROR from that, in both the query-string path and the URLENCODED body processor, which is what the documentation already promises.
3. Leave the URI-structure failure to its own variable, since it is a different condition.

Point 1 is the only real work; it touches a hot path, so it should avoid a second pass over the input — a bool tracked in the existing loop is enough.

Happy to take this if the shape sounds right. The main thing worth agreeing first is that this is a signal and never a rejection: no existing traffic should start being decoded differently.