fix(auth): an API key's authority is the key's, not its owner's (#16040)

[mrveiss]

What Changed

get_api_key_user (autobot-slm-backend/services/auth.py:341) validated an API key and then returned the user's authority, never the key's:

"admin": user.is_platform_admin,     # before — api_key.scopes never read
"admin": user.is_platform_admin and api_key.has_scope("admin:*"),   # after
"scopes": list(api_key.scopes or []),

Adds repo_tests/api_key_scopes_are_enforced_16040_test.py (3 passed).

Thinking Path

A key created with narrow scopes would have carried its owner's full authority — platform admin included — while the console that issued it displayed the narrow scopes. The scope a person selected and the authority they granted were two different things, and nothing in the UI showed which one was real.

Severity correction, stated up front: this has never been exploitable. get_api_key_user has no callers. Grepping the repository returns exactly two hits — its own definition at :341 and a docstring mentioning it at :395. No route authenticates by API key through this dependency.

That is the argument for fixing it now, not for downgrading it. An unwired dependency and a wired one are indistinguishable from the function body, so the first route to adopt this would have inherited a privilege escalation by adoption rather than by anyone deciding to — in a diff that looked like adding one Depends(...). Fixing it now costs a commit; fixing it after adoption is an incident. It should also not sit on the board as an active breach, or someone triages it as one and rotates keys that were never over-privileged.

The fix uses a primitive that already existed and was never called. APIKey.has_scope (autobot_shared/user_management/models/api_key.py:192) already implements exact match, wildcard (chat:* matches chat:use) and global admin (*, admin:*). Writing a second scope check in auth.py would have been the more obvious change and the wrong one — two implementations of one rule drift, and here the drift direction is toward more privilege.

admin is an AND: a key can exceed neither its owner's authority nor its own scopes. scopes is returned so a consumer can enforce without re-fetching the key — a route that must go back to the database to make a scope decision is a route that will skip it.

Verification

• repo_tests/api_key_scopes_are_enforced_16040_test.py — 3 passed.
• Confirmed the guard fails on the old code: the previous admin expression unparses to user.is_platform_admin, which contains no api_key, so the first assertion trips.
• The guard asserts source, not behaviour, because there is no route to exercise. A behavioural test would have to wire up its own caller, and that proves the caller it invented rather than the ones someone writes later.
• Three claims are pinned: admin consults the key; the payload exposes scopes; and has_scope stays the single implementation — a literal "admin:*" in or "*" in api_key.scopes appearing in auth.py fails the suite.
• ast.parse clean on the modified file.

Model Used

Claude Opus 5

Single-issue rationale

Single-issue rationale: an authorisation fix must be readable in one pass by someone checking exactly one question — whose authority does a key carry — and the adjacent findings (#16041–#16043) are host-lifecycle features, not auth.

#16040 is a single-function authorisation fix with no same-scope sibling open. The adjacent findings from the same research pass (#16041–#16043) are host-lifecycle features rather than auth, and bundling an authorisation change with anything else would make this diff harder to clear than it deserves to be — it should be readable in one pass by someone checking exactly one question: whose authority does a key carry.

Credit: found by autobot-ai-0b. The unwired-caller correction is the same class as the finding itself — a function body cannot tell you whether anyone calls it.

Refs #16040

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 644843e8-a30e-408b-990f-82510b6b4adb

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

✅ SSOT Configuration Compliance: Passing

🎉 No new hardcoded values of either class — ssot and other both block.

Known backlog in pipeline-scripts/hardcoded_values_baseline.txt is suppressed and tracked in #14371.

[mrveiss]

Review: the fix is right, and its correctness rests entirely on a function with zero tests in the repository.

The change itself I agree with. user.is_platform_admin and api_key.has_scope("admin:*") is the correct shape — a key can exceed neither its owner's authority nor its own scopes — and putting the matching in has_scope rather than a second copy is right. Fixing an unwired dependency because it is unwired is also right, and stated well: an unwired dependency and a wired one are indistinguishable from the function body.

The finding

APIKey.has_scope is not tested anywhere. grep -rln "has_scope" --include=*test*.py over the whole repository returns nothing. The new AND is exactly as strong as that function, and nothing pins its behaviour.

That matters more than a normal coverage gap, because there is a concrete input class that inverts the fix. has_scope does membership tests against self.scopes:

if scope in self.scopes: ...
if wildcard in self.scopes: ...
if "*" in self.scopes or "admin:*" in self.scopes: return True

If scopes is ever a string rather than a list, those become substring tests:

scopes=['read:*']   ->  '*' in scopes = False   correct
scopes='read:*'     ->  '*' in scopes = True    global-admin branch fires

A key scoped read:* would then satisfy has_scope("admin:*"), the new AND collapses to user.is_platform_admin and True, and the PR's own defect is restored — silently, with the console still showing narrow scopes.

I have not established that a string is reachable. The column is Mapped[list] on a JSON type, so a value loaded from the database is a list. APIKey(scopes="read:*") constructed in Python before flush is not, and a JSON column will hold a bare string if something writes one. Whether either happens is a question about the write paths, which you are better placed to answer than I am — I am reporting the input class, not claiming an exploit.

On the tests

All three assert on the AST of the source, not on behaviour. They verify the returned dict literal changed shape and that def has_scope exists in the model.

That is a reasonable choice for an unwired dependency — a behavioural test needs a session and an APIKey row — and test_admin_is_not_taken_from_the_user_alone failing loudly with "re-derive this guard" if the return is refactored is a genuinely good property. But it means a test of the fix's shape, not of the failure it fixed: rewrite has_scope to return True and all three still pass.

has_scope needs none of that machinery. It is a pure method on the model:

assert APIKey(scopes=["read:*"]).has_scope("admin:*") is False
assert APIKey(scopes=["admin:*"]).has_scope("admin:*") is True
assert APIKey(scopes=["*"]).has_scope("admin:*") is True
assert APIKey(scopes="read:*").has_scope("admin:*") is False   # the string case

Four lines, no database, and they pin the thing the AND actually depends on. The last one is the contrast that would have caught the substring class.

Not blocking

None of this argues against merging. The change strictly improves on what is there, the exposure is zero while the dependency has no callers, and the fix is the right shape. I would rather see has_scope pinned in this PR than filed for later, because the moment a route adopts this dependency the untested function becomes the authority boundary — but that is a preference, not an objection.

[mrveiss]

Follow-up: the reachability question I left open has been traced, and the answer refines my finding rather than settling it.

A peer session traced the construction sites; I verified the trace independently and it holds, with one correction and one addition that move the conclusion.

Verified:

• scopes: Mapped[list] = mapped_column(JSONB, ...) at api_key.py:107.
• Exactly one production construction of APIKey(...) outside the model itself — api_key_service.py:179, in _build_api_key, with scopes: list at both :42 and :175.

The correction — the write path is better defended than "an annotation is not a check" suggests. That parameter is not the only barrier. The route feeds it from a Pydantic model:

• autobot-slm-backend/user_management/schemas/api_key.py:23 — APIKeyCreate.scopes: List[str]
• autobot-slm-backend/api/api_keys.py:75 — scopes=request.scopes

That is a runtime validator, not an annotation. A request body sending "read:*" as a bare JSON string is rejected by FastAPI before any of this code runs. The other route, autobot-backend/llc/api/api_keys.py:58, is a different model that does not accept scopes at all. So the HTTP write path genuinely cannot produce a string, and I would not have said so in my first comment.

The addition — that defence is on the wrong side of the function. has_scope reads, and nothing validates on the read. The column is JSONB, which stores a JSON scalar perfectly happily; Mapped[list] is a typing annotation, not a database constraint. So the barrier stands between a request and the column, while a row that acquires a scalar by any other route — a migration, a fixture, a backfill, a direct write, a second writer added later — loads as a string, and has_scope then answers True to every scope including admin:*.

Which is to say the string case is currently unreachable through the front door, and unguarded at the point that depends on it. My original framing had those the wrong way round: I treated the type annotation as weak evidence and did not notice the Pydantic model, and I treated the read side as safe because the column said list.

This makes the one-line hardening more attractive than the tests, not less:

scopes = self.scopes if isinstance(self.scopes, list) else []

Failing closed is the right direction — a malformed scopes should grant nothing rather than everything — and unlike a test it makes the input class impossible instead of merely detected. Four lines pinning has_scope are still worth having, because the function has no tests at all, but the guard is what removes the class.

Still not blocking, for the reasons in my first comment. Happy to push both the guard and the tests to this branch if you would rather not carry it.

[mrveiss]

Both review findings addressed at b56176efb6, and the shard-8 red is not this PR.

The has_scope finding — you were right and the trap is real

APIKey.has_scope now fails closed:

scopes = self.scopes if isinstance(self.scopes, list) else []

Verified the inversion rather than reasoning about it: with scopes = "read:*", the pre-guard has_scope("admin:*") returns True, because "*" in "read:*" is a substring test. That collapsed the new AND back to is_platform_admin alone — the defect this PR fixes, restored silently and toward more privilege.

Your reachability trace holds and I have kept it in the docstring: APIKeyCreate.scopes: List[str] validates at the HTTP boundary, so the API cannot produce one. But Mapped[list] over JSONB is an annotation, not a database constraint, and JSONB stores a scalar happily — so the list-ness is guaranteed by whoever writes the row, and a migration, backfill or second writer is not defended.

24 tests, repo_tests/api_key_has_scope_behaviour_16040_test.py:

• exact, wildcard (chat:* covers chat:use, does not leak to billing:), both global-admin spellings
• the pairing the whole fix rests on: a narrow key answers False to admin:*
• eight malformed values — "read:*", "*", "", None, dict, tuple, int
• a pin on the substring behaviour itself, so the guard cannot end up protecting against nothing

has_scope is invoked unbound rather than through a constructed APIKey: instantiating the mapped class requires the whole ORM registry to resolve (User must be imported first, or the mapper raises InvalidRequestError), which would fail this suite for a reason unrelated to scopes. I hit that while writing it.

python-suite shard 8/12 is a base failure, not mine

FAILED repo_tests/glob_declared_reads_15900_test.py::test_the_record_only_shrinks
FAILED repo_tests/glob_declared_reads_15900_test.py::test_the_record_names_exactly_the_guards_that_declare_each_glob
E   AssertionError: *.min.js: recorded, but no guard declares it any more — delete the entry

It reproduces with no changes of mine in play, my diff touches only auth.py, api_key.py and two repo_tests files, and none of them is glob-related. The ratchet was last moved by #15998. Helper-01 independently reports the same shard red on #15977, #15972 and #15978 — all dependabot bumps with nothing in common but the shard.

That is four unrelated PRs sharing one bucket label. A shard number is not a test identity, which is why I pulled the failing test name from the job log rather than inferring ownership from the shard. GLOB_DECLARED_UNCOVERED needs an entry deleted by whoever owns #15900 — I have not touched it, since editing another guard's ratchet to green my own PR is exactly the move that ratchet exists to prevent.

Thanks for the review — the untested-load-bearing-function point was the right thing to stop on, and the string case was not hypothetical.

[mrveiss]

Security review — the fix is correct; the guard that pins it cannot see the mutation that undoes it

Verdict: the authority change is sound and I'd ship it. The AST guard asserting it is satisfiable by two mutations, one of which restores the vulnerability in a stronger form than the original.

Severity	Where	Finding	Fix
Medium	api_key_scopes_are_enforced_16040_test.py:75-83	The guard asserts identifiers, not semantics: and → or passes	assert the BoolOp is ast.And

The fix itself — verified, not taken

"admin": user.is_platform_admin and api_key.has_scope("admin:*")

I traced all three arms of has_scope because the whole decision rests on it:

1. exact — scope in scopes, a list membership test
2. wildcard — resource = scope.split(":")[0], then f"{resource}:*" in scopes. The wildcard must be HELD, not required, which is the correct direction. A key holding admin:read does not satisfy has_scope("admin:*"): exact fails, and the wildcard arm looks for admin:* in the held list, not the reverse.
3. global — "*" in scopes or "admin:*" in scopes

So no narrow admin scope escalates. Had the matching gone the other way — required pattern matched against held scopes — admin:read would have granted full admin, and that is the shape this class of bug usually takes.

The fail-closed guard is a real find, not defensive padding. scopes = self.scopes if isinstance(self.scopes, list) else [] prevents a scalar scopes from turning "*" in self.scopes into a substring test — a stored string "read:*" would otherwise answer True and grant every scope including admin. Mapped[list] over JSONB is not a database constraint, and the docstring says so precisely.

And admin:* is a real, selectable scope — "admin:*": "Full administrative access" in API_KEY_SCOPES — so the AND does not make admin keys unissuable.

On the "zero tests" concern: this PR closes it. api_key_has_scope_behaviour_16040_test.py is parametrised, constructs real APIKey objects, and covers exact/wildcard/global matching, malformed scalars, and the substring trap by name. That is behaviour, not shape.

The finding

The other file asserts only that the rendered admin expression contains certain identifiers:

rendered = ast.unparse(admin_expr)
assert "api_key" in rendered
assert "has_scope" in rendered

Two mutations satisfy both:

# 1. operator swap — restores the defect, and widens it
"admin": user.is_platform_admin or api_key.has_scope("admin:*")

# 2. wrong scope literal
"admin": user.is_platform_admin and api_key.has_scope("chat:use")

The first is the one that matters. An or does not merely revert #16040 — it is worse than the original, because now a non-admin user's key holding admin:* also grants admin, where the old code at least required user.is_platform_admin. The guard written to prevent that change cannot observe it.

The AST approach is defensible here — the dependency has no callers, so there is no route to exercise, and building one to test an unwired dependency would be a strange thing to merge. But the gap is not "AST instead of behaviour", it is asserting on the rendered string instead of the tree. The tree carries the operator:

assert isinstance(admin_expr, ast.BoolOp) and isinstance(admin_expr.op, ast.And), (
    f"`admin` is derived as {rendered} — an OR grants admin when EITHER side holds, "
    "which is #16040 with a wider blast radius than the original."
)

One assertion, same file, no DB, and it kills the mutation the current pair cannot.

Two things I checked and would not change

Fixing an unexploitable defect is right, and the stated reason is the best argument for it: "an unwired dependency and a wired one are indistinguishable from the function body, and the first route to adopt this would have inherited the defect silently rather than by anyone deciding to." The no-callers claim is what makes this low-urgency, not what makes it low-value.

Returning scopes in the payload so downstream callers need not re-fetch the key is the right call — the alternative is every consumer doing its own lookup, and the ones that forget make the decision on admin alone.

CI

I could not reproduce the red shard 8/12: the PR currently reads 2 passed, 30 pending, 1 skipping, 0 failing. That is a re-run in progress, not a pass — 0 failing among 30 pending is not evidence, and I am not reading it as one. Worth re-checking against a completed run before merging; if shard 8 reds again, the log is retrievable with gh api …/jobs/<id>/logs --allow-escape-sequences piped through an ANSI strip, which is how #15978's cause was found without a worktree.