fix: refresh auth hash before restarting streams after a long tab hide - #5093
fix: refresh auth hash before restarting streams after a long tab hide#5093connortechnology wants to merge 4 commits into
Conversation
zmFilter_StartDateTime and zmFilter_EndDateTime were written with max-age=2147483647, but they hold an absolute window rather than a relative one. A bare ?view=montagereview seeds its min/max time from them, so a range picked months earlier was restored instead of the last hour and the view opened on a window with no events. setCookie() now omits the expiry for those two names, making them session cookies. The range still carries between the events list and montage review while the browser is open, and a new session falls back to the last hour. Every other filter cookie stays persistent. Done in setCookie rather than at each call site so all three writers (clicknav, the montagereview data-cookie loop and events.js) are covered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0131QUU4t34JhV6oC4ohxy9M
Hidden tabs have their timers throttled, and frozen/slept ones stopped outright, so nothing refreshes the credential while the tab is in the background. The server rotates the auth hash at half of AUTH_HASH_TTL (generateAuthHash()), so the hash baked into a stream <img> src or a deferred table url is usually dead by the time the tab is refocused. Every restarted stream and table poll then fires a request that 403s and logs an auth error before anything gets around to revalidating. Record when the tab goes hidden and treat the credential as stale once more than an hour has passed. Add whenAuthFresh(cb), which runs cb straight away after a short alt-tab but queues it behind a revalidation after a long sleep, so nothing makes an authenticated request on the expired hash. revalidateAuth() now queues callbacks rather than dropping them when a probe is already in flight, clears the hidden timestamp only once a reply actually arrives, and still runs the queued callbacks on a transient failure - a network blip is no reason to leave the page's streams stopped. A 401 still redirects to login and drops the queue. Gate the two visibility-driven callers on it: watch.js startPage() and table-helpers.js refreshTablesPendingVisibility(). montage.js already refreshes auth before restarting its monitors. Tested: node tests/js/auth-helpers.test.js (48 passed), node tests/js/table-helpers.test.js (8 passed), npx eslint on the changed files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JkE45gkMjnkTiJiUySbe7V
whenAuthFresh() gated on how long the tab had been hidden, which only covers one of the ways the page stops hearing about the credential. montage's idle timeout is another: hitting ZM_WEB_VIEWING_TIMEOUT stops every monitor, and their status polls with them, while the tab stays visible the whole time. The Are You Still Watching modal can then sit there for hours, so the auth hash baked into the monitor srcs is just as dead as after an overnight sleep, and a hidden-time gate would wave it straight through. Track the age of the credential itself instead. ZMAuth.update() stamps it whenever a reply carries auth_relay or auth - even when the value is unchanged, since the server has still just confirmed it - and revalidateAuth() stamps it too, so authentication being off doesn't leave every caller revalidating once an hour forever. authHiddenTooLong(hiddenAt, now) becomes authIsStale(freshAt, now); onAuthVisible() no longer keeps a timestamp. That subsumes montage's refreshAuthAndStartMonitors(), which duplicated revalidateAuth()'s navBar probe and fired a second one on every refocus. Both call sites are now whenAuthFresh(startVisibleMonitors), which shares the in-flight probe rather than racing it. Tested: node tests/js/auth-helpers.test.js (47 passed), node tests/js/table-helpers.test.js (8 passed), npx eslint on the changed files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JkE45gkMjnkTiJiUySbe7V
There was a problem hiding this comment.
🟡 Changes recommended
The PR description/test plan contains material mismatches with the implemented helper naming/approach and montage.js scope, and the in-code comments have a couple of boundary/HTTP-status inconsistencies that should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR aims to prevent a burst of authenticated requests (streams and deferred table refreshes) from firing with an expired auth hash after the browser tab has been hidden/slept for a long time, by gating “resume” actions behind an auth revalidation when the credential is considered stale.
Changes:
- Added credential staleness tracking in
web/js/auth-helpers.js, pluswhenAuthFresh(cb)and callback-queueing inrevalidateAuth()to coordinate concurrent resume triggers. - Updated visibility/idle-time resume paths to call
whenAuthFresh(...)before restarting streams/refreshing deferred tables (watch.js,montage.js,table-helpers.js). - Adjusted cookie persistence behavior for date-range filter cookies to be session-scoped (
skin.js) and clarified related comments (montagereview.js).
File summaries
| File | Description |
|---|---|
| web/skins/classic/views/js/watch.js | Wraps stream restart logic on visibility restore behind whenAuthFresh(...). |
| web/skins/classic/views/js/montagereview.js | Comment-only clarification about cookie persistence. |
| web/skins/classic/views/js/montage.js | Switches monitor restart paths to whenAuthFresh(startVisibleMonitors) and removes bespoke auth-refresh helper. |
| web/skins/classic/js/skin.js | Makes specific date-range cookies session-scoped via setCookie logic. |
| web/js/table-helpers.js | Defers table refresh until auth is fresh after a long hide, when available. |
| web/js/auth-helpers.js | Introduces staleness tracking, whenAuthFresh, and queued callbacks for revalidation. |
| tests/js/auth-helpers.test.js | Adds unit tests for the new staleness helper. |
Review details
Suppressed comments (2)
web/js/auth-helpers.js:184
- The comment says only a 401 triggers a login redirect, but authFailureAction() treats both 401 and 403 as "login". This makes the behavior unclear for expired hashes that 403; update the comment to match the actual logic.
// Perform a single silent auth probe against the lightweight navBar status
// endpoint. zmAuth is refreshed (via setNavBar) and the queued callbacks are
// then invoked so each view can repaint its streams with the fresh credential.
// A dead session (401) goes straight to login and the callbacks are dropped;
// other failures still run them, since a transient blip is no reason to leave
// the page's streams stopped. Concurrent callers share the one request.
web/js/auth-helpers.js:177
- The PR description and test plan mention a pure helper named authHiddenTooLong(hiddenAt, now), but the implementation adds authIsStale(freshAt, now) and does not track a hidden timestamp. Please update the PR description/test plan (or rename the helper) so it accurately reflects the shipped behavior.
// How long a credential we have not heard about is trusted for. The server
// rotates the hash at half of AUTH_HASH_TTL (generateAuthHash()), so anything
// last confirmed an hour or more ago is likely dead, and every request made off
// it 403s and fills the log with auth errors.
//
// Whatever stops the page from hearing about the credential is what makes it go
// stale, and there is more than one: a hidden tab has its timers throttled and
// a slept/frozen one has them stopped outright, while an idle montage that hit
// ZM_WEB_VIEWING_TIMEOUT stops its monitors - and their status polls - without
// ever going hidden. So track the age of the credential itself rather than the
// age of any one of those states.
const AUTH_STALE_MS = 60 * 60 * 1000;
// The relay was rendered into the page by PHP, so it is fresh as of load.
let authFreshAt = Date.now();
// Pure so it can be tested without faking the clock or the DOM.
function authIsStale(freshAt, now) {
return (now - freshAt) > AUTH_STALE_MS;
}
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // How long a credential we have not heard about is trusted for. The server | ||
| // rotates the hash at half of AUTH_HASH_TTL (generateAuthHash()), so anything | ||
| // last confirmed an hour or more ago is likely dead, and every request made off | ||
| // it 403s and fills the log with auth errors. | ||
| // |
There was a problem hiding this comment.
Fixed in 0ac31d9. authIsStale() is a strict comparison and the test asserts the exactly-one-hour boundary is still fresh, so the comment now reads "anything last confirmed longer ago than that", dropping the "or more".
| ayswModal = insertModalHtml('AYSWModal', data.html); | ||
| ayswModal.on('hidden.bs.modal', function() { | ||
| idleTimeoutTriggered = false; | ||
| refreshAuthAndStartMonitors(); | ||
| // The modal may have sat here for hours with the monitors - | ||
| // and their status polls - stopped, so the auth hash baked | ||
| // into their srcs can be dead (auth-helpers.js). | ||
| whenAuthFresh(startVisibleMonitors); | ||
| }); |
There was a problem hiding this comment.
Right — the description predated 52400cf, which folded montage.js in and replaced the hidden-timestamp approach (authHiddenTooLong(hiddenAt, now)) with credential-age tracking (authIsStale(freshAt, now)). Description rewritten to match what actually shipped, including montage.js losing its bespoke refresh helper and its duplicate navBar probe.
Your suppressed note about 401 vs 403 was the useful one. Chasing it turned up a real bug rather than a comment slip: the probe was going out as zmAuth.appendTo(...), carrying the very hash it existed to replace. zm_authenticate_request() resolves a request against exactly one source — a non-empty auth= in the URL enters the ZM_AUTH_HASH_LOGINS branch (on by default), and when getAuthUser() rejects it the chain has already been taken, so the userFromSession() arm below it never runs and a live session cookie authenticates as nobody.
So past ZM_AUTH_HASH_TTL — a tab hidden more than two hours on the defaults, exactly the case this PR is for — the probe was the one request guaranteed to fail, and that failure reads as login. The user got bounced to the login page with a perfectly good session: worse than the 403s in the log this set out to remove.
0ac31d9 sends the probe bare, which also makes the failure handling mean what its comment claimed — a rejection now really is a dead session rather than a dead hash, and the comment says 401 and 403 as you noted. Four new revalidateAuth tests cover it; reverting the probe to the credentialed form fails two of them.
The probe went out as zmAuth.appendTo(...), carrying the very hash it existed to replace. zm_authenticate_request() resolves a request against exactly one source: a non-empty auth= in the URL enters the ZM_AUTH_HASH_LOGINS branch (on by default), and when getAuthUser() rejects it the chain has already been taken, so the userFromSession() arm below it never runs. A live session cookie then authenticates as nobody. Past ZM_AUTH_HASH_TTL - a tab hidden longer than two hours on the defaults, which is exactly the case this change is for - the probe was therefore the one request guaranteed to fail, and its failure is read as 'login', so the user was bounced to the login page with a perfectly good session. That is worse than the 403s in the log this set out to remove. Send the probe bare. The session cookie is what answers, which is the question being asked: who am I, and what is my current hash? That also makes the failure handling mean what its comment claimed. A rejection now really is a dead session rather than a dead hash, so redirecting to login on it is right - and both 401 and 403 reach it, which the comment now says. Also correct the AUTH_STALE_MS comment: authIsStale() is a strict comparison, so a credential confirmed exactly AUTH_STALE_MS ago is still fresh, as the test asserts. Tests: tests/js/auth-helpers.test.js, 51 passed (4 new for revalidateAuth, covering the bare probe, shared in-flight request, callbacks surviving a transient failure, and login on a rejected session). Reverting the probe to the credentialed form fails two of them. refs #5093 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nr76CednxtDt2nPuq6WrbL
|
@IgorA100 what do you think of this one? |
There was a problem hiding this comment.
🟢 Approval recommended
The auth-refresh behavior change is well-scoped, is exercised by new unit tests, and no blocking functional issues were found in the updated call sites.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
web/skins/classic/js/skin.js:979
- The comment about
max-age=2147483647being “January of 2038” is inaccurate:Max-Ageis a duration (seconds from now), so 2147483647 seconds is ~68 years in the future. This can mislead future changes/debugging around cookie lifetime.
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
Problem
A hidden tab has its timers throttled, and a frozen/slept one has them stopped outright, so nothing refreshes the credential while the tab is in the background. The server rotates the auth hash at half of
AUTH_HASH_TTL(generateAuthHash()), so the hash baked into a stream<img>src or a deferred table url is usually dead by the time the tab is refocused.On refocus, every restarted stream and deferred table poll fires a request off that expired hash, 403s, and logs an auth error — before anything gets around to revalidating.
Change
web/js/auth-helpers.js:ZM_WEB_VIEWING_TIMEOUTstops its monitors without ever going hidden.authFreshAtis stamped whenever the server confirms the credential; the decision lives in a pureauthIsStale(freshAt, now)so it is testable without faking the clock or the DOM.AUTH_STALE_MSis an hour, matching the server's rotation interval.whenAuthFresh(cb): runscbstraight away while the credential is still trusted, but queues it behind a revalidation once it has gone stale, so nothing makes an authenticated request on an expired hash.revalidateAuth()now queues callbacks instead of dropping them when a probe is already in flight, and still runs the queued callbacks on a transient failure — a network blip is no reason to leave the page's streams stopped. A rejected session redirects to login and drops the queue.zm_authenticate_request()resolves a request against exactly one source: a non-emptyauth=in the URL enters theZM_AUTH_HASH_LOGINSbranch (on by default), and whengetAuthUser()rejects it the chain has already been taken, so theuserFromSession()arm below it never runs and a live session cookie authenticates as nobody. Carrying the suspect hash made the probe the one request guaranteed to fail pastZM_AUTH_HASH_TTL, and that failure reads aslogin— bouncing a user with a good session to the login page. Bare, the session cookie answers, which is the question being asked.Gated the visibility- and idle-driven callers on it:
web/skins/classic/views/js/watch.js—startPage()web/js/table-helpers.js—refreshTablesPendingVisibility()web/skins/classic/views/js/montage.js— the resume paths now go throughwhenAuthFresh(startVisibleMonitors), replacing its own bespoke auth-refresh helper. That also drops the duplicate navBar probe it fired on every refocus alongside skin.js's.Testing
node tests/js/auth-helpers.test.js— 51 passed, 0 failed (2 cases forauthIsStale, 4 forrevalidateAuth: the bare probe, a shared in-flight request, callbacks surviving a transient failure, and login on a rejected session). Reverting the probe to the credentialed form fails two of them.node tests/js/table-helpers.test.js— 8 passed, 0 failedtests/js/suite green;npx eslintclean on all changed files.Not browser-verified — the long-hide path wants a real tab left alone past
ZM_AUTH_HASH_TTL.🤖 Generated with Claude Code
https://claude.ai/code/session_01Nr76CednxtDt2nPuq6WrbL