fix: make prompt=none redirect loop work for api requests#3638
Open
tefkah wants to merge 2 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes the regression from #3627 where prompt=none silent re-auth worked for browser navigations but not for API/POST requests, leading to failures after the short session expired.
Changes:
- Enable rolling sessions so activity extends the session cookie expiry.
- Update
silentReauthMiddlewareto return401 { error: 'sessionExpired' }for/api/*(instead of only handling page GET navigations via 302). - Add a client-side hidden-iframe renewal flow (
apiFetchRaw) that renews on401 sessionExpiredand retries once; wire this into Altcha’s challenge fetch.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/server.ts | Enables rolling sessions to extend expiry on activity. |
| server/middleware/silentReauth.ts | Returns a structured 401 for API calls so the client can renew/retry. |
| server/kf/api.ts | Adds /auth/renew-done endpoint to signal iframe renewal completion to the parent window. |
| client/utils/apiFetch.ts | Adds apiFetchRaw + hidden-iframe renewal + retry-once behavior. |
| client/components/Altcha/Altcha.tsx | Routes Altcha widget fetches through apiFetchRaw to survive session expiry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+28
to
+31
| // ── API requests: tell the frontend to renew ── | ||
| if (req.path.startsWith('/api')) { | ||
| return res.status(401).json({ error: 'sessionExpired' }); | ||
| } |
Comment on lines
+69
to
+77
| function rawFetch(path: string, opts?: RequestInit): Promise<Response> { | ||
| return fetch(path, { | ||
| ...opts, | ||
| headers: { | ||
| Accept: 'application/json', | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| credentials: 'include', | ||
| }).then((response) => { | ||
| if (!response.ok) { | ||
| return response.json().then((err) => { | ||
| if (response.status === 423 && err?.error === 'readOnly') { | ||
| window.dispatchEvent(new CustomEvent('pubpub:readOnly')); | ||
| } | ||
| throw err; | ||
| }); |
Comment on lines
+51
to
+52
| // needs to be done like this, doesnt work when just passing it to altcha-widget | ||
| (w as unknown as { customfetch?: typeof apiFetchRaw }).customfetch = apiFetchRaw; |
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
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.
Issue(s) Resolved
The recently merged #3627 has a bug that somehow slipped through: while GET requests would automatically invisibly force a reauth loop to kfauth, any api request or POST request wouldnt (because you cant really force a renav in the same way).
This would cause actions such as creating a Pub to fail if your session of 15 minutes expired, and would only allow you to do it again after reloading the page or otherwise navigating.
This PR fixes that in two ways
This has got me questioning the validatity of this approach, but I will go with it for now to not make the experience very bad.
Test Plan
Screenshots (if applicable)
Optional
Notes/Context/Gotchas
Supporting Docs