From 0c50736dc3d04b127b0203dd31f0fae9764a1b3e Mon Sep 17 00:00:00 2001 From: Matthieu Sieben Date: Tue, 13 Aug 2024 16:41:36 +0200 Subject: [PATCH] Ensure presence of DPoP related response headers (#2711) * fix(pds): ensure presence of DPoP related response headers * Expose the request context for AuthVerifier and StreamAuthVerifier as distinct types * Properly type ReqCtx for stream auth --- .changeset/giant-starfishes-fry.md | 5 +++++ .changeset/happy-eggs-swim.md | 5 +++++ packages/pds/src/auth-verifier.ts | 36 ++++++++++++++++-------------- packages/pds/src/pipethrough.ts | 2 +- packages/xrpc-server/src/types.ts | 16 +++++++++---- 5 files changed, 42 insertions(+), 22 deletions(-) create mode 100644 .changeset/giant-starfishes-fry.md create mode 100644 .changeset/happy-eggs-swim.md diff --git a/.changeset/giant-starfishes-fry.md b/.changeset/giant-starfishes-fry.md new file mode 100644 index 00000000000..8f20fdb00cc --- /dev/null +++ b/.changeset/giant-starfishes-fry.md @@ -0,0 +1,5 @@ +--- +"@atproto/xrpc-server": patch +--- + +Expose the request context for AuthVerifier and StreamAuthVerifier as distinct types diff --git a/.changeset/happy-eggs-swim.md b/.changeset/happy-eggs-swim.md new file mode 100644 index 00000000000..e7da6125c16 --- /dev/null +++ b/.changeset/happy-eggs-swim.md @@ -0,0 +1,5 @@ +--- +"@atproto/pds": patch +--- + +Properly authenticate OAuth requests in catch all handler. diff --git a/packages/pds/src/auth-verifier.ts b/packages/pds/src/auth-verifier.ts index 953e7a79806..90b82a9a5ed 100644 --- a/packages/pds/src/auth-verifier.ts +++ b/packages/pds/src/auth-verifier.ts @@ -1,5 +1,8 @@ import { KeyObject, createPublicKey, createSecretKey } from 'node:crypto' +import { IncomingMessage, ServerResponse } from 'node:http' +import { getVerificationMaterial } from '@atproto/common' +import { IdResolver, getDidKeyFromMultibase } from '@atproto/identity' import { OAuthError, OAuthVerifier, @@ -7,24 +10,19 @@ import { } from '@atproto/oauth-provider' import { AuthRequiredError, + AuthVerifierContext, ForbiddenError, InvalidRequestError, + StreamAuthVerifierContext, XRPCError, verifyJwt as verifyServiceJwt, } from '@atproto/xrpc-server' -import { IdResolver, getDidKeyFromMultibase } from '@atproto/identity' -import express from 'express' import * as jose from 'jose' import KeyEncoder from 'key-encoder' import { AccountManager } from './account-manager' import { softDeleted } from './db' -import { getVerificationMaterial } from '@atproto/common' -type ReqCtx = { - req: express.Request - // StreamAuthVerifier does not have "res" - res?: express.Response -} +type ReqCtx = AuthVerifierContext | StreamAuthVerifierContext // @TODO sync-up with current method names, consider backwards compat. export enum AuthScope { @@ -462,7 +460,8 @@ export class AuthVerifier { this.setAuthHeaders(ctx) - const { req, res } = ctx + const { req } = ctx + const res = 'res' in ctx ? ctx.res : null // https://datatracker.ietf.org/doc/html/rfc9449#section-8.2 if (res) { @@ -474,9 +473,11 @@ export class AuthVerifier { } try { - const url = new URL(req.originalUrl || req.url, this._publicUrl) + const originalUrl = + ('originalUrl' in req && req.originalUrl) || req.url || '/' + const url = new URL(originalUrl, this._publicUrl) const result = await this.oauthVerifier.authenticateRequest( - req.method, + req.method || 'GET', url, req.headers, { audience: [this.dids.pds] }, @@ -619,7 +620,8 @@ export class AuthVerifier { } } - protected setAuthHeaders({ res }: ReqCtx) { + protected setAuthHeaders(ctx: ReqCtx) { + const res = 'res' in ctx ? ctx.res : null if (res) { res.setHeader('Cache-Control', 'private') vary(res, 'Authorization') @@ -661,22 +663,22 @@ export const parseAuthorizationHeader = ( ) } -const isAccessToken = (req: express.Request): boolean => { +const isAccessToken = (req: IncomingMessage): boolean => { const [type] = parseAuthorizationHeader(req.headers.authorization) return type === AuthType.BEARER || type === AuthType.DPOP } -const isBearerToken = (req: express.Request): boolean => { +const isBearerToken = (req: IncomingMessage): boolean => { const [type] = parseAuthorizationHeader(req.headers.authorization) return type === AuthType.BEARER } -const isBasicToken = (req: express.Request): boolean => { +const isBasicToken = (req: IncomingMessage): boolean => { const [type] = parseAuthorizationHeader(req.headers.authorization) return type === AuthType.BASIC } -const bearerTokenFromReq = (req: express.Request) => { +const bearerTokenFromReq = (req: IncomingMessage) => { const [type, token] = parseAuthorizationHeader(req.headers.authorization) return type === AuthType.BEARER ? token : null } @@ -715,7 +717,7 @@ export const createPublicKeyObject = (publicKeyHex: string): KeyObject => { const keyEncoder = new KeyEncoder('secp256k1') -function vary(res: express.Response, value: string) { +function vary(res: ServerResponse, value: string) { const current = res.getHeader('Vary') if (current == null || typeof current === 'number') { res.setHeader('Vary', value) diff --git a/packages/pds/src/pipethrough.ts b/packages/pds/src/pipethrough.ts index 855fb438202..5a85d62b3e0 100644 --- a/packages/pds/src/pipethrough.ts +++ b/packages/pds/src/pipethrough.ts @@ -21,7 +21,7 @@ export const proxyHandler = (ctx: AppContext): CatchallHandler => { return async (req, res, next) => { try { const { url, aud, nsid } = await formatUrlAndAud(ctx, req) - const auth = await accessStandard({ req }) + const auth = await accessStandard({ req, res }) if (!auth.credentials.isPrivileged && PRIVILEGED_METHODS.has(nsid)) { throw new InvalidRequestError('Bad token method', 'InvalidToken') } diff --git a/packages/xrpc-server/src/types.ts b/packages/xrpc-server/src/types.ts index c12c3c35891..db171e9ddea 100644 --- a/packages/xrpc-server/src/types.ts +++ b/packages/xrpc-server/src/types.ts @@ -90,14 +90,22 @@ export type XRPCStreamHandler = (ctx: { export type AuthOutput = HandlerAuth | HandlerError -export type AuthVerifier = (ctx: { +export interface AuthVerifierContext { req: express.Request res: express.Response -}) => Promise | AuthOutput +} + +export type AuthVerifier = ( + ctx: AuthVerifierContext, +) => Promise | AuthOutput -export type StreamAuthVerifier = (ctx: { +export interface StreamAuthVerifierContext { req: IncomingMessage -}) => Promise | AuthOutput +} + +export type StreamAuthVerifier = ( + ctx: StreamAuthVerifierContext, +) => Promise | AuthOutput export type CalcKeyFn = (ctx: XRPCReqContext) => string | null export type CalcPointsFn = (ctx: XRPCReqContext) => number