fix: properly bind OAuth state data to a browser session#1327
fix: properly bind OAuth state data to a browser session#1327danielroe merged 6 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
matthieusieben
changed the title
| import { clientUri } from '#oauth/config' | ||
|
|
||
| //I did not have luck with other ones than these. I got this list from the PDS language picker | ||
| const OAUTH_LOCALES = new Set(['en', 'fr-FR', 'ja-JP']) |
There was a problem hiding this comment.
Other Atproto Authorization Server implementation exists out there. We should let the AS decide how to handle the locale passed in, based on what it supports.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReworks the atproto OAuth handler into distinct initiation and callback flows: initiation validates the Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/api/auth/atproto.get.ts (1)
55-70:⚠️ Potential issue | 🟡 MinorQuery parameters may be arrays;
toString()silently concatenates them.
getQueryreturns values that can be arrays. Using?.toString()onquery.returnTo(line 57) andquery.locale(line 69) will silently convert['a', 'b']to'a,b', which could produce unexpected redirect paths or invalid locale strings. Consider normalising these parameters explicitly.🛡️ Suggested fix to validate single-value parameters
+function getSingleQueryParam(value: unknown): string | undefined { + if (typeof value === 'string') return value + if (Array.isArray(value) && value.length === 1 && typeof value[0] === 'string') return value[0] + return undefined +} + // Validate returnTo is a safe relative path (prevent open redirect) // Only set cookie on initial auth request, not the callback let redirectPath = '/' try { const clientOrigin = new URL(clientUri).origin - const returnToUrl = new URL(query.returnTo?.toString() || '/', clientUri) + const returnToUrl = new URL(getSingleQueryParam(query.returnTo) || '/', clientUri) if (returnToUrl.origin === clientOrigin) { redirectPath = returnToUrl.pathname + returnToUrl.search + returnToUrl.hash } } catch { // Invalid URL, fall back to root } try { const redirectUrl = await atclient.authorize(query.handle, { scope, prompt: query.create ? 'create' : undefined, - ui_locales: query.locale?.toString(), + ui_locales: getSingleQueryParam(query.locale), state: encodeOAuthState(event, { redirectPath }), })
🧹 Nitpick comments (3)
server/api/auth/atproto.get.ts (3)
146-155: Consider addingsameSiteandpathattributes to the SID cookie.The cookie is missing
sameSiteandpathattributes. AddingsameSite: 'lax'provides additional CSRF protection, and settingpathto the callback endpoint limits cookie exposure.🔧 Suggested improvement
setCookie(event, `${SID_COOKIE_NAME}:${sid}`, SID_COOKIE_VALUE, { maxAge: 60 * 5, httpOnly: true, // secure only if NOT in dev mode secure: !import.meta.dev, + sameSite: 'lax', + path: '/api/auth/atproto', })Note: Ensure the same
pathis used indeleteCookiewithindecodeOAuthState.
183-200: Add runtime validation for parsed state structure.The type assertion
as { data: OAuthStateData; sid: string }at line 185 assumes the parsed JSON has the expected shape. If the state is malformed, accessingdecoded.sidordecoded.datacould throw or return undefined, leading to unexpected behaviour. Consider adding basic runtime validation.🛡️ Suggested fix to add validation
// The state string was encoded using encodeOAuthState. No need to protect // against JSON parsing since the StateStore should ensure its integrity. - const decoded = JSON.parse(state) as { data: OAuthStateData; sid: string } + let decoded: { data: OAuthStateData; sid: string } + try { + const parsed = JSON.parse(state) + if ( + typeof parsed !== 'object' || + parsed === null || + typeof parsed.sid !== 'string' || + typeof parsed.data?.redirectPath !== 'string' + ) { + throw new Error('Invalid state structure') + } + decoded = parsed as { data: OAuthStateData; sid: string } + } catch { + throw createError({ + statusCode: 400, + message: 'Invalid state parameter', + }) + }
256-261: Storerefin a variable to avoid redundant optional chaining.The
validatedResponse.avatar?.refexpression is evaluated twice. Since the condition at line 258 already confirmsrefexists, the optional chain at line 260 is redundant. Storing it in a variable improves clarity.♻️ Suggested refactor
- if (validatedResponse.avatar?.ref) { + const avatarRef = validatedResponse.avatar?.ref + if (avatarRef) { // Use Bluesky CDN for faster image loading - avatar = `https://cdn.bsky.app/img/feed_thumbnail/plain/${did}/${validatedResponse.avatar?.ref}@jpeg` + avatar = `https://cdn.bsky.app/img/feed_thumbnail/plain/${did}/${avatarRef}@jpeg` }
matthieusieben
changed the title
| function encodeOAuthState(event: H3Event, data: OAuthStateData): string { | ||
| const sid = generateRandomHexString() | ||
| setCookie(event, `${SID_COOKIE_PREFIX}_${sid}`, SID_COOKIE_VALUE, { | ||
| maxAge: 60 * 5, | ||
| httpOnly: true, | ||
| // secure only if NOT in dev mode | ||
| secure: !import.meta.dev, | ||
| sameSite: 'lax', | ||
| path: event.path.split('?', 1)[0], | ||
| }) | ||
| return JSON.stringify({ data, sid }) | ||
| } | ||
|
|
There was a problem hiding this comment.
useSession generates an encrypted, stateless cookie that validates it was issued by the server
doesn't that provide enough protection here?
There was a problem hiding this comment.
It does. This way of doing things allows to keep the "oauth sid" (the name might be poorly chosen) ephemeral without having to cleanup the h3 cookie.
There was a problem hiding this comment.
For example, if the user initiates an oauth flow (causing the useSession cookie to contain that "sid" value), then the user performs a "back" navigation. In that case, the "sid" would probably not be removed from the useSession cookie, unless every time we read that cookie, we performed additional logic to clean things up. I wanted to avoid doing that.
There was a problem hiding this comment.
I tried to explain this in the JSdoc right above.
2 participants
Currently, the OAuth state data (redirect path) is stored in a dedicated cookie. However, the
NodeOAuthClientalready provides a way to store this state data in the OAuth state store.Relatedly, the OAuth state data should be stored server side (see #1322). Once it does, the server should verify that the browser for which it is performing the OAuth callback is indeed the browser that initiated that OAuth request.
This PR improves the handling of the OAuth state by: