fix: remove prompt=none redirect loop, instead use refresh token checks#3638
Merged
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; |
tefkah
changed the title
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 ways1. Turn on rolling sessions. I didnt want to do it initially but it makes sense. Basically if you are doing stuff within the 15 min window your session will get extended.2. Standard practice but stupid reauth loop for api requests. Basically, when we get a 401 Session expired from an api request (in apiFetch), we silently spawn a small iframe, do the reauth loop there, then make the request again. This forces the request to succeeed.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.This shows that this is not really the correct way to go, in my opinion. Instead, I have changed PubPub to "Request Offline Access Tokens". PubPub instead of just authenticating the user, it will now, when a user logs in, it will get an access token and a refresh token. The access token is not valid for very long, the refresh token is stored in the session encrypted, and then when the user, after a certain amount of time, the refresh token is... Well, yeah, the refresh token is still valid. Actually, I don't think we do the access token part of this, but we do do the refresh token part of it. We didn't reuse the access token, per se. This is much simpler than what we had before. For one, it just makes the loop go through the client, through the server instead of the client, which is much nicer, and doesn't break in a lot of cases. Most of all, we didn't need to go through the client in the first place, because we're not a single page app, we have a server. So now, basically, every 15 minutes or something, we check whether the refresh token is still valid. If it's not valid, we just log the user out, because that means our session has expired and we didn't handle the... Not the refresh, but the session revocation endpoint correctly.
Test Plan
Screenshots (if applicable)
Optional
Notes/Context/Gotchas
Supporting Docs