Skip to content

fix(acl): a whitespace-padded audience label was silently ungrantable#47

Merged
sturlese merged 1 commit into
mainfrom
fix/bughunt-acl-padded-label
Jul 25, 2026
Merged

fix(acl): a whitespace-padded audience label was silently ungrantable#47
sturlese merged 1 commit into
mainfrom
fix/bughunt-acl-padded-label

Conversation

@sturlese

Copy link
Copy Markdown
Owner

Found by an autonomous bughunt sweep (lead 0e, surfaced by PR #37's gate and verified here).

Bug

An ACL config can mint an audience label that no client scope is able to match, and nothing errors. A client's scope is split and stripped (answer.settings._labels), while enforcement compares labels exactly (answer.index.visible). So:

config loaded OK, rule audiences: [' finance ', 'leadership']
resolve_acl -> [' finance ', 'leadership']
stored on the page as: ' finance ,leadership'
  ANSWER_AUDIENCES=' finance '  -> scope {'finance'}  visible=False
  ANSWER_AUDIENCES='finance'    -> scope {'finance'}  visible=False
Can ANY env value ever produce ' finance '? False

A rule whose only audience is padded is reachable by nobody at all:

a rule granting ONLY ' finance ' -> page acl ' finance '
   scope {'finance'} -> visible = False
   scope {'leadership'} -> visible = False
   scope {'finance','leadership','all'} -> visible = False
   unrestricted (None) -> visible = True

The operator who wrote audiences: [" finance "] believes they granted finance access. Nobody receives it, and the ACL file — the access-control authority — silently means something other than what it says.

Root cause

clean.acl._check_labels rejects commas and blank labels (#30) but not surrounding whitespace, so the label passes validation and is stored verbatim onto every page and facts row.

Fix

Reject it at config load, with a message naming the label the author meant. Rejected rather than stripped on purpose: silently normalizing would make the file mean something other than what it says, and the padding is usually a typo worth surfacing.

This is the mirror image of the comma case — silently over-restrictive rather than silently open, so milder, but just as silent.

Both halves of the contract move together. _label_ok's docstring promised the index rejects "exactly these" labels, so fixing only the config side would have made that promise false — and the index re-derives the rule from page text, so it cannot assume whoever wrote the page ran that validation. A page carrying a padded label now resolves to restricted-to-nobody: still closed, but discoverable by an unrestricted operator instead of looking like a working grant.

Label validity was only ever a promise in a docstring. It is now a machine-checked clause beside the visibility truth table, so the halves can't drift again — reverting either side alone drops it to 7/12 labels. Scorecard 25 → 26.

Everyday spellings are unaffected: YAML strips a trailing space from a plain scalar, so acl: [finance , leadership] still indexes as finance,leadership, and internal whitespace ("board members") remains legal.

Test

  • pipeline/clean: padded labels rejected across 5 shapes (f, f , f, tab, newline), the suggestion names the intended label, and "board members" still loads.
  • answer: a page carrying a padded label resolves to acl == "" — invisible to {finance}, visible to an unrestricted operator — plus the plain-scalar case that must keep working.
  • evals: new contract: ACL label validity parity (clean vs answer) clause over 12 label shapes.

Full CI-equivalent suite green locally (7 packages, ruff, evals 26/26). Verified the test fails without the fix (git stash push -- acl.py → 1 failed).

Mutation-tested with hash-verified restores, zero survivors:

mutant clean suite answer suite evals
clean: strip check removed (= main) 1 failed FAIL
clean: lstrip only 1 failed FAIL
clean: rstrip only 1 failed FAIL
clean: also reject internal spaces 1 failed FAIL
answer: strip check removed (= main) 1 failed FAIL
answer: strip() instead of reject (renames the label) 1 failed FAIL

The two answer mutants initially survived that package's own suite and were caught only by the eval, so I added the answer-side unit test — one mechanism is not enough to pin a contract.

🤖 Generated with Claude Code

An ACL config could mint a label no client scope can ever match, with no
error. A client's scope is split and stripped (answer.settings._labels),
while enforcement compares labels exactly (answer.index.visible), so
`{"unit": "Finance", "audiences": [" finance "]}` loaded fine, stamped
` finance ` onto every page and facts row, and reached NOBODY — verified
against scopes {finance}, {leadership} and {finance,leadership,all}; only
an unrestricted client saw the page. The operator who wrote the rule got
the opposite of what the file says.

This is the mirror image of the comma case (#30): silently over-restrictive
rather than silently open, so milder, but just as silent — and this file is
the access-control authority, which must never mean something other than
what it says. Rejected rather than stripped for the same reason, and
because the padding is usually a typo worth surfacing.

Both halves of the contract move together. _label_ok's docstring promised
the index rejects "exactly these" labels, so fixing only the config side
would have made that promise false; the index re-derives the rule from page
text and cannot assume the writer validated it. A page carrying a padded
label now resolves to restricted-to-nobody, which is the existing
all-or-nothing rule for an unrepresentable label rather than a new choice —
pinned here too, since a mixed [" finance ", "sales"] page served sales
before and serves nobody now (availability only; it can never widen, and an
unrestricted operator still sees the page, which is how it gets noticed).

Validation moves to cli(): the pass loads the config deep in the work path,
AFTER dedup_pending has deleted pages and facts rows and saved state, and
compose runs the worker `restart: unless-stopped` — so a rejected label
would have crash-looped a container that destroys state every lap and
processes nothing. Now it exits 1 with the message and no traceback, before
any work. CHANGELOG carries the upgrade note (a live padded label makes
clean refuse to start; no re-index is needed, since both encodings enforce
identically).

Label validity was only ever a promise in a docstring, so it is now a
machine-checked eval clause beside the visibility truth table, over 16
string shapes including NBSP/ZWSP/ideographic space: reverting either half
alone drops it. Scoped to STRING labels on purpose — the halves diverge for
non-str input, where the index refuses to coerce because str() would rename
a label into a grantable one. That divergence is a separate config-side
defect (rules' labels are stored raw, so a numeric label TypeErrors in
worker.py's ",".join) and is deliberately not what the clause measures.
Scorecard 25 -> 26.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sturlese

Copy link
Copy Markdown
Owner Author

Gate round 1 — core fix survived, 5 findings fixed

Adversarial review (opus, instructed to refute) plus my own probes. The central claim held up under attack; five surrounding issues did not, and all are fixed.

Refuted attacks (fix survives)

  • Fail-closed everywhere. No path yields acl IS NULL for a padded label — verified across list-padded, mixed, scalar, [], null, mapping and unclosed-block pages: every one indexes acl=''. visible_sql agrees with visible on every pair.
  • No migration needed. refresh does not re-read an unchanged page, so an existing index keeps the padded value — but visible(' finance ', s) == visible('', s) for every scope, so a stale row is semantically identical. Unlike fix(acl): the index trusted the acl value it re-derived from page text #38 there is no leak to migrate away from.
  • The rule is exactly right for strings. A brute force over {a,x,space,tab,newline,comma,NBSP,ZWSP,U+3000,VT}^≤3 found 0 strings where _label_ok disagrees with "producible by a client scope". Both sides call the same str.strip(), so NBSP/U+3000/VT/NEL are rejected by both and ZWSP is accepted by both — the agreement is structural, not enumerated.

Confirmed and fixed

1. BREAKING-CHANGE — one padded label would crash-loop the whole pipeline. Validation lived deep in the work path, after dedup_pending deletes pages and facts rows and saves state, and compose runs the worker restart: unless-stopped — so a rejected label meant a container destroying state every lap and processing nothing, including documents matched by valid rules. Worse, it wasn't even fail-fast: with nothing pending the same invalid config reported pass OK and exit 0, so an operator editing the ACL file got no feedback at edit time. Now validated in cli(): exit 1, the message, zero traceback frames, before any work. A CHANGELOG upgrade note follows #30's precedent.

2. CORRECTNESS — a mixed list revokes a grant that used to work. [" finance ", "sales"] served sales before and serves nobody now. The review argued for dropping just the bad label; I did not take that, because test_a_comma_inside_a_label_cannot_widen_access already pins all-or-nothing for ["finance", " "] — that behaviour is intended, not something this PR chose. Instead it is now explicitly pinned and documented for the padding case, since the consequence is visible. Availability only: it can never widen, and the page stays visible to an unrestricted operator, which is how the misconfiguration gets noticed.

3. TEST-GAP — my own eval clause overclaimed. The halves deliberately disagree on every non-str label (config validates str(a); the index refuses to coerce because str() renames a label into a grantable one), and none was in my 12-item list — so index.py's "rejects exactly these three" was contradicted three lines later by its own next paragraph. Clause renamed to string-label validity, scope documented, list widened to 16 shapes including NBSP/ZWSP/ideographic space. The review also reproduced a live crash behind that gap (audiences: [0, "sales"]",".join TypeError → document stuck status=error, retrying forever). That is a separate pre-existing defect (load_acl_config coerces default but stores rules raw); I left it out of this PR and recorded it rather than widening scope.

4. Mutation — the one survivor is killed. M2 (padding check before the comma/blank check) survived because the blank test only matched "invalid audience label"; it now asserts "must be non-empty", so check order is pinned. The fail-fast fix was also unpinned at first — a test now asserts cli() raises SystemExit and the pass never starts.

5. DOC-DRIFT — the published contract was left false. docs/pipeline/brain-page-contract.md told third-party client implementers that clean.acl rejects "exactly those" two classes. I'd updated both docstrings and forgotten the doc outsiders actually read. Fixed, including why the new class fails the other way.

State

Full CI-equivalent suite green (7 packages, ruff, evals 26/26). clean 261 tests, answer 67. Mutation set with hash-verified restores, zero survivors across 9 mutants spanning both halves, the fail-fast path, and the eval clause.

@sturlese
sturlese force-pushed the fix/bughunt-acl-padded-label branch from dc3cdcf to fed2ea7 Compare July 25, 2026 08:32
@sturlese
sturlese merged commit 126f6ec into main Jul 25, 2026
10 checks passed
@sturlese
sturlese deleted the fix/bughunt-acl-padded-label branch July 25, 2026 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant