feat(auth): deny by default on the per-channel scope axis, and page GET /uploads (BACKLOG #1152) - #832
Open
wshallwshall wants to merge 8 commits into
Open
feat(auth): deny by default on the per-channel scope axis, and page GET /uploads (BACKLOG #1152)#832wshallwshall wants to merge 8 commits into
wshallwshall wants to merge 8 commits into
Conversation
…152) Retires the NULL-means-every-channel encoding. An absent per-channel scope now denies, so a rule written against this axis protects a default install instead of resting on a premise that was false whenever nobody typed anything. Identity.allowed_channels defaults to the empty set rather than None. The resolver stops mapping a NULL channel_scope column to "all": create_user's INSERT never lists that column, so every account was minted permissive and all the per-channel checks in the API narrowed nobody on a shipped install. All-channels survives as a grant somebody typed. The token is "*" in the stored scope list, reusing the string the AD-group-to-channel map has always used for a wildcard row rather than inventing a second spelling. The AD sync persists ["*"] for a wildcard group instead of collapsing to NULL, which after this flip would have inverted a deliberate estate-wide mapping into a deny-everything one. The first administrator is not locked out: ADMINISTRATOR is all-channels by role, unchanged. api.security._SYSTEM_IDENTITY passes allowed_channels=None explicitly, since it stands in for authorization being off. First-run console handling, deliberately a page banner and not a start-time refusal (refusing to start would make a fresh single-operator install unbootable for the same condition). The landing page tells an operator with an empty scope why the lists are empty and who fixes it, read off the identity rather than off an empty result -- an estate with nothing configured yet is a different empty. The admin scope editor writes ["*"] for its all-channels mode, and the user list no longer renders an unset scope as "all", which would have told an administrator an account is wide open when it reaches nothing. Test inversion: the test that pinned an unscoped non-administrator's full access is replaced by one asserting a freshly minted non-administrator reaches nothing until granted, with the grant asserted in the same test so a regression that re-widens the default cannot pass by leaving both arms denied. Files that gate on roles or on the uploads owner axis grant the estate explicitly rather than inherit it. Corrects, rather than deletes, the two records whose stated reason this change falsifies. ADR 0134's rejection of the channel axis for uploaded files stands on its surviving clause -- an uploaded file carries no channel to match on -- and the expired clause is noted where a reader who checks it against today's code will find it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ator (BACKLOG #1152) Migrates the test corpus to the deny-by-default channel scope. Twenty-five files created a non-administrator and then acted on a channel-bearing route, relying on the permissive default to reach it; each now grants the estate explicitly at the point the fixture mints the account. The grant says what the fixture always meant. These files gate on roles, permissions, step-up, PHI budgets or the uploads owner axis, and none of them asserts anything about channel scoping -- so leaving them to inherit a second, unrelated control decides their outcomes for a reason they never state. The channel axis is exercised where it belongs, in tests/test_channel_rbac.py, whose own fixtures deliberately do NOT take this grant. Measured before and after with the same interpreter and the same file list: 68 failed / 1091 passed before the migration; the fifteen engine files that carried the clearest failures now report 379 passed, 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… item still open The uploads-pagination limb and the scorecard re-score are untouched, so reasons (2), (3) and (4) for staying open all still stand. Reason (1) is answered and now describes the pre-fix state. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…#1152) The uploaded-files listing was pageless, so one response carried every file an install had ever accumulated and its size grew with the age of the deployment. An uploads directory has no bound, and list_files decrypts a sidecar per entry, so the cost was paid on both sides of the wire. limit and offset are declared with the same bounds on the JSON route and on the console route, so the /ui door is not the looser of the two: an out-of-range value is refused at the boundary rather than clamped silently somewhere behind it. The window is applied AFTER the owner filter, and that order is the security-relevant part. Paging first would make a page's length depend on how many of another operator's files fell inside it, which turns the page size into a count of files the caller may not know exist. total keeps meaning the whole visible set. The audit row keeps its `count` key with that same meaning, so an existing reader of the trail is not re-based onto a page size without noticing; `returned`, `limit` and `offset` are added beside it. No filename and no owner: the audit of a listing is a count, not an inventory. UploadStore.list_files now sorts by (uploaded_at, file_id) rather than uploaded_at alone. A timestamp is not a total order -- files written in the same instant compare equal and the underlying directory walk is not guaranteed to repeat its order -- so without the tiebreak a tied file could appear on two pages or on none. The engine/console seam digest moves with the UploadedFileList shape. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reason (3) for staying open is answered: GET /uploads takes limit/offset with the same bounds on the /ui twin, total still counts the whole visible set, and the window is applied after the owner filter. Reasons (2) and (4) still stand, as do the four limbs this item names that nobody has built. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ad (BACKLOG #1152) The pagination row asserted it satisfied the first clause of the master test plan's exit criterion 12. That plan is vaulted -- ADR 0160 moved it, and `git ls-files docs/testing` returns exactly one file here, VERIFY.md -- so the claim was unverifiable from an engine checkout and would have had to be re-derived by every later reader. Replaced with what is true: the pagination is built and tested, and whether it closes that clause is a question only a reader holding the plan can answer. Also adds the paging bounds to the GET /uploads route-map row, and softens a completeness claim in the allowed_channels note to name one example site rather than assert it is the only one (SDS-3.6). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…umn (BACKLOG #1152) Two defects this branch introduced, both caught by the console suite. The delete and resend confirm pages read one file's metadata THROUGH the owner-scoped listing, deliberately: a file the caller may not see is simply absent and the page 303s rather than disclosing that it exists. Paging that listing broke the shape twice over. The visible break was a TypeError -- these handlers are invoked by reference across the seam, never over HTTP, so the FastAPI Query(...) defaults arrived as Query OBJECTS and reached a list slice. The quiet one would have outlived it: a first-page-only scan redirects an operator away from their OWN file the moment they hold more than a page of uploads, and reports it as not found, which is indistinguishable from the real denial. Both pages now walk every page. A regression test pins the second by shrinking the scan page rather than uploading five hundred files, since the assertion is about the loop and not about the number. The users list's Channel scope column printed the stored scope. That was already half wrong and this branch made it wrong the other way: an unset scope now denies, so it must not read "all" -- but an ADMINISTRATOR is all-channels by role whatever is stored, so printing their column would have said "(none)" beside an account holding the whole estate. Both directions matter and they fail differently: "all" for an account that reaches nothing sends someone hunting a permission bug that is not there, while "(none)" for an account that reaches everything hides real access from the person whose job is to review it. The column now resolves both inputs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… side Three conflicts, all on the SAME generated value: the engine seam digest in messagefoundry/api/_ui_seam.py, its console twin in messagefoundry_webconsole/__init__.py, and tests/golden/webconsole_seam.snapshot. NEITHER SIDE WAS CORRECT. Both branches changed the seam surface, so the merged surface has a THIRD digest. This branch read d4ae2ce03d5723a7 and main read 266cbfd342b22819; regenerating from the merged source produced 767521d4399d1ef4. Taking either side would have shipped a digest that described neither tree, and the console would have refused the engine at startup. Regenerated with scripts/webconsole_seam_snapshot.py --write, with PYTHONPATH pinned to this worktree and interpreter provenance printed first -- both packages confirmed to resolve inside this worktree. That matters here specifically: run from a worktree without an anchor the generator imports from the PRIMARY checkout and writes the primary's digest while reporting success. Main now carries the anchor fix, and the pin is belt-and-braces on top of it. The console side is not written by the tool and was set by hand in this same commit, as the tool instructs. All three now agree. Verified: 25 seam and golden-surface tests pass, and 79 pass across the auth-hardening, upload-API and uploaded-logs console legs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
wshallwshall
added
the
reviewed
github-merge-queue
Bot
removed this pull request from the merge queue due to no response for status checks
Sep 5, 2026
github-actions
Bot
added
the
ci-red
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BACKLOG #1152 stays open — four limbs it names are unbuilt, and the scorecard re-score is not this seat's act.
Limb 1 — principal narrowing
Identity.allowed_channelsnow defaults to the empty set, and_allowed_channelsresolves a NULLchannel_scopeto no channels. The item records this axis as untouched and notes that a channel-scoped rule elsewhere would protect nobody on a default install — a control resting on a false premise.All-channels survives as a typed grant: the
*token in the stored list, reusing the stringad_group_channels.channelalready uses for a wildcard row. The AD sync now persists["*"]instead of collapsing to NULL, which after the flip would have inverted a deliberate estate-wide mapping into a deny-everything one.The first administrator is not locked out. ADMINISTRATOR is all-channels by role, pinned in the same test as the denial so a regression cannot pass by leaving both arms denied. The store needed no change —
_append_channel_scopealready renders an empty scope as1=0, which was read rather than assumed. First-run handling is a landing-page banner, not a start-time refusal.Blast radius measured, not estimated: 68 failed / 1091 passed immediately after the flip across the 55 files naming a non-administrator role. Every one was a fixture relying on the permissive default; 25 files now grant the estate explicitly.
Limb 2 — pagination
GET /uploadstakeslimit/offset(50, 1..500 / 0..), same bounds on the/uitwin. The window is applied after the owner filter, so a page's length cannot encode another operator's file count.list_filessorts by(uploaded_at, file_id)— a timestamp is not a total order, so without the tiebreak a tied file could land on two pages or none.The exit-criterion clause could not be verified from an engine checkout. The item grades this limb against the master test plan's exit criterion 12;
git ls-files docs/testingreturns one file and it is not that plan. The pagination is built and tested on its own merits. No claim is made that the criterion is satisfied, because it was never read. The vault was not opened.Two defects found and fixed in this branch
TypeError(seam callers get FastAPIQueryobjects, not ints), and a quiet one that would have outlived it: a first-page-only scan redirects an operator away from their own file once they hold more than a page, reported as not found.For whoever lands this
This branch's
docs/BACKLOG.mdedits are interleaved with code commits rather than confined to a final commit. It was pushed before that convention reached this seat, and rewriting an already-pushed branch is a worse risk than the conflict it would avoid. So this is the one to expect a manual row-merge on.Checks
Interpreter provenance verified: both packages resolve inside this worktree.
test_upload_api.py26 passed; 39 engine files 762 passed 1 skipped;test_uploaded_logs_ui.py21 passed;test_webui.py252 passed; console suite plus seam, mount and doc-drift 443 passed after the two defects above were fixed and re-run.backlog_status_checkOK.ruff check,ruff format --check,mypystrict all clean.The full suite was never run in one pass locally — given the size of the fixture migration, that is the leg to read.
🤖 Generated with Claude Code