OAuth: Allow onSignedUp hook to access hcaptcha result data (#3645)

* OAuth: Allow `onSignedUp` hook to access hcaptcha result data

* OAuth hcatpcha fixes

* Update .changeset/slimy-flowers-punch.md

Co-authored-by: devin ivy <devinivy@gmail.com>

---------

Co-authored-by: devin ivy <devinivy@gmail.com>

.changeset/afraid-spies-exist.md

@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-provider": patch
+---
+
+Fix hcaptcha verification based on score

.changeset/great-tips-hide.md

@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-provider": minor
+---
+
+Remove onSignupHcaptchaResult hook

.changeset/serious-timers-notice.md

@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-provider": minor
+---
+
+Allow `onSignedUp` hook to access hcaptcha result data.

.changeset/slimy-flowers-punch.md

@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-provider": patch
+---
+
+Improve HCaptcha error reporting

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<void> {
-    let hcaptchaResult: undefined | HcaptchaVerifyResult
+  ): Promise<HcaptchaVerifyResult | undefined> {
+    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<string | undefined> {
+    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<SignUpData> {
+    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<AccountInfo> {
-    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.

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<typeof inviteCodeSchema>
 
 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

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<typeof signUpDataSchema>
+export type SignUpInput = z.TypeOf<typeof signUpInputSchema>

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)

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<undefined | OAuthAuthorizationDetails>
 
-  /**
-   * 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<void>
-
   /**
    * 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<void>
 
   /**

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)
       }),
     )