fix(auth): limit the sign-out toast message to a known allowlist - #8037
fix(auth): limit the sign-out toast message to a known allowlist#8037yhabib wants to merge 5 commits into
Conversation
displayAndCleanLogoutMsg passed the msg query parameter straight to toastsShow as a labelKey. translate returns the raw key when the catalog has no match, so a crafted link showed any text as an official toast. The level parameter set the toast color with no check. The url now carries only a key from LOGOUT_MSGS. The map owns the level, so the url cannot pick the styling. An unknown key shows no toast and the url is still cleaned.
The spec checks three things in the browser. A msg that is not in the allowlist shows no toast. An allowlisted msg keeps the level of the app, not the level in the url. The automatic sign-out in a second tab still shows the session expiry toast.
The mocked window.location set only `search`. `cleanUpMsgUrl` builds its url from `href`, so the url assertions held whatever the code did. The mock now sets `href` with the same query.
|
✅ No security or compliance issues detected. Reviewed everything up to 3e25ff3. Security Overview
Detected Code Changes
|
There was a problem hiding this comment.
🟡 Changes recommended
displayAndCleanLogoutMsg() does not currently clean the URL when only the level query parameter is present, which conflicts with the stated goal of always cleaning sign-out toast parameters.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens sign-out toast handling by restricting URL-provided logout messages to a fixed allowlist, preventing crafted links from spoofing “official” toasts on the sign-in page.
Changes:
- Introduced a
LOGOUT_MSGSallowlist and narrowedlogout()to accept onlyLogoutMsgKeyvalues (toast level now derived from the allowlist). - Updated
displayAndCleanLogoutMsg()to ignore unknown/prototype keys and to ignore any URL-providedlevel. - Added unit + e2e coverage for allowlist behavior, spoofed
level, prototype keys, and URL cleanup; documented the security change in the changelog.
File summaries
| File | Description |
|---|---|
| frontend/src/lib/services/auth.services.ts | Adds allowlisted logout message keys, derives toast level from allowlist, and updates URL read/write behavior. |
| frontend/src/lib/services/worker-auth.services.ts | Updates auth worker logout call site to pass the new allowlisted message key. |
| frontend/src/tests/lib/services/auth.services.spec.ts | Expands unit tests to cover allowlist behavior, spoofed level, prototype keys, and URL cleanup. |
| frontend/src/tests/e2e/logout-msg.spec.ts | Adds e2e coverage for unknown/crafted messages and session-expiry sign-out toast behavior. |
| CHANGELOG-Nns-Dapp-unreleased.md | Notes the security hardening of sign-out toast messages sourced from URL parameters. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
displayAndCleanLogoutMsg only cleaned the url when msg was present. A url with only level stayed untouched. It now cleans the url whenever msg or level is present.
There was a problem hiding this comment.
🔵 Needs a closer look
The new Playwright E2E test uses toHaveTitle() with a global expect timeout of 0, which can hang CI indefinitely on failures unless bounded timeouts are added.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
frontend/src/tests/e2e/logout-msg.spec.ts:66
- The Playwright config sets
expecttimeout to 0 (no limit), sotoHaveTitle(...)can hang indefinitely if the page fails to load or the title changes. Add an explicit timeout here to keep CI from stalling on failures.
This issue also appears on line 69 of the same file.
frontend/src/tests/e2e/logout-msg.spec.ts:71
- Same as above: with a global
expecttimeout of 0, thistoHaveTitle(...)can wait forever and hang the test run. Add a bounded timeout.
await page2.goto("/accounts");
await expect(page2).toHaveTitle("Account | Network Nervous System");
const appPo2 = new AppPo(PlaywrightPageObjectElement.fromPage(page2));
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Disagree. The zero expect timeout is repo-wide config (frontend/playwright.config.ts), and every other e2e spec calls toHaveTitle() the same way (accounts.spec.ts, address-book.spec.ts, multi-tab-auth.spec.ts, and more). The 300s per-test timeout already bounds a failure; this is not something this PR introduced. |
There was a problem hiding this comment.
🟡 Changes recommended
The new e2e helper asserts level removal without waiting/polling, which can introduce test flakiness if URL cleanup is not synchronous with msg removal.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The write path still preserves any pre-existing untrusted level query param in the current URL when appending msg, which should be removed for a fully deterministic and hardened URL state.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
appendMsgToUrl set msg but left an already-present level param in place. It now deletes level before it sets msg, so a crafted or legacy url cannot carry level into the post-logout url.
There was a problem hiding this comment.
🟢 Approved
The allowlist-based implementation and the added unit/e2e coverage appear consistent with the stated security goals and do not introduce obvious regressions.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
Motivation
displayAndCleanLogoutMsgread themsgandlevelquery params from the url and passed them straight to the toast, with no check. A crafted link could show any text as an official error, warning, or success toast on the sign-in page.Changes
LOGOUT_MSGSallowlist that maps each known message key to a fixed toast level.logout()to take aLogoutMsgKeyinstead of a free-form label and level.displayAndCleanLogoutMsgto show a toast only for a key in the allowlist, and to always clean the url.levelquery param from the write path; the level now comes from the allowlist only.getAuthenticatedIdentity,worker-auth.services.ts) to pass the new key.Tests
auth.services.spec.tsfor an unknownmsg, a spoofedlevel, a prototype key (constructor), and the cleaned url.frontend/src/tests/e2e/logout-msg.spec.ts, covering the unknownmsgcase and the automatic sign-out toast after a session expiry.npm run checkandnpm run testlocally, both pass.Todos
#### SecurityinCHANGELOG-Nns-Dapp-unreleased.md.