From dee817b6e0fc02351d51ce310b5e65239b7c5ed7 Mon Sep 17 00:00:00 2001 From: Matthieu Sieben Date: Tue, 27 Aug 2024 19:43:29 +0200 Subject: [PATCH] OAuth: Add authorization scopes & remove OpenID compatibility (#2734) * Re-use code definition of oauthResponseTypeSchema * Generate proper invalid_authorization_details * Remove OpenID compatibility * tidy * properly verify presence of jti claim in client assertion * Remove non-standard "sub" from OAuthTokenResponse * Remove nonce from authorization request * tidy * Enforce uniqueness of code_challenge * remove unused "atproto" scope * Improve reporting of validation errors * Allow empty set of scopes * Do not remove scopes not advertised in the AS's "scopes_supported" when building the authorization request. * Prevent empty scope string * Remove invalid check from token response * remove un-necessary session refresh * Validate scopes characters according to OAuth 2.1 spec * Mandate the use of "atproto" scope * Disable ability to list app passwords when using an app password * Use locally defined authPassthru in com.atproto.admin.* handlers * provide proper production handle resolver in example * properly compote login method * feat(oauth-provider): always rotate session cookie on sign-in * feat(oauth-provider): do not require consent from first party apps * update request parameter's prompt before other param validation checks * feat(oauth-provider): rework display of client name * feat(oauth-client-browser:example): add token info introspection * feat(oauth-client-browser:example): allow defining scope globally * Display requested scopes during the auth flow * Add, and verify, a "typ" header to access and refresh tokens * Ignore case when checking for dpop auth scheme * Add "jwtAlg" option to verifySignature() function * Verify service JWT header values. Add iat claim to service JWT * Add support for "transition:generic" and "transition:chat.bsky" oauth scopes in PDS * oauth-client-browser(example): add scope request * Add missing "atproto" scope * Allow missing 'typ' claim in service auth jwt * Improved 401 feedback Co-authored-by: devin ivy * Properly parse scopes upon verification Co-authored-by: devin ivy * Rename "atp" to "credential" auth in oauth-client-browser example * add key to iteration items * Make CORS protection stronger * Allow OAuthProvider to define its own CORS policies * Revert "Allow missing 'typ' claim in service auth jwt" This reverts commit 15c6b9e2197064eb5de61a96de6497060edb824e. * Revert "Verify service JWT header values. Add iat claim to service JWT" This reverts commit 08df8df322a3f4b631c4a63a61d55b2c84c60c11. * Revert "Add "jwtAlg" option to verifySignature() function" This reverts commit d0f77354e6904678e7f5d76bb026f07537443ba9. * Revert "Add, and verify, a "typ" header to access and refresh tokens" This reverts commit 3e21be9e4b5875caa5e862c11f2196786fb2366d. * pds: implement protected service auth methods * Prevent app password management using sessions initiated from an app password. * Alphabetically sort PROTECTED_METHODS * Revert changes to app password management permissions * tidy --------- Co-authored-by: devin ivy --- .changeset/cool-toes-rescue.md | 5 + .changeset/fluffy-apples-do.md | 5 + .changeset/green-bags-flash.md | 5 + .changeset/healthy-bottles-hear.md | 5 + .changeset/lemon-mice-rule.md | 6 + .changeset/light-dingos-dream.md | 5 + .changeset/ninety-ants-collect.md | 5 + .changeset/odd-spies-boil.md | 5 + .changeset/polite-humans-sleep.md | 6 + .changeset/poor-socks-sniff.md | 5 + .changeset/short-toes-battle.md | 12 + .changeset/six-ties-arrive.md | 5 + .changeset/smart-drinks-repeat.md | 5 + .changeset/smart-gifts-itch.md | 5 + .changeset/thin-cycles-live.md | 5 + .changeset/tidy-cars-thank.md | 5 + packages/api/OAUTH.md | 5 +- packages/oauth/oauth-client-browser/README.md | 2 +- .../oauth-client-browser/example/src/app.tsx | 23 ++ .../example/src/auth/auth-form.tsx | 26 +- .../example/src/auth/auth-provider.tsx | 18 +- .../credential-sign-in-form.tsx} | 2 +- .../use-credential-auth.ts} | 2 +- .../example/src/auth/oauth/use-oauth.ts | 19 +- .../oauth-client-browser/example/src/main.tsx | 9 +- .../src/browser-oauth-database.ts | 1 - packages/oauth/oauth-client-node/README.md | 3 +- packages/oauth/oauth-client/README.md | 1 - .../oauth/oauth-client/src/oauth-client.ts | 32 +- .../oauth-client/src/oauth-server-agent.ts | 26 +- .../oauth/oauth-client/src/oauth-session.ts | 2 +- packages/oauth/oauth-client/src/runtime.ts | 96 +---- .../oauth/oauth-client/src/state-store.ts | 1 - packages/oauth/oauth-client/src/types.ts | 4 +- packages/oauth/oauth-provider/package.json | 1 - .../oauth-provider/src/account/account.ts | 18 +- .../src/assets/app/backend-data.ts | 11 +- .../src/assets/app/components/accept-form.tsx | 116 +++--- .../app/components/client-identifier.tsx | 31 -- .../src/assets/app/components/client-name.tsx | 40 ++- .../src/assets/app/components/url-viewer.tsx | 6 +- .../src/assets/app/views/accept-view.tsx | 11 +- .../src/assets/app/views/authorize-view.tsx | 3 +- .../src/assets/assets-middleware.ts | 16 +- .../src/client/client-manager.ts | 138 +++---- .../oauth/oauth-provider/src/client/client.ts | 5 +- .../oauth/oauth-provider/src/constants.ts | 3 + .../src/device/device-manager.ts | 8 +- .../invalid-authorization-details-error.ts | 13 +- .../oauth-provider/src/lib/http/request.ts | 76 +++- .../src/metadata/build-metadata.ts | 51 +-- .../oauth/oauth-provider/src/oauth-hooks.ts | 16 +- .../oauth-provider/src/oauth-provider.ts | 340 ++++++++++-------- .../oauth-provider/src/oauth-verifier.ts | 3 +- .../oauth/oauth-provider/src/oidc/claims.ts | 35 -- .../oauth/oauth-provider/src/oidc/userinfo.ts | 11 - .../src/output/build-authorize-data.ts | 8 + .../src/parameters/claims-requested.ts | 106 ------ .../src/parameters/oidc-payload.ts | 28 -- .../src/replay/replay-manager.ts | 9 + .../oauth-provider/src/replay/replay-store.ts | 2 +- .../src/request/request-info.ts | 2 + .../src/request/request-manager.ts | 188 +++++----- .../oauth/oauth-provider/src/signer/signer.ts | 63 ---- .../oauth-provider/src/token/token-manager.ts | 49 +-- .../src/atproto-loopback-client-metadata.ts | 5 +- ...oauth-authentication-request-parameters.ts | 22 +- .../oauth-types/src/oauth-response-type.ts | 4 +- .../oauth-types/src/oauth-token-response.ts | 1 - .../src/api/com/atproto/admin/sendEmail.ts | 10 +- .../com/atproto/admin/updateAccountEmail.ts | 2 +- .../atproto/admin/updateAccountPassword.ts | 2 +- .../identity/requestPlcOperationSignature.ts | 18 +- .../com/atproto/identity/signPlcOperation.ts | 21 +- .../api/com/atproto/identity/updateHandle.ts | 13 +- .../api/com/atproto/server/activateAccount.ts | 19 +- .../api/com/atproto/server/confirmEmail.ts | 18 +- .../com/atproto/server/createAppPassword.ts | 15 +- .../com/atproto/server/deactivateAccount.ts | 15 +- .../atproto/server/getAccountInviteCodes.ts | 19 +- .../api/com/atproto/server/getServiceAuth.ts | 23 +- .../com/atproto/server/listAppPasswords.ts | 14 +- .../atproto/server/requestAccountDelete.ts | 16 +- .../server/requestEmailConfirmation.ts | 16 +- .../com/atproto/server/requestEmailUpdate.ts | 17 +- .../com/atproto/server/revokeAppPassword.ts | 13 +- .../src/api/com/atproto/server/updateEmail.ts | 20 +- packages/pds/src/api/proxy.ts | 6 +- packages/pds/src/auth-routes.ts | 4 +- packages/pds/src/auth-verifier.ts | 46 ++- packages/pds/src/index.ts | 13 +- packages/pds/src/oauth/provider.ts | 2 + packages/pds/src/pipethrough.ts | 26 +- packages/pds/tests/app-passwords.test.ts | 4 +- packages/pds/tests/entryway.test.ts | 34 +- pnpm-lock.yaml | 8 - 96 files changed, 1096 insertions(+), 1118 deletions(-) create mode 100644 .changeset/cool-toes-rescue.md create mode 100644 .changeset/fluffy-apples-do.md create mode 100644 .changeset/green-bags-flash.md create mode 100644 .changeset/healthy-bottles-hear.md create mode 100644 .changeset/lemon-mice-rule.md create mode 100644 .changeset/light-dingos-dream.md create mode 100644 .changeset/ninety-ants-collect.md create mode 100644 .changeset/odd-spies-boil.md create mode 100644 .changeset/polite-humans-sleep.md create mode 100644 .changeset/poor-socks-sniff.md create mode 100644 .changeset/short-toes-battle.md create mode 100644 .changeset/six-ties-arrive.md create mode 100644 .changeset/smart-drinks-repeat.md create mode 100644 .changeset/smart-gifts-itch.md create mode 100644 .changeset/thin-cycles-live.md create mode 100644 .changeset/tidy-cars-thank.md rename packages/oauth/oauth-client-browser/example/src/auth/{atp/atp-sign-in-form.tsx => credential/credential-sign-in-form.tsx} (98%) rename packages/oauth/oauth-client-browser/example/src/auth/{atp/use-atp-auth.ts => credential/use-credential-auth.ts} (98%) delete mode 100644 packages/oauth/oauth-provider/src/assets/app/components/client-identifier.tsx delete mode 100644 packages/oauth/oauth-provider/src/oidc/claims.ts delete mode 100644 packages/oauth/oauth-provider/src/oidc/userinfo.ts delete mode 100644 packages/oauth/oauth-provider/src/parameters/claims-requested.ts delete mode 100644 packages/oauth/oauth-provider/src/parameters/oidc-payload.ts diff --git a/.changeset/cool-toes-rescue.md b/.changeset/cool-toes-rescue.md new file mode 100644 index 00000000000..19ab036705c --- /dev/null +++ b/.changeset/cool-toes-rescue.md @@ -0,0 +1,5 @@ +--- +"@atproto/oauth-provider": patch +--- + +Display requested scopes during the auth flow diff --git a/.changeset/fluffy-apples-do.md b/.changeset/fluffy-apples-do.md new file mode 100644 index 00000000000..22747fe35d8 --- /dev/null +++ b/.changeset/fluffy-apples-do.md @@ -0,0 +1,5 @@ +--- +"@atproto/pds": patch +--- + +Use locally defined authPassthru diff --git a/.changeset/green-bags-flash.md b/.changeset/green-bags-flash.md new file mode 100644 index 00000000000..2bbc6286710 --- /dev/null +++ b/.changeset/green-bags-flash.md @@ -0,0 +1,5 @@ +--- +"@atproto/pds": patch +--- + +Add support for "transition:generic" and "transition:chat.bsky" oauth scopes diff --git a/.changeset/healthy-bottles-hear.md b/.changeset/healthy-bottles-hear.md new file mode 100644 index 00000000000..d210a116c98 --- /dev/null +++ b/.changeset/healthy-bottles-hear.md @@ -0,0 +1,5 @@ +--- +"@atproto/oauth-provider": patch +--- + +Generate proper invalid_authorization_details diff --git a/.changeset/lemon-mice-rule.md b/.changeset/lemon-mice-rule.md new file mode 100644 index 00000000000..9bff8aa1682 --- /dev/null +++ b/.changeset/lemon-mice-rule.md @@ -0,0 +1,6 @@ +--- +"@atproto/oauth-provider": minor +"@atproto/oauth-client": minor +--- + +Remove "nonce" from authorization request diff --git a/.changeset/light-dingos-dream.md b/.changeset/light-dingos-dream.md new file mode 100644 index 00000000000..a25c1025a87 --- /dev/null +++ b/.changeset/light-dingos-dream.md @@ -0,0 +1,5 @@ +--- +"@atproto/pds": patch +--- + +Ignore case when checking for dpop auth scheme diff --git a/.changeset/ninety-ants-collect.md b/.changeset/ninety-ants-collect.md new file mode 100644 index 00000000000..8abe5d741ce --- /dev/null +++ b/.changeset/ninety-ants-collect.md @@ -0,0 +1,5 @@ +--- +"@atproto/oauth-provider": patch +--- + +Stronger CORS protections diff --git a/.changeset/odd-spies-boil.md b/.changeset/odd-spies-boil.md new file mode 100644 index 00000000000..5a01cf7f127 --- /dev/null +++ b/.changeset/odd-spies-boil.md @@ -0,0 +1,5 @@ +--- +"@atproto/oauth-types": patch +--- + +Validate scopes characters according to OAuth 2.1 spec diff --git a/.changeset/polite-humans-sleep.md b/.changeset/polite-humans-sleep.md new file mode 100644 index 00000000000..2879f0f6be9 --- /dev/null +++ b/.changeset/polite-humans-sleep.md @@ -0,0 +1,6 @@ +--- +"@atproto/oauth-provider": minor +"@atproto/oauth-client": minor +--- + +Mandate the use of "atproto" scope diff --git a/.changeset/poor-socks-sniff.md b/.changeset/poor-socks-sniff.md new file mode 100644 index 00000000000..a3332467791 --- /dev/null +++ b/.changeset/poor-socks-sniff.md @@ -0,0 +1,5 @@ +--- +"@atproto/oauth-provider": patch +--- + +Do not require user consent during oauth flow for first party apps. diff --git a/.changeset/short-toes-battle.md b/.changeset/short-toes-battle.md new file mode 100644 index 00000000000..cb5f9265ef4 --- /dev/null +++ b/.changeset/short-toes-battle.md @@ -0,0 +1,12 @@ +--- +"@atproto/oauth-provider": minor +"@atproto/oauth-client": minor +"@atproto/oauth-client-browser": minor +"@atproto/oauth-client-node": minor +--- + +Remove "openid" compatibility. The reason is that although we were technically "openid" compatible, ATProto identifiers are distributed identifiers. When a client relies on OpenID to authenticate users, it will use the auth provider in combination with the identifier to uniquely identify the user. Since ATProto identifiers are meant to be able to move from one provider to the other, OpenID compatibility could break authentication after a user was migrated to a different provider. + +The way OpenID compliant clients would adapt to this particularity would typically be to remove the provider + identifier combination and use the identifier alone. While this is indeed the right way to handle ATProto identifiers, it requires more work to avoid impersonation. In particular, when obtaining a user identifier, the client **must** verify that the issuer of the identity token is indeed the server responsible for that user. This mechanism being not enforced by the OpenID standard, OpenID compatibility could lead to security issues. For this reason, we decided to remove OpenID compatibility from the OAuth provider. + +Note that a trusted central authority could still offer OpenID compatibility by relying on ATProto's regular OAuth flow under the hood. This capability is out of the scope of this library. diff --git a/.changeset/six-ties-arrive.md b/.changeset/six-ties-arrive.md new file mode 100644 index 00000000000..a930161f3e9 --- /dev/null +++ b/.changeset/six-ties-arrive.md @@ -0,0 +1,5 @@ +--- +"@atproto/oauth-types": patch +--- + +Re-use code definition of oauthResponseTypeSchema diff --git a/.changeset/smart-drinks-repeat.md b/.changeset/smart-drinks-repeat.md new file mode 100644 index 00000000000..37d86c1d4a7 --- /dev/null +++ b/.changeset/smart-drinks-repeat.md @@ -0,0 +1,5 @@ +--- +"@atproto/oauth-client": patch +--- + +Do not remove scopes not advertised in the AS's "scopes_supported" when building the authorization request. diff --git a/.changeset/smart-gifts-itch.md b/.changeset/smart-gifts-itch.md new file mode 100644 index 00000000000..08ea7f32d87 --- /dev/null +++ b/.changeset/smart-gifts-itch.md @@ -0,0 +1,5 @@ +--- +"@atproto/oauth-types": patch +--- + +Remove non-standard "sub" from OAuthTokenResponse diff --git a/.changeset/thin-cycles-live.md b/.changeset/thin-cycles-live.md new file mode 100644 index 00000000000..8650de71a23 --- /dev/null +++ b/.changeset/thin-cycles-live.md @@ -0,0 +1,5 @@ +--- +"@atproto/pds": patch +--- + +Allow OAuthProvider to define its own CORS policies diff --git a/.changeset/tidy-cars-thank.md b/.changeset/tidy-cars-thank.md new file mode 100644 index 00000000000..f0ecef9684d --- /dev/null +++ b/.changeset/tidy-cars-thank.md @@ -0,0 +1,5 @@ +--- +"@atproto/oauth-provider": patch +--- + +Improve reporting of validation errors diff --git a/packages/api/OAUTH.md b/packages/api/OAUTH.md index 0f6bf4f9f51..e60c59f914d 100644 --- a/packages/api/OAUTH.md +++ b/packages/api/OAUTH.md @@ -40,7 +40,7 @@ Here is an example client metadata. "tos_uri": "https://example.com/tos", "policy_uri": "https://example.com/policy", "redirect_uris": ["https://example.com/callback"], - "scope": "offline_access", + "scope": "atproto", "grant_types": ["authorization_code", "refresh_token"], "response_types": ["code"], "token_endpoint_auth_method": "none", @@ -73,8 +73,7 @@ Here is an example client metadata. the user during the authentication process. - If you don't want or need the user to stay authenticated for long periods - (better for security), you can remove the `offline_access` scope, and - `refresh_token` from the `grant_types`. + (better for security), you can remove `refresh_token` from the `grant_types`. > [!NOTE] > diff --git a/packages/oauth/oauth-client-browser/README.md b/packages/oauth/oauth-client-browser/README.md index 4f3e16fbd94..ce10c240ac5 100644 --- a/packages/oauth/oauth-client-browser/README.md +++ b/packages/oauth/oauth-client-browser/README.md @@ -45,7 +45,7 @@ needs of your application and must respect the [ATPROTO] spec. "tos_uri": "https://my-app.com/tos", "policy_uri": "https://my-app.com/policy", "redirect_uris": ["https://my-app.com/callback"], - "scope": "profile email offline_access", + "scope": "atproto", "grant_types": ["authorization_code", "refresh_token"], "response_types": ["code"], "token_endpoint_auth_method": "none", diff --git a/packages/oauth/oauth-client-browser/example/src/app.tsx b/packages/oauth/oauth-client-browser/example/src/app.tsx index 3625d2f67ca..233c9f21be7 100644 --- a/packages/oauth/oauth-client-browser/example/src/app.tsx +++ b/packages/oauth/oauth-client-browser/example/src/app.tsx @@ -1,9 +1,19 @@ import { useCallback, useState } from 'react' import { useAuthContext } from './auth/auth-provider' +import { OAuthSession } from '@atproto/oauth-client' function App() { const { pdsAgent, signOut } = useAuthContext() + const hasTokenInfo = pdsAgent.sessionManager instanceof OAuthSession + + const [tokeninfo, setTokeninfo] = useState(undefined) + const loadTokeninfo = useCallback(async () => { + if (pdsAgent.sessionManager instanceof OAuthSession) { + setTokeninfo(await pdsAgent.sessionManager.getTokenInfo()) + } + }, [pdsAgent]) + // A call that requires to be authenticated const [serviceAuth, setServiceAuth] = useState(undefined) const loadServiceAuth = useCallback(async () => { @@ -30,6 +40,19 @@ function App() {

Logged in!

+ {hasTokenInfo && ( + <> + + +
+              {tokeninfo !== undefined
+                ? JSON.stringify(tokeninfo, undefined, 2)
+                : null}
+            
+
+ + )} +
diff --git a/packages/oauth/oauth-client-browser/example/src/auth/auth-form.tsx b/packages/oauth/oauth-client-browser/example/src/auth/auth-form.tsx
index 336ef420f45..acec28717c5 100644
--- a/packages/oauth/oauth-client-browser/example/src/auth/auth-form.tsx
+++ b/packages/oauth/oauth-client-browser/example/src/auth/auth-form.tsx
@@ -1,6 +1,9 @@
-import { useCallback, useEffect, useState } from 'react'
+import { useEffect, useState } from 'react'
 
-import { AtpSignIn, AtpSignInForm } from './atp/atp-sign-in-form'
+import {
+  AtpSignIn,
+  CredentialSignInForm,
+} from './credential/credential-sign-in-form'
 import { OAuthSignIn, OAuthSignInForm } from './oauth/oauth-sign-in-form'
 
 export function AuthForm({
@@ -10,19 +13,20 @@ export function AuthForm({
   atpSignIn?: AtpSignIn
   oauthSignIn?: OAuthSignIn
 }) {
-  const defaultMethod = useCallback(
-    () => (oauthSignIn ? 'oauth' : atpSignIn ? 'atp' : undefined),
-    [],
-  )
+  const defaultMethod = oauthSignIn
+    ? 'oauth'
+    : atpSignIn
+      ? 'credential'
+      : undefined
 
-  const [method, setMethod] = useState(
+  const [method, setMethod] = useState(
     defaultMethod,
   )
 
   useEffect(() => {
     if (method === 'oauth' && !oauthSignIn) {
       setMethod(defaultMethod)
-    } else if (method === 'atp' && !atpSignIn) {
+    } else if (method === 'credential' && !atpSignIn) {
       setMethod(defaultMethod)
     } else if (!method) {
       setMethod(defaultMethod)
@@ -45,9 +49,9 @@ export function AuthForm({
 
         
{method === 'oauth' && } - {method === 'atp' && } + {method === 'credential' && } {method == null &&
No auth method available
} ) diff --git a/packages/oauth/oauth-client-browser/example/src/auth/auth-provider.tsx b/packages/oauth/oauth-client-browser/example/src/auth/auth-provider.tsx index f083caba9de..5bb2e94eb37 100644 --- a/packages/oauth/oauth-client-browser/example/src/auth/auth-provider.tsx +++ b/packages/oauth/oauth-client-browser/example/src/auth/auth-provider.tsx @@ -3,7 +3,7 @@ import { Agent } from '@atproto/api' import { createContext, ReactNode, useContext, useMemo } from 'react' -import { useAtpAuth } from './atp/use-atp-auth' +import { useCredentialAuth } from './credential/use-credential-auth' import { AuthForm } from './auth-form' import { useOAuth, UseOAuthOptions } from './oauth/use-oauth' @@ -30,19 +30,19 @@ export const AuthProvider = ({ } = useOAuth(options) const { - agent: atpAgent, - signIn: atpSignIn, - signOut: atpSignOut, - } = useAtpAuth() + agent: credentialAgent, + signIn: credentialSignIn, + signOut: credentialSignOut, + } = useCredentialAuth() const value = useMemo( () => oauthAgent ? { pdsAgent: oauthAgent, signOut: oauthSignOut } - : atpAgent - ? { pdsAgent: atpAgent, signOut: atpSignOut } + : credentialAgent + ? { pdsAgent: credentialAgent, signOut: credentialSignOut } : null, - [oauthAgent, oauthSignOut, atpAgent, atpSignOut], + [oauthAgent, oauthSignOut, credentialAgent, credentialSignOut], ) if (isLoginPopup) { @@ -56,7 +56,7 @@ export const AuthProvider = ({ if (!value) { return ( ) diff --git a/packages/oauth/oauth-client-browser/example/src/auth/atp/atp-sign-in-form.tsx b/packages/oauth/oauth-client-browser/example/src/auth/credential/credential-sign-in-form.tsx similarity index 98% rename from packages/oauth/oauth-client-browser/example/src/auth/atp/atp-sign-in-form.tsx rename to packages/oauth/oauth-client-browser/example/src/auth/credential/credential-sign-in-form.tsx index 00387181ce5..40a651cb7ca 100644 --- a/packages/oauth/oauth-client-browser/example/src/auth/atp/atp-sign-in-form.tsx +++ b/packages/oauth/oauth-client-browser/example/src/auth/credential/credential-sign-in-form.tsx @@ -11,7 +11,7 @@ export type AtpSignIn = (input: { * @returns Nice tailwind css form asking to enter either a handle or the host * to use to login. */ -export function AtpSignInForm({ +export function CredentialSignInForm({ signIn, ...props }: { diff --git a/packages/oauth/oauth-client-browser/example/src/auth/atp/use-atp-auth.ts b/packages/oauth/oauth-client-browser/example/src/auth/credential/use-credential-auth.ts similarity index 98% rename from packages/oauth/oauth-client-browser/example/src/auth/atp/use-atp-auth.ts rename to packages/oauth/oauth-client-browser/example/src/auth/credential/use-credential-auth.ts index 431b81dbd5d..1dd360b79cf 100644 --- a/packages/oauth/oauth-client-browser/example/src/auth/atp/use-atp-auth.ts +++ b/packages/oauth/oauth-client-browser/example/src/auth/credential/use-credential-auth.ts @@ -3,7 +3,7 @@ import { useCallback, useMemo, useState } from 'react' type Session = AtpSessionData & { service: string } -export function useAtpAuth() { +export function useCredentialAuth() { const createAgent = useCallback((service: string) => { const agent = new AtpAgent({ service, diff --git a/packages/oauth/oauth-client-browser/example/src/auth/oauth/use-oauth.ts b/packages/oauth/oauth-client-browser/example/src/auth/oauth/use-oauth.ts index 7256a8c4340..057896a28a0 100644 --- a/packages/oauth/oauth-client-browser/example/src/auth/oauth/use-oauth.ts +++ b/packages/oauth/oauth-client-browser/example/src/auth/oauth/use-oauth.ts @@ -12,13 +12,11 @@ import { OAuthSession, } from '@atproto/oauth-client-browser' +type Gettable = () => PromiseLike | T + export type OnRestored = (session: OAuthSession | null) => void export type OnSignedIn = (session: OAuthSession, state: null | string) => void export type OnSignedOut = () => void -export type GetState = () => - | undefined - | string - | PromiseLike function useCallbackRef any>( fn: T, @@ -145,13 +143,15 @@ export type UseOAuthOptions = ClientOptions & { onRestored?: OnRestored onSignedIn?: OnSignedIn onSignedOut?: OnSignedOut - getState?: GetState + getScope?: Gettable + getState?: Gettable } export function useOAuth(options: UseOAuthOptions) { const onRestored = useCallbackRef(options.onRestored) const onSignedIn = useCallbackRef(options.onSignedIn) const onSignedOut = useCallbackRef(options.onSignedOut) + const getScope = useCallbackRef(options.getScope) const getState = useCallbackRef(options.getState) const clientForInit = useOAuthClient(options) @@ -243,9 +243,6 @@ export function useOAuth(options: UseOAuthOptions) { ) } - // Force fetching the token info in order to trigger a token refresh - void session?.getTokenInfo(true) - return () => { controller.abort() } @@ -256,11 +253,13 @@ export function useOAuth(options: UseOAuthOptions) { if (!client) throw new Error('Client not initialized') const state = options?.state ?? (await getState()) ?? undefined - const session = await client.signIn(input, { ...options, state }) + const scope = options?.scope ?? (await getScope()) ?? 'atproto' + + const session = await client.signIn(input, { ...options, scope, state }) setSession(session) await onSignedIn(session, state ?? null) }, - [client, getState, onSignedIn], + [client, getScope, getState, onSignedIn], ) // Memoize the return value to avoid re-renders in consumers diff --git a/packages/oauth/oauth-client-browser/example/src/main.tsx b/packages/oauth/oauth-client-browser/example/src/main.tsx index aa85dcc1379..ad1488626e8 100644 --- a/packages/oauth/oauth-client-browser/example/src/main.tsx +++ b/packages/oauth/oauth-client-browser/example/src/main.tsx @@ -10,11 +10,12 @@ ReactDOM.createRoot(document.getElementById('root')!).render( 'atproto transition:generic'} > diff --git a/packages/oauth/oauth-client-browser/src/browser-oauth-database.ts b/packages/oauth/oauth-client-browser/src/browser-oauth-database.ts index 18ce8b582d5..b641649ebc5 100644 --- a/packages/oauth/oauth-client-browser/src/browser-oauth-database.ts +++ b/packages/oauth/oauth-client-browser/src/browser-oauth-database.ts @@ -41,7 +41,6 @@ export type Schema = { dpopKey: EncodedKey iss: string - nonce: string verifier?: string appState?: string }> diff --git a/packages/oauth/oauth-client-node/README.md b/packages/oauth/oauth-client-node/README.md index 9a90afb6b87..1499d5b522e 100644 --- a/packages/oauth/oauth-client-node/README.md +++ b/packages/oauth/oauth-client-node/README.md @@ -36,7 +36,6 @@ const client = new NodeOAuthClientOptions({ tos_uri: 'https://my-app.com/tos', policy_uri: 'https://my-app.com/policy', redirect_uris: ['https://my-app.com/callback'], - scope: 'profile email offline_access', grant_types: ['authorization_code', 'refresh_token'], response_types: ['code'], application_type: 'web', @@ -158,7 +157,7 @@ The client metadata will typically contain: "tos_uri": "https://my-app.com/tos", "policy_uri": "https://my-app.com/policy", "redirect_uris": ["https://my-app.com/atproto-oauth-callback"], - "scope": "profile email offline_access", + "scope": "atproto", "grant_types": ["authorization_code", "refresh_token"], "response_types": ["code"], "application_type": "native", diff --git a/packages/oauth/oauth-client/README.md b/packages/oauth/oauth-client/README.md index 7e9b973868b..1057127ed2b 100644 --- a/packages/oauth/oauth-client/README.md +++ b/packages/oauth/oauth-client/README.md @@ -341,7 +341,6 @@ or ```ts const url = await client.authorize(handle, { state, - max_age: 600, // Require re-authentication after 10 minutes }) ``` diff --git a/packages/oauth/oauth-client/src/oauth-client.ts b/packages/oauth/oauth-client/src/oauth-client.ts index 9187911f325..b113fdb97f7 100644 --- a/packages/oauth/oauth-client/src/oauth-client.ts +++ b/packages/oauth/oauth-client/src/oauth-client.ts @@ -261,7 +261,6 @@ export class OAuthClient extends CustomEventTarget { options, ) - const nonce = await this.runtime.generateNonce() const pkce = await this.runtime.generatePKCE() const dpopKey = await this.runtime.generateKey( metadata.dpop_signing_alg_values_supported || [FALLBACK_ALG], @@ -272,17 +271,15 @@ export class OAuthClient extends CustomEventTarget { await this.stateStore.set(state, { iss: metadata.issuer, dpopKey, - nonce, - verifier: pkce?.verifier, + verifier: pkce.verifier, appState: options?.state, }) const parameters = { client_id: this.clientMetadata.client_id, redirect_uri: redirectUri, - code_challenge: pkce?.challenge, - code_challenge_method: pkce?.method, - nonce, + code_challenge: pkce.challenge, + code_challenge_method: pkce.method, state, login_hint: identity ? input // If input is a handle or a DID, use it as a login_hint @@ -295,13 +292,8 @@ export class OAuthClient extends CustomEventTarget { ) ?? 'code', display: options?.display, - id_token_hint: options?.id_token_hint, - max_age: options?.max_age, // this.clientMetadata.default_max_age prompt: options?.prompt, - scope: options?.scope - ?.split(' ') - .filter((s) => metadata.scopes_supported?.includes(s)) - .join(' '), + scope: options?.scope || undefined, ui_locales: options?.ui_locales, } @@ -434,24 +426,12 @@ export class OAuthClient extends CustomEventTarget { const tokenSet = await server.exchangeCode(codeParam, stateData.verifier) try { - if (tokenSet.id_token) { - await this.runtime.validateIdTokenClaims( - tokenSet.id_token, - stateParam, - stateData.nonce, - codeParam, - tokenSet.access_token, - ) - } - - const { sub } = tokenSet - - await this.sessionGetter.setStored(sub, { + await this.sessionGetter.setStored(tokenSet.sub, { dpopKey: stateData.dpopKey, tokenSet, }) - const session = this.createSession(server, sub) + const session = this.createSession(server, tokenSet.sub) return { session, state: stateData.appState ?? null } } catch (err) { diff --git a/packages/oauth/oauth-client/src/oauth-server-agent.ts b/packages/oauth/oauth-client/src/oauth-server-agent.ts index 07231dac523..636bb7490fb 100644 --- a/packages/oauth/oauth-client/src/oauth-server-agent.ts +++ b/packages/oauth/oauth-client/src/oauth-server-agent.ts @@ -1,6 +1,6 @@ import { Fetch, Json, bindFetch, fetchJsonProcessor } from '@atproto-labs/fetch' import { SimpleStore } from '@atproto-labs/simple-store' -import { Key, Keyset, SignedJwt } from '@atproto/jwk' +import { Key, Keyset } from '@atproto/jwk' import { CLIENT_ASSERTION_TYPE_JWT_BEARER, OAuthAuthorizationServerMetadata, @@ -26,9 +26,8 @@ export type TokenSet = { iss: string sub: string aud: string - scope?: string + scope: string - id_token?: SignedJwt refresh_token?: string access_token: string token_type: OAuthTokenType @@ -128,8 +127,17 @@ export class OAuthServerAgent { tokenResponse: OAuthTokenResponse, ): Promise { const { sub } = tokenResponse - // ATPROTO requires that the "sub" is always present in the token response. - if (!sub) throw new TypeError(`Missing "sub" in token response`) + + if (!sub || typeof sub !== 'string') { + throw new TypeError(`Unexpected ${typeof sub} "sub" in token response`) + } + + // Using an array to check for the presence of the "atproto" scope (we don't + // want atproto to be a substring of another scope) + const scopes = tokenResponse.scope?.split(' ') + if (!scopes?.includes('atproto')) { + throw new TypeError('Missing "atproto" scope in token response') + } // @TODO (?) make timeout configurable using signal = timeoutSignal(10e3) @@ -138,7 +146,7 @@ export class OAuthServerAgent { signal, }) - if (resolved.metadata.issuer !== this.serverMetadata.issuer) { + if (this.serverMetadata.issuer !== resolved.metadata.issuer) { // Best case scenario; the user switched PDS. Worst case scenario; a bad // actor is trying to impersonate a user. In any case, we must not allow // this token to be used. @@ -146,12 +154,12 @@ export class OAuthServerAgent { } return { - sub, aud: resolved.identity.pds.href, iss: resolved.metadata.issuer, - scope: tokenResponse.scope, - id_token: tokenResponse.id_token, + sub, + + scope: tokenResponse.scope!, refresh_token: tokenResponse.refresh_token, access_token: tokenResponse.access_token, token_type: tokenResponse.token_type ?? 'Bearer', diff --git a/packages/oauth/oauth-client/src/oauth-session.ts b/packages/oauth/oauth-client/src/oauth-session.ts index 94b14e8f29d..4c5f25f04cb 100644 --- a/packages/oauth/oauth-client/src/oauth-session.ts +++ b/packages/oauth/oauth-client/src/oauth-session.ts @@ -138,7 +138,7 @@ export class OAuthSession { if (isInvalidTokenResponse(finalResponse)) { // TODO: Is there a "softer" way to handle this, e.g. by marking the // session as "expired" in the session store, allowing the user to trigger - // a new login (using login_hint/id_token_hint)? + // a new login (using login_hint)? await this.sessionGetter.delStored( this.sub, new TokenInvalidError(this.sub), diff --git a/packages/oauth/oauth-client/src/runtime.ts b/packages/oauth/oauth-client/src/runtime.ts index 6de49d01d93..88ac002b0a5 100644 --- a/packages/oauth/oauth-client/src/runtime.ts +++ b/packages/oauth/oauth-client/src/runtime.ts @@ -1,12 +1,8 @@ -import { JwtHeader, JwtPayload, Key, unsafeDecodeJwt } from '@atproto/jwk' +import { Key } from '@atproto/jwk' import { base64url } from 'multiformats/bases/base64' import { requestLocalLock } from './lock.js' -import { - DigestAlgorithm, - RuntimeImplementation, - RuntimeLock, -} from './runtime-implementation.js' +import { RuntimeImplementation, RuntimeLock } from './runtime-implementation.js' export class Runtime { readonly hasImplementationLock: boolean @@ -38,64 +34,6 @@ export class Runtime { return base64url.baseEncode(bytes) } - public async validateIdTokenClaims( - token: string, - state: string, - nonce: string, - code?: string, - accessToken?: string, - ): Promise<{ - header: JwtHeader - payload: JwtPayload - }> { - // It's fine to use unsafeDecodeJwt here because the token was received from - // the server's token endpoint. The following checks are to ensure that the - // oauth flow was indeed initiated by the client. - const { header, payload } = unsafeDecodeJwt(token) - if (!payload.nonce || payload.nonce !== nonce) { - throw new TypeError('Nonce mismatch') - } - if (payload.c_hash) { - await this.validateHashClaim(payload.c_hash, code, header) - } - if (payload.s_hash) { - await this.validateHashClaim(payload.s_hash, state, header) - } - if (payload.at_hash) { - await this.validateHashClaim(payload.at_hash, accessToken, header) - } - return { header, payload } - } - - private async validateHashClaim( - claim: unknown, - source: unknown, - header: { alg: string; crv?: string }, - ): Promise { - if (typeof claim !== 'string' || !claim) { - throw new TypeError(`string "_hash" claim expected`) - } - if (typeof source !== 'string' || !source) { - throw new TypeError(`string value expected`) - } - const expected = await this.generateHashClaim(source, header) - if (expected !== claim) { - throw new TypeError(`"_hash" does not match`) - } - } - - protected async generateHashClaim( - source: string, - header: { alg: string; crv?: string }, - ) { - const algo = getHashAlgo(header) - const bytes = new TextEncoder().encode(source) - const digest = await this.implementation.digest(bytes, algo) - if (digest.length % 2 !== 0) throw new TypeError('Invalid digest length') - const digestHalf = digest.slice(0, digest.length / 2) - return base64url.baseEncode(digestHalf) - } - public async generatePKCE(byteLength?: number) { const verifier = await this.generateVerifier(byteLength) return { @@ -127,36 +65,6 @@ export class Runtime { } } -function getHashAlgo(header: { alg: string; crv?: string }): DigestAlgorithm { - switch (header.alg) { - case 'HS256': - case 'RS256': - case 'PS256': - case 'ES256': - case 'ES256K': - return { name: 'sha256' } - case 'HS384': - case 'RS384': - case 'PS384': - case 'ES384': - return { name: 'sha384' } - case 'HS512': - case 'RS512': - case 'PS512': - case 'ES512': - return { name: 'sha512' } - case 'EdDSA': - switch (header.crv) { - case 'Ed25519': - return { name: 'sha512' } - default: - throw new TypeError('unrecognized or invalid EdDSA curve provided') - } - default: - throw new TypeError('unrecognized or invalid JWS algorithm provided') - } -} - function extractJktComponents(jwk) { const get = (field) => { const value = jwk[field] diff --git a/packages/oauth/oauth-client/src/state-store.ts b/packages/oauth/oauth-client/src/state-store.ts index 928148f02db..f49c36dff3a 100644 --- a/packages/oauth/oauth-client/src/state-store.ts +++ b/packages/oauth/oauth-client/src/state-store.ts @@ -3,7 +3,6 @@ import { Key } from '@atproto/jwk' export type InternalStateData = { iss: string - nonce: string dpopKey: Key verifier?: string appState?: string diff --git a/packages/oauth/oauth-client/src/types.ts b/packages/oauth/oauth-client/src/types.ts index ed81aeb42e6..741610560b4 100644 --- a/packages/oauth/oauth-client/src/types.ts +++ b/packages/oauth/oauth-client/src/types.ts @@ -16,10 +16,8 @@ export type AuthorizeOptions = { state?: string signal?: AbortSignal - // Only for OIDC compatible + // Borrowed from OIDC ui_locales?: string - id_token_hint?: string - max_age?: number } export const clientMetadataSchema = oauthClientMetadataSchema.extend({ diff --git a/packages/oauth/oauth-provider/package.json b/packages/oauth/oauth-provider/package.json index 242ba0dfbf4..f272e12af16 100644 --- a/packages/oauth/oauth-provider/package.json +++ b/packages/oauth/oauth-provider/package.json @@ -44,7 +44,6 @@ "ioredis": "^5.3.2", "jose": "^5.2.0", "keygrip": "^1.1.0", - "oidc-token-hash": "^5.0.3", "psl": "^1.9.0", "zod": "^3.23.8" }, diff --git a/packages/oauth/oauth-provider/src/account/account.ts b/packages/oauth/oauth-provider/src/account/account.ts index 0728232375f..7a850f791ca 100644 --- a/packages/oauth/oauth-provider/src/account/account.ts +++ b/packages/oauth/oauth-provider/src/account/account.ts @@ -1,10 +1,14 @@ -import { OIDCStandardPayload } from '../oidc/claims.js' import { Sub } from '../oidc/sub.js' import { Simplify } from '../lib/util/type.js' -export type Account = Simplify< - { - sub: Sub // Account id - aud: string | [string, ...string[]] // Resource server URL - } & OIDCStandardPayload -> +export type Account = Simplify<{ + sub: Sub // Account id + aud: string | [string, ...string[]] // Resource server URL + + // OIDC inspired + preferred_username?: string + email?: string + email_verified?: boolean + picture?: string + name?: string +}> diff --git a/packages/oauth/oauth-provider/src/assets/app/backend-data.ts b/packages/oauth/oauth-provider/src/assets/app/backend-data.ts index 5bf4c4dac0d..ae9f4874812 100644 --- a/packages/oauth/oauth-provider/src/assets/app/backend-data.ts +++ b/packages/oauth/oauth-provider/src/assets/app/backend-data.ts @@ -14,6 +14,7 @@ export type Account = { export type Session = { account: Account + info?: never // Prevent relying on this in the frontend selected: boolean loginRequired: boolean @@ -37,15 +38,21 @@ export type ErrorData = { error_description: string } +export type ScopeDetail = { + scope: string + description?: string +} + export type AuthorizeData = { clientId: string clientMetadata: OAuthClientMetadata clientTrusted: boolean requestUri: string csrfCookie: string - sessions: Session[] - newSessionsRequireConsent: boolean loginHint?: string + scopeDetails?: ScopeDetail[] + newSessionsRequireConsent: boolean + sessions: Session[] } // see "declareBackendData()" in the backend diff --git a/packages/oauth/oauth-provider/src/assets/app/components/accept-form.tsx b/packages/oauth/oauth-provider/src/assets/app/components/accept-form.tsx index d0f57af7403..204493976b4 100644 --- a/packages/oauth/oauth-provider/src/assets/app/components/accept-form.tsx +++ b/packages/oauth/oauth-provider/src/assets/app/components/accept-form.tsx @@ -1,22 +1,23 @@ import { OAuthClientMetadata } from '@atproto/oauth-types' import { FormEvent } from 'react' -import { Account } from '../backend-data' +import { Account, ScopeDetail } from '../backend-data' import { Override } from '../lib/util' import { AccountIdentifier } from './account-identifier' import { Button } from './button' -import { ClientIdentifier } from './client-identifier' import { ClientName } from './client-name' import { FormCard, FormCardProps } from './form-card' -import { Fieldset } from './fieldset' export type AcceptFormProps = Override< FormCardProps, { - account: Account clientId: string clientMetadata: OAuthClientMetadata clientTrusted: boolean + + account: Account + scopeDetails?: ScopeDetail[] + onAccept: () => void acceptLabel?: string @@ -29,10 +30,13 @@ export type AcceptFormProps = Override< > export function AcceptForm({ - account, clientId, clientMetadata, clientTrusted, + + account, + scopeDetails, + onAccept, acceptLabel = 'Accept', onReject, @@ -62,54 +66,64 @@ export function AcceptForm({ } {...props} > -
- } - > - {clientTrusted && clientMetadata.logo_uri && ( -
- {clientMetadata.client_name} -
- )} + {clientTrusted && clientMetadata.logo_uri && ( +
+ {clientMetadata.client_name} +
+ )} +

+ is + asking for permission to access your account ( + + ). +

-

- {' '} - is asking for permission to access your{' '} - account. -

+

+ By clicking {acceptLabel}, you allow this application to perform + the following actions in accordance to their{' '} + + terms of service + + {' and '} + + privacy policy + + : +

-

- By clicking {acceptLabel}, you allow this application to access - your information in accordance to their{' '} - - terms of service - - {' and '} - - privacy policy - - . -

-
+ {scopeDetails?.length ? ( +
    + {scopeDetails.map( + ({ scope, description = getScopeDescription(scope) }) => ( +
  • {description}
  • + ), + )} +
+ ) : null} ) } + +function getScopeDescription(scope: string): string { + switch (scope) { + case 'atproto': + return 'Uniquely identify you' + default: + return scope + } +} diff --git a/packages/oauth/oauth-provider/src/assets/app/components/client-identifier.tsx b/packages/oauth/oauth-provider/src/assets/app/components/client-identifier.tsx deleted file mode 100644 index ed8add6131c..00000000000 --- a/packages/oauth/oauth-provider/src/assets/app/components/client-identifier.tsx +++ /dev/null @@ -1,31 +0,0 @@ -import { - isOAuthClientIdDiscoverable, - isOAuthClientIdLoopback, - OAuthClientMetadata, -} from '@atproto/oauth-types' -import { HTMLAttributes } from 'react' - -import { UrlViewer } from './url-viewer' - -export type ClientIdentifierProps = { - clientId: string - clientMetadata: OAuthClientMetadata - as?: keyof JSX.IntrinsicElements -} - -export function ClientIdentifier({ - clientId, - clientMetadata, - as: As = 'span', - ...attrs -}: ClientIdentifierProps & HTMLAttributes) { - if (isOAuthClientIdLoopback(clientId)) { - return An application on your device - } - - if (isOAuthClientIdDiscoverable(clientId)) { - return - } - - return {clientMetadata.client_name || clientId} -} diff --git a/packages/oauth/oauth-provider/src/assets/app/components/client-name.tsx b/packages/oauth/oauth-provider/src/assets/app/components/client-name.tsx index abafc87d23c..e47575dfa08 100644 --- a/packages/oauth/oauth-provider/src/assets/app/components/client-name.tsx +++ b/packages/oauth/oauth-provider/src/assets/app/components/client-name.tsx @@ -1,30 +1,38 @@ -import { OAuthClientMetadata } from '@atproto/oauth-types' +import { + isOAuthClientIdDiscoverable, + isOAuthClientIdLoopback, + OAuthClientMetadata, +} from '@atproto/oauth-types' import { HTMLAttributes } from 'react' -import { ClientIdentifier } from './client-identifier' +import { UrlViewer } from './url-viewer' export type ClientNameProps = { clientId: string clientMetadata: OAuthClientMetadata - as?: keyof JSX.IntrinsicElements -} +} & HTMLAttributes export function ClientName({ clientId, clientMetadata, - as: As = 'span', ...attrs -}: ClientNameProps & HTMLAttributes) { - if (clientMetadata.client_name) { - return {clientMetadata.client_name} +}: ClientNameProps) { + if (isOAuthClientIdLoopback(clientId)) { + return An application on your device + } + + if (isOAuthClientIdDiscoverable(clientId)) { + if (clientMetadata.client_name) { + return ( + + {clientMetadata.client_name} ( + ) + + ) + } + + return } - return ( - - ) + return {clientMetadata.client_name || clientId} } diff --git a/packages/oauth/oauth-provider/src/assets/app/components/url-viewer.tsx b/packages/oauth/oauth-provider/src/assets/app/components/url-viewer.tsx index 4879a5d8e23..fca5f848587 100644 --- a/packages/oauth/oauth-provider/src/assets/app/components/url-viewer.tsx +++ b/packages/oauth/oauth-provider/src/assets/app/components/url-viewer.tsx @@ -1,4 +1,4 @@ -import { HTMLAttributes, useMemo } from 'react' +import { Component, HTMLAttributes, useMemo } from 'react' export type UrlPartRenderingOptions = { faded?: boolean @@ -28,7 +28,7 @@ export function UrlViewer({ const urlObj = useMemo(() => new URL(url), [url]) return ( - + {proto && ( )} - + ) } diff --git a/packages/oauth/oauth-provider/src/assets/app/views/accept-view.tsx b/packages/oauth/oauth-provider/src/assets/app/views/accept-view.tsx index 6601390fd89..daf99046812 100644 --- a/packages/oauth/oauth-provider/src/assets/app/views/accept-view.tsx +++ b/packages/oauth/oauth-provider/src/assets/app/views/accept-view.tsx @@ -1,6 +1,6 @@ import { OAuthClientMetadata } from '@atproto/oauth-types' -import { Session } from '../backend-data' +import { Account, ScopeDetail } from '../backend-data' import { AcceptForm } from '../components/accept-form' import { LayoutTitlePage } from '../components/layout-title-page' @@ -8,7 +8,9 @@ export type AcceptViewProps = { clientId: string clientMetadata: OAuthClientMetadata clientTrusted: boolean - session: Session + + account: Account + scopeDetails?: ScopeDetail[] onAccept: () => void onReject: () => void @@ -19,12 +21,12 @@ export function AcceptView({ clientId, clientMetadata, clientTrusted, - session, + account, + scopeDetails, onAccept, onReject, onBack, }: AcceptViewProps) { - const { account } = session return ( doAccept(session.account)} onReject={doReject} onBack={ diff --git a/packages/oauth/oauth-provider/src/assets/assets-middleware.ts b/packages/oauth/oauth-provider/src/assets/assets-middleware.ts index 2471e4c9075..f699f33764a 100644 --- a/packages/oauth/oauth-provider/src/assets/assets-middleware.ts +++ b/packages/oauth/oauth-provider/src/assets/assets-middleware.ts @@ -1,8 +1,13 @@ -import { writeStream } from '../lib/http/index.js' +import { + Middleware, + validateFetchDest, + validateFetchSite, + writeStream, +} from '../lib/http/index.js' import { ASSETS_URL_PREFIX, getAsset } from './index.js' -export function authorizeAssetsMiddleware() { +export function authorizeAssetsMiddleware(): Middleware { return async function assetsMiddleware(req, res, next): Promise { if (req.method !== 'GET' && req.method !== 'HEAD') return next() if (!req.url?.startsWith(ASSETS_URL_PREFIX)) return next() @@ -17,6 +22,13 @@ export function authorizeAssetsMiddleware() { const asset = await getAsset(filename).catch(() => null) if (!asset) return next() + try { + validateFetchSite(req, res, ['same-origin']) + validateFetchDest(req, res, ['style', 'script']) + } catch (err) { + return next(err) + } + if (req.headers['if-none-match'] === asset.sha256) { return void res.writeHead(304).end() } diff --git a/packages/oauth/oauth-provider/src/client/client-manager.ts b/packages/oauth/oauth-provider/src/client/client-manager.ts index e44f55b3166..42ca7c328c5 100644 --- a/packages/oauth/oauth-provider/src/client/client-manager.ts +++ b/packages/oauth/oauth-provider/src/client/client-manager.ts @@ -17,6 +17,7 @@ import { isLoopbackUrl, isOAuthClientIdDiscoverable, isOAuthClientIdLoopback, + OAuthAuthorizationServerMetadata, OAuthClientIdDiscoverable, OAuthClientIdLoopback, OAuthClientMetadata, @@ -55,9 +56,10 @@ export type LoopbackMetadataGetter = ( export class ClientManager { protected readonly jwks: CachedGetter - protected readonly metadata: CachedGetter + protected readonly metadataGetter: CachedGetter constructor( + protected readonly serverMetadata: OAuthAuthorizationServerMetadata, protected readonly keyset: Keyset, protected readonly hooks: OAuthHooks, protected readonly store: ClientStore | null, @@ -76,7 +78,7 @@ export class ClientManager { return jwks }, clientJwksCache) - this.metadata = new CachedGetter(async (uri, options) => { + this.metadataGetter = new CachedGetter(async (uri, options) => { const metadata = await fetch(buildJsonGetRequest(uri, options)).then( fetchMetadataHandler, ) @@ -159,7 +161,7 @@ export class ClientManager { ): Promise { const metadataUrl = parseDiscoverableClientId(clientId) - const metadata = await this.metadata.get(metadataUrl.href) + const metadata = await this.metadataGetter.get(metadataUrl.href) // Note: we do *not* re-validate the metadata here, as the metadata is // validated within the getter. This is to avoid double validation. @@ -195,6 +197,18 @@ export class ClientManager { ) } + // Known OIDC specific parameters + for (const k of [ + 'default_max_age', + 'userinfo_signed_response_alg', + 'id_token_signed_response_alg', + 'userinfo_encrypted_response_alg', + ] as const) { + if (metadata[k] != null) { + throw new InvalidClientMetadataError(`Unsupported "${k}" parameter`) + } + } + const clientUriUrl = metadata.client_uri ? new URL(metadata.client_uri) : null @@ -204,13 +218,27 @@ export class ClientManager { throw new InvalidClientMetadataError('client_uri must be a valid URL') } - const scopes = metadata.scope?.split(' ') - if ( - metadata.grant_types.includes('refresh_token') !== - (scopes?.includes('offline_access') ?? false) - ) { + const scopes = metadata.scope?.split(' ').filter(Boolean) + + const dupScope = scopes?.find(isDuplicate) + if (dupScope) { + throw new InvalidClientMetadataError(`Duplicate scope "${dupScope}"`) + } + + if (scopes) { + for (const scope of scopes) { + // Note, once we have dynamic scopes, this check will need to be + // updated to check against the server's supported scopes. + if (!this.serverMetadata.scopes_supported?.includes(scope)) { + throw new InvalidClientMetadataError(`Unsupported scope "${scope}"`) + } + } + } + + const dupGrantType = metadata.grant_types.find(isDuplicate) + if (dupGrantType) { throw new InvalidClientMetadataError( - 'Grant type "refresh_token" requires scope "offline_access" (and vice versa)', + `Duplicate grant type "${dupGrantType}"`, ) } @@ -218,8 +246,8 @@ export class ClientManager { switch (grantType) { case 'authorization_code': case 'refresh_token': - case 'implicit': // Required by OIDC (for id_token) continue + case 'implicit': case 'password': throw new InvalidClientMetadataError( `Grant type "${grantType}" is not allowed`, @@ -241,35 +269,6 @@ export class ClientManager { ) } - if ( - metadata.userinfo_signed_response_alg && - !this.keyset.signAlgorithms.includes( - metadata.userinfo_signed_response_alg, - ) - ) { - throw new InvalidClientMetadataError( - `Unsupported "userinfo_signed_response_alg" ${metadata.userinfo_signed_response_alg}`, - ) - } - - if ( - metadata.id_token_signed_response_alg && - !this.keyset.signAlgorithms.includes( - metadata.id_token_signed_response_alg, - ) - ) { - throw new InvalidClientMetadataError( - `Unsupported "id_token_signed_response_alg" ${metadata.id_token_signed_response_alg}`, - ) - } - - if (metadata.userinfo_encrypted_response_alg) { - // We only support signature for now. - throw new InvalidClientMetadataError( - 'Encrypted userinfo response is not supported', - ) - } - const method = metadata[`token_endpoint_auth_method`] switch (method) { case undefined: @@ -338,37 +337,28 @@ export class ClientManager { } for (const responseType of metadata.response_types) { - const rt = responseType.split(' ') + if (responseType.includes('id_token')) { + throw new InvalidClientMetadataError( + `OpenID Connect response type "${responseType}" is not supported`, + ) + } // ATPROTO spec requires the use of PKCE - if (rt.includes('token')) { + if (responseType !== 'code') { throw new InvalidClientMetadataError( - '"token" response type is not compatible with PKCE (use "code" instead)', + `Unsupported response type "${responseType}"`, ) } // Consistency check if ( - rt.includes('code') && + responseType === 'code' && !metadata.grant_types.includes('authorization_code') ) { throw new InvalidClientMetadataError( `Response type "${responseType}" requires the "authorization_code" grant type`, ) } - - // Asking for "code token" or "code id_token" is fine (as long as the - // grant_types includes "authorization_code" and the scope includes - // "openid"). Asking for "token" or "id_token" (without "code") requires - // the "implicit" grant type. - if ( - (rt.includes('token') || rt.includes('id_token')) && - !metadata.grant_types.includes('implicit') - ) { - throw new InvalidClientMetadataError( - `Response type "${responseType}" requires the "implicit" grant type`, - ) - } } if (metadata.application_type === 'native') { @@ -383,11 +373,33 @@ export class ClientManager { // > accordingly. } + if (metadata.authorization_details_types?.length) { + const dupAuthDetailsType = + metadata.authorization_details_types.find(isDuplicate) + if (dupAuthDetailsType) { + throw new InvalidClientMetadataError( + `Duplicate authorization_details_type "${dupAuthDetailsType}"`, + ) + } + + const authorizationDetailsTypesSupported = + this.serverMetadata.authorization_details_types_supported + if (!authorizationDetailsTypesSupported) { + throw new InvalidClientMetadataError( + 'authorization_details_types are not supported', + ) + } + for (const type of metadata.authorization_details_types) { + if (!authorizationDetailsTypesSupported.includes(type)) { + throw new InvalidClientMetadataError( + `Unsupported authorization_details_type "${type}"`, + ) + } + } + } + if (!metadata.redirect_uris?.length) { - // https://openid.net/specs/openid-connect-registration-1_0.html#rfc.section.2 - // - // > OPs can require that request_uri values used be pre-registered with - // > the require_request_uri_registration discovery parameter. + // ATPROTO spec requires that at least one redirect URI is provided throw new InvalidClientMetadataError( 'At least one redirect_uri is required', @@ -786,6 +798,12 @@ export class ClientManager { } } +function isDuplicate< + T extends string | number | boolean | null | undefined | symbol, +>(value: T, index: number, array: T[]) { + return array.includes(value, index + 1) +} + function reverseDomain(domain: string) { return domain.split('.').reverse().join('.') } diff --git a/packages/oauth/oauth-provider/src/client/client.ts b/packages/oauth/oauth-provider/src/client/client.ts index 5034bf1fdc9..e218aa34c3b 100644 --- a/packages/oauth/oauth-provider/src/client/client.ts +++ b/packages/oauth/oauth-provider/src/client/client.ts @@ -140,6 +140,7 @@ export class Client { audience: checks.audience, subject: this.id, maxTokenAge: CLIENT_ASSERTION_MAX_AGE / 1000, + requiredClaims: ['jti'], }).catch((err) => { if (err instanceof JOSEError) { const msg = `Validation of "client_assertion" failed: ${err.message}` @@ -153,10 +154,6 @@ export class Client { throw new InvalidClientError(`"kid" required in client_assertion`) } - if (!result.payload.jti) { - throw new InvalidClientError(`"jti" required in client_assertion`) - } - const clientAuth: ClientAuth = { method: CLIENT_ASSERTION_TYPE_JWT_BEARER, jkt: await authJwkThumbprint(result.key), diff --git a/packages/oauth/oauth-provider/src/constants.ts b/packages/oauth/oauth-provider/src/constants.ts index 3804e790485..97f25b16c88 100644 --- a/packages/oauth/oauth-provider/src/constants.ts +++ b/packages/oauth/oauth-provider/src/constants.ts @@ -67,3 +67,6 @@ export const DPOP_NONCE_MAX_AGE = 3 * MINUTE /** 5 seconds */ export const SESSION_FIXATION_MAX_AGE = 5 * SECOND + +/** 1 day */ +export const CODE_CHALLENGE_REPLAY_TIMEFRAME = 1 * DAY diff --git a/packages/oauth/oauth-provider/src/device/device-manager.ts b/packages/oauth/oauth-provider/src/device/device-manager.ts index c2d69a9f86e..62431b44df9 100644 --- a/packages/oauth/oauth-provider/src/device/device-manager.ts +++ b/packages/oauth/oauth-provider/src/device/device-manager.ts @@ -100,10 +100,16 @@ export class DeviceManager { public async load( req: IncomingMessage, res: ServerResponse, + forceRotate = false, ): Promise<{ deviceId: DeviceId }> { const cookie = await this.getCookie(req) if (cookie) { - return this.refresh(req, res, cookie.value, cookie.mustRotate) + return this.refresh( + req, + res, + cookie.value, + forceRotate || cookie.mustRotate, + ) } else { return this.create(req, res) } diff --git a/packages/oauth/oauth-provider/src/errors/invalid-authorization-details-error.ts b/packages/oauth/oauth-provider/src/errors/invalid-authorization-details-error.ts index 89d9b0732dc..b9be3d35ccd 100644 --- a/packages/oauth/oauth-provider/src/errors/invalid-authorization-details-error.ts +++ b/packages/oauth/oauth-provider/src/errors/invalid-authorization-details-error.ts @@ -1,4 +1,5 @@ -import { OAuthError } from './oauth-error.js' +import { OAuthAuthenticationRequestParameters } from '@atproto/oauth-types' +import { AccessDeniedError } from './access-denied-error.js' /** * @see @@ -15,8 +16,12 @@ import { OAuthError } from './oauth-error.js' * - contains fields with invalid values for the authorization details type, or * - is missing required fields for the authorization details type. */ -export class InvalidAuthorizationDetailsError extends OAuthError { - constructor(error_description: string, cause?: unknown) { - super('invalid_authorization_details', error_description, 400, cause) +export class InvalidAuthorizationDetailsError extends AccessDeniedError { + constructor( + parameters: OAuthAuthenticationRequestParameters, + error_description: string, + cause?: unknown, + ) { + super(parameters, error_description, 'invalid_authorization_details', cause) } } diff --git a/packages/oauth/oauth-provider/src/lib/http/request.ts b/packages/oauth/oauth-provider/src/lib/http/request.ts index 23f3fb29eac..ac9b1add32c 100644 --- a/packages/oauth/oauth-provider/src/lib/http/request.ts +++ b/packages/oauth/oauth-provider/src/lib/http/request.ts @@ -28,6 +28,27 @@ export async function validateRequestPayload( return schema.parseAsync(payload, { path: ['body'] }) } +export function validateHeaderValue( + req: IncomingMessage, + name: keyof IncomingMessage['headers'], + allowedValues: readonly (string | null)[], +) { + const value = req.headers[name] ?? null + + if (Array.isArray(value)) { + throw createHttpError(400, `Invalid ${name} header`) + } + + if (!allowedValues.includes(value)) { + throw createHttpError( + 400, + value + ? `Forbidden ${name} header "${value}" (expected ${allowedValues})` + : `Missing ${name} header`, + ) + } +} + export function validateFetchMode( req: IncomingMessage, res: ServerResponse, @@ -39,20 +60,45 @@ export function validateFetchMode( | 'cors' )[], ) { - const reqMode = req.headers['sec-fetch-mode'] ?? null + validateHeaderValue(req, 'sec-fetch-mode', expectedMode) +} - if (Array.isArray(reqMode)) { - throw createHttpError(400, `Invalid sec-fetch-mode header`) - } +export function validateFetchDest( + req: IncomingMessage, + res: ServerResponse, + expectedDest: readonly ( + | null + | 'document' + | 'embed' + | 'font' + | 'image' + | 'manifest' + | 'media' + | 'object' + | 'report' + | 'script' + | 'serviceworker' + | 'sharedworker' + | 'style' + | 'worker' + | 'xslt' + )[], +) { + validateHeaderValue(req, 'sec-fetch-dest', expectedDest) +} - if (!(expectedMode as (string | null)[]).includes(reqMode)) { - throw createHttpError( - 403, - reqMode - ? `Forbidden sec-fetch-mode "${reqMode}" (expected ${expectedMode})` - : `Missing sec-fetch-mode (expected ${expectedMode})`, - ) - } +export function validateFetchSite( + req: IncomingMessage, + res: ServerResponse, + expectedSite: readonly ( + | null + | 'same-origin' + | 'same-site' + | 'cross-site' + | 'none' + )[], +) { + validateHeaderValue(req, 'sec-fetch-site', expectedSite) } export function validateReferer( @@ -64,7 +110,7 @@ export function validateReferer( const referer = req.headers['referer'] const refererUrl = referer ? new URL(referer) : null if (refererUrl ? !urlMatch(refererUrl, reference) : !allowNull) { - throw createHttpError(403, `Invalid referer ${referer}`) + throw createHttpError(400, `Invalid referer ${referer}`) } } @@ -95,7 +141,7 @@ export function validateSameOrigin( ) { const reqOrigin = req.headers['origin'] if (reqOrigin ? reqOrigin !== origin : !allowNull) { - throw createHttpError(403, `Invalid origin ${reqOrigin}`) + throw createHttpError(400, `Invalid origin ${reqOrigin}`) } } @@ -113,7 +159,7 @@ export function validateCsrfToken( !cookieName || cookies[cookieName] !== csrfToken ) { - throw createHttpError(403, `Invalid CSRF token`) + throw createHttpError(400, `Invalid CSRF token`) } if (clearCookie) { diff --git a/packages/oauth/oauth-provider/src/metadata/build-metadata.ts b/packages/oauth/oauth-provider/src/metadata/build-metadata.ts index c66820aa971..0bbcd8c9d1b 100644 --- a/packages/oauth/oauth-provider/src/metadata/build-metadata.ts +++ b/packages/oauth/oauth-provider/src/metadata/build-metadata.ts @@ -2,11 +2,9 @@ import { Keyset } from '@atproto/jwk' import { OAuthAuthorizationServerMetadata } from '@atproto/oauth-types' import { Client } from '../client/client.js' -import { OIDC_STANDARD_CLAIMS } from '../oidc/claims.js' import { VERIFY_ALGOS } from '../lib/util/crypto.js' export type CustomMetadata = { - claims_supported?: string[] scopes_supported?: string[] authorization_details_types_supported?: string[] protected_resources?: string[] @@ -25,35 +23,10 @@ export function buildMetadata( issuer, scopes_supported: [ - 'offline_access', - 'openid', - 'email', - 'phone', - 'profile', - + 'atproto', + // ...(customMetadata?.scopes_supported ?? []), ], - claims_supported: [ - /* IESG (Always provided) */ - - 'sub', // did - 'iss', // Authorization Server Origin - 'aud', - 'exp', - 'iat', - 'jti', - 'client_id', - - /* OpenID */ - - // 'acr', // "0" - // 'amr', - // 'azp', - 'auth_time', // number - seconds since epoch - 'nonce', // always required in "id_token", why would it not be supported? - - ...(customMetadata?.claims_supported ?? OIDC_STANDARD_CLAIMS), - ], subject_types_supported: [ // 'public', // The same "sub" is returned for all clients @@ -62,15 +35,15 @@ export function buildMetadata( response_types_supported: [ // OAuth 'code', - 'token', + // 'token', // OpenID - 'none', - 'code id_token token', - 'code id_token', - 'code token', - 'id_token token', - 'id_token', + // 'none', + // 'code id_token token', + // 'code id_token', + // 'code token', + // 'id_token token', + // 'id_token', ], response_modes_supported: [ // https://openid.net/specs/oauth-v2-multiple-response-types-1_0.html#ResponseModes @@ -93,7 +66,6 @@ export function buildMetadata( // 'en-US', ], - id_token_signing_alg_values_supported: [...keyset.signAlgorithms], display_values_supported: [ // 'page', @@ -110,10 +82,6 @@ export function buildMetadata( request_object_encryption_alg_values_supported: [], // None request_object_encryption_enc_values_supported: [], // None - // No claim makes sense to be translated - claims_locales_supported: [], - - claims_parameter_supported: true, request_parameter_supported: true, request_uri_parameter_supported: true, require_request_uri_registration: true, @@ -130,7 +98,6 @@ export function buildMetadata( introspection_endpoint: new URL('/oauth/introspect', issuer).href, - userinfo_endpoint: new URL('/oauth/userinfo', issuer).href, // end_session_endpoint: new URL('/oauth/logout', issuer).href, // https://datatracker.ietf.org/doc/html/rfc9126#section-5 diff --git a/packages/oauth/oauth-provider/src/oauth-hooks.ts b/packages/oauth/oauth-provider/src/oauth-hooks.ts index 2080d735285..c4a4806d2a4 100644 --- a/packages/oauth/oauth-provider/src/oauth-hooks.ts +++ b/packages/oauth/oauth-provider/src/oauth-hooks.ts @@ -11,6 +11,7 @@ import { ClientAuth } from './client/client-auth.js' import { ClientId } from './client/client-id.js' import { ClientInfo } from './client/client-info.js' import { Client } from './client/client.js' +import { InvalidAuthorizationDetailsError } from './errors/invalid-authorization-details-error.js' import { Awaitable } from './lib/util/type.js' // Make sure all types needed to implement the OAuthHooks are exported @@ -20,6 +21,7 @@ export type { ClientAuth, ClientId, ClientInfo, + InvalidAuthorizationDetailsError, Jwks, OAuthAuthenticationRequestParameters, OAuthAuthorizationDetails, @@ -42,7 +44,7 @@ export type OAuthHooks = { /** * Allows enriching the authorization details with additional information - * before the tokens are issued. + * when the tokens are issued. * * @see {@link https://datatracker.ietf.org/doc/html/rfc9396 | RFC 9396} */ @@ -51,16 +53,4 @@ export type OAuthHooks = { parameters: OAuthAuthenticationRequestParameters account: Account }) => Awaitable - - /** - * Allows altering the token response before it is sent to the client. - */ - onTokenResponse?: ( - tokenResponse: OAuthTokenResponse, - data: { - client: Client - parameters: OAuthAuthenticationRequestParameters - account: Account - }, - ) => Awaitable } diff --git a/packages/oauth/oauth-provider/src/oauth-provider.ts b/packages/oauth/oauth-provider/src/oauth-provider.ts index 237644fb42a..1df80bd9125 100644 --- a/packages/oauth/oauth-provider/src/oauth-provider.ts +++ b/packages/oauth/oauth-provider/src/oauth-provider.ts @@ -1,7 +1,7 @@ import { safeFetchWrap } from '@atproto-labs/fetch-node' import { SimpleStore } from '@atproto-labs/simple-store' import { SimpleStoreMemory } from '@atproto-labs/simple-store-memory' -import { Jwks, Keyset, SignedJwt, signedJwtSchema } from '@atproto/jwk' +import { Jwks, Keyset } from '@atproto/jwk' import { AccessToken, CLIENT_ASSERTION_TYPE_JWT_BEARER, @@ -15,12 +15,11 @@ import { oauthAuthenticationRequestParametersSchema, } from '@atproto/oauth-types' import { Redis, type RedisOptions } from 'ioredis' -import { z } from 'zod' +import z, { ZodError } from 'zod' import { AccessTokenType } from './access-token/access-token-type.js' import { AccountManager } from './account/account-manager.js' import { - AccountInfo, AccountStore, DeviceAccountInfo, SignInCredentials, @@ -58,12 +57,13 @@ import { Middleware, Router, ServerResponse, - acceptMiddleware, combineMiddlewares, setupCsrfToken, staticJsonHandler, validateCsrfToken, + validateFetchDest, validateFetchMode, + validateFetchSite, validateReferer, validateRequestPayload, validateSameOrigin, @@ -74,7 +74,6 @@ import { Override } from './lib/util/type.js' import { CustomMetadata, buildMetadata } from './metadata/build-metadata.js' import { OAuthHooks } from './oauth-hooks.js' import { OAuthVerifier, OAuthVerifierOptions } from './oauth-verifier.js' -import { Userinfo } from './oidc/userinfo.js' import { AuthorizationResultAuthorize } from './output/build-authorize-data.js' import { buildErrorPayload, @@ -86,7 +85,6 @@ import { AuthorizationResultRedirect, sendAuthorizeRedirect, } from './output/send-authorize-redirect.js' -import { oidcPayload } from './parameters/oidc-payload.js' import { ReplayStore, ifReplayStore } from './replay/replay-store.js' import { RequestInfo } from './request/request-info.js' import { RequestManager } from './request/request-manager.js' @@ -103,7 +101,7 @@ import { } from './request/types.js' import { isTokenId } from './token/token-id.js' import { TokenManager } from './token/token-manager.js' -import { TokenInfo, TokenStore, asTokenStore } from './token/token-store.js' +import { TokenStore, asTokenStore } from './token/token-store.js' import { CodeGrantRequest, Introspect, @@ -146,9 +144,7 @@ export type OAuthProviderOptions = Override< { /** * Maximum age a device/account session can be before requiring - * re-authentication. This can be overridden on a authorization request basis - * using the `max_age` parameter and on a client basis using the - * `default_max_age` client metadata. + * re-authentication. */ authenticationMaxAge?: number @@ -286,6 +282,7 @@ export class OAuthProvider extends OAuthVerifier { this.accountManager = new AccountManager(accountStore) this.clientManager = new ClientManager( + this.metadata, this.keyset, rest, clientStore || null, @@ -326,14 +323,7 @@ export class OAuthProvider extends OAuthVerifier { return true } - /** in seconds */ - const maxAge = parameters.max_age ?? client.metadata.default_max_age - - if (maxAge != null && maxAge < this.authenticationMaxAge) { - return authAge >= maxAge - } else { - return authAge >= this.authenticationMaxAge - } + return authAge >= this.authenticationMaxAge } protected async authenticateClient( @@ -551,15 +541,14 @@ export class OAuthProvider extends OAuthVerifier { throw new ConsentRequiredError(parameters) } - const redirect = await this.requestManager.setAuthorized( + const code = await this.requestManager.setAuthorized( client, uri, deviceId, ssoSession.account, - ssoSession.info, ) - return { issuer, client, parameters, redirect } + return { issuer, client, parameters, redirect: { code } } } // Automatic SSO when a did was provided @@ -568,15 +557,14 @@ export class OAuthProvider extends OAuthVerifier { if (ssoSessions.length === 1) { const ssoSession = ssoSessions[0]! if (!ssoSession.loginRequired && !ssoSession.consentRequired) { - const redirect = await this.requestManager.setAuthorized( + const code = await this.requestManager.setAuthorized( client, uri, deviceId, ssoSession.account, - ssoSession.info, ) - return { issuer, client, parameters, redirect } + return { issuer, client, parameters, redirect: { code } } } } } @@ -585,7 +573,20 @@ export class OAuthProvider extends OAuthVerifier { issuer, client, parameters, - authorize: { uri, sessions }, + authorize: { + uri, + sessions, + scopeDetails: parameters.scope + ?.split(/\s+/) + .filter(Boolean) + .sort((a, b) => a.localeCompare(b)) + .map((scope) => ({ + scope, + // @TODO Allow to customize the scope descriptions (e.g. + // using a hook) + description: undefined, + })), + }, } } catch (err) { await this.deleteRequest(uri, parameters) @@ -652,6 +653,9 @@ export class OAuthProvider extends OAuthVerifier { this.loginRequired(client, parameters, info), consentRequired: parameters.prompt === 'consent' || + // @TODO the "authorizedClients" should also include the scopes that + // were already authorized for the client. Otherwise a client could + // use silent authentication to get additional scopes without consent. !info.authorizedClients.includes(client.id), matchesHint: hint == null || matchesHint(account), @@ -660,9 +664,33 @@ export class OAuthProvider extends OAuthVerifier { protected async signIn( deviceId: DeviceId, + uri: RequestUri, + clientId: ClientId, credentials: SignInCredentials, - ): Promise { - return this.accountManager.signIn(credentials, deviceId) + ): Promise<{ + account: Account + consentRequired: boolean + }> { + const client = await this.clientManager.getClient(clientId) + + // Ensure the request is still valid (and update the request expiration) + // @TODO use the returned scopes to determine if consent is required + await this.requestManager.get(uri, clientId, deviceId) + + const { account, info } = await this.accountManager.signIn( + credentials, + deviceId, + ) + + return { + account, + consentRequired: client.info.isFirstParty + ? false + : // @TODO: the "authorizedClients" should also include the scopes that + // were already authorized for the client. Otherwise a client could + // use silent authentication to get additional scopes without consent. + !info.authorizedClients.includes(client.id), + } } protected async acceptRequest( @@ -692,12 +720,11 @@ export class OAuthProvider extends OAuthVerifier { ) } - const redirect = await this.requestManager.setAuthorized( + const code = await this.requestManager.setAuthorized( client, uri, deviceId, account, - info, ) await this.accountManager.addAuthorizedClient( @@ -707,7 +734,7 @@ export class OAuthProvider extends OAuthVerifier { clientAuth, ) - return { issuer, client, parameters, redirect } + return { issuer, client, parameters, redirect: { code } } } catch (err) { await this.deleteRequest(uri, parameters) @@ -794,6 +821,32 @@ export class OAuthProvider extends OAuthVerifier { input.code, ) + // the following check prevents re-use of PKCE challenges, enforcing the + // clients to generate a new challenge for each authorization request. The + // replay manager typically prevents replay over a certain time frame, + // which might not cover the entire lifetime of the token (depending on + // the implementation of the replay store). For this reason, we should + // ideally ensure that the code_challenge was not already used by any + // existing token or any other pending request. + // + // The current implementation will cause client devs not issuing a new + // code challenge for each authorization request to fail, which should be + // a good enough incentive to follow the best practices, until we have a + // better implementation. + // + // @TODO: Use tokenManager to ensure uniqueness of code_challenge + if (parameters.code_challenge) { + const unique = await this.replayManager.uniqueCodeChallenge( + parameters.code_challenge, + ) + if (!unique) { + throw new InvalidGrantError( + 'code_challenge', + 'Code challenge already used', + ) + } + } + const { account, info } = await this.accountManager.get(deviceId, sub) return await this.tokenManager.create( @@ -891,31 +944,6 @@ export class OAuthProvider extends OAuthVerifier { } } - /** - * @see {@link https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.5.3.2 Successful UserInfo Response} - */ - protected async userinfo({ data, account }: TokenInfo): Promise { - return { - ...oidcPayload(data.parameters, account), - - sub: account.sub, - - client_id: data.clientId, - username: account.preferred_username, - } - } - - protected async signUserinfo(userinfo: Userinfo): Promise { - const client = await this.clientManager.getClient(userinfo.client_id) - return this.signer.sign( - { - alg: client.metadata.userinfo_signed_response_alg, - typ: 'JWT', - }, - userinfo, - ) - } - protected override async authenticateToken( tokenType: OAuthTokenType, token: AccessToken, @@ -979,6 +1007,8 @@ export class OAuthProvider extends OAuthVerifier { combineMiddlewares([ function (req, res, next) { res.setHeader('Access-Control-Allow-Origin', '*') + res.setHeader('Access-Control-Allow-Headers', '*') + res.setHeader('Cache-Control', 'max-age=300') next() }, @@ -995,6 +1025,7 @@ export class OAuthProvider extends OAuthVerifier { ): Handler => async function (req, res) { res.setHeader('Access-Control-Allow-Origin', '*') + res.setHeader('Access-Control-Allow-Headers', '*') // https://www.rfc-editor.org/rfc/rfc6749.html#section-5.1 res.setHeader('Cache-Control', 'no-store') @@ -1037,11 +1068,15 @@ export class OAuthProvider extends OAuthVerifier { handler: (this: T, req: TReq, res: TRes) => void | Promise, ): Handler => async function (req, res) { + res.setHeader('Access-Control-Allow-Origin', '*') + res.setHeader('Access-Control-Allow-Headers', '*') + res.setHeader('Cache-Control', 'no-store') res.setHeader('Pragma', 'no-cache') try { validateFetchMode(req, res, ['navigate']) + validateFetchDest(req, res, ['document']) validateSameOrigin(req, res, issuerOrigin) await handler.call(this, req, res) @@ -1066,49 +1101,45 @@ export class OAuthProvider extends OAuthVerifier { //- Public OAuth endpoints - /* - * Although OpenID compatibility is not required to implement the Atproto - * OAuth2 specification, we do support OIDC discovery in this - * implementation as we believe this may: - * 1) Make the implementation of Atproto clients easier (since lots of - * libraries support OIDC discovery) - * 2) Allow self hosted PDS' to not implement authentication themselves - * but rely on a trusted Atproto actor to act as their OIDC providers. - * By supporting OIDC in the current implementation, Bluesky's - * Authorization Server server can be used as an OIDC provider for - * these users. - */ - router.get('/.well-known/openid-configuration', staticJson(server.metadata)) - router.get( '/.well-known/oauth-authorization-server', staticJson(server.metadata), ) // CORS preflight - router.options<{ - endpoint: 'jwks' | 'par' | 'token' | 'revoke' | 'introspect' | 'userinfo' - }>( - /^\/oauth\/(?jwks|par|token|revoke|introspect|userinfo)$/, - function (req, res, _next) { - res - .writeHead(204, { - 'Access-Control-Allow-Origin': req.headers['origin'] || '*', - 'Access-Control-Allow-Methods': - this.params.endpoint === 'jwks' ? 'GET' : 'POST', - 'Access-Control-Allow-Headers': 'Content-Type,Authorization,DPoP', - 'Access-Control-Max-Age': '86400', // 1 day - }) - .end() - }, - ) + const corsPreflight: Middleware = function (req, res, _next) { + res + .writeHead(204, { + // https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Origin + // + // > For requests without credentials, the literal value "*" can be + // > specified as a wildcard; the value tells browsers to allow + // > requesting code from any origin to access the resource. + // > Attempting to use the wildcard with credentials results in an + // > error. + // + // A "*" is safer to use than reflecting the request origin. + 'Access-Control-Allow-Origin': '*', + // https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Methods + // > The value "*" only counts as a special wildcard value for + // > requests without credentials (requests without HTTP cookies or + // > HTTP authentication information). In requests with credentials, + // > it is treated as the literal method name "*" without special + // > semantics. + 'Access-Control-Allow-Methods': '*', + 'Access-Control-Allow-Headers': 'Content-Type,Authorization,DPoP', + 'Access-Control-Max-Age': '86400', // 1 day + }) + .end() + } router.get('/oauth/jwks', staticJson(server.jwks)) + router.options('/oauth/par', corsPreflight) router.post( '/oauth/par', jsonHandler(async function (req, _res) { - const input = await validateRequestPayload( + const input = await validateRequest( req, pushedAuthorizationRequestSchema, ) @@ -1124,14 +1155,18 @@ export class OAuthProvider extends OAuthVerifier { ) // https://datatracker.ietf.org/doc/html/rfc9126#section-2.3 - router.addRoute('*', '/oauth/par', (req, res) => { + // > If the request did not use the POST method, the authorization server + // > responds with an HTTP 405 (Method Not Allowed) status code. + router.options('/oauth/par', corsPreflight) + router.all('/oauth/par', (req, res) => { res.writeHead(405).end() }) + router.options('/oauth/token', corsPreflight) router.post( '/oauth/token', jsonHandler(async function (req, _res) { - const input = await validateRequestPayload(req, tokenRequestSchema) + const input = await validateRequest(req, tokenRequestSchema) const dpopJkt = await server.checkDpopProof( req.headers['dpop'], @@ -1143,10 +1178,11 @@ export class OAuthProvider extends OAuthVerifier { }), ) + router.options('/oauth/revoke', corsPreflight) router.post( '/oauth/revoke', jsonHandler(async function (req, res) { - const input = await validateRequestPayload(req, revokeSchema) + const input = await validateRequest(req, revokeSchema) try { await server.revoke(input) @@ -1156,6 +1192,7 @@ export class OAuthProvider extends OAuthVerifier { }), ) + router.options('/oauth/revoke', corsPreflight) router.get( '/oauth/revoke', navigationHandler(async function (req, res) { @@ -1180,69 +1217,11 @@ export class OAuthProvider extends OAuthVerifier { router.post( '/oauth/introspect', jsonHandler(async function (req, _res) { - const input = await validateRequestPayload(req, introspectSchema) + const input = await validateRequest(req, introspectSchema) return server.introspect(input) }), ) - const userinfoBodySchema = z.object({ - access_token: signedJwtSchema.optional(), - }) - - router.addRoute( - ['GET', 'POST'], - '/oauth/userinfo', - acceptMiddleware( - async function (req, _res) { - const body = - req.method === 'POST' - ? await validateRequestPayload(req, userinfoBodySchema) - : null - - if (body?.access_token && req.headers['authorization']) { - throw new InvalidRequestError( - 'access token must be provided in either the authorization header or the request body', - ) - } - - const auth = await server.authenticateRequest( - req.method!, - this.url, - body?.access_token // Allow credentials to be parsed from body. - ? { - authorization: `Bearer ${body.access_token}`, - dpop: undefined, // DPoP can only be used with headers - } - : req.headers, - { - scope: ['profile'], - }, - ) - - const tokenInfo: TokenInfo = - 'tokenInfo' in auth - ? (auth.tokenInfo as TokenInfo) - : await server.tokenManager.getTokenInfo( - auth.tokenType, - auth.tokenId, - ) - - return server.userinfo(tokenInfo) - }, - { - '': 'application/json', - 'application/json': jsonHandler(async function (_req, _res) { - return this.data - }), - 'application/jwt': jsonHandler(async function (_req, res) { - const jwt = await server.signUserinfo(this.data) - res.writeHead(200, { 'Content-Type': 'application/jwt' }).end(jwt) - return undefined - }), - }, - ), - ) - //- Private authorization endpoints router.use(authorizeAssetsMiddleware()) @@ -1250,6 +1229,8 @@ export class OAuthProvider extends OAuthVerifier { router.get( '/oauth/authorize', navigationHandler(async function (req, res) { + validateFetchSite(req, res, ['cross-site', 'none']) + const query = Object.fromEntries(this.url.searchParams) const input = await authorizationRequestQuerySchema.parseAsync(query, { path: ['query'], @@ -1281,13 +1262,15 @@ export class OAuthProvider extends OAuthVerifier { credentials: signInCredentialsSchema, }) + router.options('/oauth/authorize/sign-in', corsPreflight) router.post( '/oauth/authorize/sign-in', jsonHandler(async function (req, res) { validateFetchMode(req, res, ['same-origin']) + validateFetchSite(req, res, ['same-origin']) validateSameOrigin(req, res, issuerOrigin) - const input = await validateRequestPayload(req, signInPayloadSchema) + const input = await validateRequest(req, signInPayloadSchema) validateReferer(req, res, { origin: issuerOrigin, @@ -1300,20 +1283,14 @@ export class OAuthProvider extends OAuthVerifier { csrfCookie(input.request_uri), ) - const { deviceId } = await deviceManager.load(req, res) + const { deviceId } = await deviceManager.load(req, res, true) - const { account, info } = await server.signIn( + return server.signIn( deviceId, + input.request_uri, + input.client_id, input.credentials, ) - - // Prevent fixation attacks - await deviceManager.rotate(req, res, deviceId) - - return { - account, - consentRequired: !info.authorizedClients.includes(input.client_id), - } }), ) @@ -1324,9 +1301,20 @@ export class OAuthProvider extends OAuthVerifier { account_sub: z.string(), }) + // Though this is a "no-cors" request, meaning that the browser will allow + // any cross-origin request, with credentials, to be sent, the handler will + // 1) validate the request origin, + // 2) validate the CSRF token, + // 3) validate the referer, + // 4) validate the sec-fetch-site header, + // 4) validate the sec-fetch-mode header, + // 5) validate the sec-fetch-dest header (see navigationHandler). + // And will error if any of these checks fail. router.get( '/oauth/authorize/accept', navigationHandler(async function (req, res) { + validateFetchSite(req, res, ['same-origin']) + const query = Object.fromEntries(this.url.searchParams) const input = await acceptQuerySchema.parseAsync(query, { path: ['query'], @@ -1367,9 +1355,20 @@ export class OAuthProvider extends OAuthVerifier { client_id: clientIdSchema, }) + // Though this is a "no-cors" request, meaning that the browser will allow + // any cross-origin request, with credentials, to be sent, the handler will + // 1) validate the request origin, + // 2) validate the CSRF token, + // 3) validate the referer, + // 4) validate the sec-fetch-site header, + // 4) validate the sec-fetch-mode header, + // 5) validate the sec-fetch-dest header (see navigationHandler). + // And will error if any of these checks fail. router.get( '/oauth/authorize/reject', navigationHandler(async function (req, res) { + validateFetchSite(req, res, ['same-origin']) + const query = Object.fromEntries(this.url.searchParams) const input = await rejectQuerySchema.parseAsync(query, { path: ['query'], @@ -1406,3 +1405,26 @@ export class OAuthProvider extends OAuthVerifier { return router } } + +async function validateRequest( + req: IncomingMessage, + schema: S, +): Promise> { + try { + return await validateRequestPayload(req, schema) + } catch (err) { + if (err instanceof ZodError) { + const issue = err.issues[0] + if (issue?.path.length) { + // "part" will typically be + const [part, ...path] = issue.path + throw new InvalidRequestError( + `Validation of ${part}'s "${path.join('.')}" with error: ${issue.message}`, + err, + ) + } + } + + throw new InvalidRequestError('Input validation error', err) + } +} diff --git a/packages/oauth/oauth-provider/src/oauth-verifier.ts b/packages/oauth/oauth-provider/src/oauth-verifier.ts index 4d8cefa443d..a9d9414aec2 100644 --- a/packages/oauth/oauth-provider/src/oauth-verifier.ts +++ b/packages/oauth/oauth-provider/src/oauth-verifier.ts @@ -36,8 +36,7 @@ export type OAuthVerifierOptions = Override< issuer: URL | string /** - * The keyset used to sign tokens. Note that OIDC requires that at least one - * RS256 key is present in the keyset. ATPROTO requires ES256. + * The keyset used to sign access tokens. */ keyset: Keyset | Iterable diff --git a/packages/oauth/oauth-provider/src/oidc/claims.ts b/packages/oauth/oauth-provider/src/oidc/claims.ts deleted file mode 100644 index cbcbfe2dc8d..00000000000 --- a/packages/oauth/oauth-provider/src/oidc/claims.ts +++ /dev/null @@ -1,35 +0,0 @@ -import { JwtPayload } from '@atproto/jwk' - -/** - * @see {@link https://openid.net/specs/openid-connect-core-1_0.html#ScopeClaims | OpenID Connect Core 1.0, 5.4. Requesting Claims using Scope Values} - */ -export const OIDC_SCOPE_CLAIMS = Object.freeze({ - email: Object.freeze(['email', 'email_verified'] as const), - phone: Object.freeze(['phone_number', 'phone_number_verified'] as const), - address: Object.freeze(['address'] as const), - profile: Object.freeze([ - 'name', - 'family_name', - 'given_name', - 'middle_name', - 'nickname', - 'preferred_username', - 'gender', - 'picture', - 'profile', - 'website', - 'birthdate', - 'zoneinfo', - 'locale', - 'updated_at', - ] as const), -}) - -export const OIDC_STANDARD_CLAIMS = Object.freeze( - Object.values(OIDC_SCOPE_CLAIMS).flat(), -) - -export type OIDCStandardClaim = (typeof OIDC_STANDARD_CLAIMS)[number] -export type OIDCStandardPayload = Partial<{ - [K in OIDCStandardClaim]?: JwtPayload[K] -}> diff --git a/packages/oauth/oauth-provider/src/oidc/userinfo.ts b/packages/oauth/oauth-provider/src/oidc/userinfo.ts deleted file mode 100644 index 3810c6e3748..00000000000 --- a/packages/oauth/oauth-provider/src/oidc/userinfo.ts +++ /dev/null @@ -1,11 +0,0 @@ -import { OIDCStandardPayload } from './claims.js' - -export type Userinfo = OIDCStandardPayload & { - // "The sub (subject) Claim MUST always be returned in the UserInfo Response." - sub: string - - // client_id is not mandatory per spec, but we require it here for convenience - client_id: string - - username?: string -} diff --git a/packages/oauth/oauth-provider/src/output/build-authorize-data.ts b/packages/oauth/oauth-provider/src/output/build-authorize-data.ts index 722fa9a15c6..225ccb0208c 100644 --- a/packages/oauth/oauth-provider/src/output/build-authorize-data.ts +++ b/packages/oauth/oauth-provider/src/output/build-authorize-data.ts @@ -8,12 +8,18 @@ import { Account } from '../account/account.js' import { Client } from '../client/client.js' import { RequestUri } from '../request/request-uri.js' +export type ScopeDetail = { + scope: string + description?: string +} + export type AuthorizationResultAuthorize = { issuer: string client: Client parameters: OAuthAuthenticationRequestParameters authorize: { uri: RequestUri + scopeDetails?: ScopeDetail[] sessions: readonly { account: Account info: DeviceAccountInfo @@ -44,6 +50,7 @@ export type AuthorizeData = { requestUri: string csrfCookie: string loginHint?: string + scopeDetails?: ScopeDetail[] newSessionsRequireConsent: boolean sessions: Session[] } @@ -59,6 +66,7 @@ export function buildAuthorizeData( csrfCookie: `csrf-${data.authorize.uri}`, loginHint: data.parameters.login_hint, newSessionsRequireConsent: data.parameters.prompt === 'consent', + scopeDetails: data.authorize.scopeDetails, sessions: data.authorize.sessions.map( (session): Session => ({ account: session.account, diff --git a/packages/oauth/oauth-provider/src/parameters/claims-requested.ts b/packages/oauth/oauth-provider/src/parameters/claims-requested.ts deleted file mode 100644 index c02d5ab667d..00000000000 --- a/packages/oauth/oauth-provider/src/parameters/claims-requested.ts +++ /dev/null @@ -1,106 +0,0 @@ -import { - OAuthAuthenticationRequestParameters, - OidcClaimsParameter, - OidcEntityType, -} from '@atproto/oauth-types' -import { InvalidRequestError } from '../errors/invalid-request-error.js' - -export function claimRequested( - parameters: OAuthAuthenticationRequestParameters, - entityType: OidcEntityType, - claimName: OidcClaimsParameter, - value: unknown, -): boolean { - if (claimAvailable(parameters, entityType, claimName, value)) { - return true - } - - const entityClaims = parameters.claims?.[entityType] - if (entityClaims?.[claimName]?.essential === true) { - // https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.5.5.1 - // - // > By requesting Claims as Essential Claims, the RP indicates to the - // > End-User that releasing these Claims will ensure a smooth - // > authorization for the specific task requested by the End-User. Note - // > that even if the Claims are not available because the End-User did - // > not authorize their release or they are not present, the - // > Authorization Server MUST NOT generate an error when Claims are not - // > returned, whether they are Essential or Voluntary, unless otherwise - // > specified in the description of the specific claim. - switch (claimName) { - case 'acr': - // https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.5.5.1.1 - // - // > If this is an Essential Claim and the requirement cannot be met, - // > then the Authorization Server MUST treat that outcome as a failed - // > authentication attempt. - throw new InvalidRequestError( - `Unable to provide essential claim: ${claimName}`, - ) - } - } - - return false -} - -function claimAvailable( - parameters: OAuthAuthenticationRequestParameters, - entityType: OidcEntityType, - claimName: OidcClaimsParameter, - value: unknown, -): boolean { - if (value === undefined) return false - - if (parameters.claims) { - const entityClaims = parameters.claims[entityType] - if (entityClaims === undefined) return false - - const claimConfig = entityClaims[claimName] - if (claimConfig === undefined) return false - if (claimConfig === null) return true - - if ( - claimConfig.value !== undefined && - !compareClaimValue(claimConfig.value, value) - ) { - return false - } - - if ( - claimConfig?.values !== undefined && - !claimConfig.values.some((v) => compareClaimValue(v, value)) - ) { - return false - } - } - - return true -} - -type DefinedValue = NonNullable | null - -function compareClaimValue( - expectedValue: DefinedValue, - value: DefinedValue, -): boolean { - const expectedType = typeof expectedValue - const valueType = typeof value - - if (expectedType !== valueType) return false - - switch (typeof expectedValue) { - case 'undefined': - case 'string': - case 'number': - case 'boolean': - return expectedValue === value - case 'object': - if (expectedValue === null) return value === null - // @TODO (?): allow object comparison - // falls through - default: - throw new InvalidRequestError( - `Unable to compare claim value of type ${expectedType}`, - ) - } -} diff --git a/packages/oauth/oauth-provider/src/parameters/oidc-payload.ts b/packages/oauth/oauth-provider/src/parameters/oidc-payload.ts deleted file mode 100644 index 5077890e388..00000000000 --- a/packages/oauth/oauth-provider/src/parameters/oidc-payload.ts +++ /dev/null @@ -1,28 +0,0 @@ -import { OAuthAuthenticationRequestParameters } from '@atproto/oauth-types' -import { Account } from '../account/account.js' -import { OIDCStandardPayload, OIDC_SCOPE_CLAIMS } from '../oidc/claims.js' -import { claimRequested } from './claims-requested.js' - -export function oidcPayload( - params: OAuthAuthenticationRequestParameters, - account: Account, -) { - const payload: OIDCStandardPayload = {} - - const scopes = params.scope ? params.scope?.split(' ') : undefined - if (scopes) { - for (const [scope, claims] of Object.entries(OIDC_SCOPE_CLAIMS)) { - const allowed = scopes.includes(scope) - for (const claim of claims) { - const value = allowed ? account[claim] : undefined - // Should not throw as RequestManager should have already checked - // that all the essential claims are available. - if (claimRequested(params, 'id_token', claim, value)) { - payload[claim] = value as any // All good as long as the account props match the claims - } - } - } - } - - return payload -} diff --git a/packages/oauth/oauth-provider/src/replay/replay-manager.ts b/packages/oauth/oauth-provider/src/replay/replay-manager.ts index 0433a7a0b66..3d8f71cb4a6 100644 --- a/packages/oauth/oauth-provider/src/replay/replay-manager.ts +++ b/packages/oauth/oauth-provider/src/replay/replay-manager.ts @@ -2,6 +2,7 @@ import { ClientId } from '../client/client-id.js' import { CLIENT_ASSERTION_MAX_AGE, DPOP_NONCE_MAX_AGE, + CODE_CHALLENGE_REPLAY_TIMEFRAME, JAR_MAX_AGE, } from '../constants.js' import { ReplayStore } from './replay-store.js' @@ -35,4 +36,12 @@ export class ReplayManager { asTimeFrame(DPOP_NONCE_MAX_AGE), ) } + + async uniqueCodeChallenge(challenge: string): Promise { + return this.replayStore.unique( + 'CodeChallenge', + challenge, + asTimeFrame(CODE_CHALLENGE_REPLAY_TIMEFRAME), + ) + } } diff --git a/packages/oauth/oauth-provider/src/replay/replay-store.ts b/packages/oauth/oauth-provider/src/replay/replay-store.ts index 95b83a965d7..67dc324a7c5 100644 --- a/packages/oauth/oauth-provider/src/replay/replay-store.ts +++ b/packages/oauth/oauth-provider/src/replay/replay-store.ts @@ -9,7 +9,7 @@ export interface ReplayStore { * strictly necessary for security purposes, the namespace should be used to * mitigate denial of service attacks from one client to the other. * - * @param timeFrame expressed in milliseconds. Will never exceed 24 hours. + * @param timeFrame expressed in milliseconds. */ unique( namespace: string, diff --git a/packages/oauth/oauth-provider/src/request/request-info.ts b/packages/oauth/oauth-provider/src/request/request-info.ts index eb4f76b7490..533339c6d9f 100644 --- a/packages/oauth/oauth-provider/src/request/request-info.ts +++ b/packages/oauth/oauth-provider/src/request/request-info.ts @@ -1,4 +1,5 @@ import { OAuthAuthenticationRequestParameters } from '@atproto/oauth-types' +import { ClientId } from '../client/client-id.js' import { ClientAuth } from '../client/client-auth.js' import { RequestId } from './request-id.js' import { RequestUri } from './request-uri.js' @@ -8,5 +9,6 @@ export type RequestInfo = { uri: RequestUri parameters: Readonly expiresAt: Date + clientId: ClientId clientAuth: ClientAuth } diff --git a/packages/oauth/oauth-provider/src/request/request-manager.ts b/packages/oauth/oauth-provider/src/request/request-manager.ts index d8d38bd44f6..6d8bdcd205a 100644 --- a/packages/oauth/oauth-provider/src/request/request-manager.ts +++ b/packages/oauth/oauth-provider/src/request/request-manager.ts @@ -4,7 +4,6 @@ import { OAuthAuthorizationServerMetadata, } from '@atproto/oauth-types' -import { DeviceAccountInfo } from '../account/account-store.js' import { Account } from '../account/account.js' import { ClientAuth } from '../client/client-auth.js' import { ClientId } from '../client/client-id.js' @@ -17,12 +16,12 @@ import { import { DeviceId } from '../device/device-id.js' import { AccessDeniedError } from '../errors/access-denied-error.js' import { ConsentRequiredError } from '../errors/consent-required-error.js' +import { InvalidAuthorizationDetailsError } from '../errors/invalid-authorization-details-error.js' import { InvalidGrantError } from '../errors/invalid-grant-error.js' import { InvalidParametersError } from '../errors/invalid-parameters-error.js' import { InvalidRequestError } from '../errors/invalid-request-error.js' import { compareRedirectUri } from '../lib/util/redirect-uri.js' import { OAuthHooks } from '../oauth-hooks.js' -import { OIDC_SCOPE_CLAIMS } from '../oidc/claims.js' import { Signer } from '../signer/signer.js' import { Code, generateCode } from './code.js' import { @@ -44,7 +43,6 @@ export class RequestManager { protected readonly signer: Signer, protected readonly metadata: OAuthAuthorizationServerMetadata, protected readonly hooks: OAuthHooks, - protected readonly pkceRequired = true, protected readonly tokenMaxAge = TOKEN_MAX_AGE, ) {} @@ -83,7 +81,7 @@ export class RequestManager { }) const uri = encodeRequestUri(id) - return { id, uri, expiresAt, parameters, clientAuth } + return { id, uri, expiresAt, parameters, clientId: client.id, clientAuth } } async validate( @@ -91,8 +89,28 @@ export class RequestManager { clientAuth: ClientAuth, parameters: Readonly, dpopJkt: null | string, - pkceRequired = this.pkceRequired, ): Promise> { + for (const k of [ + // Known unsupported OIDC parameters + 'claims', + 'id_token_hint', + 'nonce', // note that OIDC "nonce" is redundant with PKCE + ] as const) { + if (parameters[k]) { + throw new InvalidParametersError( + parameters, + `Unsupported "${k}" parameter`, + ) + } + } + + if (parameters.response_type !== 'code') { + throw new InvalidParametersError( + parameters, + 'Only "code" response type is allowed', + ) + } + // https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-10#section-1.4.1 // > The authorization server MAY fully or partially ignore the scope // > requested by the client, based on the authorization server policy or @@ -101,59 +119,75 @@ export class RequestManager { // > server MUST include the scope response parameter in the token response // > (Section 3.2.3) to inform the client of the actual scope granted. - const cScopes = client.metadata.scope?.split(' ') + const cScopes = client.metadata.scope?.split(' ').filter(Boolean) const sScopes = this.metadata.scopes_supported - const scopes = - (parameters.scope || client.metadata.scope) - ?.split(' ') - .filter((scope) => !!scope && (sScopes?.includes(scope) ?? true)) ?? [] + const scopes = new Set( + parameters.scope?.split(' ').filter(Boolean) || cScopes, + ) + + if (scopes.has('openid')) { + throw new InvalidParametersError( + parameters, + 'OpenID Connect is not supported', + ) + } + + if (!scopes.has('atproto')) { + throw new InvalidParametersError( + parameters, + 'The "atproto" scope is required', + ) + } for (const scope of scopes) { - if (!cScopes?.includes(scope)) { + // Loopback clients do not define any scope in their metadata + if (cScopes && !cScopes.includes(scope)) { throw new InvalidParametersError( parameters, `Scope "${scope}" is not registered for this client`, ) } - } - for (const [scope, claims] of Object.entries(OIDC_SCOPE_CLAIMS)) { - for (const claim of claims) { - if ( - parameters?.claims?.id_token?.[claim]?.essential === true || - parameters?.claims?.userinfo?.[claim]?.essential === true - ) { - if (!scopes?.includes(scope)) { - throw new InvalidParametersError( - parameters, - `Essential ${claim} claim requires "${scope}" scope`, - ) - } - } + // Currently, the implementation requires all the scopes to be statically + // defined in the server metadata. In the future, we might add support + // for dynamic scopes. + if (!sScopes?.includes(scope)) { + throw new InvalidParametersError( + parameters, + `Scope "${scope}" is not supported by this server`, + ) } } - parameters = { ...parameters, scope: scopes.join(' ') } - - const responseTypes = parameters.response_type.split(' ') + parameters = { ...parameters, scope: [...scopes].join(' ') || undefined } if (parameters.authorization_details) { const clientAuthDetailsTypes = client.metadata.authorization_details_types if (!clientAuthDetailsTypes) { - throw new InvalidParametersError( + throw new InvalidAuthorizationDetailsError( parameters, 'Client Metadata does not declare any "authorization_details"', ) } for (const detail of parameters.authorization_details) { - if (!clientAuthDetailsTypes?.includes(detail.type)) { - throw new InvalidParametersError( + if ( + !this.metadata.authorization_details_types_supported?.includes( + detail.type, + ) + ) { + throw new InvalidAuthorizationDetailsError( parameters, `Unsupported "authorization_details" type "${detail.type}"`, ) } + if (!clientAuthDetailsTypes?.includes(detail.type)) { + throw new InvalidAuthorizationDetailsError( + parameters, + `Client Metadata does not declare any "authorization_details" of type "${detail.type}"`, + ) + } } } @@ -197,16 +231,9 @@ export class RequestManager { ) } - if (pkceRequired && responseTypes.includes('token')) { - throw new InvalidParametersError( - parameters, - `Response type "${parameters.response_type}" is incompatible with PKCE`, - 'unsupported_response_type', - ) - } - // https://datatracker.ietf.org/doc/html/rfc7636#section-4.4.1 - if (pkceRequired && !parameters.code_challenge) { + // PKCE is mandatory + if (!parameters.code_challenge) { throw new InvalidParametersError(parameters, 'code_challenge is required') } @@ -229,50 +256,15 @@ export class RequestManager { ) } - // https://openid.net/specs/openid-connect-core-1_0.html#HybridAuthRequest - // - // > nonce: REQUIRED if the Response Type of the request is "code id_token" or - // > "code id_token token" and OPTIONAL when the Response Type of the - // > request is "code token". It is a string value used to associate a - // > Client session with an ID Token, and to mitigate replay attacks. The - // > value is passed through unmodified from the Authentication Request to - // > the ID Token. Sufficient entropy MUST be present in the nonce values - // > used to prevent attackers from guessing values. For implementation - // > notes, see Section 15.5.2. - if (responseTypes.includes('id_token') && !parameters.nonce) { - throw new InvalidParametersError( - parameters, - 'nonce is required for implicit and hybrid flows', - ) - } - - // Make "expensive" checks after the "cheaper" checks - - if (parameters.id_token_hint != null) { - const { payload } = await this.signer.verify(parameters.id_token_hint, { - // these are meant to be outdated when used as a hint - clockTolerance: Infinity, - }) - - if (!payload.sub) { - throw new InvalidParametersError( - parameters, - `Unexpected empty id_token_hint "sub"`, - ) - } else if (parameters.login_hint == null) { - parameters = { ...parameters, login_hint: payload.sub } - } else if (parameters.login_hint !== payload.sub) { - throw new InvalidParametersError( - parameters, - 'login_hint does not match "sub" of id_token_hint', - ) - } - } - - // ATPROTO extension: if the client is not trusted, force users to consent - // to authorization requests. We do this to avoid unauthenticated clients - // from being able to silently re-authenticate users. - if (clientAuth.method === 'none' && !client.info.isFirstParty) { + // ATPROTO extension: if the client is not trusted, and not authenticated, + // force users to consent to authorization requests. We do this to avoid + // unauthenticated clients from being able to silently re-authenticate + // users. + if ( + !client.info.isTrusted && + !client.info.isFirstParty && + clientAuth.method === 'none' + ) { if (parameters.prompt === 'none') { throw new ConsentRequiredError( parameters, @@ -346,6 +338,7 @@ export class RequestManager { uri, expiresAt: updates.expiresAt || data.expiresAt, parameters: data.parameters, + clientId: data.clientId, clientAuth: data.clientAuth, } } @@ -355,8 +348,7 @@ export class RequestManager { uri: RequestUri, deviceId: DeviceId, account: Account, - info: DeviceAccountInfo, - ): Promise<{ code?: Code; token?: string; id_token?: string }> { + ): Promise { const id = decodeRequestUri(uri) const data = await this.store.readRequest(id) @@ -385,18 +377,8 @@ export class RequestManager { ) } - const responseType = data.parameters.response_type.split(' ') - - if (responseType.includes('token')) { - throw new AccessDeniedError( - data.parameters, - 'Implicit "token" forbidden (use "code" with PKCE instead)', - ) - } - - const code = responseType.includes('code') - ? await generateCode() - : undefined + // Only response_type=code is supported + const code = await generateCode() // Bind the request to the account, preventing it from being used again. await this.store.updateRequest(id, { @@ -406,15 +388,7 @@ export class RequestManager { expiresAt: new Date(Date.now() + AUTHORIZATION_INACTIVITY_TIMEOUT), }) - const id_token = responseType.includes('id_token') - ? await this.signer.idToken(client, data.parameters, account, { - auth_time: info.authenticatedAt, - exp: this.createTokenExpiry(), - code, - }) - : undefined - - return { code, id_token } + return code } catch (err) { await this.store.deleteRequest(id) throw err diff --git a/packages/oauth/oauth-provider/src/signer/signer.ts b/packages/oauth/oauth-provider/src/signer/signer.ts index 54af407eef3..8a4d3bb5a25 100644 --- a/packages/oauth/oauth-provider/src/signer/signer.ts +++ b/packages/oauth/oauth-provider/src/signer/signer.ts @@ -1,5 +1,3 @@ -import { randomBytes } from 'node:crypto' - import { JwtPayload, JwtPayloadGetter, @@ -12,14 +10,10 @@ import { OAuthAuthenticationRequestParameters, OAuthAuthorizationDetails, } from '@atproto/oauth-types' -import { generate as hash } from 'oidc-token-hash' import { Account } from '../account/account.js' import { Client } from '../client/client.js' -import { InvalidClientMetadataError } from '../errors/invalid-client-metadata-error.js' import { dateToEpoch } from '../lib/util/date.js' -import { claimRequested } from '../parameters/claims-requested.js' -import { oidcPayload } from '../parameters/oidc-payload.js' import { TokenId } from '../token/token-id.js' import { SignedTokenPayload, @@ -105,61 +99,4 @@ export class Signer { return result } - - async idToken( - client: Client, - params: OAuthAuthenticationRequestParameters, - account: Account, - extra: { - exp: Date - iat?: Date - auth_time?: Date - code?: string - access_token?: string - }, - ): Promise { - // This can happen when a client is using password_grant. If a client is - // using password_grant, it should not set "require_auth_time" to true. - if (client.metadata.require_auth_time && extra.auth_time == null) { - throw new InvalidClientMetadataError( - '"require_auth_time" metadata is not compatible with "password_grant" flow', - ) - } - - return this.sign( - { - alg: client.metadata.id_token_signed_response_alg, - typ: 'JWT', - }, - async ({ alg }, key) => ({ - ...oidcPayload(params, account), - - aud: client.id, - iat: dateToEpoch(extra.iat), - exp: dateToEpoch(extra.exp), - sub: account.sub, - jti: randomBytes(16).toString('hex'), - scope: params.scope, - nonce: params.nonce, - - s_hash: params.state // - ? await hash(params.state, alg, key.crv) - : undefined, - c_hash: extra.code // - ? await hash(extra.code, alg, key.crv) - : undefined, - at_hash: extra.access_token // - ? await hash(extra.access_token, alg, key.crv) - : undefined, - - // https://openid.net/specs/openid-provider-authentication-policy-extension-1_0.html#rfc.section.5.2 - auth_time: - client.metadata.require_auth_time || - (extra.auth_time != null && params.max_age != null) || - claimRequested(params, 'id_token', 'auth_time', extra.auth_time) - ? dateToEpoch(extra.auth_time!) - : undefined, - }), - ) - } } diff --git a/packages/oauth/oauth-provider/src/token/token-manager.ts b/packages/oauth/oauth-provider/src/token/token-manager.ts index 9f9ea3cfbff..46a75e6d967 100644 --- a/packages/oauth/oauth-provider/src/token/token-manager.ts +++ b/packages/oauth/oauth-provider/src/token/token-manager.ts @@ -1,4 +1,4 @@ -import { isSignedJwt, SignedJwt } from '@atproto/jwk' +import { isSignedJwt } from '@atproto/jwk' import { AccessToken, CLIENT_ASSERTION_TYPE_JWT_BEARER, @@ -140,6 +140,10 @@ export class TokenManager { if (!('code_verifier' in input) || !input.code_verifier) { throw new InvalidGrantError('code_verifier is required') } + // Prevent client from generating too short code_verifiers + if (input.code_verifier.length < 43) { + throw new InvalidGrantError('code_verifier too short') + } switch (parameters.code_challenge_method) { case undefined: // Default is "plain" (per spec) case 'plain': { @@ -181,8 +185,7 @@ export class TokenManager { } const tokenId = await generateTokenId() - const scopes = parameters.scope?.split(' ') - const refreshToken = scopes?.includes('offline_access') + const refreshToken = client.metadata.grant_types.includes('refresh_token') ? await generateRefreshToken() : undefined @@ -222,22 +225,10 @@ export class TokenManager { authorization_details: authorizationDetails, }) - const idToken = scopes?.includes('openid') - ? await this.signer.idToken(client, parameters, account, { - exp: expiresAt, - iat: now, - // If there is no deviceInfo, we are in a "password_grant" context - auth_time: device?.info.authenticatedAt || new Date(), - access_token: accessToken, - code, - }) - : undefined - return this.buildTokenResponse( client, accessToken, refreshToken, - idToken, expiresAt, parameters, account, @@ -249,7 +240,6 @@ export class TokenManager { client: Client, accessToken: AccessToken, refreshToken: string | undefined, - idToken: SignedJwt | undefined, expiresAt: Date, parameters: OAuthAuthenticationRequestParameters, account: Account, @@ -259,8 +249,7 @@ export class TokenManager { access_token: accessToken, token_type: parameters.dpop_jkt ? 'DPoP' : 'Bearer', refresh_token: refreshToken, - id_token: idToken, - scope: parameters.scope ?? '', + scope: parameters.scope, authorization_details: authorizationDetails, get expires_in() { return dateToRelativeSeconds(expiresAt) @@ -272,12 +261,6 @@ export class TokenManager { sub: account.sub, } - await this.hooks.onTokenResponse?.call(null, tokenResponse, { - client, - parameters, - account, - }) - return tokenResponse } @@ -316,7 +299,7 @@ export class TokenManager { throw new InvalidGrantError(`Invalid refresh token`) } - const { account, info, data } = tokenInfo + const { account, data } = tokenInfo const { parameters } = data try { @@ -400,26 +383,10 @@ export class TokenManager { authorization_details, }) - // https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.3.1.3.3 - // - // > In addition to the response parameters specified by OAuth 2.0, the - // > following parameters MUST be included in the response: - // > - id_token: ID Token value associated with the authenticated session. - const scopes = parameters.scope?.split(' ') - const idToken = scopes?.includes('openid') - ? await this.signer.idToken(client, parameters, account, { - exp: expiresAt, - iat: now, - auth_time: info?.authenticatedAt, - access_token: accessToken, - }) - : undefined - return this.buildTokenResponse( client, accessToken, nextRefreshToken, - idToken, expiresAt, parameters, account, diff --git a/packages/oauth/oauth-types/src/atproto-loopback-client-metadata.ts b/packages/oauth/oauth-types/src/atproto-loopback-client-metadata.ts index f6a0346b91f..e93327b948f 100644 --- a/packages/oauth/oauth-types/src/atproto-loopback-client-metadata.ts +++ b/packages/oauth/oauth-types/src/atproto-loopback-client-metadata.ts @@ -21,9 +21,8 @@ export function atprotoLoopbackClientMetadata( return { client_id: clientId, client_name: 'Loopback client', - response_types: ['code id_token', 'code'], - grant_types: ['authorization_code', 'implicit', 'refresh_token'], - scope: 'openid profile offline_access', + response_types: ['code'], + grant_types: ['authorization_code', 'refresh_token'], redirect_uris: (redirectUris.length ? redirectUris : (['127.0.0.1', '[::1]'] as const).map( diff --git a/packages/oauth/oauth-types/src/oauth-authentication-request-parameters.ts b/packages/oauth/oauth-types/src/oauth-authentication-request-parameters.ts index be02c606410..2d060f53b91 100644 --- a/packages/oauth/oauth-types/src/oauth-authentication-request-parameters.ts +++ b/packages/oauth/oauth-types/src/oauth-authentication-request-parameters.ts @@ -3,6 +3,7 @@ import { z } from 'zod' import { oauthAuthorizationDetailsSchema } from './oauth-authorization-details.js' import { oauthClientIdSchema } from './oauth-client-id.js' +import { oauthResponseTypeSchema } from './oauth-response-type.js' import { oidcClaimsParameterSchema } from './oidc-claims-parameter.js' import { oidcClaimsPropertiesSchema } from './oidc-claims-properties.js' import { oidcEntityTypeSchema } from './oidc-entity-type.js' @@ -17,19 +18,7 @@ export const oauthAuthenticationRequestParametersSchema = z.object({ nonce: z.string().optional(), dpop_jkt: z.string().optional(), - response_type: z.enum([ - // OAuth2 (https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-10#section-4.1.1) - 'code', - 'token', - - // OIDC (https://openid.net/specs/oauth-v2-multiple-response-types-1_0.html) - 'id_token', - 'none', - 'code token', - 'code id_token', - 'id_token token', - 'code id_token token', - ]), + response_type: oauthResponseTypeSchema, // Default depend on response_type response_mode: z.enum(['query', 'fragment', 'form_post']).optional(), @@ -40,10 +29,13 @@ export const oauthAuthenticationRequestParametersSchema = z.object({ redirect_uri: z.string().url().optional(), - // email profile openid (other?) + // https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-11#section-1.4.1 + // scope = scope-token *( SP scope-token ) + // scope-token = 1*( %x21 / %x23-5B / %x5D-7E ) + // = Basically most ASCII characters except backslash and double quote scope: z .string() - .regex(/^[a-zA-Z0-9_]+( [a-zA-Z0-9_]+)*$/) + .regex(/^[!\x23-\x5B\x5D-\x7E]+( [!\x23-\x5B\x5D-\x7E]+)*$/) .optional(), // OIDC diff --git a/packages/oauth/oauth-types/src/oauth-response-type.ts b/packages/oauth/oauth-types/src/oauth-response-type.ts index 1f65d22380c..5f0269a378b 100644 --- a/packages/oauth/oauth-types/src/oauth-response-type.ts +++ b/packages/oauth/oauth-types/src/oauth-response-type.ts @@ -1,11 +1,11 @@ import { z } from 'zod' export const oauthResponseTypeSchema = z.enum([ - // OAuth + // OAuth2 (https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-10#section-4.1.1) 'code', // Authorization Code Grant 'token', // Implicit Grant - // OpenID + // OIDC (https://openid.net/specs/oauth-v2-multiple-response-types-1_0.html) 'none', 'code id_token token', 'code id_token', diff --git a/packages/oauth/oauth-types/src/oauth-token-response.ts b/packages/oauth/oauth-types/src/oauth-token-response.ts index 198f37c532b..1f8cf772a02 100644 --- a/packages/oauth/oauth-types/src/oauth-token-response.ts +++ b/packages/oauth/oauth-types/src/oauth-token-response.ts @@ -12,7 +12,6 @@ export const oauthTokenResponseSchema = z access_token: z.string(), token_type: oauthTokenTypeSchema, issuer: z.string().url().optional(), - sub: z.string().optional(), scope: z.string().optional(), id_token: signedJwtSchema.optional(), refresh_token: z.string().optional(), diff --git a/packages/pds/src/api/com/atproto/admin/sendEmail.ts b/packages/pds/src/api/com/atproto/admin/sendEmail.ts index 2cd48307686..491cf1b7f24 100644 --- a/packages/pds/src/api/com/atproto/admin/sendEmail.ts +++ b/packages/pds/src/api/com/atproto/admin/sendEmail.ts @@ -26,14 +26,14 @@ export default function (server: Server, ctx: AppContext) { if (ctx.entrywayAgent) { assert(ctx.cfg.entryway) return resultPassthru( - await ctx.entrywayAgent.com.atproto.admin.sendEmail(input.body, { - encoding: 'application/json', - ...(await ctx.serviceAuthHeaders( + await ctx.entrywayAgent.com.atproto.admin.sendEmail( + input.body, + await ctx.serviceAuthHeaders( recipientDid, ctx.cfg.entryway.did, ids.ComAtprotoAdminSendEmail, - )), - }), + ), + ), ) } diff --git a/packages/pds/src/api/com/atproto/admin/updateAccountEmail.ts b/packages/pds/src/api/com/atproto/admin/updateAccountEmail.ts index cf1aefa007b..421cc3073da 100644 --- a/packages/pds/src/api/com/atproto/admin/updateAccountEmail.ts +++ b/packages/pds/src/api/com/atproto/admin/updateAccountEmail.ts @@ -1,7 +1,7 @@ import { InvalidRequestError } from '@atproto/xrpc-server' import { Server } from '../../../../lexicon' import AppContext from '../../../../context' -import { authPassthru } from '../../../proxy' +import { authPassthru } from './util' export default function (server: Server, ctx: AppContext) { server.com.atproto.admin.updateAccountEmail({ diff --git a/packages/pds/src/api/com/atproto/admin/updateAccountPassword.ts b/packages/pds/src/api/com/atproto/admin/updateAccountPassword.ts index f553678313a..0c7a0f8c384 100644 --- a/packages/pds/src/api/com/atproto/admin/updateAccountPassword.ts +++ b/packages/pds/src/api/com/atproto/admin/updateAccountPassword.ts @@ -1,6 +1,6 @@ import { Server } from '../../../../lexicon' import AppContext from '../../../../context' -import { authPassthru } from '../../../proxy' +import { authPassthru } from './util' export default function (server: Server, ctx: AppContext) { server.com.atproto.admin.updateAccountPassword({ diff --git a/packages/pds/src/api/com/atproto/identity/requestPlcOperationSignature.ts b/packages/pds/src/api/com/atproto/identity/requestPlcOperationSignature.ts index 4aab6520062..d611eddc5a0 100644 --- a/packages/pds/src/api/com/atproto/identity/requestPlcOperationSignature.ts +++ b/packages/pds/src/api/com/atproto/identity/requestPlcOperationSignature.ts @@ -1,16 +1,24 @@ -import { Server } from '../../../../lexicon' -import AppContext from '../../../../context' +import assert from 'node:assert' + import { InvalidRequestError } from '@atproto/xrpc-server' -import { authPassthru } from '../../../proxy' + +import AppContext from '../../../../context' +import { Server } from '../../../../lexicon' +import { ids } from '../../../../lexicon/lexicons' export default function (server: Server, ctx: AppContext) { server.com.atproto.identity.requestPlcOperationSignature({ auth: ctx.authVerifier.accessFull(), - handler: async ({ auth, req }) => { + handler: async ({ auth }) => { if (ctx.entrywayAgent) { + assert(ctx.cfg.entryway) await ctx.entrywayAgent.com.atproto.identity.requestPlcOperationSignature( undefined, - authPassthru(req), + await ctx.serviceAuthHeaders( + auth.credentials.did, + ctx.cfg.entryway.did, + ids.ComAtprotoIdentityRequestPlcOperationSignature, + ), ) return } diff --git a/packages/pds/src/api/com/atproto/identity/signPlcOperation.ts b/packages/pds/src/api/com/atproto/identity/signPlcOperation.ts index eb2d05e9d90..3b1f646c671 100644 --- a/packages/pds/src/api/com/atproto/identity/signPlcOperation.ts +++ b/packages/pds/src/api/com/atproto/identity/signPlcOperation.ts @@ -1,19 +1,28 @@ -import { Server } from '../../../../lexicon' -import AppContext from '../../../../context' -import * as plc from '@did-plc/lib' +import assert from 'node:assert' + import { check } from '@atproto/common' import { InvalidRequestError } from '@atproto/xrpc-server' -import { authPassthru, resultPassthru } from '../../../proxy' +import * as plc from '@did-plc/lib' + +import AppContext from '../../../../context' +import { Server } from '../../../../lexicon' +import { ids } from '../../../../lexicon/lexicons' +import { resultPassthru } from '../../../proxy' export default function (server: Server, ctx: AppContext) { server.com.atproto.identity.signPlcOperation({ auth: ctx.authVerifier.accessFull(), - handler: async ({ auth, input, req }) => { + handler: async ({ auth, input }) => { if (ctx.entrywayAgent) { + assert(ctx.cfg.entryway) return resultPassthru( await ctx.entrywayAgent.com.atproto.identity.signPlcOperation( input.body, - authPassthru(req, true), + await ctx.serviceAuthHeaders( + auth.credentials.did, + ctx.cfg.entryway.did, + ids.ComAtprotoIdentitySignPlcOperation, + ), ), ) } diff --git a/packages/pds/src/api/com/atproto/identity/updateHandle.ts b/packages/pds/src/api/com/atproto/identity/updateHandle.ts index 591488f4285..7b0863a2575 100644 --- a/packages/pds/src/api/com/atproto/identity/updateHandle.ts +++ b/packages/pds/src/api/com/atproto/identity/updateHandle.ts @@ -1,10 +1,11 @@ +import assert from 'node:assert' import { InvalidRequestError } from '@atproto/xrpc-server' import { DAY, MINUTE } from '@atproto/common' import { normalizeAndValidateHandle } from '../../../../handle' import { Server } from '../../../../lexicon' import AppContext from '../../../../context' import { httpLogger } from '../../../../logger' -import { authPassthru } from '../../../proxy' +import { ids } from '../../../../lexicon/lexicons' export default function (server: Server, ctx: AppContext) { server.com.atproto.identity.updateHandle({ @@ -21,16 +22,22 @@ export default function (server: Server, ctx: AppContext) { calcKey: ({ auth }) => auth.credentials.did, }, ], - handler: async ({ auth, input, req }) => { + handler: async ({ auth, input }) => { const requester = auth.credentials.did if (ctx.entrywayAgent) { + assert(ctx.cfg.entryway) + // the full flow is: // -> entryway(identity.updateHandle) [update handle, submit plc op] // -> pds(admin.updateAccountHandle) [track handle, sequence handle update] await ctx.entrywayAgent.com.atproto.identity.updateHandle( { did: requester, handle: input.body.handle }, - authPassthru(req, true), + await ctx.serviceAuthHeaders( + auth.credentials.did, + ctx.cfg.entryway.did, + ids.ComAtprotoIdentityUpdateHandle, + ), ) return } diff --git a/packages/pds/src/api/com/atproto/server/activateAccount.ts b/packages/pds/src/api/com/atproto/server/activateAccount.ts index c46c73e547d..8d7c50476a4 100644 --- a/packages/pds/src/api/com/atproto/server/activateAccount.ts +++ b/packages/pds/src/api/com/atproto/server/activateAccount.ts @@ -1,20 +1,29 @@ +import assert from 'node:assert' + import { CidSet } from '@atproto/repo' -import { InvalidRequestError } from '@atproto/xrpc-server' import { INVALID_HANDLE } from '@atproto/syntax' -import { Server } from '../../../../lexicon' +import { InvalidRequestError } from '@atproto/xrpc-server' + import AppContext from '../../../../context' +import { Server } from '../../../../lexicon' +import { ids } from '../../../../lexicon/lexicons' import { assertValidDidDocumentForService } from './util' -import { authPassthru } from '../../../proxy' export default function (server: Server, ctx: AppContext) { server.com.atproto.server.activateAccount({ auth: ctx.authVerifier.accessFull(), - handler: async ({ auth, req }) => { + handler: async ({ auth }) => { // in the case of entryway, the full flow is activateAccount (PDS) -> activateAccount (Entryway) -> updateSubjectStatus(PDS) if (ctx.entrywayAgent) { + assert(ctx.cfg.entryway) + await ctx.entrywayAgent.com.atproto.server.activateAccount( undefined, - authPassthru(req), + await ctx.serviceAuthHeaders( + auth.credentials.did, + ctx.cfg.entryway.did, + ids.ComAtprotoServerActivateAccount, + ), ) return } diff --git a/packages/pds/src/api/com/atproto/server/confirmEmail.ts b/packages/pds/src/api/com/atproto/server/confirmEmail.ts index 11a935ce1e4..d01891e2bc9 100644 --- a/packages/pds/src/api/com/atproto/server/confirmEmail.ts +++ b/packages/pds/src/api/com/atproto/server/confirmEmail.ts @@ -1,12 +1,15 @@ -import { Server } from '../../../../lexicon' -import AppContext from '../../../../context' +import assert from 'node:assert' + import { InvalidRequestError } from '@atproto/xrpc-server' -import { authPassthru } from '../../../proxy' + +import AppContext from '../../../../context' +import { Server } from '../../../../lexicon' +import { ids } from '../../../../lexicon/lexicons' export default function (server: Server, ctx: AppContext) { server.com.atproto.server.confirmEmail({ auth: ctx.authVerifier.accessStandard({ checkTakedown: true }), - handler: async ({ auth, input, req }) => { + handler: async ({ auth, input }) => { const did = auth.credentials.did const user = await ctx.accountManager.getAccount(did, { @@ -17,9 +20,14 @@ export default function (server: Server, ctx: AppContext) { } if (ctx.entrywayAgent) { + assert(ctx.cfg.entryway) await ctx.entrywayAgent.com.atproto.server.confirmEmail( input.body, - authPassthru(req, true), + await ctx.serviceAuthHeaders( + auth.credentials.did, + ctx.cfg.entryway.did, + ids.ComAtprotoServerConfirmEmail, + ), ) return } diff --git a/packages/pds/src/api/com/atproto/server/createAppPassword.ts b/packages/pds/src/api/com/atproto/server/createAppPassword.ts index 2edf2c42d20..3dcd1c6e556 100644 --- a/packages/pds/src/api/com/atproto/server/createAppPassword.ts +++ b/packages/pds/src/api/com/atproto/server/createAppPassword.ts @@ -1,18 +1,27 @@ +import assert from 'node:assert' + import AppContext from '../../../../context' import { Server } from '../../../../lexicon' -import { authPassthru, resultPassthru } from '../../../proxy' +import { ids } from '../../../../lexicon/lexicons' +import { resultPassthru } from '../../../proxy' export default function (server: Server, ctx: AppContext) { server.com.atproto.server.createAppPassword({ auth: ctx.authVerifier.accessFull({ checkTakedown: true, }), - handler: async ({ auth, input, req }) => { + handler: async ({ auth, input }) => { if (ctx.entrywayAgent) { + assert(ctx.cfg.entryway) + return resultPassthru( await ctx.entrywayAgent.com.atproto.server.createAppPassword( input.body, - authPassthru(req, true), + await ctx.serviceAuthHeaders( + auth.credentials.did, + ctx.cfg.entryway.did, + ids.ComAtprotoServerCreateAppPassword, + ), ), ) } diff --git a/packages/pds/src/api/com/atproto/server/deactivateAccount.ts b/packages/pds/src/api/com/atproto/server/deactivateAccount.ts index 48f64c978bb..a73bce60f9e 100644 --- a/packages/pds/src/api/com/atproto/server/deactivateAccount.ts +++ b/packages/pds/src/api/com/atproto/server/deactivateAccount.ts @@ -1,16 +1,23 @@ -import { Server } from '../../../../lexicon' +import assert from 'node:assert' + import AppContext from '../../../../context' -import { authPassthru } from '../../../proxy' +import { Server } from '../../../../lexicon' +import { ids } from '../../../../lexicon/lexicons' export default function (server: Server, ctx: AppContext) { server.com.atproto.server.deactivateAccount({ auth: ctx.authVerifier.accessFull(), - handler: async ({ auth, input, req }) => { + handler: async ({ auth, input }) => { // in the case of entryway, the full flow is deactivateAccount (PDS) -> deactivateAccount (Entryway) -> updateSubjectStatus(PDS) if (ctx.entrywayAgent) { + assert(ctx.cfg.entryway) await ctx.entrywayAgent.com.atproto.server.deactivateAccount( input.body, - authPassthru(req, true), + await ctx.serviceAuthHeaders( + auth.credentials.did, + ctx.cfg.entryway.did, + ids.ComAtprotoServerDeactivateAccount, + ), ) return } diff --git a/packages/pds/src/api/com/atproto/server/getAccountInviteCodes.ts b/packages/pds/src/api/com/atproto/server/getAccountInviteCodes.ts index da84821435b..655921e9d3f 100644 --- a/packages/pds/src/api/com/atproto/server/getAccountInviteCodes.ts +++ b/packages/pds/src/api/com/atproto/server/getAccountInviteCodes.ts @@ -1,19 +1,28 @@ +import assert from 'node:assert' + import { InvalidRequestError } from '@atproto/xrpc-server' -import { Server } from '../../../../lexicon' + +import { CodeDetail } from '../../../../account-manager/helpers/invite' import AppContext from '../../../../context' +import { Server } from '../../../../lexicon' +import { resultPassthru } from '../../../proxy' import { genInvCodes } from './util' -import { CodeDetail } from '../../../../account-manager/helpers/invite' -import { authPassthru, resultPassthru } from '../../../proxy' +import { ids } from '../../../../lexicon/lexicons' export default function (server: Server, ctx: AppContext) { server.com.atproto.server.getAccountInviteCodes({ auth: ctx.authVerifier.accessFull({ checkTakedown: true }), - handler: async ({ params, auth, req }) => { + handler: async ({ params, auth }) => { if (ctx.entrywayAgent) { + assert(ctx.cfg.entryway) return resultPassthru( await ctx.entrywayAgent.com.atproto.server.getAccountInviteCodes( params, - authPassthru(req), + await ctx.serviceAuthHeaders( + auth.credentials.did, + ctx.cfg.entryway.did, + ids.ComAtprotoServerGetAccountInviteCodes, + ), ), ) } diff --git a/packages/pds/src/api/com/atproto/server/getServiceAuth.ts b/packages/pds/src/api/com/atproto/server/getServiceAuth.ts index 6436db59f9f..7a9a2c7a617 100644 --- a/packages/pds/src/api/com/atproto/server/getServiceAuth.ts +++ b/packages/pds/src/api/com/atproto/server/getServiceAuth.ts @@ -2,7 +2,7 @@ import { InvalidRequestError, createServiceJwt } from '@atproto/xrpc-server' import { HOUR, MINUTE } from '@atproto/common' import AppContext from '../../../../context' import { Server } from '../../../../lexicon' -import { PRIVILEGED_METHODS } from '../../../../pipethrough' +import { PRIVILEGED_METHODS, PROTECTED_METHODS } from '../../../../pipethrough' export default function (server: Server, ctx: AppContext) { server.com.atproto.server.getServiceAuth({ @@ -30,15 +30,20 @@ export default function (server: Server, ctx: AppContext) { ) } } - if ( - !auth.credentials.isPrivileged && - lxm && - PRIVILEGED_METHODS.has(lxm) - ) { - throw new InvalidRequestError( - `cannot request a service auth token for the following method with an app password: ${lxm}`, - ) + + if (lxm) { + if (PROTECTED_METHODS.has(lxm)) { + throw new InvalidRequestError( + `cannot request a service auth token for the following protected method: ${lxm}`, + ) + } + if (!auth.credentials.isPrivileged && PRIVILEGED_METHODS.has(lxm)) { + throw new InvalidRequestError( + `insufficient access to request a service auth token for the following method: ${lxm}`, + ) + } } + const keypair = await ctx.actorStore.keypair(did) const token = await createServiceJwt({ diff --git a/packages/pds/src/api/com/atproto/server/listAppPasswords.ts b/packages/pds/src/api/com/atproto/server/listAppPasswords.ts index 65db42c357a..7414e3452c9 100644 --- a/packages/pds/src/api/com/atproto/server/listAppPasswords.ts +++ b/packages/pds/src/api/com/atproto/server/listAppPasswords.ts @@ -1,16 +1,24 @@ +import assert from 'node:assert' + import AppContext from '../../../../context' import { Server } from '../../../../lexicon' -import { authPassthru, resultPassthru } from '../../../proxy' +import { ids } from '../../../../lexicon/lexicons' +import { resultPassthru } from '../../../proxy' export default function (server: Server, ctx: AppContext) { server.com.atproto.server.listAppPasswords({ auth: ctx.authVerifier.accessStandard(), - handler: async ({ auth, req }) => { + handler: async ({ auth }) => { if (ctx.entrywayAgent) { + assert(ctx.cfg.entryway) return resultPassthru( await ctx.entrywayAgent.com.atproto.server.listAppPasswords( undefined, - authPassthru(req), + await ctx.serviceAuthHeaders( + auth.credentials.did, + ctx.cfg.entryway.did, + ids.ComAtprotoServerListAppPasswords, + ), ), ) } diff --git a/packages/pds/src/api/com/atproto/server/requestAccountDelete.ts b/packages/pds/src/api/com/atproto/server/requestAccountDelete.ts index e00f1e57c84..3068ff4519a 100644 --- a/packages/pds/src/api/com/atproto/server/requestAccountDelete.ts +++ b/packages/pds/src/api/com/atproto/server/requestAccountDelete.ts @@ -1,8 +1,11 @@ +import assert from 'node:assert' + import { DAY, HOUR } from '@atproto/common' import { InvalidRequestError } from '@atproto/xrpc-server' -import { Server } from '../../../../lexicon' + import AppContext from '../../../../context' -import { authPassthru } from '../../../proxy' +import { Server } from '../../../../lexicon' +import { ids } from '../../../../lexicon/lexicons' export default function (server: Server, ctx: AppContext) { server.com.atproto.server.requestAccountDelete({ @@ -19,7 +22,7 @@ export default function (server: Server, ctx: AppContext) { }, ], auth: ctx.authVerifier.accessFull({ checkTakedown: true }), - handler: async ({ auth, req }) => { + handler: async ({ auth }) => { const did = auth.credentials.did const account = await ctx.accountManager.getAccount(did, { includeDeactivated: true, @@ -30,9 +33,14 @@ export default function (server: Server, ctx: AppContext) { } if (ctx.entrywayAgent) { + assert(ctx.cfg.entryway) await ctx.entrywayAgent.com.atproto.server.requestAccountDelete( undefined, - authPassthru(req), + await ctx.serviceAuthHeaders( + auth.credentials.did, + ctx.cfg.entryway.did, + ids.ComAtprotoServerRequestAccountDelete, + ), ) return } diff --git a/packages/pds/src/api/com/atproto/server/requestEmailConfirmation.ts b/packages/pds/src/api/com/atproto/server/requestEmailConfirmation.ts index bebd2d32a85..6e1724f4155 100644 --- a/packages/pds/src/api/com/atproto/server/requestEmailConfirmation.ts +++ b/packages/pds/src/api/com/atproto/server/requestEmailConfirmation.ts @@ -1,8 +1,11 @@ +import assert from 'node:assert' + import { DAY, HOUR } from '@atproto/common' import { InvalidRequestError } from '@atproto/xrpc-server' -import { Server } from '../../../../lexicon' + import AppContext from '../../../../context' -import { authPassthru } from '../../../proxy' +import { Server } from '../../../../lexicon' +import { ids } from '../../../../lexicon/lexicons' export default function (server: Server, ctx: AppContext) { server.com.atproto.server.requestEmailConfirmation({ @@ -19,7 +22,7 @@ export default function (server: Server, ctx: AppContext) { }, ], auth: ctx.authVerifier.accessStandard({ checkTakedown: true }), - handler: async ({ auth, req }) => { + handler: async ({ auth }) => { const did = auth.credentials.did const account = await ctx.accountManager.getAccount(did, { includeDeactivated: true, @@ -30,9 +33,14 @@ export default function (server: Server, ctx: AppContext) { } if (ctx.entrywayAgent) { + assert(ctx.cfg.entryway) await ctx.entrywayAgent.com.atproto.server.requestEmailConfirmation( undefined, - authPassthru(req), + await ctx.serviceAuthHeaders( + auth.credentials.did, + ctx.cfg.entryway.did, + ids.ComAtprotoServerRequestEmailConfirmation, + ), ) return } diff --git a/packages/pds/src/api/com/atproto/server/requestEmailUpdate.ts b/packages/pds/src/api/com/atproto/server/requestEmailUpdate.ts index 5639debacf0..fa7b0f22fcc 100644 --- a/packages/pds/src/api/com/atproto/server/requestEmailUpdate.ts +++ b/packages/pds/src/api/com/atproto/server/requestEmailUpdate.ts @@ -1,8 +1,12 @@ +import assert from 'node:assert' + import { DAY, HOUR } from '@atproto/common' import { InvalidRequestError } from '@atproto/xrpc-server' -import { Server } from '../../../../lexicon' + import AppContext from '../../../../context' -import { authPassthru, resultPassthru } from '../../../proxy' +import { Server } from '../../../../lexicon' +import { resultPassthru } from '../../../proxy' +import { ids } from '../../../../lexicon/lexicons' export default function (server: Server, ctx: AppContext) { server.com.atproto.server.requestEmailUpdate({ @@ -19,7 +23,7 @@ export default function (server: Server, ctx: AppContext) { }, ], auth: ctx.authVerifier.accessStandard({ checkTakedown: true }), - handler: async ({ auth, req }) => { + handler: async ({ auth }) => { const did = auth.credentials.did const account = await ctx.accountManager.getAccount(did, { includeDeactivated: true, @@ -30,10 +34,15 @@ export default function (server: Server, ctx: AppContext) { } if (ctx.entrywayAgent) { + assert(ctx.cfg.entryway) return resultPassthru( await ctx.entrywayAgent.com.atproto.server.requestEmailUpdate( undefined, - authPassthru(req), + await ctx.serviceAuthHeaders( + auth.credentials.did, + ctx.cfg.entryway.did, + ids.ComAtprotoServerRequestEmailUpdate, + ), ), ) } diff --git a/packages/pds/src/api/com/atproto/server/revokeAppPassword.ts b/packages/pds/src/api/com/atproto/server/revokeAppPassword.ts index 3506cdc24b3..2d1c9f2e8ac 100644 --- a/packages/pds/src/api/com/atproto/server/revokeAppPassword.ts +++ b/packages/pds/src/api/com/atproto/server/revokeAppPassword.ts @@ -1,15 +1,22 @@ +import assert from 'node:assert' + import AppContext from '../../../../context' import { Server } from '../../../../lexicon' -import { authPassthru } from '../../../proxy' +import { ids } from '../../../../lexicon/lexicons' export default function (server: Server, ctx: AppContext) { server.com.atproto.server.revokeAppPassword({ auth: ctx.authVerifier.accessStandard(), - handler: async ({ auth, input, req }) => { + handler: async ({ auth, input }) => { if (ctx.entrywayAgent) { + assert(ctx.cfg.entryway) await ctx.entrywayAgent.com.atproto.server.revokeAppPassword( input.body, - authPassthru(req, true), + await ctx.serviceAuthHeaders( + auth.credentials.did, + ctx.cfg.entryway.did, + ids.ComAtprotoServerRevokeAppPassword, + ), ) return } diff --git a/packages/pds/src/api/com/atproto/server/updateEmail.ts b/packages/pds/src/api/com/atproto/server/updateEmail.ts index 161950c4804..f6bc781be3e 100644 --- a/packages/pds/src/api/com/atproto/server/updateEmail.ts +++ b/packages/pds/src/api/com/atproto/server/updateEmail.ts @@ -1,14 +1,17 @@ -import disposable from 'disposable-email' +import assert from 'node:assert' + import { InvalidRequestError } from '@atproto/xrpc-server' -import { Server } from '../../../../lexicon' -import AppContext from '../../../../context' -import { authPassthru } from '../../../proxy' +import disposable from 'disposable-email' + import { UserAlreadyExistsError } from '../../../../account-manager/helpers/account' +import AppContext from '../../../../context' +import { Server } from '../../../../lexicon' +import { ids } from '../../../../lexicon/lexicons' export default function (server: Server, ctx: AppContext) { server.com.atproto.server.updateEmail({ auth: ctx.authVerifier.accessFull({ checkTakedown: true }), - handler: async ({ auth, input, req }) => { + handler: async ({ auth, input }) => { const did = auth.credentials.did const { token, email } = input.body if (!disposable.validate(email)) { @@ -24,9 +27,14 @@ export default function (server: Server, ctx: AppContext) { } if (ctx.entrywayAgent) { + assert(ctx.cfg.entryway) await ctx.entrywayAgent.com.atproto.server.updateEmail( input.body, - authPassthru(req, true), + await ctx.serviceAuthHeaders( + auth.credentials.did, + ctx.cfg.entryway.did, + ids.ComAtprotoServerUpdateEmail, + ), ) return } diff --git a/packages/pds/src/api/proxy.ts b/packages/pds/src/api/proxy.ts index ad16a337a14..fc51115e711 100644 --- a/packages/pds/src/api/proxy.ts +++ b/packages/pds/src/api/proxy.ts @@ -37,7 +37,11 @@ export function authPassthru(req: IncomingMessage, withEncoding?: boolean) { // This is fine since app views are usually called using the requester's // credentials when "auth.credentials.type === 'access'", which is the only // case were DPoP is used. - if (authorization.startsWith('DPoP ') || req.headers['dpop']) { + const [type] = authorization.split(' ', 1) + if (!type) { + throw new InvalidRequestError('Invalid authorization header') + } + if (type.toLowerCase() === 'dpop' || req.headers['dpop']) { throw new InvalidRequestError('DPoP requests cannot be proxied') } diff --git a/packages/pds/src/auth-routes.ts b/packages/pds/src/auth-routes.ts index 77fc23aa341..ef513bce28b 100644 --- a/packages/pds/src/auth-routes.ts +++ b/packages/pds/src/auth-routes.ts @@ -11,11 +11,13 @@ export const createRouter = ({ authProvider, cfg }: AppContext): Router => { resource: cfg.service.publicUrl, authorization_servers: [cfg.entryway?.url ?? cfg.service.publicUrl], bearer_methods_supported: ['header'], - scopes_supported: ['profile', 'email', 'phone'], + scopes_supported: [], resource_documentation: 'https://atproto.com', }) router.get('/.well-known/oauth-protected-resource', (req, res) => { + res.setHeader('Access-Control-Allow-Origin', '*') + res.setHeader('Access-Control-Allow-Method', '*') res.status(200).json(oauthProtectedResourceMetadata) }) diff --git a/packages/pds/src/auth-verifier.ts b/packages/pds/src/auth-verifier.ts index 3b764e4e91b..02aefdd497f 100644 --- a/packages/pds/src/auth-verifier.ts +++ b/packages/pds/src/auth-verifier.ts @@ -452,13 +452,6 @@ export class AuthVerifier { ctx: ReqCtx, scopes: AuthScope[], ): Promise { - if (!scopes.includes(AuthScope.Access)) { - throw new InvalidRequestError( - 'DPoP access token cannot be used for this request', - 'InvalidToken', - ) - } - this.setAuthHeaders(ctx) const { req } = ctx @@ -489,13 +482,48 @@ export class AuthVerifier { throw new InvalidRequestError('Malformed token', 'InvalidToken') } + const tokenScopes = new Set(result.claims.scope?.split(' ')) + + if (!tokenScopes.has('transition:generic')) { + throw new AuthRequiredError( + 'Missing required scope: transition:generic', + 'InvalidToken', + ) + } + + const scopeEquivalent: AuthScope = tokenScopes.has('transition:chat.bsky') + ? AuthScope.AppPassPrivileged + : AuthScope.AppPass + + if (!scopes.includes(scopeEquivalent)) { + // AppPassPrivileged is sufficient but was not provided "transition:chat.bsky" + if (scopes.includes(AuthScope.AppPassPrivileged)) { + throw new InvalidRequestError( + 'Missing required scope: transition:chat.bsky', + 'InvalidToken', + ) + } + + // AuthScope.Access and AuthScope.SignupQueued do not have an OAuth + // scope equivalent. + throw new InvalidRequestError( + 'DPoP access token cannot be used for this request', + 'InvalidToken', + ) + } + + const isPrivileged = [ + AuthScope.Access, + AuthScope.AppPassPrivileged, + ].includes(scopeEquivalent) + return { credentials: { type: 'access', did: result.claims.sub, - scope: AuthScope.Access, + scope: scopeEquivalent, audience: this.dids.pds, - isPrivileged: true, + isPrivileged, }, artifacts: result.token, } diff --git a/packages/pds/src/index.ts b/packages/pds/src/index.ts index 493120a837a..1a13b68df73 100644 --- a/packages/pds/src/index.ts +++ b/packages/pds/src/index.ts @@ -54,12 +54,6 @@ export class PDS { secrets: ServerSecrets, overrides?: Partial, ): Promise { - const app = express() - app.set('trust proxy', true) - app.use(cors({ maxAge: DAY / SECOND })) - app.use(loggerMiddleware) - app.use(compression()) - const ctx = await AppContext.fromConfig(cfg, secrets, overrides) const xrpcOpts: XrpcServerOptions = { @@ -100,7 +94,12 @@ export class PDS { server = API(server, ctx) - app.use(authRoutes.createRouter(ctx)) + const app = express() + app.set('trust proxy', true) + app.use(loggerMiddleware) + app.use(compression()) + app.use(authRoutes.createRouter(ctx)) // Before CORS + app.use(cors({ maxAge: DAY / SECOND })) app.use(basicRoutes.createRouter(ctx)) app.use(wellKnown.createRouter(ctx)) app.use(server.xrpc.router) diff --git a/packages/pds/src/oauth/provider.ts b/packages/pds/src/oauth/provider.ts index 3430c463f5f..2fb039dd2dc 100644 --- a/packages/pds/src/oauth/provider.ts +++ b/packages/pds/src/oauth/provider.ts @@ -44,6 +44,8 @@ export class PdsOAuthProvider extends OAuthProvider { // & resource server, in which case the issuer origin is also the // resource server uri. protected_resources: [new URL(issuer).origin], + + scopes_supported: ['transition:generic', 'transition:chat.bsky'], }, accountStore: new DetailedAccountStore( diff --git a/packages/pds/src/pipethrough.ts b/packages/pds/src/pipethrough.ts index f48b182194c..1d9d348b598 100644 --- a/packages/pds/src/pipethrough.ts +++ b/packages/pds/src/pipethrough.ts @@ -22,7 +22,10 @@ export const proxyHandler = (ctx: AppContext): CatchallHandler => { try { const { url, aud, nsid } = await formatUrlAndAud(ctx, req) const auth = await accessStandard({ req, res }) - if (!auth.credentials.isPrivileged && PRIVILEGED_METHODS.has(nsid)) { + if ( + PROTECTED_METHODS.has(nsid) || + (!auth.credentials.isPrivileged && PRIVILEGED_METHODS.has(nsid)) + ) { throw new InvalidRequestError('Bad token method', 'InvalidToken') } const headers = await formatHeaders(ctx, req, { @@ -276,6 +279,27 @@ export const PRIVILEGED_METHODS = new Set([ ids.ComAtprotoServerCreateAccount, ]) +// These endpoints are related to account management and must be used directly, +// not proxied or service-authed. Service auth may be utilized between PDS and +// entryway for these methods. +export const PROTECTED_METHODS = new Set([ + ids.ComAtprotoAdminSendEmail, + ids.ComAtprotoIdentityRequestPlcOperationSignature, + ids.ComAtprotoIdentitySignPlcOperation, + ids.ComAtprotoIdentityUpdateHandle, + ids.ComAtprotoServerActivateAccount, + ids.ComAtprotoServerConfirmEmail, + ids.ComAtprotoServerCreateAppPassword, + ids.ComAtprotoServerDeactivateAccount, + ids.ComAtprotoServerGetAccountInviteCodes, + ids.ComAtprotoServerListAppPasswords, + ids.ComAtprotoServerRequestAccountDelete, + ids.ComAtprotoServerRequestEmailConfirmation, + ids.ComAtprotoServerRequestEmailUpdate, + ids.ComAtprotoServerRevokeAppPassword, + ids.ComAtprotoServerUpdateEmail, +]) + const defaultService = ( ctx: AppContext, nsid: string, diff --git a/packages/pds/tests/app-passwords.test.ts b/packages/pds/tests/app-passwords.test.ts index 00130dc6a55..35bae7528c1 100644 --- a/packages/pds/tests/app-passwords.test.ts +++ b/packages/pds/tests/app-passwords.test.ts @@ -118,7 +118,7 @@ describe('app_passwords', () => { lxm: 'com.atproto.server.createAccount', }) await expect(attempt).rejects.toThrow( - /cannot request a service auth token for the following method with an app password/, + /insufficient access to request a service auth token for the following method/, ) }) @@ -159,7 +159,7 @@ describe('app_passwords', () => { lxm: 'com.atproto.server.createAccount', }) await expect(priviAttempt).rejects.toThrow( - /cannot request a service auth token for the following method with an app password/, + /insufficient access to request a service auth token for the following method/, ) // allows only full access auth diff --git a/packages/pds/tests/entryway.test.ts b/packages/pds/tests/entryway.test.ts index ceb72784f7b..30c5c3769c1 100644 --- a/packages/pds/tests/entryway.test.ts +++ b/packages/pds/tests/entryway.test.ts @@ -1,6 +1,9 @@ import * as os from 'node:os' import * as path from 'node:path' +import assert from 'node:assert' +import { decodeJwt } from 'jose' import * as plcLib from '@did-plc/lib' +import { parseReqNsid } from '@atproto/xrpc-server' import { AtpAgent } from '@atproto/api' import { Secp256k1Keypair, randomStr } from '@atproto/crypto' import { SeedClient, TestPds, TestPlc, mockResolvers } from '@atproto/dev-env' @@ -114,10 +117,11 @@ describe('entryway', () => { it('updates handle from entryway.', async () => { await entrywayAgent.api.com.atproto.identity.updateHandle( { handle: 'alice3.test' }, - { - headers: SeedClient.getHeaders(accessToken), - encoding: 'application/json', - }, + await pds.ctx.serviceAuthHeaders( + alice, + 'did:example:entryway', + 'com.atproto.identity.updateHandle', + ), ) const doc = await entryway.ctx.idResolver.did.resolve(alice) const handleToDid = @@ -182,6 +186,28 @@ const createEntryway = async ( const server = await pdsEntryway.PDS.create(cfg, secrets) await server.ctx.db.migrateToLatestOrThrow() await server.start() + // patch entryway access token verification to handle internal service auth pds -> entryway + const origValidateAccessToken = + server.ctx.authVerifier.validateAccessToken.bind(server.ctx.authVerifier) + server.ctx.authVerifier.validateAccessToken = async (req, scopes) => { + const jwt = req.headers.authorization?.replace('Bearer ', '') ?? '' + const claims = decodeJwt(jwt) + if (claims.aud === 'did:example:entryway') { + assert(claims.lxm === parseReqNsid(req), 'bad lxm claim in service auth') + assert(claims.aud, 'missing aud claim in service auth') + assert(claims.iss, 'missing iss claim in service auth') + return { + artifacts: jwt, + credentials: { + type: 'access', + scope: 'com.atproto.access' as any, + audience: claims.aud, + did: claims.iss, + }, + } + } + return origValidateAccessToken(req, scopes) + } // @TODO temp hack because entryway teardown calls signupActivator.run() by mistake server.ctx.signupActivator.run = server.ctx.signupActivator.destroy return server diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index d48c2a5ec55..80304df1c46 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -963,9 +963,6 @@ importers: jose: specifier: ^5.2.0 version: 5.3.0 - oidc-token-hash: - specifier: ^5.0.3 - version: 5.0.3 psl: specifier: ^1.9.0 version: 1.9.0 @@ -10497,11 +10494,6 @@ packages: object-keys: 1.1.1 dev: true - /oidc-token-hash@5.0.3: - resolution: {integrity: sha512-IF4PcGgzAr6XXSff26Sk/+P4KZFJVuHAJZj3wgO3vX2bMdNVp/QXTP3P7CEm9V1IdG8lDLY3HhiqpsE/nOwpPw==} - engines: {node: ^10.13.0 || >=12.0.0} - dev: false - /on-exit-leak-free@2.1.0: resolution: {integrity: sha512-VuCaZZAjReZ3vUwgOB8LxAosIurDiAW0s13rI1YwmaP++jvcxP77AWoQvenZebpCA2m8WC1/EosPYPMjnRAp/w==}