Skip to content

fix(review): reject plain ws: in isSafeEndpointUrl, matching its own "secure" doc comment - #8131

Merged
JSONbored merged 1 commit into
mainfrom
fix/safe-url-reject-plaintext-ws
Jul 23, 2026
Merged

fix(review): reject plain ws: in isSafeEndpointUrl, matching its own "secure" doc comment#8131
JSONbored merged 1 commit into
mainfrom
fix/safe-url-reject-plaintext-ws

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • isSafeEndpointUrl's (src/review/content-lane/safe-url.ts + its @loopover/engine twin) doc comment always said it "additionally permits secure WebSocket endpoints (wss:, ws:)" — but ws: is the plaintext counterpart to wss:, the same relationship http:/https: has in this same module, where isSafeHttpUrl already rejects http: for exactly that reason.
  • Made the maintainer call this issue flagged as needed (tighten vs. document): tightening, not documenting an exception. The registry-logic.ts error message this guard backs ("Surface entry URL must be a public HTTPS or WSS endpoint") already only ever mentioned WSS, never WS — confirming ws: acceptance was an oversight in the original reviewbot port, not a deliberate carve-out for e.g. self-hosted operators pointing at a local unencrypted RPC node.
  • This is not itself an SSRF-bypass fix (the host/IP private-range guard already applied identically to ws:/wss:) — it's closing the transport-security/spec-consistency gap between what the doc comment claims and what the code allowed.
  • safe-url.ts is a genuine hand-duplicated twin pair (SAFE_URL_TWIN_PAIR in scripts/check-engine-parity.ts) — applied the identical change to both src/review/content-lane/safe-url.ts and packages/loopover-engine/src/review/safe-url.ts.

Closes #8017.

Test plan

  • Updated test/unit/content-lane-safe-url.test.ts and test/unit/safe-url-engine.test.ts (one per twin side): ws://... now asserts false alongside the existing http:/ftp: rejection cases; wss:// acceptance and its SSRF host-guard coverage are unchanged.
  • Verified the tests actually catch the regression: reverted both source fixes locally, confirmed both new assertions fail (ws://node.example.com still returned true), then restored the fix and confirmed green.
  • npm run build --workspace @loopover/engine
  • npx tsc --noEmit
  • Full affected-suite run (content-lane-safe-url, safe-url-engine, content-lane-registry-logic) — 199/199 passing
  • npm run test --workspace @loopover/engine (648/648 passing)
  • npm run engine-parity:drift-check — clean, both twin-pair sides still agree

…"secure" doc comment

isSafeEndpointUrl's doc comment always said it permits "secure WebSocket
endpoints (wss:, ws:)" -- but ws: is the plaintext counterpart to wss:,
the same relationship http:/https: has, where isSafeHttpUrl already
rejects http: for exactly that reason. The registry-logic.ts error
message this guards ("must be a public HTTPS or WSS endpoint") already
only ever mentioned WSS, confirming ws: support was an oversight, not
an intentional carve-out.

Applied identically to both sides of the safe-url.ts twin pair
(content-lane + @loopover/engine).

Closes #8017.
@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
❌ Deployment failed
View logs
loopover-ui a2dc21c Jul 22 2026, 11:55 PM

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@JSONbored JSONbored self-assigned this Jul 22, 2026
@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.02%. Comparing base (c02a277) to head (a2dc21c).
⚠️ Report is 3 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8131      +/-   ##
==========================================
- Coverage   92.02%   92.02%   -0.01%     
==========================================
  Files         760      760              
  Lines       77394    77394              
  Branches    23392    23392              
==========================================
- Hits        71225    71221       -4     
  Misses       5061     5061              
- Partials     1108     1112       +4     
Flag Coverage Δ
shard-1 57.10% <0.00%> (ø)
shard-2 54.33% <0.00%> (-0.01%) ⬇️
shard-3 50.28% <100.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/loopover-engine/src/review/safe-url.ts 100.00% <100.00%> (ø)
src/review/content-lane/safe-url.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@JSONbored
JSONbored merged commit 24929e3 into main Jul 23, 2026
14 of 15 checks passed
@JSONbored
JSONbored deleted the fix/safe-url-reject-plaintext-ws branch July 23, 2026 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

isSafeEndpointUrl permits plain unencrypted ws:, contradicting its own doc comment and this module's HTTPS-only posture

1 participant