hrw4u: admit SESSION_VARS as a denyable section - #13696
masaori335 wants to merge 1 commit into
Conversation
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.
There was a problem hiding this comment.
🟢 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_VARSto the sandbox schema. - Documents denial of
VARSandSESSION_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
left a comment
There was a problem hiding this comment.
LGTM. Verified the schema enum now matches the compiler exactly: 10 entries — the 8 SectionType hooks reached via _prepare_section → check_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.
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.