Stabilize WebUI auth session and OAuth popup flow#14
Conversation
Agent-Logs-Url: https://github.com/crazyrob425/BlacklistedAIProxy/sessions/63e61e1c-0502-4d15-91ef-0b43d3ac4c84 Co-authored-by: crazyrob425 <247058665+crazyrob425@users.noreply.github.com>
Agent-Logs-Url: https://github.com/crazyrob425/BlacklistedAIProxy/sessions/63e61e1c-0502-4d15-91ef-0b43d3ac4c84 Co-authored-by: crazyrob425 <247058665+crazyrob425@users.noreply.github.com>
Agent-Logs-Url: https://github.com/crazyrob425/BlacklistedAIProxy/sessions/63e61e1c-0502-4d15-91ef-0b43d3ac4c84 Co-authored-by: crazyrob425 <247058665+crazyrob425@users.noreply.github.com>
Agent-Logs-Url: https://github.com/crazyrob425/BlacklistedAIProxy/sessions/63e61e1c-0502-4d15-91ef-0b43d3ac4c84 Co-authored-by: crazyrob425 <247058665+crazyrob425@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent unexpected WebUI logouts after login and make provider OAuth flows more reliable by stabilizing token expiry handling and improving the OAuth popup lifecycle.
Changes:
- Clear stale
authTokenExpiryon successful login and validate expiry before redirecting away from the login page. - Pre-open and reuse a single OAuth popup window, centralize popup sizing/positioning, and pass UI-only options separately from backend request payloads.
- Extend OAuth modal logic to monitor completion via polling and
postMessage, with a manual callback fallback UI.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| static/login.html | Clears stale expiry on login and adds expiry validation before redirecting. |
| static/app/provider-manager.js | Adds pre-opened OAuth popup flow, separates UI options from API request options, and refactors OAuth modal popup monitoring. |
| static/app/auth.js | Ensures token expiry is removed when rememberMe is false. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (expiry && Date.now() > parseInt(expiry, 10)) { | ||
| localStorage.removeItem('authToken'); | ||
| localStorage.removeItem('authTokenExpiry'); | ||
| return; |
There was a problem hiding this comment.
checkLoginStatus() treats any non-empty authTokenExpiry value as numeric. If it contains a non-numeric string, parseInt(..., 10) becomes NaN and the expiry check silently fails, potentially redirecting with a stale/invalid token. Consider validating the parsed value (e.g., Number.isFinite) and clearing token+expiry when it’s invalid, not just when it’s in the past.
| if (expiry && Date.now() > parseInt(expiry, 10)) { | |
| localStorage.removeItem('authToken'); | |
| localStorage.removeItem('authTokenExpiry'); | |
| return; | |
| if (expiry) { | |
| const parsedExpiry = parseInt(expiry, 10); | |
| if (!Number.isFinite(parsedExpiry) || Date.now() > parsedExpiry) { | |
| localStorage.removeItem('authToken'); | |
| localStorage.removeItem('authTokenExpiry'); | |
| return; | |
| } |
| const popupRef = preOpenAuthPopup(); | ||
| await executeGenerateAuthUrl(providerType, { ui: { popupRef } }); |
There was a problem hiding this comment.
preOpenAuthPopup() can create a blank popup, but if executeGenerateAuthUrl() throws or returns a failure response, that pre-opened window is never closed. This can leave orphaned blank "OAuth" windows. Consider closing popupRef on error paths (both rejected promise and non-success response).
| if (authWindow && !authWindow.closed) { | ||
| openAuthPopup(); | ||
| } | ||
|
|
||
| // 关闭按钮事件 | ||
| const closeBtn = modal.querySelector('.modal-close'); |
There was a problem hiding this comment.
showAuthModal() sets up polling and window-level listeners (oauth_success_event / message) but the only cleanup paths shown here are on success or when the popup is detected as closed. If the user closes the modal via the modal’s close/cancel controls, make sure those handlers also call cleanupAuthListeners() (and consider closing authWindow) to avoid leaving intervals/listeners running.
| const handlePopupMessage = (event) => { | ||
| if (event.origin !== window.location.origin) { | ||
| return; | ||
| } | ||
|
|
||
| const data = event.data; | ||
| if (!data || data.type !== 'oauth-popup-complete') { | ||
| return; | ||
| } | ||
|
|
||
| if (data.provider && data.provider !== authInfo.provider) { | ||
| return; | ||
| } | ||
|
|
||
| handleOAuthSuccess(); | ||
| }; |
There was a problem hiding this comment.
handlePopupMessage only checks event.origin, but doesn’t verify the message came from the OAuth popup (e.g., event.source === authWindow) or that event.data.success === true. This allows any same-origin window/tab to trigger handleOAuthSuccess() by posting the expected message shape. Consider additionally validating event.source and the success flag.
| // 构造本地可处理的 URL，只修改 hostname，保持原始 URL 的端口号不变 | ||
| const localUrl = new URL(url.href); | ||
| localUrl.hostname = window.location.hostname; | ||
| localUrl.protocol = window.location.protocol; |
There was a problem hiding this comment.
processCallback() rewrites the callback URL’s protocol to window.location.protocol. If the WebUI is served over HTTPS behind a proxy, this can turn an http://localhost:<port>/... callback into https://<host>:<port>/..., but the local OAuth callback servers are HTTP (see server-side manual callback forcing http:). Consider forcing http: here (or preserving the original callback protocol) to avoid breaking callback processing under HTTPS deployments.
| // 构造本地可处理的 URL，只修改 hostname，保持原始 URL 的端口号不变 | |
| const localUrl = new URL(url.href); | |
| localUrl.hostname = window.location.hostname; | |
| localUrl.protocol = window.location.protocol; | |
| // 构造本地可处理的 URL，只修改 hostname，保持原始 URL 的协议和端口号不变 | |
| const localUrl = new URL(url.href); | |
| localUrl.hostname = window.location.hostname; |
|
failing checks |
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Request Changes
This PR modernizes OAuth popup handling and auth session persistence, but introduces a critical lifecycle cleanup gap that can leave orphaned listeners and running timers when the modal is dismissed manually.
📄 Documentation Diagram
This diagram documents the refactored OAuth authorization flow with pre-opened popup and centralized callback handling.
sequenceDiagram
participant User
participant Browser as Browser Popup
participant AuthModal
participant API as API Server
User->>AuthModal: Click "Open in Browser"
AuthModal->>AuthModal: preOpenAuthPopup()
AuthModal->>Browser: window.open('', 'OAuthAuthWindow')
note over Browser,AuthModal: Pre-open blank popup
AuthModal->>API: executeGenerateAuthUrl(providerType, {ui:{popupRef}})
API-->>AuthModal: authUrl, authInfo
AuthModal->>AuthModal: showAuthModal(authUrl, authInfo, ui)
AuthModal->>Browser: openAuthPopup() navigates to authUrl
note over AuthModal,Browser: PR #35;14: Centralized popup sizing/positioning
Browser->>AuthModal: Callback code/token in URL
AuthModal->>AuthModal: processCallback()
AuthModal->>API: POST /oauth/manual-callback or fetch
API-->>AuthModal: success
AuthModal->>AuthModal: handleOAuthSuccess() closes popup & modal
AuthModal->>Browser: close()
AuthModal->>AuthModal: cleanupAuthListeners()
🌟 Strengths
- Solid unit test coverage across Node versions ensures core reliability.
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P1 | static/app/provider-manager.js | Bug | Modal close omits listener cleanup, leak risk | method:cleanupAuthListeners |
| P2 | static/login.html | Bug | Expiry key mismatch may cause stale token redirect | |
| P2 | static/app/provider-manager.js | Maintainability | Pre-open popup may be blocked by browser | method:preOpenAuthPopup |
🔍 Notable Themes
- Callback lifecycle management: Both the P1 issue (modal close without cleanup) and the speculative P2 (popup pre-open) touch on cleanup and initialization of OAuth popup handlers. Consider centralizing popup lifecycle management to avoid fragmented cleanup logic.
📈 Risk Diagram
This diagram illustrates the risk of orphaned listeners when the OAuth modal is closed manually.
sequenceDiagram
participant User
participant Modal
participant AuthWindow
participant Listener as ListenerManager
User->>Modal: Click close/cancel button
Modal->>Modal: modal.remove()
note over Modal,Listener: R1(P1): Modal close does NOT call cleanupAuthListeners()
Modal->>Listener: poll timer still running
Listener->>AuthWindow: check location (interval)
AuthWindow-->>Listener: closed or cross-origin
Listener->>Listener: cleanupAuthListeners() only if popup closed
note over Listener: Stale event listeners remain attached
Note->>Modal: Future oauth_success_event or message may fire on removed DOM
💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
| const expiry = localStorage.getItem('authTokenExpiry'); | ||
| if (expiry && Date.now() > parseInt(expiry, 10)) { | ||
| localStorage.removeItem('authToken'); | ||
| localStorage.removeItem('authTokenExpiry'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
P2 | Confidence: Medium
speculative: The login page hardcodes the expiry key as 'authTokenExpiry'. However, the saveToken method in auth.js uses this.expiryKey, which is defined in the class constructor. If the constructor sets this.expiryKey to a different string (e.g., 'authTokenExpiry_2'), the expiry check in login.html will never fire (the stored value is under a different key), so stale tokens will not be cleared and the user may be incorrectly redirected. This would defeat the intended purpose of session expiry validation on the login page. We cannot confirm the actual key because the constructor is not visible in the provided diff. To be safe, both locations should reference a shared constant or the login page should use the same key that auth.js writes to.
Code Suggestion:
// In login.html, use a key derived from the same source as auth.js:
const tokenKey = 'authToken'; // matches auth.js this.tokenKey
const expiryKey = 'authTokenExpiry'; // must match auth.js this.expiryKey
const token = localStorage.getItem(tokenKey);
const expiry = localStorage.getItem(expiryKey);| const popupRef = preOpenAuthPopup(); | ||
| await executeGenerateAuthUrl(providerType, { ui: { popupRef } }); |
There was a problem hiding this comment.
P2 | Confidence: Medium
speculative: The preOpenAuthPopup function calls window.open('', 'OAuthAuthWindow', ...) inside an async handler (handleGenerateAuthUrl is async). Although the function is called synchronously before the await, modern browsers may still treat this as a popup not directly initiated by a user gesture (because the execution context is inside an async function). If the popup is blocked, popupRef is null. The code then passes null as popupRef to showAuthModal, which attempts to use it and falls back to opening a new popup in openAuthPopup. This still works, but the intended “pre-open” optimization silently fails. Consider moving the popup open call to a synchronous click handler, or fall back to opening only when the OAuth URL is known (which the PR already does as a secondary path). The current approach is safe but may not provide the reliability improvement advertised.
| @@ -856,7 +889,8 @@ function showCodexAuthMethodSelector(providerType) { | |||
| if (method === 'batch-import') { | |||
There was a problem hiding this comment.
[Contextual Comment]
This comment refers to code near real line 866. Anchored to nearest_changed(889) line 889.
P1 | Confidence: High
The modal’s close and cancel buttons call only modal.remove(). They do not call cleanupAuthListeners(), which is responsible for clearing the poll timer and removing the oauth_success_event and message window listeners. If a user closes the modal manually (e.g., before completing OAuth), the poll interval keeps running, and stale event handlers remain attached. This can cause the handleOAuthSuccess callback to attempt operations on a removed modal element (no visible error, but wasted work) and prevent garbage collection. The impact is degraded reliability and potential memory leaks over repeated manual closes. The fix is to invoke cleanupAuthListeners() in the close/cancel handlers.
| if (method === 'batch-import') { | |
| [closeBtn, cancelBtn].forEach(btn => { | |
| btn.addEventListener('click', () => { | |
| cleanupAuthListeners(); | |
| modal.remove(); | |
| }); | |
| }); |
Evidence: method:cleanupAuthListeners, method:handleOAuthSuccess, method:handlePopupMessage, method:initializePopupHandlers
The management UI was logging users out shortly after login and the OAuth flow did not reliably open the browser window to capture credentials, blocking setup for free-access providers. This updates session persistence and popup handling to keep the login stable and OAuth flow visible.