Skip to content

hrw4u: admit SESSION_VARS as a denyable section - #13696

Open
masaori335 wants to merge 1 commit into
apache:masterfrom
masaori335:asf-master-hrw4u-sandbox-session-vars
Open

masaori335 wants to merge 1 commit into
apache:masterfrom
masaori335:asf-master-hrw4u-sandbox-session-vars

Conversation

@masaori335

Copy link
Copy Markdown
Contributor

SESSION_VARS is already checked by visitSessionVarSection like VARS, but the schema enum and the sandbox section text both listed only VARS, so a policy denying session variables worked while being invalid against the published schema. Add a fixture to pin the behavior: stubbing out the check_section call in visitSessionVarSection fails it and nothing else.

SESSION_VARS is already checked by visitSessionVarSection like VARS,
but the schema enum and the sandbox section text both listed only
VARS, so a policy denying session variables worked while being invalid
against the published schema. Add a fixture to pin the behavior:
stubbing out the check_section call in visitSessionVarSection fails it
and nothing else.
@masaori335 masaori335 added this to the 11.0.0 milestone Sep 16, 2026
@masaori335 masaori335 self-assigned this Sep 16, 2026
Copilot AI lite review requested due to automatic review settings September 16, 2026 01:37

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

Schema, documentation, and regression fixtures consistently cover SESSION_VARS denial.

Pull request overview

Updates HRW4U so SESSION_VARS can be denied consistently across schema validation, documentation, and regression tests.

Changes:

  • Adds SESSION_VARS to the sandbox schema.
  • Documents denial of VARS and SESSION_VARS.
  • Adds regression fixtures for denial behavior.
File summaries
File Description
tools/hrw4u/tests/data/sandbox/denied-section-session-vars.sandbox.yaml Denial policy fixture.
tools/hrw4u/tests/data/sandbox/denied-section-session-vars.input.txt Test input.
tools/hrw4u/tests/data/sandbox/denied-section-session-vars.error.txt Expected denial output.
tools/hrw4u/tests/data/sandbox/denied-section-session-vars.ast.txt Expected AST.
tools/hrw4u/schema/sandbox.schema.json Allows SESSION_VARS in denied sections.
doc/admin-guide/configuration/hrw4u.en.rst Documents denyable variable sections.
Review details
  • Files reviewed: 6/6 changed files
  • Comments generated: 0
  • Review effort level: Lite

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

@brbzull0 brbzull0 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.

LGTM. Verified the schema enum now matches the compiler exactly: 10 entries — the 8 SectionType hooks reached via _prepare_sectioncheck_section, plus the two grammar-level varSection / sessionVarSection blocks that hard-code their own check at visitor.py:835 and :851. No compiler-recognized section name is missing from the enum, and no enum entry is unrecognized by the compiler.

The fixture is load-bearing rather than vacuous — deleting the check_section call at visitor.py:851 does fail it, the error text matches SandboxConfig._check's construction byte-for-byte, and the AST matches ANTLR's toStringTree rendering (cross-checked against tests/data/vars/session_bool.ast.txt).

I also checked whether denying SESSION_VARS is bypassable, and it isn't: VarScope.SESSION is assigned at exactly one site (visitor.py:855), after the deny check has returned. There's no surface syntax for raw session state, and procedure files can't smuggle one in — _load_proc_file consumes only useDirective and procedureDecl, and a procedureDecl body has no variableDecl alternative in the grammar.

Three notes, none blocking.

doc/admin-guide/configuration/hrw4u.en.rst:778 — this PR updates the Denied Sections paragraph but leaves the Language Constructs table 65 lines below saying variables controls "The entire VARS section and all variable usage". visitVarSection and visitSessionVarSection both go through check_language("variables") (visitor.py:837 and :853), so it gates both. Someone who wants transaction variables denied but session variables permitted will write deny: {language: [variables]} and silently get both. Suggest "The VARS and SESSION_VARS sections" — and dropping "and all variable usage", since no check implements that; denial blocks the declaration blocks, not references.

sandbox.schema.json — the schema hunk has no test coverage. Revert it and the suite still passes, because nothing in the tree reads sandbox.schema.json; the four new files pin the runtime check instead. Worth having either way, but it means the next person to add a section type can let the enum go stale the same way.

denied-section-session-vars.ast.txt — never read by anything. collect_ast_test_files is only invoked for the conds/examples/hooks/ops/vars groups, and there's no AST test registered for sandbox, so the recorded tree can go stale invisibly.

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.

3 participants