Use URL parsing and opener isolation for prompter popout windows

[Brad-Edwards]

Review finding

The prompter popout helper builds URLs with string replacement and opens a _blank window without explicit opener isolation.

Evidence

• src/system/presenter/prompter-window.ts:15 uses baseUrl.includes('mode=') and baseUrl.replace(/mode=[^&]*/, 'mode=prompter') instead of URL parsing.
• src/system/presenter/prompter-window.ts:17 appends &mode=prompter to any string containing ?, which places the mode parameter after #fragment for hash URLs.
• src/system/presenter/prompter-window.ts:32 calls window.open(url, '_blank', features) with default features that omit noopener,noreferrer.

Impact

URLs with query values that contain mode= can be corrupted, and hash URLs may not actually enter prompter mode. If an external URL ever flows through this exported helper, the opened page can retain window.opener access unless the browser applies stronger defaults.

Recommended fix

Use the platform URL API and explicit opener isolation.

Acceptance checks

• Build the prompter URL with new URL(baseUrl, window.location.href) and searchParams.set('mode', 'prompter').
• Preserve hashes and unrelated query parameters.
• Restrict or normalize to same-origin unless a future requirement explicitly allows external prompter targets.
• Add noopener,noreferrer to the feature string and defensively null opened.opener where possible.
• Tests cover existing mode, a query value containing mode=, hash URLs, relative URLs, and absolute same-origin URLs.

[Brad-Edwards]

🛠️ Picked up by /implement — driver Codex, branch 150-prompter-popout-url, 2026-05-24T03:19:49Z.

[Brad-Edwards]

gc workflow phase recorded: preflight (issue #150). Posted by the MCP server to enforce ordering between workflow steps (issue #794 MVP-2). Do not edit or delete — used by downstream tools to gate phase prerequisites.

[Brad-Edwards]

gc workflow phase recorded: plan (issue #150). Posted by the MCP server to enforce ordering between workflow steps (issue #794 MVP-2). Do not edit or delete — used by downstream tools to gate phase prerequisites.

Implementation Plan

Issue #150 is a requirement-free bug fix. No Ground Control requirement clauses are in scope, so the acceptance contract is the issue body.

• Modify src/system/presenter/prompter-window.ts only at the prompter popout helper boundary. Replace manual query-string rewriting with new URL(baseUrl, window.location.href) and url.searchParams.set('mode', 'prompter'), then open url.href.
• Enforce the same-origin boundary before opening. If the parsed target origin differs from window.location.origin, return null and do not call window.open; external prompter targets remain a non-goal.
• Harden popup isolation by always adding noopener,noreferrer to the feature string, while preserving caller-supplied window features, and defensively clearing opened.opener when a window object is returned.
• Extend tests/system/prompter-window-url.test.ts first to cover existing mode, query values containing mode=, hash preservation, relative URLs, absolute same-origin URLs, cross-origin rejection, feature isolation flags, and defensive opener clearing.
• Add changelog.d/150.fixed.md. Do not edit CHANGELOG.md directly.

Security: the change touches browser URL parsing, same-origin gating, and the popup opener boundary only; it introduces no storage, config, secrets, network policy, or error envelope surface. Maintainability: keep the behavior in the existing helper and tests, with browser URL/URLSearchParams as the parser. Extensibility: future external targets would need an explicit allowed-origin policy instead of ad hoc string exceptions. Whole-repo view: no ADR, schema, navigation grammar, runtime prompter semantics, or presenter routing changes are needed.

[Brad-Edwards]

gc_codex_review — cycle 1 of 1 (pre-push) on issue #150 (branch 150-prompter-popout-url)

Core review

Verdict: ship

This change is shaped correctly: the URL construction moves from ad hoc query-string rewriting to the platform URL API, and the popup isolation concern is kept local to the prompter-window boundary where window.open is actually invoked. The same-origin gate is the right seam for this helper because the prompter is a same-app presenter mode, and the tests now pin the behavioral edge cases that motivated the refactor: embedded mode= text, hashes, relative resolution, absolute same-origin URLs, cross-origin rejection, and opener isolation. It does not introduce a premature abstraction or foreclose the obvious next variation, such as adding more prompter-specific query params.

No blocking findings.

Security review

Verdict: ship

This change is shaped correctly as boundary hardening at the prompter-window seam: URL parsing, same-origin enforcement, query mutation, and popup opener isolation all live at the single point where presenter code crosses into window.open. The cross-cutting concern is browser trust boundary handling, and the diff tightens the previous string-based URL rewrite without broadening the caller surface or foreclosing the obvious next variation of adding more prompter query parameters. I do not see a concrete exploitable security regression introduced by this diff.

No blocking findings.

[Brad-Edwards]

gc_codex_review pre-push cycle 1 of 1 complete for issue #150 on branch '150-prompter-popout-url'. Posted by the MCP server to enforce the pre-push hard-cap-1 contract (issues #796, #804, #906). Do not edit or delete — used by the next gc_codex_review (uncommitted) invocation to count cycles.

[Brad-Edwards]

Review decision record — codex cycle 1 (issue #150)

Reviewer: codex
Cycle: 1

Architectural read:

Core reviewer: This change is shaped correctly: the URL construction moves from ad hoc query-string rewriting to the platform URL API, and the popup isolation concern is kept local to the prompter-window boundary where window.open is actually invoked. The same-origin gate is the right seam for this helper because the prompter is a same-app presenter mode, and the tests now pin the behavioral edge cases that motivated the refactor: embedded mode= text, hashes, relative resolution, absolute same-origin URLs, cross-origin rejection, and opener isolation. It does not introduce a premature abstraction or foreclose the obvious next variation, such as adding more prompter-specific query params.

Security reviewer: This change is shaped correctly as boundary hardening at the prompter-window seam: URL parsing, same-origin enforcement, query mutation, and popup opener isolation all live at the single point where presenter code crosses into window.open. The cross-cutting concern is browser trust boundary handling, and the diff tightens the previous string-based URL rewrite without broadening the caller surface or foreclosing the obvious next variation of adding more prompter query parameters. I do not see a concrete exploitable security regression introduced by this diff.

Blocking findings: 0 (clean run)

[Brad-Edwards]

gc_test_quality_review cycle 1 of 1 — issue #150

Reviewer: test-quality (claude-sonnet-4-6 via gc_test_quality_review)
Branch: 150-prompter-popout-url
Cycle: 1 / 1
Findings: 0 (clean run)

[Brad-Edwards]

gc_test_quality_review cycle 1 of 1 complete for issue #150 on branch '150-prompter-popout-url'. Posted by the MCP server to enforce the gc_test_quality_review hard-cap-1 contract (issue #884 follow-up, default lowered in #906). Do not edit or delete — used by the next gc_test_quality_review invocation to count cycles.

[Brad-Edwards]

Review decision record — test-quality cycle 1 (issue #150)

Reviewer: test-quality
Cycle: 1

Architectural read:

The tests exercise openPrompterWindow end-to-end through a minimal fake window global injected via Object.defineProperty — the right approach for browser-API-dependent code under Vitest/Node. Every significant code path in prompter-window.ts is covered: URL normalization with URLSearchParams.set (including the edge case where mode= appears inside a query value), relative-URL resolution against window.location.href, the cross-origin security guard, hash preservation, feature-string deduplication, opener nullification, and the _blank target. Assertions are on concrete outputs — the URL string passed to window.open, the return value of openPrompterWindow, and the mutated state of openedWindow.opener — not on mock call counts. Removing or no-op'ing any of the three internal helpers (buildPrompterUrl, withPopupIsolationFeatures, isolateOpenedWindow) would cause at least one test to fail. This is shaped correctly; I would ship it.

Blocking findings: 0 (clean run)