fix(acl): a scope that parses to no labels served the whole corpus#37
Conversation
`ANSWER_AUDIENCES=","` made the instance UNRESTRICTED. The parse was
audiences=tuple(a.strip() for a in aud.split(",") if a.strip()) or None if aud else None
and `or None` turns the empty tuple into "no scope", so every value that looks
like it carries labels but carries none — ",", ",,", " , ", "\t,\t" — resolved to
the open corpus while the operator believed the instance was scoped. Measured: with
ANSWER_AUDIENCES=",", search_text, metrics_text and ask all return the restricted
payroll document, its figure and a citation to it. One template rendering an empty
audience list is enough to expose everything.
ADR 010 already states the rule, for the write side: "A malformed config errors
loudly: silently-open is the one failure mode an access-control file must not
have." The read side did the opposite. It now raises, matching the ANSWER_LLM
fail-fast from #32:
invalid ANSWER_AUDIENCES: ',' (no audience labels; unset it for an
unrestricted instance)
An unset — or purely blank, which is indistinguishable from unset after strip() —
scope still means the documented open corpus. What is refused is a value that
carries separators but no labels.
Second locus, same mistake in code rather than config: service.__init__ read the
scope with `if settings.audiences`, so an explicit `Settings(audiences=())` was
also silently unrestricted. Now `is not None`, which makes an empty tuple an EMPTY
scope — a client holding no labels, able to see open content and nothing else. That
is the same distinction index.visible already draws between acl='' and acl=None,
and the one #30 was about; it should hold on the client side too.
Grepped the rest of the package: every other scope test is already `is None` /
`is not None`.
Docs: docs/answer.md records that being unrestricted is only ever explicit, and
answer/index.md's "use these" entry now says to test the scope with `is None` and
never truthiness — the mistake was cheap to make twice, so it is worth naming where
a contributor will read it.
answer 52 (+2). scorecard 25/25, ruff clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…wo false claims
Gate review found something larger than the bug this PR set out to fix, plus two
statements of mine that are simply untrue.
**The documented deployment could not be scoped at all.** answer/docker-compose.yml
lists six keys under `environment:` and has no `env_file:`, and ANSWER_AUDIENCES was
never among them — so `cd answer && docker compose up -d` produced an unconditionally
UNRESTRICTED server no matter what the operator set, and the startup guard added in
the previous commit could never fire there. The ACL layer was inert in the one
deployment the docs describe: a strictly worse instance of "the operator believes
the instance is scoped". Now passed through as ${ANSWER_AUDIENCES:-}, verified:
$ ANSWER_AUDIENCES=finance docker compose -f answer/docker-compose.yml config
ANSWER_AUDIENCES: finance (before: the key was absent entirely)
**My rationale for leaving blank alone was wrong.** I wrote that a blank value is
"indistinguishable from unset" (it is not — `"ANSWER_AUDIENCES" in os.environ` tells
them apart) and that "," is what a template rendering an empty list produces (it is
not — `",".join([])` is `""`, which lands in the case I left open). The code choice
survives, for a different and better reason now that the var is wired: compose
always sets it to "", so raising on blank would stop an unconfigured stack from
booting, and "empty = unrestricted" is the documented contract. Blank is therefore
the ONE implicit path to an open corpus, deliberately. The comment says that, and
docs/answer.md no longer claims "being unrestricted is only ever explicit", which
was false as shipped.
**A broken implementation passed both of my new tests.** `set(settings.audiences or
())` — which collapses None into an empty scope and takes every unrestricted
deployment dark — satisfied them, because neither pinned the None direction; only an
unrelated pre-existing test caught it. The empty-scope test now asserts both
directions over the file's own _LEAK_PROBES table (all 10 surfaces, rather than the
4 I hand-picked while that harness sat 90 lines above). Verified against that mutant
and against the original truthiness bug.
Also: eval_contract_parity's truth table had no empty-audience case, though this PR
is what makes `set()` reachable. Added (None, set()), ([], set()), (["sales"],
set()) — 8 -> 11 cases, passing, so the mirrored halves are proven to agree on the
input class this PR introduces.
Recorded, not fixed (pipeline side, its own bug): clean's acl._check_labels rejects
commas and blanks but not surrounding whitespace, so a config saying
`"audiences": [" finance "]` mints a label no ANSWER_AUDIENCES value can ever match
— silently over-restrictive, the mirror image of this one.
answer 52. parity 11/11, scorecard 25/25, ruff clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Gate verdictOne adversarial reviewer, told to refute. VERDICT: WEAKENED — and it found something larger than the bug this PR set out to fix, plus two claims of mine that are false. The bigger finding: the ACL layer was inert in the shipped deployment
That is a strictly worse instance of this PR's own failure mode — the operator believes the instance is scoped — and I was editing that deployment's ENV table to describe the new error without noticing the variable never reached the container. Now passed through as Two of my claims were false
The code choice survives, for a better reason now that the variable is wired: compose always sets it to A broken implementation passed both of my new tests
The reviewer's full mutation table, all six now discriminating: reverting either fix locus fails; raise-on-every-value fails; return- Also addressed
Confirmed soundThe raise is at startup ( Recorded, not fixed
|
Fourth iteration of the autonomous
bughuntsweep. This was the most severe item found so far — surfaced by the full ACL audit of theanswerpackage that #36's gate ran, after that package produced its third leak in three passes.Bug
ANSWER_AUDIENCES=","made the instance unrestricted.or Noneturns the empty tuple into "no scope", so every value that looks like it carries labels but carries none resolved to the open corpus:With
","set,search_text,metrics_textandaskall return the restricted payroll document, its figure and a citation to it — while the operator believes the instance is scoped. One template rendering an empty audience list is enough.Root cause
ADR 010 already states the rule — but only for the write side:
The read side did exactly the opposite. And the mistake is the same one PR #30 fixed one layer down: conflating "an empty set of labels" with "no ACL at all". #30 fixed it for a document's
acl; it was still live for a client's scope.Fix
Two commits: the fix, and a round the gate sent back (detail in the gate-verdict comment). The gate's largest finding is now part of this PR:
The ACL layer was inert in the shipped deployment.
answer/docker-compose.ymllists six keys underenvironment:and has noenv_file:—ANSWER_AUDIENCESwas never among them. Socd answer && docker compose up -d, the deploymentdocs/answer.mddocuments, produced an unconditionally unrestricted server no matter what the operator set, and the startup guard below could never fire there. A strictly worse instance of this PR's own failure mode. Now passed through as${ANSWER_AUDIENCES:-}, verified renderingANSWER_AUDIENCES: financewhere the key was previously absent entirely.Config locus —
from_envnow refuses a value that yields no labels, matching theANSWER_LLMfail-fast from #32:A blank scope still means the documented open corpus. What is refused is a value carrying separators but no labels.
My first rationale for that split was wrong on both halves, and the gate caught it: blank is not indistinguishable from unset (
"ANSWER_AUDIENCES" in os.environtells them apart), and","is not what an empty-list render produces (",".join([])is""— the archetypal case my fix missed). The choice survives for a better reason now that the variable is wired: compose always sets it to"", so raising on blank would stop an unconfigured stack from booting. Blank is therefore the one implicit path to an open corpus, deliberately — anddocs/answer.mdno longer claims "being unrestricted is only ever explicit", which was false as shipped.Code locus, same mistake —
service.__init__read the scope withif settings.audiences, so an explicitSettings(audiences=())was also silently unrestricted. Nowis not None, which makes an empty tuple an empty scope: a client holding no labels, able to see open content and nothing else. That is the distinctionindex.visiblealready draws betweenacl=''andacl=None— it should hold on the client side too.Grepped the rest of the package: every other scope test is already
is None/is not None.Test
Both confirmed to fail with the source change stashed.
test_a_scope_that_parses_to_no_labels_is_refused— six separator-only values raise; four genuinely-blank/unset forms still give the documented open corpus; stray separators (" ,finance,") still parse.test_an_explicitly_empty_scope_is_empty_not_unrestricted—audiences=()yieldsset()rather thanNone, and is checked in both directions over the file's own_LEAK_PROBEStable (all 10 read surfaces): an empty scope reaches no labeled content but still serves open content, andNonestill reaches everything. That second half exists because the gate showed a broken implementation passing my first version —set(settings.audiences or ()), which collapsesNoneinto an empty scope and takes every unrestricted deployment dark, satisfied it, since nothing pinned theNonedirection.eval_contract_paritygains the empty-audience cases ((None, set()),([], set()),(["sales"], set())) — 8 → 11. The truth table had no such case although this PR is what makesset()reachable, so the mirrored halves are now proven to agree on the input class it introduces.answer 52 (+2) · parity 11/11 · clean 252 · corpus 48 · graph 40 · slack 13 · fetch 33 · benchmark 3 — bare
pytest(CI mode). ruff clean; golden scorecard 25/25. Full CI-equivalent suite green locally before opening.Docs
docs/answer.mdnow states that there is exactly one path to an open corpus — a blank scope — and that the compose default is blank, so the ACL layer does nothing until you set it.answer/index.md's "use these" entry says to test the scope withis Noneand never truthiness; the mistake was cheap enough to make twice in one file, so it is worth naming where a contributor will read it.Still open from the same audit
Three more, each its own fix:
clean.acl._check_labelsaccepts whitespace-padded labels (found by this PR's gate) — it rejects commas and blanks but not surrounding whitespace, so"audiences": [" finance "]mints a label noANSWER_AUDIENCESvalue can ever match. Silently over-restrictive: the mirror image of this bug, on the pipeline side.And the two "silently open" items still standing from #36's audit:
acl:value is encoded as "no ACL" and served open (index.py:128-137) —acl: finance(the intuitive single-label form),acl: '',acl: nullall fall through toNone. This is fix(contract): a model-chosen type/date could drop a page's ACL or kill the build #34's fix being incomplete: it handled an unreadable block but not a readable block with an unreadable value.index.py:129) —clean/acl.pyrejects commas because labels are CSV-serialised;answerre-parses the page text with no such check, soacl: ["finance,eng"]grants both.Both live in the same
refreshencoding block and will go together.Worth noting across four iterations: this is the fourth "an unreadable or degenerate ACL input resolves to open" bug in this package (#30 empty acl, #34 unparseable frontmatter, the two above, and now the client scope). Each fix has been local and correct, but the recurrence suggests the enforcement wants to be structural — a single place that decides visibility from a validated scope, rather than each call site deriving it — which is an architectural call rather than a bugfix.