Fix timing leak, CORS enforcement, CI race condition, and frontend hardcoding - #84
Fix timing leak, CORS enforcement, CI race condition, and frontend hardcoding#84potemkin666 with Copilot wants to merge 10 commits into
Conversation
…efresh - Randomize rankedScheduledSources tie-breaking with Math.random() jitter instead of stable array index, so same-priority sources are shuffled each run - Randomize selectHtmlSourcesForRun rotateTier with jitter-based tie-breaking instead of deterministic offset rotation within same weight buckets - Randomize continuationCandidates tie-breaking with jitter so backup sources are also picked in varying order Priority ordering is preserved (incidents > trusted > general), but sources at the same priority level are shuffled randomly each run to keep the feed fresh. Agent-Logs-Url: https://github.com/potemkin666/Brialert/sessions/0e50b7ca-6e9d-4b18-9f43-44f8eaeaac44 Co-authored-by: potemkin666 <183807833+potemkin666@users.noreply.github.com>
…ts, confidence bounds - Escape entry.source and entry.sourceUrl in HTML templates (alert-view-model.mjs) - Remove file paths from parseJsonFile error messages (github-persistence.js) - Sanitize workflowMessage in approve-source.js and restore-source.js - Remove raw GitHub API error forwarding in trigger-live-feed.js - Deduplicate source-processing logic: main loop now calls attemptSourceBuild() - Add missing Playwright constant exports to config.mjs - Add 0-1 bounds check on confidenceScore (alert-view-model.mjs) Agent-Logs-Url: https://github.com/potemkin666/Brialert/sessions/125f5bed-88b1-4f52-9efa-960df9dc4e41 Co-authored-by: potemkin666 <183807833+potemkin666@users.noreply.github.com>
…undant parses Agent-Logs-Url: https://github.com/potemkin666/Brialert/sessions/42b1a3c3-155f-47e7-aa93-203dba553b5b Co-authored-by: potemkin666 <183807833+potemkin666@users.noreply.github.com>
Agent-Logs-Url: https://github.com/potemkin666/Brialert/sessions/42b1a3c3-155f-47e7-aa93-203dba553b5b Co-authored-by: potemkin666 <183807833+potemkin666@users.noreply.github.com>
…ve !important, rename duplicate clean() Agent-Logs-Url: https://github.com/potemkin666/Brialert/sessions/6ba44832-573e-445c-b4f5-c0e7c05c9c0b Co-authored-by: potemkin666 <183807833+potemkin666@users.noreply.github.com>
…ct 404 inline JS Agent-Logs-Url: https://github.com/potemkin666/Brialert/sessions/192e4b0f-d0f3-4fea-890d-f58fbf3a9011 Co-authored-by: potemkin666 <183807833+potemkin666@users.noreply.github.com>
Agent-Logs-Url: https://github.com/potemkin666/Brialert/sessions/192e4b0f-d0f3-4fea-890d-f58fbf3a9011 Co-authored-by: potemkin666 <183807833+potemkin666@users.noreply.github.com>
…centralize API base URL Agent-Logs-Url: https://github.com/potemkin666/Brialert/sessions/fc5b791e-3667-44c5-998c-f7f1e5868414 Co-authored-by: potemkin666 <183807833+potemkin666@users.noreply.github.com>
…gthen stemming, rename sourceMayAutoCooldown, enhance ESLint Agent-Logs-Url: https://github.com/potemkin666/Brialert/sessions/c351d7ab-8cd8-44cb-af8a-33576acccd28 Co-authored-by: potemkin666 <183807833+potemkin666@users.noreply.github.com>
|
@copilot resolve the merge conflicts in this pull request |
…randomize-websites # Conflicts: # data/brialert.sqlite Co-authored-by: potemkin666 <183807833+potemkin666@users.noreply.github.com>
Merge conflicts resolved in |
There was a problem hiding this comment.
Pull request overview
This PR addresses several security and correctness issues across the API layer, build scripts/CI workflow, and frontend by tightening request validation/enforcement, reducing hardcoded configuration, and improving runtime behavior/performance.
Changes:
- Hardened security controls (timing-safe token verification, stricter CORS gating in API routes, SSRF hostname blocking for URL validation, improved XSS escaping in some UI rendering).
- Centralized/configured backend base URL usage across frontend modules and build outputs; improved accessibility markup in
index.html. - Build/ops improvements (CI conflict handling strategy, reduced per-call allocations/caching, refactors to reduce duplication and improve run stats).
Reviewed changes
Copilot reviewed 30 out of 32 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/modal-remote-client.test.mjs | Updates test to assert URL built from centralized DEFAULT_API_BASE. |
| styles.css | Removes !important from briefing-mode display rules. |
| shared/map-watch.mjs | Hoists country code Map to module scope to avoid repeated allocations. |
| shared/fusion.mjs | Tightens stemming guards to avoid overly-short stems. |
| shared/feed-controller.mjs | Only updates lastBrowserPollAt on successful fetch. |
| shared/api-base.mjs | Adds centralized DEFAULT_API_BASE constant for client-side API calls. |
| shared/alert-view-model.mjs | Escapes some rendered fields and clamps confidenceScore to [0,1]. |
| scripts/build-live-feed/geo.mjs | Caches geo regex compilation for performance. |
| scripts/build-live-feed/config.mjs | Exposes additional Playwright/config constants via env-backed helpers. |
| scripts/build-live-feed.mjs | Refactors source build logic into attemptSourceBuild, adds jitter-based ordering, and uses env-configured API base in generated quarantine HTML. |
| index.html | Adds CSP reminder comment, improves form accessibility via sr-only labels, and defers Leaflet script. |
| eslint.config.mjs | Enables prefer-const and no-unused-vars (with test override). |
| app/render/modal-ui-controller.mjs | Removes unused import. |
| app/render/modal-remote-client.mjs | Uses centralized DEFAULT_API_BASE for remote brief endpoint. |
| app/render/live.mjs | Avoids unused parameter lint error via _modalController. |
| app/redirect.js | Extracts redirect logic into a shared script. |
| app/feed/source-requests.mjs | Uses centralized API base; refines rate-limiting behavior; renames trim helper. |
| app/feed/index.mjs | Uses centralized API base for trigger endpoints. |
| app/boot/index.mjs | Removes unused import. |
| api/trigger-live-feed.js | Enforces CORS allowlist return value; addresses trigger timing/race behavior; tightens error messaging. |
| api/restore-source.js | Enforces CORS allowlist return value; avoids leaking internal workflow errors. |
| api/request-source.js | Enforces CORS allowlist return value. |
| api/quarantined-sources.js | Enforces CORS allowlist return value. |
| api/auth/session.js | Enforces CORS allowlist return value. |
| api/auth/logout.js | Enforces CORS allowlist return value. |
| api/approve-source.js | Enforces CORS allowlist return value; avoids leaking internal workflow errors. |
| api/_lib/github-persistence.js | Removes internal JSON parse detail leakage; adds SSRF-oriented hostname blocking to URL validation. |
| api/_lib/admin-session.js | Fixes timing leak in signature check; changes CORS helper to return allow/deny for all methods. |
| 404.html | Switches inline redirect script to app/redirect.js. |
| .gitignore | Ignores SQLite files under data/. |
| .github/workflows/update-live-feed.yml | Adjusts conflict handling to abort rebase and merge with -X ours, with force-with-lease push. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <meta http-equiv="refresh" content="0; url=/index.html"> | ||
| <script> | ||
| const canonicalTarget = location.pathname.toLowerCase().includes('/brialert') | ||
| ? '/Brialert/index.html' | ||
| : '/index.html'; | ||
| const query = location.search || ''; | ||
| const hash = location.hash || ''; | ||
| if (location.pathname.toLowerCase() !== canonicalTarget.toLowerCase()) { | ||
| location.replace(canonicalTarget + query + hash); | ||
| } | ||
| </script> | ||
| <script src="app/redirect.js"></script> |
There was a problem hiding this comment.
meta http-equiv="refresh" content="0; url=/index.html" will likely fire before app/redirect.js finishes downloading/executing, so requests under /Brialert/... can be redirected to the wrong canonical target on slower connections. Prefer making the JS redirect the primary mechanism (remove the 0s meta refresh or delay it), and optionally keep a <noscript> fallback to /index.html.
| <div id="toast" class="toast" aria-live="polite"></div> | ||
| <script> | ||
| const DEFAULT_API_BASE = 'https://brialertbackend.vercel.app'; | ||
| const DEFAULT_API_BASE = '${process.env.BRIALERT_API_BASE || 'https://brialertbackend.vercel.app'}'; |
There was a problem hiding this comment.
This interpolates process.env.BRIALERT_API_BASE directly into a single-quoted JS string inside generated HTML. If the env var contains ', newlines, or </script>, it can break the page or inject script. Serialize the value safely (e.g., use JSON.stringify(...) for the string literal) before embedding it into the <script> block.
| const DEFAULT_API_BASE = '${process.env.BRIALERT_API_BASE || 'https://brialertbackend.vercel.app'}'; | |
| const DEFAULT_API_BASE = ${JSON.stringify(process.env.BRIALERT_API_BASE || 'https://brialertbackend.vercel.app').replace(/</g, '\\u003c')}; |
| return `<div class="corroboration-list">${sources.map((entry) => ` | ||
| <article class="corroboration-item"> | ||
| <a href="${entry.sourceUrl}" target="_blank" rel="noreferrer">${entry.source}</a> | ||
| <a href="${escapeHtml(entry.sourceUrl)}" target="_blank" rel="noreferrer">${escapeHtml(entry.source)}</a> |
There was a problem hiding this comment.
Escaping entry.sourceUrl prevents HTML injection, but it doesn't prevent dangerous URL schemes like javascript: or data:. Since sourceUrl ultimately comes from feeds (item.link), consider normalising/validating to only allow http:/https: (otherwise render as # or omit the link).
| export function applyCorsHeaders(request, response, methods) { | ||
| const requestOrigin = normaliseOrigin(request?.headers?.origin); | ||
| const allowedOrigins = new Set(getAllowedOrigins()); | ||
| if (requestOrigin && allowedOrigins.has(requestOrigin)) { | ||
| const originAllowed = !requestOrigin || allowedOrigins.has(requestOrigin); | ||
| if (requestOrigin && originAllowed) { | ||
| response.setHeader('Access-Control-Allow-Origin', requestOrigin); | ||
| response.setHeader('Access-Control-Allow-Credentials', 'true'); | ||
| response.setHeader('Vary', 'Origin'); | ||
| } | ||
| response.setHeader('Access-Control-Allow-Methods', methods); | ||
| response.setHeader('Access-Control-Allow-Headers', 'Content-Type'); | ||
| if (request?.method === 'OPTIONS') { | ||
| return !requestOrigin || allowedOrigins.has(requestOrigin); | ||
| } | ||
| return true; | ||
| return originAllowed; |
There was a problem hiding this comment.
normaliseOrigin() returns '' for values like Origin: null (opaque origins) or other unparsable Origin headers. With the new “deny when applyCorsHeaders() returns false” gating, those requests will now be treated as having no origin and be allowed. Consider explicitly treating Origin: null / invalid Origin headers as disallowed when an Origin header is present, so the allowlist can’t be bypassed via an opaque origin.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29993fc857
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .debug-artifacts/ | ||
| .DS_Store | ||
| Thumbs.db | ||
| data/*.sqlite |
There was a problem hiding this comment.
Stop ignoring the SQLite artifact committed by CI
Adding data/*.sqlite to .gitignore makes git add data/brialert.sqlite fail in the commit-live-feed job (.github/workflows/update-live-feed.yml), so scheduled feed updates will error whenever the builder outputs that file. This is a regression from the previous behavior where the SQLite artifact could be staged and committed; now the workflow exits non-zero with "path is ignored" instead of publishing refreshed data.
Useful? React with 👍 / 👎.
| // Claim the slot before the async dispatch so concurrent requests see | ||
| // the lock immediately (avoids check-then-act race). | ||
| const previousTriggerTime = lastTriggerTime; | ||
| lastTriggerTime = now; |
There was a problem hiding this comment.
Roll back trigger lock when dispatch throws
Setting lastTriggerTime = now before dispatch is fine for race prevention, but this path only rolls it back for non-OK HTTP responses; exceptions (e.g., network failure or early config error) leave the lock set and cause subsequent requests to return 429 for 60 seconds even though no workflow was triggered. That blocks legitimate retries after transient failures and makes manual recovery unnecessarily slower.
Useful? React with 👍 / 👎.
| location.replace(canonicalTarget + query + hash); | ||
| } | ||
| </script> | ||
| <script src="app/redirect.js"></script> |
There was a problem hiding this comment.
Use a path-safe redirect script reference on 404 page
Switching from inline redirect logic to <script src="app/redirect.js"> introduces a path-resolution bug for deep URLs: on routes like /Brialert/foo/bar, the browser resolves this to /Brialert/foo/app/redirect.js, so the script fails to load and the canonical /Brialert/index.html redirect logic never runs. In that case users fall back to the meta refresh target /index.html, which can send them to the wrong site root.
Useful? React with 👍 / 👎.
Multiple security, correctness, and maintainability issues across API routes, CI workflow, and frontend configuration.
Security
Timing-safe token verification (
admin-session.js):verifySignedTokendid a bareBuffer.lengthcomparison beforetimingSafeEqual, leaking token format information via timing. Now pads both buffers to equal length before the constant-time comparison:CORS enforcement (all 7 API routes):
applyCorsHeaders()returned a boolean but every caller discarded it. All routes now gate on the return value and early-return 403 for disallowed origins. The function itself now returnsfalsefor disallowed origins on all HTTP methods, not just OPTIONS.CI
update-live-feed.yml): Replaced per-filegit checkout --theirs(silently discards the other run's data) withgit rebase --abort→git merge -X ours→git push --force-with-lease.Frontend
brialertbackend.vercel.appfrom 5 files intoshared/api-base.mjs. Build script readsBRIALERT_API_BASEenv var with fallback.Performance & correctness
countryCodeFor()Map allocation (map-watch.mjs): 70-entry Map was recreated on every call during cluster rendering. Hoisted to module-levelCOUNTRY_CODE_MAP.lastBrowserPollAton failure (feed-controller.mjs): Was set unconditionally, making poll timing data misleading after errors. Now only set on success.fusion.mjs): Suffix removal for-ing,-ed,-es,-scould produce 1–2 char stems. Tightened length guards so stems are always ≥ 3 chars.Cleanup
sourceMayAutoCooldown()→getAutoCooldownStatus()(returns object/null, not boolean)prefer-const,no-unused-vars(with^_ignore patterns), test-specific override. Fixed 5 pre-existing unused-var issues surfaced by the new rules.