fix: enhance error handling and user feedback in authentication flow#214
Merged
Conversation
✅ Deploy Preview for cubeindex ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughAuth callback and confirm endpoints now handle OAuth error query params, redirect with ChangesAuth Error Redirects and URL-Driven Toast Notifications
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/routes/`(auth)/auth/callback/+server.ts:
- Around line 12-25: The redirect branches in the auth callback are being
blocked because `logError(...)` throws before `redirect(303, ...)` can run.
Update the `callback` handler in `src/routes/(auth)/auth/callback/+server.ts` to
use a non-throwing logging path for these error cases, or change the helper
contract before calling it here. Keep the existing `errorMessage` handling, but
ensure the flow can reach each `redirect(...)` after logging instead of
terminating inside `logError(...)`.
In `@src/routes/`(auth)/auth/confirm/+server.ts:
- Around line 12-25: The confirm handler’s failure paths are still calling
logError(...) before redirect(303, ...), but logError is meant to log and then
throw, so the toast redirect never runs. Update the confirm flow in the
+server.ts handler to avoid using logError for branches that should continue
into a redirect, and instead log the error details separately (or otherwise
ensure no throw occurs) before the redirect. Apply the same control-flow fix
across the other failure branches in this handler so the new toast UX
consistently reaches the redirect path.
In `@src/routes/`+layout.svelte:
- Around line 27-46: The toast handling in +layout.svelte is rendering free-form
query-param text from page.url directly through toast.error and toast.success,
which allows crafted links to inject arbitrary copy. Update the URL contract to
use an allowlisted toast code parameter instead of toast_error/toast_success,
then map that code to local message strings inside the layout before calling the
toast helpers. Keep removeToastParam and goto for clearing the query param, but
only after validating the code against the allowed set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e292f7b6-5621-4232-9ad7-a6d6f056828b
📒 Files selected for processing (3)
src/routes/(auth)/auth/callback/+server.tssrc/routes/(auth)/auth/confirm/+server.tssrc/routes/+layout.svelte
Comment on lines
+27
to
+46
| const toastError = page.url.searchParams.get("toast_error"); | ||
| const toastSuccess = page.url.searchParams.get("toast_success"); | ||
|
|
||
| const removeToastParam = (newUrl: URL) => | ||
| goto((newUrl.pathname + newUrl.search) as ResolvedPathname, { | ||
| replaceState: true, | ||
| keepFocus: true, | ||
| noScroll: true, | ||
| }); | ||
|
|
||
| if (toastError) { | ||
| toast.error(toastError); | ||
| const newUrl = new URL(page.url); | ||
| newUrl.searchParams.delete("toast_error"); | ||
| removeToastParam(newUrl); | ||
| } else if (toastSuccess) { | ||
| toast.success(toastSuccess); | ||
| const newUrl = new URL(page.url); | ||
| newUrl.searchParams.delete("toast_success"); | ||
| removeToastParam(newUrl); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Don’t render raw toast text from query params.
This now lets any crafted link inject arbitrary success/error copy into a trusted site toast. Please switch the URL contract to an allowlisted toast code (for example toast=auth_exchange_failed) and map that code to local strings instead of displaying free-form query-param text. As per coding guidelines, "Validate all user inputs on both the client and server."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/`+layout.svelte around lines 27 - 46, The toast handling in
+layout.svelte is rendering free-form query-param text from page.url directly
through toast.error and toast.success, which allows crafted links to inject
arbitrary copy. Update the URL contract to use an allowlisted toast code
parameter instead of toast_error/toast_success, then map that code to local
message strings inside the layout before calling the toast helpers. Keep
removeToastParam and goto for clearing the query param, but only after
validating the code against the allowed set.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why:
Errors in +server.ts shows the default SvelteKit error screen which is not good UX, they cannot easily return to the homepage.
Also when there are auth related errors given in the url parameters, they were not shown to users but could contain important information on the issue and how to fix it.
Type of change
What's being changed (if available, include any code snippets, screenshots, or gifs):
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes