Require an agent key on /v1 and fail closed for keyless off-loopback callers - #358
Merged
Conversation
…callers
Five verified defects, two of them P0, all present in v0.15.1 and every
release before it.
**/v1 was unauthenticated even on a keyed deployment.** The middleware that
enforces identity opened with an early return for any path outside /v0/, and
the routers behind /v1 carried no auth of their own, so no mechanism existed
that could have required a key. A new path-prefix middleware now resolves the
bound user and, once that user has one active agent key, refuses every /v1
request without a valid Bearer. A user_id in the body or query naming another
user is refused rather than ignored, so a payload cannot widen privilege.
Verified against no header, empty header, bare Bearer, Basic, bare key,
comma-joined credentials, every method, and path variants that normalise
toward /v1.
**Keyless mode was reachable from any peer.** The loopback check existed but
guarded only legacy /v0 non-vnext paths. It now applies unconditionally to /v1
and /v0/vnext, before any database work. X-Forwarded-For still counts only
when the peer is a configured trusted proxy. The gate is deliberately not
conditional on APP_ENV or APP_HOST: settings can disagree with the real bind,
and a fail-closed control derived from a value that can be wrong is not
fail-closed.
APP_RELOAD now defaults false, in local_server and in the systemd unit, with
the Makefile still able to opt in. The production advisory audit exits 0 again
after moving nanoid to 3.3.18 and postcss to 8.5.26; no production exception
was added.
Importers no longer follow links out of the selected root. A new
importer_paths module walks with followlinks=False, refuses symlinked
directories and candidate files, and opens each file once with O_NOFOLLOW.
One defect in that module was found by its own test and is worth naming: the
docstring promised the descriptor was checked to be a regular file before any
bytes were consumed, and the check was absent, leaving a bare pass. A FIFO
under an import root was listed, opened, and imported as an empty document,
and on a platform where the read-open parks it would hang the import instead.
The check is now performed with fstat on the descriptor rather than the path,
so a swap between listing and open cannot change what is measured.
Test client hosts move from ("testclient", 50000) to ("127.0.0.1", 50000)
across 51 files. Those tests drive the app in process from loopback; the
synthetic host made them describe something they were not, and under an
unconditional gate it also made two files pass whether or not the mechanism
they name was working. Three occurrences already used the realistic form.
Unit 4539 passed 2 skipped. Integration 408 passed 1 skipped. ruff and mypy
clean across 220 files. Web test, typecheck, lint, build and budget all pass.
Mutants: bypassing the /v1 middleware fails 25 tests, neutralising the
loopback gate fails 8, and removing the regular-file check hangs on a FIFO,
which is the failure it exists to prevent.
No version change. No MCP tool names, OpenAPI operation IDs, RLS SQL, public
error vocabulary or core registry entries were altered.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
added 2 commits
August 14, 2026 12:38
Two advisories published after the v0.15.1 release fail the full dependency audit on main as well as on this branch, so they are not caused by the hotfix. GHSA-rgw5-rvv9-x895 states explicitly that it bypasses the CVE-2026-14257 mitigation the standing exception relied on, and GHSA-5p4m-2wfm-xmqj covers js-yaml 4 below 4.3.1. The standing exception was written when no patched release existed inside the 1.x and 2.x lines, so the only route to a fix was major 5, which forces minimatch 10 and breaks eslint and the vitest coverage provider. That is no longer true: the maintainers backported the fix within each major line. Pinning per line keeps every consumer on the version it already resolves to and changes no majors. brace-expansion@1 1.1.18, @2 2.1.4, @5 5.0.9 js-yaml@4 4.3.1 The two stale range-based rules are removed, since a rule written as a range stops applying the moment the tree moves past it and that silence reads the same as a fix. The exception itself is deleted rather than left to expire. Its stated blocker no longer exists, and an exception that outlives its reason is how the next finding gets hidden. Verified on the resolved tree: full audit 553 packages, 0 matching advisories; production audit 93 packages, 0 matching advisories; both with zero exceptions in force. The two gates the exception predicted would break, vitest coverage and eslint, both pass, along with typecheck, build and the budget check.
An external audit of this branch returned four P1 blockers. Reproducing each one against a real Postgres and against shipped v0.15.1 confirms every mechanism it describes, downgrades all four, and blocks none of them. The reason is the baseline it did not measure. On v0.15.1, in production configuration, from an off-loopback peer with no credential at all, the same three mutations succeed: workspace bootstrap, provider registration, memory supersession. This branch requires a minted key for them. It permits nothing that was previously refused, so the /v1 authorization gap is an incomplete new control rather than a regression, and holding the hotfix would keep anonymous remote writes live to avoid authenticated-key overprivilege on a surface that is loopback-only by documented contract. Two of the four findings, importer ancestor swaps and the OpenClaw archive divergence, reproduce identically on v0.15.1 and predate this work; importer_paths.py does not exist there. Three things were worth fixing. OpenClaw selection now runs on the directory listing rather than on text that has already been read. An unrelated neighbour is no longer opened or decoded, so one stray .json with undecodable bytes stops failing an import that never needed to look at it. It also makes the archived set the set the parse is handed, so the parse can no longer reach content that was never archived. A 63-layout matrix across v0.15.1, this branch, and the fix found no layout where an import that previously succeeded now imports less. The decode is now inside the error contract. A bad byte leaves as the caller's own validation error naming the file instead of a bare UnicodeDecodeError carrying a byte offset and no path. The original survives as __cause__. The middleware docstring claimed the resolved key "is the actor for that request". That was untrue: nothing outside this branch's own tests reads the resolved identity, and no /v1 write records an agent. It now says plainly that /v1 authenticates and does not authorize, that any valid key can do what any other valid key can, that a revoked key's writes are out of reach of an agent-keyed quarantine sweep, and that /v1 must stay loopback-only. The same retracted sentence about archived bytes survived in three importers and two test comments. All are corrected together, and they say text rather than bytes, because the read applies universal newlines and a CRLF source archives as LF. Both new tests were checked by reverting each fix and confirming they fail. The two EXPECTED_CARRIER_AST_SHA256 receipts move because the docstring is inside the hashed AST. Before updating them the AST of every top-level definition in main.py was diffed: exactly one changed, and only its docstring constant. No logic moved. Verified: 4541 unit passed, 408 integration passed, ruff and mypy clean.
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.
Five verified defects, two of them P0, all present in
v0.15.1and every release before it. Each was confirmed by execution before any code was written.P0:
/v1was unauthenticated even on a keyed deploymentenforce_authenticated_user_identityopened with an early return for any path outside/v0/, androuters/providers.py,continuity.pyandworkspaces.pycontained zero auth references. No mechanism existed that could have required a key.A new path-prefix middleware resolves the bound user and, once that user has one active agent key, refuses every
/v1request without a validBearer alice_sk_.... Auser_idin body or query naming another user is refused rather than ignored, so a payload cannot widen privilege.Attacked without success: no header, empty header, bare
Bearer,Basic, bare key, comma-joined credentials, every HTTP method, every client host, cross-user claims in query and body, and path variants (//v1/x,/./v1/x,/V1/x) which resolve to no route at all.P0: keyless mode was reachable from any peer
_request_client_is_loopbackexisted but was called once, guarding only legacy/v0non-vnext paths. It now applies unconditionally to/v1and/v0/vnext, before any database work.X-Forwarded-Forstill counts only when the peer is inTRUSTED_PROXY_IPS.The gate is deliberately not conditional on
APP_ENVorAPP_HOST. Those can disagree with the real bind —uvicorn --host 0.0.0.0by hand is enough — and a fail-closed control derived from a value that can be wrong is not fail-closed.Three P1s
APP_RELOADdefaults false inlocal_server.pyand in the systemd unit, withAPP_RELOAD=true make devstill opting in. The production advisory audit exits 0 again afternanoidto 3.3.18 andpostcssto 8.5.26; no production exception was added. Importers no longer follow links out of the selected root.A defect found by its own test
importer_paths.read_contained_source_textdocumented that "the descriptor is checked to be a regular file before any bytes are consumed". That check was absent — a barepasssat where it belonged.A FIFO under an import root was listed, opened, and imported as an empty document. On a platform where the read-open parks, it would hang the import. Now checked with
fstaton the descriptor rather than the path, so a swap between listing and open cannot change what is measured.Disabling that check makes the containment suite hang, which is the failure mode it prevents.
The 51 test client hosts
("testclient", 50000)becomes("127.0.0.1", 50000). Those tests drive the app in process from loopback; the synthetic host described something they were not, and under an unconditional gate it also made two files pass whether or not the mechanism they name was working. Three occurrences already used the realistic form.Verification
/v1middleware bypassedNo version change. No MCP tool names, OpenAPI operation IDs, RLS SQL, public error vocabulary or core registry entries altered.
Upgrade Overview
Protected Areas
Touched:
apps/api/src/alicebot_api/main.py.Compatibility Impact
This is a breaking change for any deployment relying on the defects.
A keyed deployment that called
/v1without a Bearer now receives 401. A keyless deployment reachable from a non-loopback peer now receives 401 on/v1and/v0/vnextregardless of environment. Both were the documented intent and neither was enforced.The operator web console is unaffected: it makes zero
/v1calls and already attaches a Bearer with the operator key.Importers now hard-fail on symlinked directories, symlinked candidate files, and non-regular files under an import root. Anyone symlinking notes into a folder they import will see a refusal rather than a silent import.
APP_RELOADdefaults false, so a process relying on the previous true default no longer reloads on edit.No database migration. No schema change. No API surface change: no route added or removed, no OpenAPI operation ID altered, no security scheme added.
Migration / Rollout
No migration. Deploy the version.
Before rolling out, confirm the deployment has at least one active agent key if anything calls
/v1, and that remote callers present a Bearer. A deployment behind a reverse proxy must have the proxy peer inTRUSTED_PROXY_IPSforX-Forwarded-Forto be honoured; otherwise the proxy's own address is the peer and keyless remote calls are refused.Operator Action
Mint an agent key if one does not exist:
If an ingestion begins failing with a refusal about symlinks or non-regular files, a link or a FIFO is present under the import root. That is the guard working; inspect the path rather than reverting.
If a development process stops reloading on edit, set
APP_RELOAD=trueexplicitly.Validation
Unit and integration suites both green against a migrated PostgreSQL with role separation. Three mutants confirm the new controls are load-bearing rather than decorative: bypassing the
/v1middleware fails 25 tests, neutralising the loopback gate fails 8, and removing the regular-file check hangs on a FIFO.The two claims this release makes were attacked directly and held:
/v1is unreachable without a valid key on a keyed deployment, and keyless is loopback-only.Rollback
Revert the merge. There is no migration and no persisted state, so the previous behaviour returns immediately.
Reverting restores an unauthenticated
/v1on keyed deployments and removes the keyless loopback restriction. Prefer fixing forward: if a legitimate caller is being refused, the correct response is to give it a key or to configureTRUSTED_PROXY_IPS, not to reopen the hole.