-
-
Notifications
You must be signed in to change notification settings - Fork 189
fix: preserve page context via cookie after Bluesky redirect #930
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: preserve page context via cookie after Bluesky redirect #930
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
📝 WalkthroughWalkthroughThe changes implement a mechanism to preserve page context during authentication flows. The client-side component now captures the current route path (route.fullPath) and passes it as a returnTo parameter when initiating authentication. The server-side endpoint receives this value, validates it as a safe relative path, stores it securely in a cookie with a 5-minute expiration, and redirects users to the stored path following successful OAuth callback instead of redirecting to the homepage. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
app/components/Header/AuthModal.client.vue (1)
26-36:⚠️ Potential issue | 🟠 MajorMissing
returnToparameter inhandleLogincreates inconsistent redirect behaviour.The
handleLogin()function for manual handle input does not include thereturnToquery parameter, whilsthandleBlueskySignIn()andhandleCreateAccount()do. Users logging in with a custom handle will be redirected to the homepage instead of their original page.🐛 Proposed fix to add returnTo parameter
async function handleLogin() { if (handleInput.value) { await navigateTo( { path: '/api/auth/atproto', - query: { handle: handleInput.value }, + query: { handle: handleInput.value, returnTo: route.fullPath }, }, { external: true }, ) } }
|
would you mind resolving the conflicts? 🙏 |
app/pages/package-docs/[...path].vue
Outdated
| /* Individual symbol articles */ | ||
| .docs-content .docs-symbol { | ||
| @apply mb-10 pb-10 border-b border-border/30 last:border-0; | ||
| word-break: break-word; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does it have to do with the described fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was working on another issue and forgot to switch branches. Please ignore that commit for now
|
yeah, working on it |
wojtekmaj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Listen to the rabbit 🐰
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
wojtekmaj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from the style change that sneaked in, LGTM.
server/api/auth/atproto.get.ts
Outdated
| const query = getQuery(event) | ||
| const rawReturnTo = query.returnTo?.toString() || '/' | ||
| // Validate returnTo is a safe relative path (prevent open redirect) | ||
| const isRelativePath = rawReturnTo.startsWith('/') && !rawReturnTo.startsWith('//') && !rawReturnTo.includes(':') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be safer to construct a new URL, validate the hostname, and pass this to setCookie
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/Header/AuthModal.client.vue (1)
29-33:⚠️ Potential issue | 🟠 MajorCustom handle login does not preserve page context.
The
handleLoginfunction usesauthRedirect(handleInput.value)without passing thereturnToparameter. Users who enter a custom handle will be redirected to the homepage after authentication, inconsistent with the other two login flows (handleBlueskySignInandhandleCreateAccount) which both passreturnTo: route.fullPath.The
authRedirectfunction currently accepts onlyidentifierand optionalcreateparameters, and does not includereturnToin its query object. Update the function signature to accept an optionalreturnToparameter:-export async function authRedirect(identifier: string, create: boolean = false) { +export async function authRedirect(identifier: string, create: boolean = false, returnTo?: string) { let query: LocationQueryRaw = { handle: identifier } if (create) { query = { ...query, create: 'true' } } + if (returnTo) { + query = { ...query, returnTo } + } await navigateTo( { path: '/api/auth/atproto', query, }, { external: true }, ) }Then call it with the return path in
handleLogin:async function handleLogin() { if (handleInput.value) { - await authRedirect(handleInput.value) + await authRedirect(handleInput.value, false, route.fullPath) } }
fatfingers23
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you for adding it! Was a big ask we got
fixes #901
Previously, users logging in via Bluesky were redirected to the homepage regardless of which page they started from.