You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
SFLW-51 — dashboard WebSocket fails in dev mode (corrected root cause)
In dev mode (npm run dev:dashboard), the /ws upgrade failed with Unexpected response code: 500 and the connection indicator never reached Connected.
Root cause — not what the issue hypothesized
The issue blamed a localhost → IPv6 ::1 mismatch. Empirical probing disproved that: both localhost and 127.0.0.1 handshake fine directly against the backend, with or without IPv6. The 500 body was:
{"statusCode":500,"error":"Internal Server Error","message":"Not allowed by CORS"}
@fastify/cors was rejecting the WS upgrade GET. generateAllowedOrigins() builds the allowlist from the backend's own port plus a hardcoded Vite port 5173. Any other dev-server port sends a cross-origin Origin header that CORS throws on. The e2e smoke harness runs Vite on 5185, so it always failed. Production is same-origin (frontend served from the backend process), so it never triggered — matching the "dev mode only" tag.
Fix
src/core/security-utils.ts — in non-production, getCorsConfig now accepts any loopback origin regardless of port (new isLoopbackOrigin helper). The dashboard already binds localhost-only, so this does not widen exposure beyond the local machine. Production (same-origin) behavior is unchanged.
src/dashboard_frontend/vite.config.ts — point the /api + /ws proxy targets at 127.0.0.1 to match the backend's IPv4 bind exactly (defensive; not the primary fix).
e2e/helpers/dashboard-smoke-harness.ts — remove the now-obsolete PRE_EXISTING_WS_PROXY_ERROR suppression so the smoke suite actively guards the fix.
Verification
security-utils unit tests + 6 new cases for isLoopbackOrigin / getCorsConfig — green
Full unit suite — 342 passed
Dashboard smoke e2e (test:e2e:smoke) — 2 passed with the suppression removed and no console WS/500 errors
npm run build — clean
Notes
No version bump — rolls into the next release.
/gsd chore; closes SFLW-51.
Summary by CodeRabbit
Tests
Added comprehensive test coverage for loopback origin validation and CORS configuration behavior across different environments.
Bug Fixes
Improved CORS handling to properly support loopback origins in non-production environments, enabling better local development support.
Updated dev-server proxy configuration for more reliable IPv4/IPv6 compatibility.
Removed overly broad WebSocket error filtering from test utilities.
…nects in dev mode
The /ws upgrade failed in dev mode (vite dev:dashboard) with a 500 handshake.
Root cause was NOT the localhost/IPv6 mismatch the issue hypothesized — both
localhost and 127.0.0.1 handshake fine directly against the backend. The 500
was @fastify/cors rejecting the upgrade: the allowlist only contained the
backend's own port plus a hardcoded Vite port (5173), so any other dev-server
port (the e2e smoke harness uses 5185) sent a cross-origin Origin that CORS
threw on ("Not allowed by CORS").
Fix: in non-production, getCorsConfig now accepts any loopback origin
regardless of port (new isLoopbackOrigin helper). The dashboard already binds
localhost-only, so this does not widen exposure beyond the local machine.
Production (same-origin, frontend served by the backend) is unchanged.
Also point the vite proxy targets at 127.0.0.1 to match the backend's IPv4
bind exactly, and remove the now-obsolete PRE_EXISTING_WS_PROXY_ERROR e2e
suppression so the smoke suite actively guards the fix.
Verified: security-utils unit tests (+6 new), full unit suite (342), and the
dashboard smoke e2e suite all pass with the suppression removed.
Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).
⌛ How to resolve this issue?
After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.
We recommend that you space out your commits to avoid hitting the rate limit.
🚦 How do rate limits work?
CodeRabbit enforces hourly rate limits for each developer per organization.
Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.
Reviewing files that changed from the base of the PR and between 5f33eea and 531ba1c.
📒 Files selected for processing (3)
src/core/__tests__/security-utils.test.ts
src/core/security-utils.ts
src/dashboard_frontend/vite.config.ts
Walkthrough
Adds an exported isLoopbackOrigin helper to security-utils.ts and updates getCorsConfig to allow loopback origins in non-production environments regardless of port. The Vite dev-server proxy targets change from localhost to 127.0.0.1. The e2e harness removes the pre-existing WebSocket error filter and the PRE_EXISTING_WS_PROXY_ERROR export.
Changes
Loopback origin CORS and proxy fix
Layer / File(s)
Summary
isLoopbackOrigin helper and updated CORS callback src/core/security-utils.ts, src/core/__tests__/security-utils.test.ts
Exports isLoopbackOrigin that returns true for localhost, 127.x.x.x, and ::1 (with IPv6 bracket normalization). getCorsConfig's origin callback is updated to allow loopback origins in non-production in addition to exact allowedOrigins matches, rejecting everything else with Error('Not allowed by CORS'). New Vitest suites cover isLoopbackOrigin return values and getCorsConfig behavior across NODE_ENV states.
Vite proxy targets src/dashboard_frontend/vite.config.ts
/api and /ws proxy targets change from localhost to 127.0.0.1, with added comments explaining IPv4 vs. IPv6 loopback resolution behavior.
collectConsoleErrors removes the WebSocket proxy error regex filter (PRE_EXISTING_WS_PROXY_ERROR export deleted); all console.error events are now recorded without exclusion.
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
lbruton/SpecFlow#31: Directly modifies the same collectConsoleErrors function in dashboard-smoke-harness.ts, introducing the PRE_EXISTING_WS_PROXY_ERROR filter that this PR removes.
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes dashboard WebSocket failures in dev mode by relaxing CORS handling for local development origins (so Vite can run on non-default ports) and aligning Vite’s proxy target with the backend’s IPv4 loopback bind, while also tightening the e2e smoke harness to fail on any console error.
Changes:
Add isLoopbackOrigin() and update getCorsConfig() to allow loopback origins on any port in non-production environments.
Update Vite dev proxy targets to use 127.0.0.1 for /api and /ws.
Remove the previously-whitelisted WebSocket proxy error filter so smoke tests actively enforce the fix.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File
Description
src/dashboard_frontend/vite.config.ts
Switch dev proxy targets to 127.0.0.1 for more reliable loopback routing.
src/core/security-utils.ts
Expand non-production CORS to permit loopback origins regardless of port via isLoopbackOrigin().
src/core/tests/security-utils.test.ts
Add unit tests covering the new loopback-origin and updated CORS behavior.
e2e/helpers/dashboard-smoke-harness.ts
Remove the prior WS error suppression so smoke tests fail on any console error.
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
The PR successfully addresses development-mode WebSocket connectivity issues by refining CORS loopback handling and standardizing Vite proxy targets to IPv4. Codacy analysis indicates the changes are up to standards, and all primary acceptance criteria have been addressed.
The primary concern that should be addressed before merging is a security flaw in the loopback origin validation. The current implementation uses a prefix check that allows hostnames like '127.attacker.com' to be treated as valid loopback origins. This exposes the application to cross-site risks in development environments. Improving the regex and simplifying the hostname extraction will resolve this risk.
Test suggestions
Verify isLoopbackOrigin correctly identifies localhost, 127.0.0.1, and [::1] as valid loopback origins regardless of port.
Verify isLoopbackOrigin rejects external IP addresses and malformed URLs.
Verify getCorsConfig allows loopback origins when NODE_ENV is set to development.
Verify getCorsConfig rejects loopback origins in production if they are not in the explicit allowlist.
Verify the smoke test suite successfully detects console errors for WebSocket failures.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/security-utils.ts`:
- Around line 73-77: The isLocalhostAddress helper function is too permissive in
its validation and accepts any hostname starting with 127., creating a CORS
bypass vulnerability where hostnames like 127.attacker.com would be incorrectly
treated as loopback. Fix the isLocalhostAddress function to only accept exact
loopback addresses (127.0.0.1, localhost, ::1, and IPv6 loopback equivalents)
rather than any hostname with a 127. prefix, ensuring that hostnames containing
additional characters after 127. (such as 127.attacker.com) are properly
rejected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f5ac6ecf-86f5-4475-81c4-88773a7cc701
📥 Commits
Reviewing files that changed from the base of the PR and between 9125f9b and 5f33eea.
…ot review)
isLoopbackOrigin delegated to isLocalhostAddress, whose loose
startsWith('127.') would also match hostnames like "127.example.com" —
a CORS-bypass in non-production. Replace with a strict IPv4 127.0.0.0/8
check (four numeric octets, first === 127, each 0-255); localhost and ::1
are still accepted. isLocalhostAddress is left unchanged (its bindAddress
use is a separate, pre-existing concern).
Also consolidate the duplicated getCorsConfig test describe into the
existing block and add a regression case asserting 127.example.com is
rejected. Unit suite 340 green; build clean.
…0.0.0/8
Codacy + CodeRabbit both flagged that isLocalhostAddress's loose
startsWith('127.') accepts hostnames like "127.example.com" — a hole in
both the CORS loopback path and the bindAddress-validation guard
(config.ts, multi-server.ts). Replace the prefix check with a strict IPv4
127.0.0.0/8 regex (each octet 0-255); "localhost" and "::1" still pass.
All legitimate bindAddress values (127.0.0.1, localhost, ::1) are
preserved, and existing 127.x.x.x test cases stay green.
isLoopbackOrigin now delegates to the hardened helper (single source of
truth), keeping the IPv6 bracket-strip — Node's URL.hostname returns
"[::1]" with brackets, so the strip is required, not redundant.
Added an isLocalhostAddress regression case for 127.example.com /
invalid-octet inputs. Unit suite 341 green; build clean.
…able proxy host (Copilot review)
Addresses Copilot round-3 feedback:
- getCorsConfig: the non-production loopback relaxation now applies only when
the configured allowedOrigins already trusts loopback. A user who overrides
allowedOrigins to exclude loopback (e.g. a shared dev/staging box with
NODE_ENV!=='production') is respected instead of being silently widened open.
- vite.config.ts: proxy target host is now configurable via VITE_DASHBOARD_HOST
(default 127.0.0.1), so dev setups whose backend binds ::1/localhost can point
the proxy accordingly.
- tests: restore NODE_ENV via a describe-level afterEach that deletes the var
when it was originally unset (assignment would set the literal "undefined" and
leak into later tests). Added a regression test for the allowlist-gating.
Unit suite 342 green; smoke e2e 2 green; build clean.
… gate
The repetitive per-test boilerplate (build config → getCorsConfig → vi.fn() →
origin()) across the getCorsConfig cases tripped Codacy's duplication metric
(+6, threshold ≤5). Extract a checkOrigin(origin, allowedOrigins?) helper and
collapse each case to a single assertion. Test-only; 342 unit tests still green.
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
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.
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
SFLW-51 — dashboard WebSocket fails in dev mode (corrected root cause)
In dev mode (
npm run dev:dashboard), the/wsupgrade failed withUnexpected response code: 500and the connection indicator never reached Connected.Root cause — not what the issue hypothesized
The issue blamed a
localhost→ IPv6::1mismatch. Empirical probing disproved that: bothlocalhostand127.0.0.1handshake fine directly against the backend, with or without IPv6. The 500 body was:@fastify/corswas rejecting the WS upgrade GET.generateAllowedOrigins()builds the allowlist from the backend's own port plus a hardcoded Vite port5173. Any other dev-server port sends a cross-originOriginheader that CORS throws on. The e2e smoke harness runs Vite on 5185, so it always failed. Production is same-origin (frontend served from the backend process), so it never triggered — matching the "dev mode only" tag.Fix
src/core/security-utils.ts— in non-production,getCorsConfignow accepts any loopback origin regardless of port (newisLoopbackOriginhelper). The dashboard already binds localhost-only, so this does not widen exposure beyond the local machine. Production (same-origin) behavior is unchanged.src/dashboard_frontend/vite.config.ts— point the/api+/wsproxy targets at127.0.0.1to match the backend's IPv4 bind exactly (defensive; not the primary fix).e2e/helpers/dashboard-smoke-harness.ts— remove the now-obsoletePRE_EXISTING_WS_PROXY_ERRORsuppression so the smoke suite actively guards the fix.Verification
security-utilsunit tests + 6 new cases forisLoopbackOrigin/getCorsConfig— greentest:e2e:smoke) — 2 passed with the suppression removed and no console WS/500 errorsnpm run build— cleanNotes
/gsdchore; closes SFLW-51.Summary by CodeRabbit
Tests
Bug Fixes