Skip to content

hrw4u: enforce sandbox NOT and in on their implicit spellings - #13675

Open
masaori335 wants to merge 3 commits into
apache:masterfrom
masaori335:asf-master-hrw4u-sandbox-gaps
Open

masaori335 wants to merge 3 commits into
apache:masterfrom
masaori335:asf-master-hrw4u-sandbox-gaps

Conversation

@masaori335

Copy link
Copy Markdown
Contributor

modifiers: [NOT] only caught an explicit with NOT, and language: [in] only caught the [...] value form, so !expr, !=, !~, !in and in {10.0.0.0/8} all compiled unchecked. AND and OR were already checked at && and ||; negation and IP-range membership are now consistent with that.

The check sits at the two sites where the source introduces negation, not at _make_condition, whose negate argument is also true for the [NOT] the compiler synthesises for a bare header test. Denying NOT must not reject if inbound.req.X-Foo; allowed-implicit-not pins that.

modifiers was undocumented, so the sandbox section gains it.

`modifiers: [NOT]` only caught an explicit `with NOT`, and
`language: [in]` only caught the `[...]` value form, so `!expr`, `!=`,
`!~`, `!in` and `in {10.0.0.0/8}` all compiled unchecked. AND and OR
were already checked at `&&` and `||`; negation and IP-range membership
are now consistent with that.

The check sits at the two sites where the source introduces negation,
not at `_make_condition`, whose `negate` argument is also true for the
`[NOT]` the compiler synthesises for a bare header test. Denying `NOT`
must not reject `if inbound.req.X-Foo`; allowed-implicit-not pins that.

`modifiers` was undocumented, so the sandbox section gains it.
@masaori335 masaori335 added this to the 11.0.0 milestone Sep 14, 2026
@masaori335 masaori335 self-assigned this Sep 14, 2026
Copilot AI lite review requested due to automatic review settings September 14, 2026 08:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

This PR tightens hrw4u sandbox enforcement so implicit negation spellings (!expr, !=, !~, !in) and IP-range membership (in {...} / !in {...}) are checked consistently with existing && / || enforcement, and updates documentation plus golden tests accordingly.

Changes:

  • Enforce sandbox policy checks for implicit NOT spellings and in {iprange} comparisons during compilation.
  • Add new sandbox golden test cases covering denied NOT, denied in with IP ranges, and allowed compiler-synthesized [NOT].
  • Document the previously-undocumented modifiers sandbox list and clarify what language: in covers.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tools/hrw4u/src/visitor.py Adds sandbox checks for implicit negation and IP-range in / !in comparisons during AST visiting.
tools/hrw4u/tests/data/sandbox/denied-modifier-not.* Golden test verifying sandbox denial of implicit NOT usages (!, !=, !in).
tools/hrw4u/tests/data/sandbox/denied-language-in-iprange.* Golden test verifying sandbox denial of in {cidr} and !in {cidr}.
tools/hrw4u/tests/data/sandbox/allowed-implicit-not.* Golden test ensuring compiler-synthesized [NOT] is not blocked by sandbox deny.modifiers: [NOT].
doc/admin-guide/configuration/hrw4u.en.rst Documents modifiers policy and clarifies language: in semantics.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread doc/admin-guide/configuration/hrw4u.en.rst Outdated
Comment thread tools/hrw4u/src/visitor.py
Comment thread tools/hrw4u/src/visitor.py
Comment thread tools/hrw4u/src/visitor.py
The modifiers example said `%{HEADER:X-Foo}`, but `inbound.req.X-Foo`
lowers to CLIENT-HEADER, as allowed-implicit-not's golden output shows.
Kept the literal on one line while here.
Copilot AI review requested due to automatic review settings September 14, 2026 23:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Moderate test-coverage findings and a documentation clarification remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

doc/admin-guide/configuration/hrw4u.en.rst:790

  • I, L, and QSA are operator modifiers, not condition modifiers (tools/hrw4u/src/states.py:68-82), and the forward visitor rejects them in a with modifier list via CondState.add_modifier. Calling this subsection “Condition Modifiers” and presenting all three in the same condition-modifier list is misleading; please distinguish the condition and operator modifiers while retaining the implicit-spelling explanation for AND/OR/NOT.
The ``modifiers`` list accepts ``AND``, ``OR``, ``NOT``, ``NOCASE``, ``PRE``,
``SUF``, ``EXT``, ``MID``, ``I``, ``L`` and ``QSA``. Entries match the modifier

tools/hrw4u/src/visitor.py:1033

  • The new denial fixture exercises !, !=, and !in, but not the NOT_TILDE branch for !~ selected immediately above. Since !~ is one of the implicit spellings this change promises to enforce, please add a sandbox-denied regex non-match case (including its expected error) so a regression in that branch is detected.
            if negate and not self._sandbox_check(ctx, lambda: self._sandbox.check_modifier("NOT")):
                return
  • Files reviewed: 14/14 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread tools/hrw4u/tests/data/sandbox/denied-modifier-not.input.txt
NOT_TILDE is its own lexer token; without a case for it, dropping it
from the negate tuple still passed.
Copilot AI review requested due to automatic review settings September 14, 2026 23:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No unresolved review comments were identified.

Review details
  • Files reviewed: 14/14 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants