diff --git a/.changeset/afraid-spies-exist.md b/.changeset/afraid-spies-exist.md new file mode 100644 index 000000000..39ad59fd2 --- /dev/null +++ b/.changeset/afraid-spies-exist.md @@ -0,0 +1,5 @@ +--- +"@atproto/oauth-provider": patch +--- + +Fix hcaptcha verification based on score diff --git a/.changeset/great-tips-hide.md b/.changeset/great-tips-hide.md new file mode 100644 index 000000000..919e7dce4 --- /dev/null +++ b/.changeset/great-tips-hide.md @@ -0,0 +1,5 @@ +--- +"@atproto/oauth-provider": minor +--- + +Remove onSignupHcaptchaResult hook diff --git a/.changeset/serious-timers-notice.md b/.changeset/serious-timers-notice.md new file mode 100644 index 000000000..deefd3843 --- /dev/null +++ b/.changeset/serious-timers-notice.md @@ -0,0 +1,5 @@ +--- +"@atproto/oauth-provider": minor +--- + +Allow `onSignedUp` hook to access hcaptcha result data. diff --git a/.changeset/slimy-flowers-punch.md b/.changeset/slimy-flowers-punch.md new file mode 100644 index 000000000..235cac458 --- /dev/null +++ b/.changeset/slimy-flowers-punch.md @@ -0,0 +1,5 @@ +--- +"@atproto/oauth-provider": patch +--- + +Improve HCaptcha error reporting diff --git a/packages/oauth/oauth-provider/src/account/account-manager.ts b/packages/oauth/oauth-provider/src/account/account-manager.ts index 1abc3d05e..a575ded1b 100644 --- a/packages/oauth/oauth-provider/src/account/account-manager.ts +++ b/packages/oauth/oauth-provider/src/account/account-manager.ts @@ -18,9 +18,10 @@ import { AccountStore, ResetPasswordConfirmData, ResetPasswordRequestData, + SignUpData, } from './account-store.js' import { SignInData } from './sign-in-data.js' -import { SignUpData } from './sign-up-data.js' +import { SignUpInput } from './sign-up-input.js' const TIMING_ATTACK_MITIGATION_DELAY = 400 const BRUTE_FORCE_MITIGATION_DELAY = 300 @@ -41,59 +42,79 @@ export class AccountManager { : undefined } - protected async verifySignupData( - data: SignUpData, + protected async processHcaptchaToken( + input: SignUpInput, deviceId: DeviceId, deviceMetadata: RequestMetadata, - ): Promise { - let hcaptchaResult: undefined | HcaptchaVerifyResult + ): Promise { + if (!this.hcaptchaClient) { + return undefined + } - if (this.inviteCodeRequired && !data.inviteCode) { + if (!input.hcaptchaToken) { + throw new InvalidRequestError('hCaptcha token is required') + } + + const { allowed, result } = await this.hcaptchaClient + .verify( + 'signup', + input.hcaptchaToken, + deviceMetadata.ipAddress, + input.handle, + deviceMetadata.userAgent, + ) + .catch((err) => { + throw InvalidRequestError.from(err, 'hCaptcha verification failed') + }) + + if (!allowed) { + throw new InvalidRequestError('hCaptcha verification failed') + } + + return result + } + + protected async enforceInviteCode( + input: SignUpInput, + _deviceId: DeviceId, + _deviceMetadata: RequestMetadata, + ): Promise { + if (!this.inviteCodeRequired) { + return undefined + } + + if (!input.inviteCode) { throw new InvalidRequestError('Invite code is required') } - if (this.hcaptchaClient) { - if (!data.hcaptchaToken) { - throw new InvalidRequestError('hCaptcha token is required') - } + return input.inviteCode + } - const { allowed, result } = await this.hcaptchaClient.verify( - 'signup', - data.hcaptchaToken, - deviceMetadata.ipAddress, - data.handle, - deviceMetadata.userAgent, - ) + protected async buildSignupData( + input: SignUpInput, + deviceId: DeviceId, + deviceMetadata: RequestMetadata, + ): Promise { + const [hcaptchaResult, inviteCode] = await Promise.all([ + this.processHcaptchaToken(input, deviceId, deviceMetadata), + this.enforceInviteCode(input, deviceId, deviceMetadata), + ]) - await callAsync(this.hooks.onSignupHcaptchaResult, { - data, - allowed, - result, - deviceId, - deviceMetadata, - }) - - if (!allowed) { - throw new InvalidRequestError('hCaptcha verification failed') - } - - hcaptchaResult = result - } - - await callAsync(this.hooks.onSignupAttempt, { - data, - deviceId, - deviceMetadata, - hcaptchaResult, - }) + return { ...input, hcaptchaResult, inviteCode } } public async signUp( - data: SignUpData, + input: SignUpInput, deviceId: DeviceId, deviceMetadata: RequestMetadata, ): Promise { - await this.verifySignupData(data, deviceId, deviceMetadata) + await callAsync(this.hooks.onSignupAttempt, { + input, + deviceId, + deviceMetadata, + }) + + const data = await this.buildSignupData(input, deviceId, deviceMetadata) // Mitigation against brute forcing email of users. // @TODO Add rate limit to all the OAuth routes. diff --git a/packages/oauth/oauth-provider/src/account/account-store.ts b/packages/oauth/oauth-provider/src/account/account-store.ts index dd7a6d2b1..38ad50a8b 100644 --- a/packages/oauth/oauth-provider/src/account/account-store.ts +++ b/packages/oauth/oauth-provider/src/account/account-store.ts @@ -4,6 +4,7 @@ import { z } from 'zod' import { ensureValidHandle, normalizeHandle } from '@atproto/syntax' import { ClientId } from '../client/client-id.js' import { DeviceId } from '../device/device-id.js' +import { HcaptchaVerifyResult } from '../lib/hcaptcha.js' import { localeSchema } from '../lib/locale.js' import { Awaitable, buildInterfaceChecker } from '../lib/util/type.js' import { @@ -13,6 +14,7 @@ import { } from '../oauth-errors.js' import { Sub } from '../oidc/sub.js' import { Account } from './account.js' +import { SignUpInput } from './sign-up-input.js' // @NOTE Change the length here to force stronger passwords (through a reset) export const oldPasswordSchema = z.string().min(1) @@ -49,6 +51,7 @@ export const emailSchema = z }) .transform((value) => value.toLowerCase()) export const inviteCodeSchema = z.string().min(1) +export type InviteCode = z.infer export const authenticateAccountDataSchema = z .object({ @@ -118,6 +121,11 @@ export type AccountInfo = { info: DeviceAccountInfo } +export type SignUpData = SignUpInput & { + hcaptchaResult?: HcaptchaVerifyResult + inviteCode?: InviteCode +} + export interface AccountStore { /** * @throws {HandleUnavailableError} - To indicate that the handle is already taken diff --git a/packages/oauth/oauth-provider/src/account/sign-up-data.ts b/packages/oauth/oauth-provider/src/account/sign-up-input.ts similarity index 65% rename from packages/oauth/oauth-provider/src/account/sign-up-data.ts rename to packages/oauth/oauth-provider/src/account/sign-up-input.ts index cf9aac8ab..e3677e176 100644 --- a/packages/oauth/oauth-provider/src/account/sign-up-data.ts +++ b/packages/oauth/oauth-provider/src/account/sign-up-input.ts @@ -2,10 +2,10 @@ import { z } from 'zod' import { hcaptchaTokenSchema } from '../lib/hcaptcha.js' import { createAccountDataSchema } from './account-store.js' -export const signUpDataSchema = createAccountDataSchema +export const signUpInputSchema = createAccountDataSchema .extend({ hcaptchaToken: hcaptchaTokenSchema.optional(), }) .strict() -export type SignUpData = z.TypeOf +export type SignUpInput = z.TypeOf diff --git a/packages/oauth/oauth-provider/src/lib/hcaptcha.ts b/packages/oauth/oauth-provider/src/lib/hcaptcha.ts index 1ea0c32cd..45ffb0844 100644 --- a/packages/oauth/oauth-provider/src/lib/hcaptcha.ts +++ b/packages/oauth/oauth-provider/src/lib/hcaptcha.ts @@ -27,9 +27,11 @@ export const hcaptchaConfigSchema = z.object({ */ tokenSalt: z.string().min(1), /** - * The risk score over which the user is considered a threat and will be + * The risk score above which the user is considered a threat and will be * denied access. This will be ignored if the enterprise features are not * available. + * + * Note: Score values ranges from 0.0 (no risk) to 1.0 (confirmed threat). */ scoreThreshold: z.number().optional(), }) @@ -128,7 +130,7 @@ export class HCaptchaClient { this.fetch = bindFetch(fetch) } - async verify( + public async verify( behaviorType: 'login' | 'signup', response: string, remoteip: string, @@ -160,20 +162,21 @@ export class HCaptchaClient { } } - isAllowed({ success, hostname, score }: HcaptchaVerifyResult) { + protected isAllowed({ success, hostname, score }: HcaptchaVerifyResult) { return ( success && // Fool-proofing: If this is false, the user is trying to use a token // generated for the same siteKey, but on another domain. hostname === this.hostname && // Ignore if enterprise feature is not enabled - score != null && - this.config.scoreThreshold != null && - score < this.config.scoreThreshold + (score == null || + // Ignore if disabled through config + this.config.scoreThreshold == null || + score < this.config.scoreThreshold) ) } - hashToken(value: string) { + protected hashToken(value: string) { const hash = createHash('sha256') hash.update(this.config.tokenSalt) hash.update(value) diff --git a/packages/oauth/oauth-provider/src/oauth-hooks.ts b/packages/oauth/oauth-provider/src/oauth-hooks.ts index 56af40b75..27e270466 100644 --- a/packages/oauth/oauth-provider/src/oauth-hooks.ts +++ b/packages/oauth/oauth-provider/src/oauth-hooks.ts @@ -7,7 +7,7 @@ import { } from '@atproto/oauth-types' import { Account } from './account/account.js' import { SignInData } from './account/sign-in-data.js' -import { SignUpData } from './account/sign-up-data.js' +import { SignUpInput } from './account/sign-up-input.js' import { ClientAuth } from './client/client-auth.js' import { ClientId } from './client/client-id.js' import { ClientInfo } from './client/client-info.js' @@ -17,7 +17,7 @@ import { HcaptchaConfig, HcaptchaVerifyResult } from './lib/hcaptcha.js' import { RequestMetadata } from './lib/http/request.js' import { Awaitable } from './lib/util/type.js' import { AccessDeniedError, OAuthError } from './oauth-errors.js' -import { DeviceAccountInfo, DeviceId } from './oauth-store.js' +import { DeviceAccountInfo, DeviceId, SignUpData } from './oauth-store.js' // Make sure all types needed to implement the OAuthHooks are exported export { @@ -42,6 +42,7 @@ export { type RequestMetadata, type SignInData, type SignUpData, + type SignUpInput, } export type OAuthHooks = { @@ -71,36 +72,14 @@ export type OAuthHooks = { account: Account }) => Awaitable - /** - * This hook is called whenever an hcaptcha challenge is verified - * during sign-up (if hcaptcha is enabled). - * - * @throws {InvalidRequestError} to deny the sign-up - */ - onSignupHcaptchaResult?: (data: { - data: SignUpData - /** - * This indicates not only wether the hCaptcha challenge succeeded, but also - * if the score was low enough according to the - * {@link HcaptchaConfig.scoreThreshold}. - * - * @see {@link HCaptchaClient.isAllowed} - */ - allowed: boolean - result: HcaptchaVerifyResult - deviceId: DeviceId - deviceMetadata: RequestMetadata - }) => Awaitable - /** * This hook is called when a user attempts to sign up, after every validation * has passed (including hcaptcha). */ onSignupAttempt?: (data: { - data: SignUpData + input: SignUpInput deviceId: DeviceId deviceMetadata: RequestMetadata - hcaptchaResult?: HcaptchaVerifyResult }) => Awaitable /** diff --git a/packages/oauth/oauth-provider/src/oauth-provider.ts b/packages/oauth/oauth-provider/src/oauth-provider.ts index 45b62bdf8..ea12731cf 100644 --- a/packages/oauth/oauth-provider/src/oauth-provider.ts +++ b/packages/oauth/oauth-provider/src/oauth-provider.ts @@ -45,7 +45,7 @@ import { } from './account/account-store.js' import { Account } from './account/account.js' import { signInDataSchema } from './account/sign-in-data.js' -import { signUpDataSchema } from './account/sign-up-data.js' +import { signUpInputSchema } from './account/sign-up-input.js' import { authorizeAssetsMiddleware } from './assets/assets-middleware.js' import { ClientAuth, authJwkThumbprint } from './client/client-auth.js' import { @@ -1588,7 +1588,7 @@ export class OAuthProvider extends OAuthVerifier { router.post( '/oauth/authorize/sign-up', - apiHandler(signUpDataSchema, async function (req, res, data, ctx) { + apiHandler(signUpInputSchema, async function (req, res, data, ctx) { return server.signUp(ctx, data) }), )