Skip to content

security: relayUrl validation, remove dead pending code, fix about:blank#12

Merged
bazfer merged 5 commits into
masterfrom
security-review-fixes
May 22, 2026
Merged

security: relayUrl validation, remove dead pending code, fix about:blank#12
bazfer merged 5 commits into
masterfrom
security-review-fixes

Conversation

@DeetBot

@DeetBot DeetBot commented May 22, 2026

Copy link
Copy Markdown
Collaborator

Summary

Test plan

  • Set an http:// URL in extension options → save rejected with error message
  • Set wss://127.0.0.1:18792 → saves and connects normally
  • Target.createTarget with no URL → creates about:blank tab and attaches
  • Target.createTarget with javascript:alert(1) → throws "URL scheme not allowed"
  • No requestFromRelay or pending references remain

Luna added 2 commits May 22, 2026 16:26
- getRelayUrl(): validate wss:// scheme — invalid storage value falls back
  to DEFAULT_RELAY_URL with console.warn; prevents token exfil to attacker
  relay if chrome.storage is compromised
- options.js save(): reject non-wss:// URLs before writing to storage
- Remove requestFromRelay, pending map, and all usage sites — never called;
  dead code introduced by PR #11 adding a timeout to an uncalled function
- Target.createTarget: add about: to allowed URL schemes alongside http/https
  fixes regression from PR #10 where about:blank (DevTools default) was blocked

@DeetGates DeetGates left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Architect/security review: requesting changes.

CI: lint is green. I checked PR review comments, including chatgpt-codex-connector[bot]; there are no inline review comments on this PR.

Blocker: invalid stored relayUrl should fail closed/loud instead of silently falling back to DEFAULT_RELAY_URL. The options page now rejects bad saves, which is good, but getRelayUrl() handles corrupted/externally-written storage by warning in the console and connecting to the default public relay. That can violate operator intent and can leak the saved bearer token to a relay the operator did not configure. For a security fix, prefer: clear connection state/badge, surface an actionable error, and do not connect until the operator fixes the URL. Falling back is OK only for an empty/unset value.

SECURITY.md is directionally accurate about the extension trusting the relay, but it currently mixes a loopback trust model (Relay server (127.0.0.1:18792), "v1 single-tenant with a loopback relay") with the extension default of wss://relay.artificeia.mx. Before multi-tenant/public relay, it should explicitly separate local plugin relay vs hosted relay assumptions and say that any relay the extension connects to is trusted with full attached-tab CDP authority unless/until the extension has its own CDP allowlist.

Also consider documenting token scope/handling in SECURITY.md: wss:// protects against plaintext token exposure in transit, but it does not make a malicious or unintended relay safe.

@DeetGates DeetGates left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review: requesting changes.

CI: lint is green. Local gate: node --check background.js and node --check options.js pass. I rechecked inline PR comments, including chatgpt-codex-connector[bot]; none found.

The previous relayUrl fallback finding is fixed: invalid stored URLs now throw instead of silently connecting to the default relay, so no unintended token leak there.

Remaining blocker: SECURITY.md still misdescribes the local relay perimeter. It documents Relay server (127.0.0.1:18792) → wss:// token-authenticated and says the token in transit never leaves the machine, but the plugin relay is a plain ws://127.0.0.1:18792 WebSocket server with no TLS (the plugin README still tells users to configure ws://127.0.0.1:18792). Meanwhile this extension now rejects every non-wss: URL, so either local plugin relay is no longer supported directly, or loopback ws://127.0.0.1 needs an explicit exception. The security doc needs to be internally consistent and consistent with the actual extension behavior before approval.

Suggested fix: choose and document one model clearly:

  1. Hosted/TLS-only extension: say direct local ws:// plugin relay is unsupported unless fronted by TLS/reverse proxy, and update the local-relay diagram/docs accordingly; or
  2. Allow loopback ws://127.0.0.1/localhost only, while still rejecting non-loopback plaintext URLs.

The hosted/public relay trust model and extension-trusts-relay warning otherwise look directionally right.

@DeetGates DeetGates left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review: requesting changes.

CI: lint is green. Local gate: node --check background.js and node --check options.js pass. I rechecked inline PR comments, including chatgpt-codex-connector[bot]; none found.

The chosen policy is right: wss:// for remote, plaintext ws:// only for loopback. But two small consistency issues remain:

  1. IPv6 loopback is documented as allowed but currently rejected. In JS new URL('ws://[::1]:18792').hostname returns "[::1]", not "::1", so both background.js and options.js reject the documented ws://[::1] form. Fix by accepting [::1] too, or remove ::1 from SECURITY.md.

  2. SECURITY.md's local relay diagram still says ↓ wss:// token-authenticated for 127.0.0.1:18792, but the local plugin relay is plaintext ws://. That was the prior blocker. The new validation section is correct, but the local diagram must say ws:// token-authenticated (or explicitly note plaintext loopback exception) so the document is internally consistent.

Once those two are fixed, I expect this PR to be approvable.

@DeetGates DeetGates left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review: approved.

CI: lint is green. Local gates:

  • node --check background.js
  • node --check options.js
  • URL policy smoke: wss:// remote allowed; ws://127.0.0.1, ws://localhost, and ws://[::1] allowed; non-loopback ws:// and http:// rejected.

I rechecked inline PR comments, including chatgpt-codex-connector[bot]; none found.

The previous blockers are resolved: IPv6 loopback now matches JS URL hostname behavior with [::1], and SECURITY.md now labels the local relay path as ws:// plaintext loopback while keeping hosted relay as wss://. Good to merge from my side.

@bazfer bazfer merged commit e26cbc9 into master May 22, 2026
1 check passed
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.

3 participants