hacklab/T-HUB
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

fix: oidc hardening — sid-less consent refused, rotation race closed, iss on errors, logout honesty

Browse source 
From the logic audit:
- consent rejects sid-less bearers and requires a LIVE hub session
  (sessionAlive) before minting codes — a logged-out bearer can no longer
  mint downstream tokens inside its TTL
- refresh rotation race closed: consume returns whether THIS call won;
  the loser revokes the family (reuse detection now holds under
  concurrency)
- error redirects carry iss (RFC 9207) on error responses too
- logout awaits the revocation and no longer returns ok for missing or
  forged bearers
This commit is contained in:
glass beadian 2026-10-18 11:54:59 +00:00
parent e486e62220
commit 74f0802f1a
10 changed files with 355 additions and 24 deletions
Download patch file Download diff file Expand all files Collapse all files

5
apps/api/src/oidc/oidc.config.ts
View file

@ -161,11 +161,14 @@ export function validateAuthorize(params: AuthorizeParams): AuthorizeValidation
}
/** `redirectUri?error=…&error_description=…&state=…` for a redirectable failure. */
export function errorRedirect(redirectUri: string, state: string | null, error: string, description: string): string {
export function errorRedirect(redirectUri: string, state: string | null, error: string, description: string, issuer?: string): string {
const url = new URL(redirectUri);
url.searchParams.set("error", error);
url.searchParams.set("error_description", description);
if (state !== null) url.searchParams.set("state", state);
// RFC 9207 §2: iss is required on error responses too when the server
// advertises authorization_response_iss_parameter_supported.
if (issuer !== undefined) url.searchParams.set("iss", issuer);
return url.toString();
}

9
apps/api/src/oidc/oidc.service.ts
View file

@ -223,7 +223,7 @@ export class OidcService implements OnModuleInit {
const params = authorizeParamsFrom(query);
const result = validateAuthorize(params);
if (!result.ok && !result.redirect) throw new BadRequestException(`${result.error}: ${result.description}`);
if (!result.ok) return errorRedirect(result.redirectUri, result.state, result.error, result.description);
if (!result.ok) return errorRedirect(result.redirectUri, result.state, result.error, result.description, this.config().issuer);
const consent = new URL(config.consentUrl);
for (const name of AUTHORIZE_PARAM_NAMES) {
const value = params[name];
@ -235,9 +235,14 @@ export class OidcService implements OnModuleInit {
/** POST /oidc/authorize/consent — the session's user approves (auto, first-party) and gets a code. */
async consent(workspaceId: string, userId: string, body: unknown, sid: string | null = null): Promise<{ redirectTo: string }> {
const config = this.config();
// The session must still be alive: a logged-out (or sid-less) bearer must
// never mint codes or downstream tokens, even inside its TTL.
if (sid === null || !(await this.deps.refreshStore.sessionAlive(workspaceId, sid))) {
throw new UnauthorizedException("the hub session has ended — sign in again");
}
const result = validateAuthorize(authorizeParamsFrom(body));
if (!result.ok && !result.redirect) throw new BadRequestException(`${result.error}: ${result.description}`);
if (!result.ok) return { redirectTo: errorRedirect(result.redirectUri, result.state, result.error, result.description) };
if (!result.ok) return { redirectTo: errorRedirect(result.redirectUri, result.state, result.error, result.description, this.config().issuer) };
const request = result.request;
if (!isFirstPartyClient(request.clientId)) {
// No consent UI exists yet: anything but a first-party client fails closed.

28
apps/api/src/oidc/oidc.test.ts
View file

@ -79,6 +79,8 @@ function harness(): { service: OidcService; users: FakeUsers; clock: { now: numb
const vault = new Vault();
const store = new MemoryRefreshStore();
const sessions = store.liveSessions;
store.liveSessions.add(`${WORKSPACE}:test-sid`);
store.liveSessions.add(`${WORKSPACE}:test-sid`);
const service = new OidcService({
config: () => config,
keyring: (c) => OidcKeyring.open(new FileKeyStore(c.keyDir), vault),
@ -91,7 +93,7 @@ function harness(): { service: OidcService; users: FakeUsers; clock: { now: numb
}
async function issueCode(service: OidcService, challenge: string, extra: Record<string, string> = {}): Promise<string> {
const { redirectTo } = await service.consent(WORKSPACE, ADA.id, authorizeParams(challenge, extra));
const { redirectTo } = await service.consent(WORKSPACE, ADA.id, authorizeParams(challenge, extra), "test-sid");
const url = new URL(redirectTo);
const code = url.searchParams.get("code");
if (code === null) throw new Error(`no code in redirect: ${url.searchParams.get("error") ?? ""}`);
@ -228,7 +230,8 @@ describe("OidcService", () => {
it("auto-consents for client_id=web: the session gets a code straight back, no consent screen", async () => {
const { service } = harness();
const { challenge } = pkce();
const { redirectTo } = await service.consent(WORKSPACE, ADA.id, authorizeParams(challenge));
await expect(service.consent(WORKSPACE, ADA.id, authorizeParams(challenge))).rejects.toBeInstanceOf(UnauthorizedException); // sid-less bearers cannot mint
const { redirectTo } = await service.consent(WORKSPACE, ADA.id, authorizeParams(challenge), "test-sid");
const url = new URL(redirectTo);
expect(`${url.origin}${url.pathname}`).toBe(WEB_REDIRECT_URI);
expect(url.searchParams.get("code")).toMatch(/^[A-Za-z0-9_-]{43}$/);
@ -239,19 +242,19 @@ describe("OidcService", () => {
it("only issues codes to active members with a usable, unreserved username", async () => {
const { service, users } = harness();
const { challenge } = pkce();
await expect(service.consent(WORKSPACE, "33333333-3333-4333-8333-333333333333", authorizeParams(challenge))).rejects.toBeInstanceOf(ForbiddenException);
await expect(service.consent("44444444-4444-4444-8444-444444444444", ADA.id, authorizeParams(challenge))).rejects.toBeInstanceOf(ForbiddenException);
await expect(service.consent(WORKSPACE, "33333333-3333-4333-8333-333333333333", authorizeParams(challenge), "test-sid")).rejects.toBeInstanceOf(ForbiddenException);
await expect(service.consent("44444444-4444-4444-8444-444444444444", ADA.id, authorizeParams(challenge), "test-sid")).rejects.toBeInstanceOf(UnauthorizedException); // cross-workspace sid is not live
users.members.set(ADA.id, { ...ADA, username: "admin" });
await expect(service.consent(WORKSPACE, ADA.id, authorizeParams(challenge))).rejects.toBeInstanceOf(ForbiddenException);
await expect(service.consent(WORKSPACE, ADA.id, authorizeParams(challenge), "test-sid")).rejects.toBeInstanceOf(ForbiddenException);
users.members.set(ADA.id, { ...ADA, username: "Ada" });
await expect(service.consent(WORKSPACE, ADA.id, authorizeParams(challenge))).rejects.toBeInstanceOf(ForbiddenException);
await expect(service.consent(WORKSPACE, ADA.id, authorizeParams(challenge), "test-sid")).rejects.toBeInstanceOf(ForbiddenException);
});
it("re-validates relayed params: a forged redirect_uri from the consent page is refused", async () => {
const { service } = harness();
const { challenge } = pkce();
await expect(service.consent(WORKSPACE, ADA.id, authorizeParams(challenge, { redirect_uri: "https://evil.example/cb" }))).rejects.toBeInstanceOf(BadRequestException);
const { redirectTo } = await service.consent(WORKSPACE, ADA.id, authorizeParams(challenge, { code_challenge_method: "plain" }));
await expect(service.consent(WORKSPACE, ADA.id, authorizeParams(challenge, { redirect_uri: "https://evil.example/cb" }), "test-sid")).rejects.toBeInstanceOf(BadRequestException);
const { redirectTo } = await service.consent(WORKSPACE, ADA.id, authorizeParams(challenge, { code_challenge_method: "plain" }), "test-sid");
expect(new URL(redirectTo).searchParams.has("code")).toBe(false);
});
@ -388,11 +391,10 @@ describe("OidcService refresh tokens (phase 2)", () => {
expect(rows.filter((row) => row.consumedAt === null)).toHaveLength(1);
});
it("a code from a pre-phase2 session (no sid) mints no refresh token", async () => {
const h = harness();
const { verifier, challenge } = pkce();
const tokens = await exchange(h.service, await issueCode(h.service, challenge), verifier);
expect(tokens.refresh_token).toBeUndefined();
it("a consent without a session id (pre-phase2 client) is rejected outright", async () => {
const { service } = harness();
const { challenge } = pkce();
await expect(service.consent(WORKSPACE, ADA.id, authorizeParams(challenge))).rejects.toBeInstanceOf(UnauthorizedException);
});
it("rejects a revoked (logged-out) session's refresh token and revokes the chain", async () => {