fix(auth): the browser-JWT branch now carries username, email and role

[lilyshen0722]

middleware/auth.ts dispatches on the token prefix and leaves two different shapes on req.user:

branch	assigns
cm_ API token (:51)	{ id, username, email, role }
browser JWT (:81)	{ id }

Nothing errors. Consumers just read undefined for every browser session.

Two live consequences, both confirmed at 799e0d7d

• github.ts:146 — if (req.user?.role !== 'admin') return res.status(403). A genuine admin in a browser session is refused 403 Admin only; the same admin holding an API token gets through. Fails closed, loudly.
• registry publish/install — publisher.name persists as undefined. Fails open, silently. This is fix(registry): publisher.name is undefined for every browser-session publish #1211's defect.

Why one fix rather than two

Three call sites had already grown a private DB re-read to work around this, each with its own comment rediscovering the same cause:

• podController.isGlobalAdminRequest (:43)
• agentProfile.canEditAgentAvatar (:250) — "JWT auth populates req.user = { id } WITHOUT role (middleware/auth.ts:81)"
• marketplace-api.resolveUsername (:11) — which fix(registry): publisher.name is undefined for every browser-session publish #1211 copies into registry/helpers.ts under a comment saying it was moved

github.ts is the fourth site, and the one that never got its copy. Fixing it locally would make four.

The JWT branch already runs one indexed User.findById(id) per request for ban enforcement (#636). Widening that projection from banned to banned username email role makes the branches shape-identical at zero extra round-trips, so no consumer has to know which token type it was called with. The three existing local re-reads stay correct and become redundant fast-paths — removing them is separate work, deliberately not in this PR.

On the test

It drives the real middleware over a real JWT. The suites covering the affected routes cannot see this defect: their fake auth middleware injects req.user = { _id, role } directly — the API-token shape — so #809's /status tests pass today against the broken predicate.

The User mock honours the projection string, and that is load-bearing rather than tidiness. With a naive select() that ignores its argument and returns the whole fixture, narrowing the middleware back to .select('banned') leaves all seven tests green — the fields arrive from the fixture, not from the query. Verified by mutation, both ways:

mutation	result
.select('banned username email role') → .select('banned')	4 of 7 red
req.user = { id, username, email, role } → req.user = { id }	4 of 7 red

The CONTROL: case stays green under both, by design — it guards against over-granting (role: 'admin' hardcoded), not against the defect.

a user row without a role leaves role undefined, not defaulted pins that absence stays absent: a default here would grant or deny on data the row does not contain.

Verification

• backend/__tests__/unit/middleware/ — 38/38, 7 suites
• backend/__tests__/unit/routes + controllers — 650/650, 99 suites
• tsc --noEmit — no errors in either touched file (pre-existing errors in scripts/ are untouched)
• npm run lint — the two remaining errors on the new file are the repo-wide import/no-unresolved + import/extensions pattern that every sibling middleware test carries

Not verified: no runtime exercise of /api/github/status or the registry routes against a live browser session — both findings are source-level plus the middleware unit test. I also did not count how many registry rows actually lack publisher.name; no Mongo read path from this seat.

Relates to #1211 (which fixes the registry sites at the consumer) and #809 (which touches /status but leaves :146 unchanged).

🤖 Generated with Claude Code

[lilyshen0722]

Reviewed at c9cce6010. The change is right and the test is a real reader of it. Two precision points and one stale comment; none of them block.

What I reproduced

• 7/7 new, 38/38 across __tests__/unit/middleware/ (node@22).
• Both mutations in your table land exactly as claimed: .select('banned username email role') → .select('banned') reds 4; req.user = { id } reds 4. The projection-honouring mock is load-bearing, as you say.
• Three more of my own, to check the suite discriminates rather than just counts:

mutation	result
drop only email: from req.user	2 red
role: live.role → role: 'admin'	2 red (the CONTROL: case earns its place)
role: live.role → role: live.role || 'user'	1 red — the absence test

• Base is 41 behind main, but auth.ts is unchanged on main since the merge-base and merge-tree is clean. At origin/main nothing outside tests reads req.user.username/.email as a presence check, so no consumer was using the narrow shape to detect which branch ran.

The CodeQL red is not yours

198 alerts, all js/missing-rate-limiting, zero located in either file you changed. main itself carries 398 open alerts of that same rule. They get re-attributed to the PR because the middleware you touched sits on their dataflow path. It's a non-required check (the PR is UNSTABLE, not BLOCKED) — recording it so the next reader doesn't have to re-derive it.

1. "shape-identical" holds for the key set, not for values

The cm_ branch is hydrated (findOne().select(...), no .lean()); the JWT branch is .lean(). User.ts:203 declares role: { ..., default: 'user' }, and mongoose applies schema defaults on hydrated docs but not on lean ones. Measured against the real model on mongodb-memory-server, raw-inserting a row with no role field:

HYDRATED role = "user"
LEAN     role = undefined
RAW      role = undefined

So on a role-less row the two branches still disagree — and a user row without a role leaves role undefined, not defaulted pins the lean side of that disagreement as correct without exercising the other. The suite can't see it: the findOne mock returns a plain object, so both branches are effectively lean in the test.

No live consequence today — every consumer compares against 'admin' only (github.ts:146, agentProfile.ts:252, podController.ts:43), and both values are non-admin. So this is wording, or one more fixture: either narrow "shape-identical" to the key set, or run the parity assertion over the role-less fixture and pin whichever answer you want.

2. agentProfile.ts:249-251 goes false on merge

// JWT auth populates req.user = { id } WITHOUT role (middleware/auth.ts:81)
// — only the cm_ API-token branch carries role.

The code stays correct — the DB fallback just becomes redundant, which is the separate work you scoped out. But the comment cites the exact line this PR rewrites, and it's what the next reader will trust. One line, cheap to fix here. podController.ts:43 and marketplace-api.ts:11 make no such claim and can stay as they are.

Not verified

The registry publish/install path (#1211's territory — no file overlap, merge-tree clean between the two), github.ts /status end-to-end against a real browser session, and any frontend consumer of these fields.

[lilyshen0722]

PASS on the code at c9cce6010. Not mergeable as it stands — two operational blockers below, neither of them about the diff.

The fix is right, and the test earns its keep

Widening a projection this branch already runs is the correct shape: both auth branches now assign { id, username, email, role }, at zero extra round-trips, so no consumer has to know which token type it was called with. I checked the parity claim rather than taking it — the cm_ branch at :50-55 assigns exactly those four keys from _id.toString(), username, email, role, and all four fields exist on the schema (models/User.ts:177, :178, :203).

Two things I went looking for and did not find, both of which would have made this riskier than it looks:

• No consumer sniffs the shape. I grepped for code inferring the auth type from req.user.username/role/email being undefined; there is none, and the only req.authType readers are auth.ts itself and apiTokenScopes.ts. So no branch silently flips direction when the field starts arriving.
• The widened projection does not disarm the guard beside it. .select('banned username email role') still carries banned, and the suite pins it — a projection that drops a field its own predicate reads is a fail-open this repo has been bitten by before.

Ran the suite at this head under node 22: 7/7 pass.

Mutation-checked it, and it discriminates precisely. Reverting the projection to .select('banned') — the exact regression a future editor would introduce while "tidying an over-wide query" — reds 4 of 7:

✕ the browser-JWT branch carries username, email and role
✕ the two branches agree on the key set
✕ the github.ts:146 predicate now admits a browser-session admin
✓ CONTROL: the predicate still refuses a browser-session non-admin
✕ a user row without a role leaves role undefined, not defaulted
✓ the ban check still fires on the same widened read
✓ a deleted account is still refused

The three survivors are the right three: they do not depend on the widening, so the control genuinely controls rather than the suite being globally sensitive to any edit.

Blocker 1: behind = 82, on a green stale-base tick

Stale-base merge guard shows pass. Recomputed just now against main 8ca1ef60: the merge-base is 82 commits behind, against MAX_BEHIND: 40. The tick is a property of (PR, main-as-it-was) — it ran when the PR was fresh and has not been re-evaluated since; the guard only fires on opened, synchronize, reopened, edited. Read the number, not the checkmark. This needs a rebase before it can merge, independent of anything else here.

Blocker 2: CodeQL is red, and I cannot clear it

title:   "198 new alerts including 198 high severity security vulnerabilities"
summary: "New alerts in code changed by this pull request"
ran:     2026-08-25T11:00:16Z (2s)

198 high-severity alerts attributed to a 24-line projection widening in one file is not credible on its face, and zero of the repo's open alerts are located in backend/middleware/auth.ts. Three other equally stale ungated PRs (#1171, #1207, #1236) are green on the same gate, so it is not a global outage either.

I looked for the obvious explanation — that a 5-day-old run against an 82-commit-stale base is attributing main's drift to the PR — and it does not fit cleanly: the repo carries 505 open alerts, 491 of them high, so 198 is a subset of something rather than "everything." I could not determine the attribution mechanism, and I am not going to invent one. What I can say is that the gate is red, its stated content is implausible, and the cheapest way to find out is the rebase that Blocker 1 already requires — a fresh run against a current base either reproduces the 198 (in which case it is real and this becomes a block on substance) or clears.

Follow-up, not a defect

The three private DB re-reads the description names as workarounds (podController.isGlobalAdminRequest and the two others) are untouched by this diff — correctly, since a fix should not also remove the thing that was compensating for it. They are now redundant reads rather than load-bearing ones. Worth a separate PR once this lands, and worth it soon: a workaround left in place after its cause is fixed is the kind of code whose comment teaches the next reader something false.