Skip to content

fix(limit-count): validate variable-resolved count/time_window bounds#13573

Merged
shreemaan-abhishek merged 4 commits into
apache:masterfrom
shreemaan-abhishek:fix/limit-count-var-bounds
Jun 24, 2026
Merged

fix(limit-count): validate variable-resolved count/time_window bounds#13573
shreemaan-abhishek merged 4 commits into
apache:masterfrom
shreemaan-abhishek:fix/limit-count-var-bounds

Conversation

@shreemaan-abhishek

@shreemaan-abhishek shreemaan-abhishek commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Description

limit-count lets count and time_window be set from a request variable, e.g. "count": "${http_count ?? 2}". At request time the resolved string was only coerced with tonumber and checked for being a number. It was never bounds-checked, so a client-supplied value of 0, a negative, a fractional, or an out-of-range number bypassed the schema's > 0 integer constraint:

  • 0/negative hit the limiter constructor's assert(limit > 0 and window > 0), turning a client header into a request-time error.
  • fractional/huge values skewed the limiter's math.

This mirrors the validation limit-conn already performs on its variable-resolved conn/burst: the resolved value must be a positive integer within the safe integer range (2^53-1). Invalid values are now rejected through the normal error path instead of reaching the limiter.

limit-conn's identical gap was already fixed; this closes the same hole in limit-count.

Which issue(s) this PR fixes:

N/A (hardening; reported internally against the API7 fork)

Compatibility

No valid configuration changes behavior: a request variable that resolves to a positive integer is handled exactly as before. There is one observable change at the out-of-spec edge:

  • 0/negative resolved values already errored at request time (via the limiter's assert); they now error through the normal path with a clearer message. Same client-visible outcome.
  • fractional or out-of-range (>2^53-1) resolved values were previously accepted and served with skewed limiting math; they are now rejected (rejected_code/500). These values were never valid per the schema (integer, > 0) and produced incorrect limiting, so this is the correct behavior, but a client that was unknowingly sending such values and getting served will now be rejected.
  • rules mode: previously a rule whose count/time_window resolved to an invalid value was silently skipped (goto CONTINUE), bypassing that rule's limit; it now rejects the request, the same as the non-rules path. A rule whose key does not resolve is still skipped, since such a rule legitimately does not apply to the request.

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change (docs already state > 0; behavior now matches)
  • I have verified that this change is backward compatible

count and time_window may be set from request variables (e.g.
"${http_count}"). The resolved value was only coerced with tonumber,
so a client could supply 0, a negative, a fractional, or an out-of-range
value, which slipped past the schema's >0 integer bounds and hit the
limiter constructor's assert (500) or skewed the limit.

Validate the resolved value the same way limit-conn already does:
positive, integer, within safe integer range.
@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. bug Something isn't working labels Jun 19, 2026
…error

Address review: use a generic error message instead of echoing the raw
resolved value (request-derived), clarify TEST 9 as route setup, and add
TEST 11 exercising out-of-bounds time_window rejection.
@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Jun 22, 2026
nic-6443
nic-6443 previously approved these changes Jun 22, 2026
@membphis

Copy link
Copy Markdown
Member

This looks incomplete for rules mode.

The new resolve_var() validation correctly rejects zero, negative, fractional, and out-of-range count / time_window values, but get_rules() still treats errors from rule.count and rule.time_window as “skip this rule”.

As a result, when count or time_window is resolved from a client-controlled variable, an invalid value can silently disable that rule instead of rejecting the request. This leaves limit-count-advanced / rules-based configurations uncovered by the fix.

Could we return the validation error for count / time_window here, and only skip the rule when the rule key itself does not resolve?

In rules mode get_rules() swallowed resolve_var errors via goto CONTINUE,
so a client-controlled variable resolving to an invalid count/time_window
silently dropped the rule instead of rejecting the request, bypassing the
limit. Return the error for count/time_window; only skip a rule when its
key does not resolve. Add a rules-mode regression test.
@shreemaan-abhishek

Copy link
Copy Markdown
Contributor Author

Good catch, you're right. In rules mode get_rules() was treating a resolve_var error on count/time_window as goto CONTINUE, so an invalid client-controlled value silently dropped the rule. With multiple rules that bypasses the bad rule's limit, and with a single rule + allow_degradation it fails open entirely.

Fixed: count/time_window resolution errors now return nil, err (rejected, same as the non-rules path); only an unresolved rule *key* still skips the rule, since a rule keyed on a variable absent for this request legitimately doesn't apply. Added a rules-mode regression test (verified it fails without the fix).

…e_window

A rule whose key resolves to nothing must be skipped before its count/
time_window are validated, otherwise a rule meant not to apply (its
count/time_window vars also absent) wrongly rejects the whole request.
@shreemaan-abhishek shreemaan-abhishek merged commit c956152 into apache:master Jun 24, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants