Session Expired window#186
Conversation
…r id or close session expired windows if an authenticated request has been made
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughImplements cross-tab session synchronization: adds a BroadcastChannel that emits a “session-alive” message after admin API responses, exposes the current user ID to the client, and updates the reauth mixin to listen for messages to reset state or reload when a different user is detected. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as User (Tab A)
participant FE as Frontend (Tab A)
participant AX as Axios Interceptor
participant BC as BroadcastChannel "session-sync"
participant FE2 as Frontend (Tab B, reauthMixin)
User->>FE: Trigger admin action
FE->>AX: /admin/api request
AX-->>FE: Response
AX->>BC: postMessage { type: "session-alive", userId }
Note over BC: Message fan-out to all tabs
BC-->>FE2: message event
alt msg.userId !== window.__userId__
FE2->>FE2: location.reload()
else msg.type === "session-alive"
FE2->>FE2: reauthMixin.resetState()
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
✨ Finishing touches🧪 Generate unit tests
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
enferno/templates/layout.html (1)
100-105: Guard reading window.userId to avoid runtime errors and standardize value.Accessing a missing element throws. Also ensure it resolves to null when absent.
-window.__userId__ = document.querySelector('[data-user-id]').dataset.userId; +{ + const el = document.querySelector('[data-user-id]'); + window.__userId__ = el ? (el.dataset.userId ?? null) : null; +}enferno/static/js/mixins/reauth-mixin.js (1)
130-133: Fix axios error handling: use response.status, not request.status.Axios populates
error.response.statusfor HTTP errors. Checkingerr.request.statuswon’t work and blocks the intended fallback.- if (err?.request?.status === 404) return this.goBackToSignIn(); + if (err?.response?.status === 404) return this.goBackToSignIn();Also applies to: 160-163
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
enferno/static/js/common/config.js(2 hunks)enferno/static/js/mixins/reauth-mixin.js(1 hunks)enferno/templates/layout.html(1 hunks)
enferno/static/js/common/config.js
Outdated
| const sessionSyncChannel = new BroadcastChannel("session-sync"); | ||
|
|
There was a problem hiding this comment.
Harden BroadcastChannel init and expose it globally with a no-op fallback.
On browsers without BroadcastChannel (or if CSP blocks it), this throws and breaks all subsequent JS, and reauth-mixin depends on this binding. Provide a safe fallback and attach to window so downstream code can rely on it.
-const sessionSyncChannel = new BroadcastChannel("session-sync");
+let sessionSyncChannel;
+try {
+ sessionSyncChannel = new BroadcastChannel('session-sync');
+} catch (e) {
+ // Fallback no-op shim
+ sessionSyncChannel = {
+ postMessage: () => {},
+ addEventListener: () => {},
+ removeEventListener: () => {},
+ };
+}
+// Expose for consumers (e.g., reauth-mixin)
+window.sessionSyncChannel = sessionSyncChannel;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const sessionSyncChannel = new BroadcastChannel("session-sync"); | |
| let sessionSyncChannel; | |
| try { | |
| sessionSyncChannel = new BroadcastChannel('session-sync'); | |
| } catch (e) { | |
| // Fallback no-op shim | |
| sessionSyncChannel = { | |
| postMessage: () => {}, | |
| addEventListener: () => {}, | |
| removeEventListener: () => {}, | |
| }; | |
| } | |
| // Expose for consumers (e.g., reauth-mixin) | |
| window.sessionSyncChannel = sessionSyncChannel; |
🤖 Prompt for AI Agents
In enferno/static/js/common/config.js around lines 247-248, the BroadcastChannel
constructor can throw in browsers without support or under restrictive CSP;
replace the direct new call with a safe try/catch that attempts to construct
BroadcastChannel and on failure creates a no-op fallback object implementing the
same minimal API (postMessage, addEventListener, removeEventListener, close) so
consumers can call methods safely, then assign the result to
window.sessionSyncChannel and to a local const sessionSyncChannel so downstream
code (like reauth-mixin) can rely on window.sessionSyncChannel being present.
enferno/static/js/common/config.js
Outdated
| const isAuthenticatedRequest = response.config.url?.startsWith('/admin/api') | ||
| if (isAuthenticatedRequest) { | ||
| sessionSyncChannel.postMessage({ type: 'session-alive', userId: window.__userId__ }); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make URL matching robust and also emit on successful auth endpoints to immediately clear modals across tabs.
startsWith('/admin/api') misses absolute URLs and won’t fire after successful /login//verify flows, delaying the cross-tab “session-alive” signal.
-const isAuthenticatedRequest = response.config.url?.startsWith('/admin/api')
-if (isAuthenticatedRequest) {
- sessionSyncChannel.postMessage({ type: 'session-alive', userId: window.__userId__ });
-}
+const rawUrl = response?.config?.url || '';
+let path = '';
+try {
+ const u = rawUrl.startsWith('http') ? new URL(rawUrl) : new URL(rawUrl, window.location.origin);
+ path = u.pathname || '';
+} catch {}
+const isAdminApi = path.startsWith('/admin/api');
+const isAuthFlow = ['/login', '/verify', '/tf-validate', '/wan-signin', '/tf-select'].includes(path);
+if (isAdminApi || isAuthFlow) {
+ window.sessionSyncChannel?.postMessage({ type: 'session-alive', userId: window.__userId__ ?? null });
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isAuthenticatedRequest = response.config.url?.startsWith('/admin/api') | |
| if (isAuthenticatedRequest) { | |
| sessionSyncChannel.postMessage({ type: 'session-alive', userId: window.__userId__ }); | |
| } | |
| const rawUrl = response?.config?.url || ''; | |
| let path = ''; | |
| try { | |
| const u = rawUrl.startsWith('http') ? new URL(rawUrl) : new URL(rawUrl, window.location.origin); | |
| path = u.pathname || ''; | |
| } catch {} | |
| const isAdminApi = path.startsWith('/admin/api'); | |
| const isAuthFlow = ['/login', '/verify', '/tf-validate', '/wan-signin', '/tf-select'].includes(path); | |
| if (isAdminApi || isAuthFlow) { | |
| window.sessionSyncChannel?.postMessage({ type: 'session-alive', userId: window.__userId__ ?? null }); | |
| } |
🤖 Prompt for AI Agents
In enferno/static/js/common/config.js around lines 252 to 256, the current check
uses response.config.url?.startsWith('/admin/api') which misses absolute URLs
and doesn't emit for successful auth endpoints; replace it by normalizing the
request URL (e.g. new URL(response.config.url, window.location.origin)) and test
the pathname: if pathname.startsWith('/admin/api') then post the session-alive
message; additionally, if the pathname equals '/login' or '/verify' (or whatever
auth callback paths you use) only emit the session-alive message when the
response indicates a successful auth (e.g. status 200) so cross-tab modals clear
immediately after successful login/verify. Ensure you handle missing/invalid
URLs safely (guard the URL constructor) and keep the single postMessage call.
| callbackQueue: [], | ||
| sessionSyncChannel: sessionSyncChannel | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hard dependency on an identifier; use the global and handle absence.
If the channel wasn’t created (older browser/CSP) this would ReferenceError at load. Read from window and tolerate null.
- callbackQueue: [],
- sessionSyncChannel: sessionSyncChannel
+ callbackQueue: [],
+ sessionSyncChannel: window.sessionSyncChannel || null📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| callbackQueue: [], | |
| sessionSyncChannel: sessionSyncChannel | |
| }), | |
| callbackQueue: [], | |
| sessionSyncChannel: window.sessionSyncChannel || null | |
| }), |
🤖 Prompt for AI Agents
In enferno/static/js/mixins/reauth-mixin.js around lines 15–17, avoid
referencing the identifier sessionSyncChannel directly which can throw a
ReferenceError in environments where it wasn't created; instead read it off the
global (e.g. window) safely and allow null. Replace the direct identifier with a
safe lookup (for example use typeof window !== "undefined" ?
window.sessionSyncChannel : null, or typeof sessionSyncChannel === "undefined" ?
null : sessionSyncChannel) so the mixin assigns either the channel or null and
will not crash in older browsers or CSP-restricted contexts.
| this.sessionSyncChannel.onmessage = (event) => { | ||
| const msg = event.data; | ||
| // If new userId is not the same reload window | ||
| if (msg.userId !== window.__userId__) { | ||
| window.location.reload(); | ||
| } else if (msg.type === 'session-alive') { | ||
| this.resetState(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use addEventListener, validate message shape, and clean up listeners.
onmessage allows only one handler and no cleanup. Also guard against malformed messages to prevent unintended reloads.
- this.sessionSyncChannel.onmessage = (event) => {
- const msg = event.data;
- // If new userId is not the same reload window
- if (msg.userId !== window.__userId__) {
- window.location.reload();
- } else if (msg.type === 'session-alive') {
- this.resetState();
- }
- };
+ if (this.sessionSyncChannel?.addEventListener) {
+ this._onSessionMessage = (event) => {
+ const msg = event?.data || {};
+ const incomingId = msg?.userId ?? null;
+ if (incomingId && window.__userId__ && incomingId !== window.__userId__) {
+ window.location.reload();
+ return;
+ }
+ if (msg?.type === 'session-alive') {
+ this.resetState();
+ }
+ };
+ this.sessionSyncChannel.addEventListener('message', this._onSessionMessage);
+ }Apply outside this range to remove the listener:
beforeUnmount() {
document.removeEventListener('authentication-required', this.showLoginDialog);
+ if (this.sessionSyncChannel?.removeEventListener && this._onSessionMessage) {
+ this.sessionSyncChannel.removeEventListener('message', this._onSessionMessage);
+ }
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.sessionSyncChannel.onmessage = (event) => { | |
| const msg = event.data; | |
| // If new userId is not the same reload window | |
| if (msg.userId !== window.__userId__) { | |
| window.location.reload(); | |
| } else if (msg.type === 'session-alive') { | |
| this.resetState(); | |
| } | |
| }; | |
| if (this.sessionSyncChannel?.addEventListener) { | |
| this._onSessionMessage = (event) => { | |
| const msg = event?.data || {}; | |
| const incomingId = msg?.userId ?? null; | |
| if (incomingId && window.__userId__ && incomingId !== window.__userId__) { | |
| window.location.reload(); | |
| return; | |
| } | |
| if (msg?.type === 'session-alive') { | |
| this.resetState(); | |
| } | |
| }; | |
| this.sessionSyncChannel.addEventListener('message', this._onSessionMessage); | |
| } | |
| beforeUnmount() { | |
| document.removeEventListener('authentication-required', this.showLoginDialog); | |
| if (this.sessionSyncChannel?.removeEventListener && this._onSessionMessage) { | |
| this.sessionSyncChannel.removeEventListener('message', this._onSessionMessage); | |
| } | |
| }, |
🤖 Prompt for AI Agents
In enferno/static/js/mixins/reauth-mixin.js around lines 21-29, replace the
assignment to this.sessionSyncChannel.onmessage with a named handler registered
via addEventListener('message', handler) so multiple handlers and cleanup are
possible; inside the handler, first validate event.data exists and has the
expected shape/types (e.g., typeof event.data === 'object' and check typeof
event.data.userId === 'string' and typeof event.data.type === 'string') before
comparing userId or checking type to avoid malformed-message reloads; only call
window.location.reload() when the validated userId is different from
window.__userId__ and handle 'session-alive' via this.resetState(); store the
handler reference and add code outside this range (e.g., on channel
close/teardown lifecycle) to call removeEventListener('message', handler) to
properly clean up.
enferno/templates/layout.html
Outdated
| data-user-id="{{ current_user.id }}" | ||
| ></div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent crash when not authenticated (null deref on data-user-id).
On pages where the user isn’t authenticated, there’s no [data-user-id] element, yet the script always reads it (Line 100). Add a placeholder in the unauthenticated block to avoid querySelector(...).dataset throwing.
-{% else %}
- <div class="d-none" data-settings='{}' data-username=""></div>
+{% else %}
+ <div class="d-none" data-settings='{}' data-username="" data-user-id=""></div>
{% endif %}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| data-user-id="{{ current_user.id }}" | |
| ></div> | |
| {% if current_user.is_authenticated %} | |
| <div class="d-none" data-settings='{}' data-username="{{ current_user.username }}" | |
| data-user-id="{{ current_user.id }}" | |
| ></div> | |
| {% else %} | |
| <div class="d-none" data-settings='{}' data-username="" data-user-id=""></div> | |
| {% endif %} |
🤖 Prompt for AI Agents
enferno/templates/layout.html around lines 92 to 93: the template emits a div
with data-user-id when the user is authenticated but nothing when
unauthenticated, causing document.querySelector(...).dataset to throw; add a
placeholder element in the unauthenticated branch such as a hidden div or same
element with data-user-id="" (empty string) so the selector always returns an
element and reading dataset.userId is safe; ensure the placeholder matches the
same selector/attributes used by the script and does not expose any sensitive
data.
Jira Issue
Description
Can’t close/remove Session Expired window when logging in from another window
The user need to log off from another window to be able to log in and bypass this window.
Steps to reproduce the behavior:
Of course you can open another tab and log off from there to be able to log in and remove Session Expired window.
Checklist
API Changes (if applicable)
Additional Notes
[Any other relevant information]
Summary by CodeRabbit