feat(auth): sessions and roles - #910
Merged
Merged
Conversation
arberx
force-pushed
the
ax/target-runner-poc
branch
from
August 2, 2026 04:51
fabfe1e to
0a2cf04
Compare
arberx
force-pushed
the
ax/target-runner-poc
branch
from
August 2, 2026 07:32
0a2cf04 to
550e17e
Compare
arberx
force-pushed
the
ax/session-auth
branch
2 times, most recently
from
August 2, 2026 07:44
c520b96 to
8cbe7d4
Compare
arberx
marked this pull request as ready for review
August 2, 2026 07:52
Rebuilt on top of the full runner tip. The previous restack computed this branch's delta against the runner's HEAD rather than against the runner commit the auth work was actually written on, so it carried a reversion of the two newest runner commits: it deleted measurement-run-completeness.ts, the notifier's completeness guard, and the scheduler's provider-roster wiring, none of which this branch ever touched. Those are restored. The accounts migration is v121, above the runner's v119 and v120. Still a draft; the review blockers are not fixed here. A narrow key such as users.read can still reach Ads live delivery and its paid upstream calls. The X-Canonry-Embed-Tabs header can still widen the server's configured allowlist. Cloud proxy trust is still optional, so without CANONRY_TRUST_PROXY set the login budget collapses onto the proxy address.
…lists Three exposures from review of the session-auth work. The Ads live-delivery read fans out to roughly four thousand paid upstream requests and was gated by a deny-list: it refused only keys that had opted into the read-only token. A key scoped to something unrelated, or carrying no scopes at all, passed straight through and could spend real money. It is now an allow-list, refusing anything without the wildcard or an explicit ads scope, matching the reasoning already written into requireBroadInstanceKey: a deny list silently widens every time a new scope is invented. The X-Canonry-Embed-Tabs request header replaced the server's configured tab allowlist rather than narrowing it, so a caller could name a tab the server had not granted. The header is now intersected with the configured allowlist. Two pre-existing tests encoded the old behaviour as correct and were rewritten rather than relaxed; both are named in the diff. Trusting the proxy was optional. Left unset behind a load balancer, every request resolved to the proxy's own address, collapsing per-caller login budgets into one shared bucket and disabling per-caller rate limiting. Startup now refuses rather than degrading silently. Each change is pinned by a test proven by reverting the production change.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| * table holds a record of a session rather than a working copy of one. | ||
| */ | ||
| export function hashSessionToken(token: string): string { | ||
| return crypto.createHash('sha256').update(token).digest('hex') |
Comment on lines
+396
to
+487
| app.post('/auth/login', async (request, reply) => { | ||
| const parsed = loginRequestSchema.safeParse(request.body) | ||
| if (!parsed.success) { | ||
| throw validationError('Enter a name and a password.', { issues: parsed.error.issues }) | ||
| } | ||
|
|
||
| const nameKey = normalizeUserName(parsed.data.name) | ||
| // Null when a proxy is in front and has not been declared trusted: the | ||
| // address in hand is the proxy's, shared by everyone, so budgeting against | ||
| // it would let one attacker lock out the whole install. The per-name budget | ||
| // and the in-flight ceiling still apply in that case. | ||
| const caller = resolveCallerKey(request, opts.trustProxyConfigured ?? false) | ||
| const nowMs = Date.now() | ||
|
|
||
| // Three budgets, and they answer three different questions. Per name: is | ||
| // somebody guessing at ONE account. Per caller: is somebody spending this | ||
| // server's key derivations, which cycling names would otherwise make free. | ||
| // In flight: is the threadpool already saturated right now, whoever is | ||
| // asking. All three are checked BEFORE any derivation is paid for. | ||
| // Keyed by name AND source: a stranger's failures must never be able to | ||
| // lock the real owner of the account out of it. | ||
| const nameFromCaller = `${nameKey}\u0000${caller ?? 'unidentified'}` | ||
| if (perNameLimiter.isBlocked(nameFromCaller, nowMs)) { | ||
| throw new AppError( | ||
| 'QUOTA_EXCEEDED', | ||
| 'Too many failed sign-in attempts for this name. Wait a few minutes and try again.', | ||
| 429, | ||
| ) | ||
| } | ||
| if (caller !== null && perCallerLimiter.isBlocked(caller, nowMs)) { | ||
| throw new AppError( | ||
| 'QUOTA_EXCEEDED', | ||
| 'Too many failed sign-in attempts. Wait a few minutes and try again.', | ||
| 429, | ||
| ) | ||
| } | ||
| // Per caller first: one identified caller can never be the reason another | ||
| // is turned away. The global ceiling below only bounds the threadpool. | ||
| if (caller !== null && (callerVerificationsInFlight.get(caller) ?? 0) >= LOGIN_MAX_IN_FLIGHT_PER_CALLER) { | ||
| throw new AppError( | ||
| 'QUOTA_EXCEEDED', | ||
| 'A sign-in for this caller is already being checked. Try again in a moment.', | ||
| 429, | ||
| ) | ||
| } | ||
| if (verificationsInFlight >= LOGIN_MAX_IN_FLIGHT) { | ||
| throw new AppError( | ||
| 'QUOTA_EXCEEDED', | ||
| 'The server is busy checking sign-ins. Try again in a moment.', | ||
| 429, | ||
| ) | ||
| } | ||
|
|
||
| const account = app.db.select().from(users).where(eq(users.nameKey, nameKey)).get() | ||
| // Verify against a throwaway digest when the name is unknown so a missing | ||
| // account and a wrong password take the same amount of time to refuse. | ||
| const storedHash = account?.passwordHash ?? UNKNOWN_ACCOUNT_DIGEST | ||
| verificationsInFlight++ | ||
| if (caller !== null) { | ||
| callerVerificationsInFlight.set(caller, (callerVerificationsInFlight.get(caller) ?? 0) + 1) | ||
| } | ||
| let passwordMatches: boolean | ||
| try { | ||
| passwordMatches = await verifyUserPassword(parsed.data.password, storedHash) | ||
| } finally { | ||
| verificationsInFlight-- | ||
| if (caller !== null) { | ||
| const remaining = (callerVerificationsInFlight.get(caller) ?? 1) - 1 | ||
| if (remaining <= 0) callerVerificationsInFlight.delete(caller) | ||
| else callerVerificationsInFlight.set(caller, remaining) | ||
| } | ||
| } | ||
|
|
||
| if (!account || !passwordMatches) { | ||
| perNameLimiter.recordFailure(nameFromCaller, nowMs) | ||
| if (caller !== null) perCallerLimiter.recordFailure(caller, nowMs) | ||
| const err = authRequired(LOGIN_FAILED_MESSAGE) | ||
| return reply.status(err.statusCode).send(err.toJSON()) | ||
| } | ||
|
|
||
| perNameLimiter.clear(nameFromCaller) | ||
| if (caller !== null) perCallerLimiter.clear(caller) | ||
| const now = new Date() | ||
| const sessionId = createUserSession(app.db, account.id, now) | ||
| app.db.update(users).set({ lastLoginAt: now.toISOString() }).where(eq(users.id, account.id)).run() | ||
| setSessionCookie(request, reply, sessionId) | ||
|
|
||
| return reply.send({ | ||
| authRequired: true, | ||
| user: { name: account.name, role: account.role }, | ||
| } satisfies AuthSessionDto) | ||
| }) |
Comment on lines
+491
to
+511
| app.get('/auth/sessions', async (request, reply) => { | ||
| const token = cookieSessionId(request) | ||
| const resolved = token ? resolveUserSession(app.db, token) : null | ||
| if (!token || !resolved) { | ||
| const err = authRequired('Sign in to see where you are signed in.') | ||
| return reply.status(err.statusCode).send(err.toJSON()) | ||
| } | ||
|
|
||
| const currentHash = hashSessionToken(token) | ||
| const rows = app.db.select().from(userSessions) | ||
| .where(eq(userSessions.userId, resolved.user.id)).all() | ||
| return reply.send({ | ||
| sessions: rows.map(row => ({ | ||
| // Never the token or its digest — a list of your own sessions must not | ||
| // itself become a way to become one of them. | ||
| current: row.tokenHash === currentHash, | ||
| createdAt: row.createdAt, | ||
| expiresAt: row.expiresAt, | ||
| })), | ||
| }) | ||
| }) |
Comment on lines
+518
to
+534
| app.delete('/auth/sessions', async (request, reply) => { | ||
| const token = cookieSessionId(request) | ||
| const resolved = token ? resolveUserSession(app.db, token) : null | ||
| if (!token || !resolved) { | ||
| const err = authRequired('Sign in to end your sessions.') | ||
| return reply.status(err.statusCode).send(err.toJSON()) | ||
| } | ||
| // This route is on the auth skip-list so it keeps working from a session the | ||
| // rest of the API might refuse, which means the same-origin rule it would | ||
| // normally inherit has to be applied here by hand. Without it, any site | ||
| // could sign this person out of everything. | ||
| assertCookieWriteOrigin(request) | ||
|
|
||
| app.db.delete(userSessions).where(eq(userSessions.userId, resolved.user.id)).run() | ||
| clearSessionCookie(request, reply) | ||
| return reply.status(204).send() | ||
| }) |
| const cookieToken = await signIn('owner', ADMIN_PASSWORD) | ||
|
|
||
| const stored = db.select().from(userSessions).all()[0]! | ||
| expect(stored.tokenHash).toBe(crypto.createHash('sha256').update(cookieToken).digest('hex')) |
| test('the status poll that renews a session also refreshes the browser cookie', async () => { | ||
| await createAccount('owner', ADMIN_PASSWORD, 'admin') | ||
| const session = await signIn('owner', ADMIN_PASSWORD) | ||
| const tokenHash = crypto.createHash('sha256').update(session).digest('hex') |
| test('a session cannot be renewed past its absolute lifetime', async () => { | ||
| await createAccount('owner', ADMIN_PASSWORD, 'admin') | ||
| const session = await signIn('owner', ADMIN_PASSWORD) | ||
| const tokenHash = crypto.createHash('sha256').update(session).digest('hex') |
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
2 participants
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.
What
Generic session auth for the engine: named accounts with two roles, on top of the existing scope machinery. Stacked on #909 (reviewable delta = auth only).
users+user_sessions(migration v119, additive-only, downgrade-safe).canonry user create --name <n> --role <admin|viewer>(hidden prompt or--password-stdin; deliberately no--passwordflag),user list,user delete(refuses removing the last admin).['*'], viewer['read']— and enforcement falls through the existing global write gate andrequireScope. One new concept (request.principal) with two kinds:api-keyanduser.readSemanticroute config: compile-preview, diff-preview, and measurement-discovery are callable by viewer sessions (they write nothing). The annotation only ever relaxes a signed-in viewer — a read-only API key is refused exactly as before, and a test asserts both directions.requireAdmin-gated server-side (settings, keys, users, publish, run trigger).Validation
run-queue/job-runnerabsent from the diff; only the three one-linereadSemanticannotations touch the measurement stackOut of scope (known, documented)
Full E2 default-deny audit: an enumeration test asserting every registered non-GET route is viewer-refused or explicitly annotated; a drift guard for the hand-placed admin-only reads;
GET /keys/selffor user sessions; the login limiter is per-process (multiplies under an HTTP/worker split).Notes for review
Commits are
wip(auth):red→green TDD cycles; will squash/reword on un-draft.🤖 Generated with Claude Code