fix(routing): polish CIDR/regex/port-range matchers and document capset stance#24
fix(routing): polish CIDR/regex/port-range matchers and document capset stance#24andypost wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes a bug by ensuring a non-zero ovector size is passed to PCRE2 match-data creation and refines the handling of malformed port ranges in HTTP routing. Additionally, it introduces documentation clarifying capability detection, case-insensitive host matching, and URI decoding symmetry. Review feedback identifies an opportunity to simplify the port range validation to better reject malformed inputs and suggests explicitly setting the match type for /32 CIDR blocks to maintain consistency with the updated comments.
| if (port.length < 3) { | ||
| delim = NULL; | ||
| } else { | ||
| delim = memchr(port.start, '-', port.length); | ||
| } |
There was a problem hiding this comment.
The check port.length < 3 prevents the fix from working for 2-byte malformed inputs like 8-, which the pull request description specifically mentions as a target for rejection. By setting delim = NULL for 2-byte strings, these inputs continue to fall through to the single-port parsing branch (nxt_int_parse). If nxt_int_parse is lax, they will still be accepted. Removing this conditional and simply using memchr with port.length allows the range parsing logic to correctly identify and reject these malformed patterns because the second part of the range will be empty, causing the subsequent nxt_int_parse call to fail.
delim = memchr(port.start, '-', port.length);| /* | ||
| * /32 is the single-host case; fall through to the inet_addr | ||
| * parse below, but set EXACT here so the state machine reflects | ||
| * the user's intent rather than relying on the implicit default. | ||
| */ |
There was a problem hiding this comment.
The comment states that EXACT is set here, but the code actually falls through to line 277 where base->match_type is eventually assigned. To make the code match the documentation and improve clarity, it is better to explicitly set the match type within this block before exiting the CIDR parsing logic.
/*
* /32 is the single-host case; fall through to the inet_addr
* parse below, but set EXACT here so the state machine reflects
* the user's intent rather than relying on the implicit default.
*/
base->match_type = NXT_HTTP_ROUTE_ADDR_EXACT;
…et stance Audit-driven routing-polish + isolation-docs pass (PR-I from security-audit.md / #10). Six low-priority findings; no behaviour change for any valid existing config. * V2 [Medium] IPv4 /32 CIDR fallthrough (nxt_http_route_addr.c) When cidr_prefix == 32 the parser today falls through to the inet_addr path below, which then sets match_type = EXACT. The current code is correct, but the fallthrough is fragile under refactor. Add a comment making the intent explicit so a future reader doesn't add an early goto and accidentally skip the inet_addr parse. * V2 [Medium] Asymmetric pattern/request URI decoding (nxt_http_route.c) Both configured pattern strings and request URIs decode through nxt_decode_uri / nxt_decode_uri_plus; %XX semantics are symmetric. Document the invariant at nxt_http_route_decode_str so a future helper change keeps the two paths in lock-step. * V2 [Medium] nxt_regex_match_create(.., 0) size argument (route.c) pcre2_match_data_create(0, ctx) is undefined per the PCRE2 docs; 1 is the documented minimum (one ovector pair, holding the overall match offsets — captures aren't consulted here). Bump the size from 0 to 1 and document. * V2 [Low] Port-range parsing off-by-one (route_addr.c) memchr(.., port.length - 1) silently accepted a trailing '-' in a 2-byte input ("N-"). Validate port.length >= 3 (the minimum for "N-N") before searching, drop the - 1, and let the single-port parse below reject the malformed range. * V2 [Low] Host matching always lowercased (nxt_http_route.c) Host matching is hardcoded to LOWCASE; admin-supplied case-sensitivity is silently ignored. Per RFC 9110 §4.2.3 hosts are case-insensitive, so the behaviour is correct; just document it so the surprise is in the comment, not in the operator's logs. * V6 [Informational] capset never called (nxt_capability.c) The module exposes capget for detection but does not call capset(2) to drop capabilities programmatically. Operators expecting an explicit cap-drop step should not assume one exists — privilege barriers are setuid + PR_SET_NO_NEW_PRIVS. Top-of-file comment. No behaviour change for any valid existing config; the only visible change is that a route pattern with a malformed port range like "X-" (2 bytes, trailing dash) is now rejected with NXT_ADDR_PATTERN_PORT_ERROR via the single-port parse branch instead of being silently accepted as a host-only entry. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
andypost
force-pushed
the
claude/audit-pr-i-routing-polish
branch
from
5dbeb17 to
352fae9
Compare
|
Both Gemini findings addressed in
Force-pushed; build clean. |
1 participant
Summary
Audit-driven routing polish + isolation docs (PR-I from `security-audit.md` / PR #10). Six low-priority findings, all low-impact; closes the tail of the audit's actionable items.
After this lands, the only open audit findings are: PR-H (controller robustness — JSON depth/element caps, PHP TrueAsync EG state, Ruby per-request IO; not yet implemented), and two excluded by the maintainer's DoS policy.
Findings addressed
Behaviour change
One: a route pattern with a malformed port range like `X-` (2 bytes, trailing dash) is now rejected with `NXT_ADDR_PATTERN_PORT_ERROR` via the single-port parse branch instead of being silently accepted. Pre-existing behaviour was to fall through and accept the host-only entry; no valid Unit config relies on this.
Files changed
4 files, +62 / -2.
Test plan
Upstream
Same fixes apply to `freeunitorg/freeunit`; will forward after merge here.
Generated by Claude Code