fix(frontend): guard requests on auth and batch settings fetches - #3749
Open
jmthomas wants to merge 3 commits into
Open
fix(frontend): guard requests on auth and batch settings fetches#3749jmthomas wants to merge 3 commits into
jmthomas wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## auth-verify-token-logging #3749 +/- ##
=============================================================
- Coverage 79.11% 78.99% -0.12%
=============================================================
Files 894 895 +1
Lines 66871 66925 +54
Branches 2591 2607 +16
=============================================================
- Hits 52906 52869 -37
- Misses 13302 13404 +102
+ Partials 663 652 -11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR centralizes “no usable token” handling to prevent bursts of guaranteed-401 requests on page load, and reduces redundant settings round-trips by batching settings fetches.
Changes:
- Introduces a shared
authGuard(token refresh + auth-required detection + “log unless auth”) and wires it intoApi/OpenC3Apiand selected UI callers. - Batches settings reads via
get_settingsinsettingsCacheand updates base components to use the cache. - Updates Login flow to validate session tokens via
POST /auth/verify-tokenand opt out of interceptor banners for expected 401/429 responses.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| openc3-cosmos-init/plugins/packages/openc3-vue-common/src/widgets/ArrayplotWidget.vue | Suppresses subscription attempt when token refresh rejects. |
| openc3-cosmos-init/plugins/packages/openc3-vue-common/src/util/settingsCache.js | Implements per-tick batching for settings fetches + cache invalidation. |
| openc3-cosmos-init/plugins/packages/openc3-vue-common/src/util/index.js | Re-exports new invalidateCachedSetting. |
| openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/base/UserMenu.vue | Switches to cached settings + auth-aware logging for requests. |
| openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/base/ThemeSwitcher.vue | Switches theme read to cached settings. |
| openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/base/Notifications.vue | Uses auth-aware logging for expected auth redirects. |
| openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/base/Login.vue | Adds verify-token path + ignores expected 401/429 in interceptor. |
| openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/base/ContextTag.vue | Uses cached setting + invalidates cache for polling refresh. |
| openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/base/ClassificationBanners.vue | Switches classification banner read to cached settings. |
| openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/base/AppNav.vue | Batches base settings reads + auth-aware logging for tool list fetch, suppresses token-refresh rejection noise. |
| openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/base/AppFooter.vue | Switches source_url read to cached settings + auth-aware logging for version call. |
| openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/admin/tabs/ToolsTab.vue | Makes “add tool” failures resilient to missing error.response. |
| openc3-cosmos-init/plugins/packages/openc3-vue-common/src/components/Graph.vue | Suppresses subscription attempt when token refresh rejects. |
| openc3-cosmos-init/plugins/packages/openc3-tool-base/public/js/auth.js | Makes core Auth reject with AuthRequiredError when redirecting to login. |
| openc3-cosmos-init/plugins/packages/openc3-js-common/src/services/openc3Api.js | Uses refreshToken() guard before JSON-RPC requests. |
| openc3-cosmos-init/plugins/packages/openc3-js-common/src/services/index.js | Exports new auth-guard helpers (without exporting the error class). |
| openc3-cosmos-init/plugins/packages/openc3-js-common/src/services/cable.js | Treats auth-required rejection as a non-tool-error for subscriptions. |
| openc3-cosmos-init/plugins/packages/openc3-js-common/src/services/axios.js | Updates interceptor ordering; 401 triggers refresh/redirect without banner spam. |
| openc3-cosmos-init/plugins/packages/openc3-js-common/src/services/authGuard.js | Adds shared auth-required classification, guarded refresh, and unhandledrejection suppression. |
| openc3-cosmos-init/plugins/packages/openc3-js-common/src/services/api.js | Uses refreshToken() guard before REST requests. |
| openc3-cosmos-init/plugins/packages/openc3-cosmos-tool-dataviewer/src/tools/DataViewer/DataViewer.vue | Suppresses subscription attempts when token refresh rejects. |
| openc3-cosmos-init/plugins/packages/openc3-cosmos-tool-dataextractor/src/tools/DataExtractor/DataExtractor.vue | Suppresses subscription attempts when token refresh rejects. |
Suppressed comments (1)
openc3-cosmos-init/plugins/packages/openc3-vue-common/src/components/Graph.vue:1
- Catching and swallowing all
updateToken()rejections hides non-auth failures (e.g., unexpected exceptions inside the auth implementation), which can make real issues very hard to diagnose. Consider only swallowing the redirect case (e.g., viaisAuthRequiredError(error)), and logging/rethrowing anything else. This pattern appears in multiple subscription sites in this PR; tightening the catch consistently would improve debuggability.
<!--
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Member
Author
|
Test on main by doing the following in the dev console:
Results in a bunch of CmdTlmServer logs: As well as a bunch of console errors. Another slightly different way to test:
After running this branch you should see only a single CmdTlmServer message and very few console log errors. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated no new comments.
Suppressed comments (8)
openc3-cosmos-init/plugins/packages/openc3-cosmos-tool-dataviewer/src/tools/DataViewer/DataViewer.vue:588
- This also swallows non-auth token-refresh failures, silently preventing packet subscriptions from being added. Only
AuthRequiredErrorshould be consumed; rethrow or report other errors, consistent withCable.createSubscription.
.catch(() => {})
openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/base/AppNav.vue:344
- The empty handler also hides non-auth refresh failures, despite this file already importing
logUnlessAuthRequired. Use that handler so expected redirects stay quiet while real refresh failures remain visible.
.catch(() => {})
openc3-cosmos-init/plugins/packages/openc3-tool-base/public/js/auth.js:40
- The redirect-once guard in
authGuardis bypassed here: every concurrent missing-token or 401 path callslogin()before rejecting, whilerefreshToken()rethrowsAuthRequiredErrorwithout usingredirectToLogin(). A page-load burst can therefore still schedule multiple redirects. Make navigation idempotent inside the shared Auth implementation, or move it entirely behind the guarded helper.
this.login(location.href)
openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/base/UserMenu.vue:235
- This request now cannot reach the server:
OpenC3Auth.logout()clearslocalStorage.openc3Tokenfirst, so the newApipreflight guard rejects before sending the authenticated logout request. The endpoint requires the current token to terminate the server session. Send it first and perform the local logout infinally.
Api.put(`/openc3-api/users/logout/${this.username}`).catch(
logUnlessAuthRequired,
)
openc3-cosmos-init/plugins/packages/openc3-vue-common/src/widgets/ArrayplotWidget.vue:246
- This swallows every token-refresh failure, not only
AuthRequiredError. A transient or edition-specific refresh failure will silently skip the subscription and leave the widget idle. CheckisAuthRequiredErrorand rethrow/log any other rejection, asCable.createSubscriptiondoes.
.catch(() => {})
openc3-cosmos-init/plugins/packages/openc3-vue-common/src/components/Graph.vue:1769
- This swallows every token-refresh failure, not only
AuthRequiredError. A transient or edition-specific refresh failure will silently skip the subscription and leave the graph idle. CheckisAuthRequiredErrorand rethrow/log any other rejection, asCable.createSubscriptiondoes.
.catch(() => {})
openc3-cosmos-init/plugins/packages/openc3-cosmos-tool-dataviewer/src/tools/DataViewer/DataViewer.vue:555
- This swallows every token-refresh failure, not only
AuthRequiredError. A transient or edition-specific refresh failure will silently skip adding the item subscriptions. CheckisAuthRequiredErrorand rethrow/log any other rejection, asCable.createSubscriptiondoes.
This issue also appears on line 588 of the same file.
.catch(() => {})
openc3-cosmos-init/plugins/packages/openc3-cosmos-tool-dataextractor/src/tools/DataExtractor/DataExtractor.vue:850
- This swallows every token-refresh failure, not only
AuthRequiredError. A transient or edition-specific refresh failure will silently skip adding the subscription and leave extraction idle. CheckisAuthRequiredErrorand rethrow/log any other rejection, asCable.createSubscriptiondoes.
.catch(() => {})
jmthomas
force-pushed
the
auth-guard-frontend
branch
from
August 20, 2026 21:27
ef2c1af to
80d1d77
Compare
|
jmthomas
force-pushed
the
auth-guard-frontend
branch
from
August 20, 2026 23:24
80d1d77 to
6b6f79a
Compare
A page load with a stale or missing token fired one request per component, each a guaranteed 401 logged server side as an error, and each rejection a console stack trace on the way to the login page. Add services/authGuard.js as the single place that decides "we have no usable token": refreshToken() rejects with an AuthRequiredError before the request goes out, isUnauthorizedError() recognizes both axios 401s and the synthetic errors OpenC3Api.exec builds from Ruby exception names, and logUnlessAuthRequired() replaces .catch(console.error) on requests that fire before login is settled. Errors are matched by name, not identity, so this works across core and enterprise Auth implementations. Settings reads now coalesce: getCachedSetting queues into one batched get_settings call per tick instead of a separate round trip per component, and a failed batch settles its callers with fallbacks without caching them, so a transient 401 doesn't pin every setting to its default for the session. Login.vue uses the new POST /auth/verify-token instead of auth#verify, which rejected session tokens as bad passwords and consumed rate limit. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Add an optionalAuth request option so AppNav's unauthorized-by-design /openc3-api/tools/all call still goes out with no token, which the nav needs to render on the login page itself. Replace swallow-all token refresh catches with logUnlessAuthRequired so only the expected redirect to login is silent, and give the add-tool notification a string fallback for JSON error bodies. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The server terminates only the session named by the Authorization header, but OpenC3Auth.logout() cleared the token and reloaded first, so the logout request either never went out or arrived unauthenticated. Await it and do the local logout in finally. Auth.login() now navigates at most once per page load, since a burst of tokenless requests calls updateToken (and therefore login) once each. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
jmthomas
force-pushed
the
auth-guard-frontend
branch
from
August 21, 2026 22:21
6b6f79a to
dcfaff5
Compare
|
|
❌ The last analysis has failed. |
EmilyRagan
reviewed
Aug 24, 2026
Comment on lines
+18
to
+40
| import { | ||
| isAuthRequiredError, | ||
| isUnauthorizedError, | ||
| logUnlessAuthRequired, | ||
| refreshToken, | ||
| } from './authGuard' | ||
| import axios from './axios' | ||
| import Cable from './cable' | ||
| import { ConfigParserError, ConfigParserService } from './configParser' | ||
| import OpenC3Api from './openc3Api' | ||
|
|
||
| export { Api, axios, Cable, ConfigParserError, ConfigParserService, OpenC3Api } | ||
| export { | ||
| Api, | ||
| axios, | ||
| Cable, | ||
| ConfigParserError, | ||
| ConfigParserService, | ||
| isAuthRequiredError, | ||
| isUnauthorizedError, | ||
| logUnlessAuthRequired, | ||
| OpenC3Api, | ||
| refreshToken, | ||
| } |
Contributor
There was a problem hiding this comment.
Suggested change
| import { | |
| isAuthRequiredError, | |
| isUnauthorizedError, | |
| logUnlessAuthRequired, | |
| refreshToken, | |
| } from './authGuard' | |
| import axios from './axios' | |
| import Cable from './cable' | |
| import { ConfigParserError, ConfigParserService } from './configParser' | |
| import OpenC3Api from './openc3Api' | |
| export { Api, axios, Cable, ConfigParserError, ConfigParserService, OpenC3Api } | |
| export { | |
| Api, | |
| axios, | |
| Cable, | |
| ConfigParserError, | |
| ConfigParserService, | |
| isAuthRequiredError, | |
| isUnauthorizedError, | |
| logUnlessAuthRequired, | |
| OpenC3Api, | |
| refreshToken, | |
| } | |
| export { | |
| isAuthRequiredError, | |
| isUnauthorizedError, | |
| logUnlessAuthRequired, | |
| refreshToken, | |
| } from './authGuard' | |
| import axios from './axios' | |
| import Cable from './cable' | |
| import { ConfigParserError, ConfigParserService } from './configParser' | |
| import OpenC3Api from './openc3Api' | |
| export { | |
| Api, | |
| axios, | |
| Cable, | |
| ConfigParserError, | |
| ConfigParserService, | |
| OpenC3Api, | |
| } |
EmilyRagan
reviewed
Aug 24, 2026
| } | ||
| } | ||
|
|
||
| export class AuthRequiredError extends Error { |
Contributor
There was a problem hiding this comment.
Since this is not used anywhere outside of this file and seems like that will not change, perhaps this class should not be exported, and it might be better to move the comment about intentionally not exporting it from index.js to here instead
Suggested change
| export class AuthRequiredError extends Error { | |
| // NOTE: AuthRequiredError itself is deliberately not exported. It's matched by | |
| // name, not by identity, so callers use isAuthRequiredError instead - exporting | |
| // the class would invite instanceof checks that break across editions. | |
| class AuthRequiredError extends Error { |
EmilyRagan
reviewed
Aug 24, 2026
Comment on lines
+15
to
+17
| // NOTE: AuthRequiredError itself is deliberately not exported. It's matched by | ||
| // name, not by identity, so callers use isAuthRequiredError instead - exporting | ||
| // the class would invite instanceof checks that break across editions. |
Contributor
There was a problem hiding this comment.
Moving this comment to the definition of the class where it is not exported is clearer
Suggested change
| // NOTE: AuthRequiredError itself is deliberately not exported. It's matched by | |
| // name, not by identity, so callers use isAuthRequiredError instead - exporting | |
| // the class would invite instanceof checks that break across editions. |
EmilyRagan
reviewed
Aug 24, 2026
Comment on lines
+33
to
+36
| // @param value [Number] unused in core, minimum token validity in seconds | ||
| // @param from_401 [Boolean] whether a request just came back unauthorized | ||
| // @return [Promise<Boolean>] whether the token was refreshed, or a rejection | ||
| // with an AuthRequiredError if we're redirecting to login instead |
Contributor
There was a problem hiding this comment.
Multi-line comment style will treat this as a JSDoc comment, associating the documentation with the referenced parameters
Suggested change
| // @param value [Number] unused in core, minimum token validity in seconds | |
| // @param from_401 [Boolean] whether a request just came back unauthorized | |
| // @return [Promise<Boolean>] whether the token was refreshed, or a rejection | |
| // with an AuthRequiredError if we're redirecting to login instead | |
| /** | |
| * @param value {number} unused in core, minimum token validity in seconds | |
| * @param from_401 {boolean} whether a request just came back unauthorized | |
| * @return {Promise<boolean>} whether the token was refreshed, or a rejection | |
| * with an AuthRequiredError if we're redirecting to login instead | |
| */ |
EmilyRagan
reviewed
Aug 24, 2026
Comment on lines
+179
to
+188
| if (!isUnauthorizedError(error)) { | ||
| return | ||
| } | ||
| // Stale token - drop it and let the user log in normally. Only if it's | ||
| // still the token we checked: the user can finish logging in while | ||
| // this request is in flight, and deleting the new token would send | ||
| // them straight back to the login form. | ||
| if (localStorage.openc3Token === token) { | ||
| delete localStorage.openc3Token | ||
| } |
Contributor
There was a problem hiding this comment.
Clearer logically to act on the positive case of "if unauthorized and the token is still the same as when the request was made, clear it"
Suggested change
| if (!isUnauthorizedError(error)) { | |
| return | |
| } | |
| // Stale token - drop it and let the user log in normally. Only if it's | |
| // still the token we checked: the user can finish logging in while | |
| // this request is in flight, and deleting the new token would send | |
| // them straight back to the login form. | |
| if (localStorage.openc3Token === token) { | |
| delete localStorage.openc3Token | |
| } | |
| if (isUnauthorizedError(error) && localStorage.openc3Token === token) { | |
| // Stale token - drop it and let the user log in normally. Only if it's | |
| // still the token we checked: the user can finish logging in while | |
| // this request is in flight, and deleting the new token would send | |
| // them straight back to the login form. | |
| delete localStorage.openc3Token | |
| } |
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.
Stacked on #3748 — review/merge that first; this branch's base is
auth-verify-token-logging, so the diff shown here is frontend only.Login.vuecalls thePOST /auth/verify-tokenendpoint added in #3748.Problem
A page load with a stale or missing token fired one request per component. Each was a guaranteed 401, logged server side as an error, and each rejection was a console stack trace on the way to the login page. The login page itself checked its token via
auth#verify, where a session token is never a valid password — so the check failed and consumed rate limit.services/authGuard.jsSingle place that decides "we have no usable token":
refreshToken()— rejects with anAuthRequiredErrorbefore the request goes out. Also redirects to login at most once per page load: a second redirect can only mean the first didn't navigate, which would otherwise bounce forever.isUnauthorizedError()— matches axios 401s and the synthetic errorsOpenC3Api.execbuilds, which carry no response, only a name copied from the Ruby exception class (OpenC3::AuthError,OpenC3::ForbiddenError). Without the name check every 401 throughOpenC3Apislipped past.logUnlessAuthRequired()— drop-in for.catch(console.error)on requests that fire before login is settled.unhandledrejectionhandler swallowsAuthRequiredErroronly. Being sent to login is a navigation, not a bug.Errors are matched by name, not identity —
AuthRequiredErroris deliberately not re-exported fromservices/index.js, sinceinstanceofbreaks across editions (core and enterprise each ship their own tool-base and Auth implementation).Batched settings
getCachedSettingnow queues into oneget_settingscall per tick instead of a round trip per component (theme, astro, subtitle, time_zone, source_url, … all mount together). Details that matter:generationcounter means a batch queued beforeresetSettingsCache()won't write into the cache afterwards — but still settles its promises, so awaiting callers aren't left hanging.invalidateCachedSetting()added for components that poll a setting on an interval (ContextTag) and would otherwise be served the same cached value forever.Test plan
pnpm lint --max-warnings 0clean inopenc3-js-common,openc3-vue-common,openc3-cosmos-tool-dataextractor,openc3-cosmos-tool-dataviewer🤖 Generated with Claude Code