feat(ambient-ui): live preview with visual feedback and BFF proxy#1618
Merged
Conversation
Add the live preview feature vertical slice enabling "use the tool to build the tool" — users can preview deployed apps inside the session detail view, leave visual feedback on specific elements/regions, and send batched feedback to the agent as session messages. Core features: - Annotation registry: 19 registered annotation keys with categories, labels, and icons matching the spec's annotation system - Live preview overlay: sandboxed iframe with device size toggles (desktop/tablet/mobile), host allowlist validation, and CSP frame-src - BFF preview proxy: reverse-proxies preview URLs through the BFF, strips X-Frame-Options/CSP frame-ancestors, injects navigation interceptor for OAuth redirect chain handling, forwards SSO tokens - Visual feedback mode: crosshair selection, element/region capture, comment cards, right-side batch panel with inline send confirmation - PostMessage bridge: preview-bridge.js for cross-origin HTML capture with hover highlighting, graceful fallback for unbridged previews - Session messages port/adapter: direct-fetch adapter bypassing broken SDK SessionMessageAPI, with payload sanitization for prompt injection - Domain-oriented observability: feedbackSent/feedbackDeliveryFailed probes, session event type constants Security hardening from review: - Sandbox policy: allow-scripts allow-same-origin allow-forms (no top-navigation, no popups) - referrerPolicy="no-referrer" on preview iframe - maxLength=2000 on feedback comment textarea - Backtick/newline sanitization in feedback payloads - 10MB response size limit on preview proxy - Manual redirect handling to prevent off-allowlist redirect chains 167 tests across 13 test files, 0 type errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ambient-ui was missing from components-build-deploy.yml, so the image was never built/pushed to quay.io. Also switches oauth-proxy client-id to ambient-frontend to reuse the existing OAuthClient on the cluster. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
Contributor
|
Warning Review limit reached
More reviews will be available in 2 minutes and 8 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the 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. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR adds a complete preview and feedback system to Ambient UI, enabling users to preview web content in a proxied iframe, capture element details or region annotations, and submit feedback as session messages. It includes domain models, URL allowlisting, a preview proxy route, cross-origin bridge utilities, UI components, state management, and deployment configuration. ChangesPreview & Feedback System
Possibly Related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 2 warnings)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Add backslash escaping before backtick escaping in sanitizeForPayload to prevent escape sequence bypass (CodeQL #214) - Add explicit protocol guard before fetch in preview proxy route to make SSRF validation visible to static analysis (CodeQL #213) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Comment on lines
+44
to
+52
| upstream = await fetch(targetUrl, { | ||
| headers: { | ||
| Authorization: `Bearer ${accessToken}`, | ||
| Accept: | ||
| "text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8", | ||
| }, | ||
| redirect: "manual", | ||
| signal: AbortSignal.timeout(15_000), | ||
| }) |
… SSRF Rebuild the fetch target from parsedUrl.origin + pathname + search so CodeQL's taint analysis sees a fresh URL not linked to raw user input. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Turbopack (Next.js 16 default) cannot resolve file: linked ambient-sdk dependency in the Docker build context. Use --webpack flag matching the dev script. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The package.json references ambient-sdk via file:../ambient-sdk/ts-sdk (monorepo-relative). In the Docker build context where WORKDIR is /app, this resolves to /ambient-sdk/ts-sdk which doesn't exist. Create a symlink /ambient-sdk -> /app/ambient-sdk in both deps and builder stages. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
components/ambient-ui/src/lib/preview-hosts.ts (1)
29-37: 💤 Low valueClarify host:port glob expectations; optionally cache regex compilation
matchesGlobPatternalready defines*as matching one or more chars, andlocalhost:*will still not match portless URLs likehttp://localhost/...because the pattern contains:andisAllowedPreviewHostonly falls back to hostname-only for patterns without:—so changing.+→.*wouldn’t address that; add a test or doc if portless should be allowed.new RegExp(...)is built on every request fromNEXT_PUBLIC_PREVIEW_ALLOWED_HOSTSpatterns; since the attacker controls the tested URL host but not the pattern, consider caching compiled regexes per pattern (and/or capping pattern complexity) to reduce worst-case matching cost.🤖 Prompt for 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. In `@components/ambient-ui/src/lib/preview-hosts.ts` around lines 29 - 37, matchesGlobPattern currently treats * as .+ and recompiles a RegExp on every call; update matchesGlobPattern to (a) use .* instead of .+ if you want star to allow zero-length matches (so patterns like "localhost:*" can match hostnames without a port when desired) or document/testing the current expectation, and (b) add a module-level cache (e.g., Map<string, RegExp>) to store compiled regexes by pattern and reuse them instead of calling new RegExp(...) on each invocation; also review isAllowedPreviewHost to ensure it explicitly tries hostname-only matching when patterns may include ':' (or add a clear comment/test to reflect the chosen behavior).components/ambient-ui/src/components/preview/feedback-overlay.tsx (1)
172-214: ⚡ Quick winThrottle hover work on
mousemove. When not dragging, every pointer move runselementFromPoint+getBoundingClientRect(same-origin viatryGetElementBounds) or fires arequestHoverpostMessage (cross-origin, Line 127) and callssetHoverHighlight, triggering a rerender per pixel. On complex previews this is janky and floods the bridge. Coalesce to one update per frame withrequestAnimationFrame(track a pending-frame ref and skip if one is queued).🤖 Prompt for 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. In `@components/ambient-ui/src/components/preview/feedback-overlay.tsx` around lines 172 - 214, The mousemove handler handleMouseMove currently runs expensive DOM checks and posts requestHover on every move; throttle non-drag hover work by using requestAnimationFrame: add a ref like pendingFrameRef to track a queued RAF, and in handleMouseMove when dragStart is falsy, if pendingFrameRef.current is set then return early; otherwise set pendingFrameRef.current = requestAnimationFrame(() => { const pos = getRelativePosition(e) // or capture last event coords; then call tryGetElementBounds(...) and finally call setHoverHighlight(...) or post the requestHover message; clear pendingFrameRef.current after executing }). Keep the immediate drag-path unchanged (when dragStart is truthy) so selection remains responsive and include references to handleMouseMove, getRelativePosition, tryGetElementBounds, and setHoverHighlight when implementing.
🤖 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 `@components/ambient-ui/public/preview-bridge.js`:
- Line 36: The code currently serializes el.className (which is an
SVGAnimatedString for SVG elements) and can send an object via postMessage;
update the object construction that sets className to instead detect and use a
real string: if el.className is a plain string use it, otherwise fallback to
el.getAttribute('class') and finally null if neither; reference the existing
expression using el.className in the preview-bridge.js object so you replace
that assignment with a safe string extraction using getAttribute.
- Around line 18-78: The three window 'message' event handlers (the
ambient-capture, ambient-hover and ambient-hover-clear listeners) currently
reply with e.source.postMessage(..., '*'), which leaks DOM data to any origin;
change those postMessage calls to use e.origin as the targetOrigin and validate
e.origin against an expected parent origin before replying (reject or no-op if
e.origin is missing or not allowed). Locate the postMessage calls in the
handlers for messages of type 'ambient-capture', 'ambient-hover', and
'ambient-hover-clear' and replace '*' with e.origin (and add an origin-check
gate at the top of each handler to whitelist/validate the parent origin).
In `@components/ambient-ui/src/app/api/preview-proxy/route.ts`:
- Around line 77-119: The HTML path currently calls await upstream.text() which
buffers the entire response and can OOM; the non-HTML path streams without any
byte limit. Replace both usages with a counting TransformStream that monitors
bytes against MAX_RESPONSE_BYTES: for HTML, pipe upstream.body through the
counting stream into a reader, accumulate chunks up to the cap (and abort/return
Response.json({ error: "Response too large" }, { status: 413 }) if exceeded)
before calling injectBaseTag(buildBaseHref(parsedUrl)); for non-HTML, pipe
upstream.body through the same counting TransformStream into the returned
Response readable side and abort the upstream/pipeline and log/return 413 when
the counter exceeds MAX_RESPONSE_BYTES. Keep existing header handling
(responseHeaders), reuse stripFrameBlockingHeaders, and preserve the existing
pipe error/AbortError checks when aborting.
- Around line 36-45: The current upstream fetch attaches the caller's
accessToken to every allowlisted host (see parsedUrl and accessToken), so change
the logic to only include the Authorization header when the target is a
same-issuer SSO target: call your existing auth helper (e.g., resolveAccessToken
/ sameIssuer / issuer check in "`@/lib/auth`") or obtain the trusted issuer origin
and compare it to parsedUrl.origin; if they match, include Authorization:
`Bearer ${accessToken}`, otherwise omit the header and proxy the request without
credentials. Ensure the check runs immediately before building the fetch headers
for upstream so only same-issuer requests receive the token.
In `@components/ambient-ui/src/components/preview/feedback-overlay.tsx`:
- Around line 136-159: The message handler in useEffect (handleHoverResponse)
does not validate MessageEvent.source, allowing spoofed messages to set hover
highlights; before treating e.data as HoverResponse call isHoverResponse, obtain
iframeRef.current and ensure iframe.contentWindow exists and that e.source ===
iframe.contentWindow (or e.source === iframe.contentWindow?.window) and only
then proceed to compute iframeRect/overlayRect and call setHoverHighlight; keep
existing checks for overlayRef and isHoverResponse but add this source guard to
prevent cross-frame spoofing.
In `@components/ambient-ui/src/components/preview/preview-overlay.tsx`:
- Around line 172-181: The current handleSendAll calls feedback.markAsSent
immediately after invoking onSendFeedback, causing items to move to sent even if
delivery fails; change handleSendAll to await the feedback mutation's success
before calling feedback.markAsSent (either call mutateAsync/await the Promise
returned by sendFeedback or have useSendFeedback invoke feedback.markAsSent in
its onSuccess and handle errors in onError), and ensure you only call
feedback.markAsSent when the mutation resolves successfully (referencing
handleSendAll, onSendFeedback, feedback.markAsSent, and the sendFeedback
mutation).
- Around line 282-290: The iframe sandbox currently allows same-origin scripts
which lets proxied pages run with the app origin; remove "allow-same-origin"
from the SANDBOX_POLICY used in preview-overlay.tsx (the constant referenced by
the iframe props) so the iframe remains origin-isolated, and instead implement
explicit communication via the existing bridge; additionally, harden the
preview-proxy server (preview-proxy/route.ts and src/lib/preview-proxy.ts) to
serve a restrictive Content-Security-Policy (and ideally serve from an isolated
subdomain) that prevents execution of inline or remote scripts as
defense-in-depth.
In `@components/ambient-ui/src/hooks/use-feedback.ts`:
- Around line 46-51: The markAsSent updater currently calls setSentItems inside
setPendingItems, which can double-apply in StrictMode; replace the nested setter
with two independent updates: have markAsSent depend on the pendingItems state,
call setSentItems(prev => [...prev, ...pendingItems]) and then call
setPendingItems([]) (or otherwise clear pendingItems) so both updates are
computed independently; adjust the markAsSent useCallback dependency list to
include pendingItems and remove the nested setSentItems call inside the
setPendingItems updater.
In `@components/ambient-ui/src/lib/preview-hosts.ts`:
- Around line 68-77: The allowlist check in the return block rejects hostnames
without an explicit port because it skips the hostname-only match whenever the
pattern contains ':'; update the logic so that if a pattern includes a port and
that port is a wildcard (e.g., after splitting pattern by ':' the port segment
=== '*'), you also call matchesGlobPattern(hostnameOnly, patternWithoutPort) or
otherwise attempt matchesGlobPattern(hostnameOnly, pattern) so that "localhost"
(no port) matches "localhost:*". Modify the anonymous predicate used in
patterns.some(...) to detect the wildcard-port case and fall back to matching
hostnameOnly when the port part is '*' while keeping existing behavior for other
patterns.
In `@components/manifests/overlays/production/ambient-ui-oauth-patch.yaml`:
- Around line 31-32: The manifest sets --client-id=ambient-frontend and mounts
the ambient-ui-oauth-config Secret for client-secret/cookie_secret, but there is
no OAuthClient in this repo to verify redirectURIs or the registered client
secret; confirm the cluster's OAuthClient named "ambient-frontend" exists and
its clientSecret and redirectURIs match the mounted secret, or make them
consistent by either updating --client-id to the actual registered OAuthClient
name or updating the mounted Secret (ambient-ui-oauth-config) to hold the
registered client secret and cookie_secret; optionally add a kind: OAuthClient
manifest for "ambient-frontend" with the correct redirectURIs to this repo or
document the required redirectURIs so deployment will not fail.
---
Nitpick comments:
In `@components/ambient-ui/src/components/preview/feedback-overlay.tsx`:
- Around line 172-214: The mousemove handler handleMouseMove currently runs
expensive DOM checks and posts requestHover on every move; throttle non-drag
hover work by using requestAnimationFrame: add a ref like pendingFrameRef to
track a queued RAF, and in handleMouseMove when dragStart is falsy, if
pendingFrameRef.current is set then return early; otherwise set
pendingFrameRef.current = requestAnimationFrame(() => { const pos =
getRelativePosition(e) // or capture last event coords; then call
tryGetElementBounds(...) and finally call setHoverHighlight(...) or post the
requestHover message; clear pendingFrameRef.current after executing }). Keep the
immediate drag-path unchanged (when dragStart is truthy) so selection remains
responsive and include references to handleMouseMove, getRelativePosition,
tryGetElementBounds, and setHoverHighlight when implementing.
In `@components/ambient-ui/src/lib/preview-hosts.ts`:
- Around line 29-37: matchesGlobPattern currently treats * as .+ and recompiles
a RegExp on every call; update matchesGlobPattern to (a) use .* instead of .+ if
you want star to allow zero-length matches (so patterns like "localhost:*" can
match hostnames without a port when desired) or document/testing the current
expectation, and (b) add a module-level cache (e.g., Map<string, RegExp>) to
store compiled regexes by pattern and reuse them instead of calling new
RegExp(...) on each invocation; also review isAllowedPreviewHost to ensure it
explicitly tries hostname-only matching when patterns may include ':' (or add a
clear comment/test to reflect the chosen behavior).
🪄 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: CHILL
Plan: Enterprise
Run ID: 8b7ba04a-9de0-48f9-b506-6d1901369f9f
📒 Files selected for processing (34)
.github/workflows/components-build-deploy.ymlcomponents/ambient-ui/next.config.jscomponents/ambient-ui/public/preview-bridge.jscomponents/ambient-ui/src/adapters/__tests__/session-messages.test.tscomponents/ambient-ui/src/adapters/index.tscomponents/ambient-ui/src/adapters/mappers.tscomponents/ambient-ui/src/adapters/session-messages.tscomponents/ambient-ui/src/app/(dashboard)/[projectId]/fleet/[sessionId]/_components/session-header.tsxcomponents/ambient-ui/src/app/api/ambient/v1/[...path]/route.tscomponents/ambient-ui/src/app/api/preview-proxy/route.tscomponents/ambient-ui/src/components/preview/comment-card.tsxcomponents/ambient-ui/src/components/preview/feedback-overlay.tsxcomponents/ambient-ui/src/components/preview/feedback-panel.tsxcomponents/ambient-ui/src/components/preview/preview-overlay.tsxcomponents/ambient-ui/src/components/ui/textarea.tsxcomponents/ambient-ui/src/domain/__tests__/annotations.test.tscomponents/ambient-ui/src/domain/annotations.tscomponents/ambient-ui/src/domain/events.tscomponents/ambient-ui/src/domain/types.tscomponents/ambient-ui/src/hooks/__tests__/use-feedback.test.tscomponents/ambient-ui/src/hooks/use-feedback.tscomponents/ambient-ui/src/lib/__tests__/preview-bridge.test.tscomponents/ambient-ui/src/lib/__tests__/preview-hosts.test.tscomponents/ambient-ui/src/lib/__tests__/preview-proxy.test.tscomponents/ambient-ui/src/lib/observability.tscomponents/ambient-ui/src/lib/preview-bridge.tscomponents/ambient-ui/src/lib/preview-hosts.tscomponents/ambient-ui/src/lib/preview-proxy.tscomponents/ambient-ui/src/ports/index.tscomponents/ambient-ui/src/ports/session-messages.tscomponents/ambient-ui/src/queries/__tests__/use-send-feedback.test.tscomponents/ambient-ui/src/queries/index.tscomponents/ambient-ui/src/queries/use-send-feedback.tscomponents/manifests/overlays/production/ambient-ui-oauth-patch.yaml
Comment on lines
+36
to
+45
| try { | ||
| upstream = await fetch(parsedUrl.toString(), { | ||
| headers: { | ||
| Authorization: `Bearer ${accessToken}`, | ||
| Accept: | ||
| "text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8", | ||
| }, | ||
| redirect: "manual", | ||
| signal: AbortSignal.timeout(15_000), | ||
| }) |
Contributor
There was a problem hiding this comment.
User access token forwarded to every allowlisted preview host.
accessToken is the caller's bearer credential, but it's attached to the upstream fetch for any URL that merely passes the host allowlist. The PR description states tokens should only be forwarded to same-issuer SSO targets — that gate is missing here, so a third-party (or compromised) allowlisted host receives the user's token.
Restrict forwarding to same-issuer targets, e.g. only attach Authorization when parsedUrl.origin matches the trusted SSO/issuer origin; otherwise proxy without the credential.
Confirm whether a same-issuer check exists in @/lib/auth or upstream:
#!/bin/bash
rg -nP -C3 '\b(resolveAccessToken|issuer|sameIssuer|same_issuer)\b' --type=ts -g '!**/__tests__/**'🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 37-45: Server-side request forgery
The URL of this request depends on a user-provided value.
🤖 Prompt for 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.
In `@components/ambient-ui/src/app/api/preview-proxy/route.ts` around lines 36 -
45, The current upstream fetch attaches the caller's accessToken to every
allowlisted host (see parsedUrl and accessToken), so change the logic to only
include the Authorization header when the target is a same-issuer SSO target:
call your existing auth helper (e.g., resolveAccessToken / sameIssuer / issuer
check in "`@/lib/auth`") or obtain the trusted issuer origin and compare it to
parsedUrl.origin; if they match, include Authorization: `Bearer ${accessToken}`,
otherwise omit the header and proxy the request without credentials. Ensure the
check runs immediately before building the fetch headers for upstream so only
same-issuer requests receive the token.
Comment on lines
+77
to
+119
| // Check Content-Length if provided | ||
| const contentLength = upstream.headers.get("content-length") | ||
| if (contentLength && Number(contentLength) > MAX_RESPONSE_BYTES) { | ||
| return Response.json({ error: "Response too large" }, { status: 413 }) | ||
| } | ||
|
|
||
| const cleaned = stripFrameBlockingHeaders(upstream.headers) | ||
|
|
||
| const responseHeaders = new Headers(cleaned) | ||
| responseHeaders.set("Cache-Control", "no-store") | ||
| responseHeaders.set("X-Content-Type-Options", "nosniff") | ||
|
|
||
| const contentType = upstream.headers.get("content-type") ?? "" | ||
|
|
||
| if (contentType.includes("text/html")) { | ||
| const html = await upstream.text() | ||
| if (html.length > MAX_RESPONSE_BYTES) { | ||
| return Response.json({ error: "Response too large" }, { status: 413 }) | ||
| } | ||
| const modified = injectBaseTag(html, buildBaseHref(parsedUrl)) | ||
| return new Response(modified, { | ||
| status: 200, | ||
| headers: responseHeaders, | ||
| }) | ||
| } | ||
|
|
||
| // Non-HTML: stream body through | ||
| if (upstream.body) { | ||
| const { readable, writable } = new TransformStream() | ||
| upstream.body.pipeTo(writable).catch((err: unknown) => { | ||
| if ( | ||
| err instanceof Error && | ||
| err.name !== "AbortError" && | ||
| !err.message?.includes("ResponseAborted") | ||
| ) { | ||
| console.error("[Preview proxy] pipe error:", err) | ||
| } | ||
| }) | ||
| return new Response(readable, { | ||
| status: upstream.status, | ||
| headers: responseHeaders, | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
10MB limit is bypassable; HTML path buffers unbounded before checking.
Two gaps in size enforcement:
- L92-95:
await upstream.text()fully buffers the body before thehtml.lengthcheck, so a target sending a huge body (or lying/omittingcontent-length) can OOM the route before the guard runs. - L104-119: the streaming (non-HTML) path enforces no byte cap at all — the
content-lengthheader check (L78-81) is trivially defeated by chunked transfer encoding.
Enforce the cap incrementally on both paths (count bytes through the TransformStream and abort past MAX_RESPONSE_BYTES; cap the HTML read similarly).
🤖 Prompt for 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.
In `@components/ambient-ui/src/app/api/preview-proxy/route.ts` around lines 77 -
119, The HTML path currently calls await upstream.text() which buffers the
entire response and can OOM; the non-HTML path streams without any byte limit.
Replace both usages with a counting TransformStream that monitors bytes against
MAX_RESPONSE_BYTES: for HTML, pipe upstream.body through the counting stream
into a reader, accumulate chunks up to the cap (and abort/return Response.json({
error: "Response too large" }, { status: 413 }) if exceeded) before calling
injectBaseTag(buildBaseHref(parsedUrl)); for non-HTML, pipe upstream.body
through the same counting TransformStream into the returned Response readable
side and abort the upstream/pipeline and log/return 413 when the counter exceeds
MAX_RESPONSE_BYTES. Keep existing header handling (responseHeaders), reuse
stripFrameBlockingHeaders, and preserve the existing pipe error/AbortError
checks when aborting.
Comment on lines
+136
to
+159
| useEffect(() => { | ||
| function handleHoverResponse(e: MessageEvent) { | ||
| if (!isHoverResponse(e.data)) return | ||
|
|
||
| const iframe = iframeRef.current | ||
| const overlay = overlayRef.current | ||
| if (!iframe || !overlay) return | ||
|
|
||
| const hoverData = e.data as HoverResponse | ||
| const iframeRect = iframe.getBoundingClientRect() | ||
| const overlayRect = overlay.getBoundingClientRect() | ||
|
|
||
| setHoverHighlight({ | ||
| kind: 'element', | ||
| left: Math.round(iframeRect.left - overlayRect.left + hoverData.rect.x), | ||
| top: Math.round(iframeRect.top - overlayRect.top + hoverData.rect.y), | ||
| width: Math.round(hoverData.rect.width), | ||
| height: Math.round(hoverData.rect.height), | ||
| }) | ||
| } | ||
|
|
||
| window.addEventListener('message', handleHoverResponse) | ||
| return () => window.removeEventListener('message', handleHoverResponse) | ||
| }, [iframeRef]) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="components/ambient-ui/src/components/preview/feedback-overlay.tsx"
# Basic info
echo "== File =="
wc -l "$FILE"
sed -n '1,220p' "$FILE"
sed -n '220,420p' "$FILE"Repository: ambient-code/platform
Length of output: 9981
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="components/ambient-ui/src/components/preview/feedback-overlay.tsx"
# Find isHoverResponse definition and all uses
rg -n "isHoverResponse|HoverResponse|addEventListener\\('message'|message\\s*:" "$FILE"
# Show the specific block around the effect lines even if line numbers shifted
python3 - <<'PY'
import itertools, re
path="components/ambient-ui/src/components/preview/feedback-overlay.tsx"
with open(path,'r',encoding='utf-8') as f:
lines=f.readlines()
for i in range(len(lines)):
if "useEffect(() => {" in lines[i] and "handleHoverResponse" in "".join(lines[i:i+80]):
start=max(0,i-10); end=min(len(lines), i+120)
print(f"== Approx effect block at line {i+1} ==")
for j in range(start,end):
print(f"{j+1:4d}:{lines[j].rstrip()}")
break
PYRepository: ambient-code/platform
Length of output: 4834
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="components/ambient-ui/src/lib/preview-bridge.ts"
if [ ! -f "$FILE" ]; then
# fallback: find actual location by filename
FOUND=$(fd -a "preview-bridge.*" . || true)
echo "$FOUND"
fi
fd -a "preview-bridge.*" . | head -n 20Repository: ambient-code/platform
Length of output: 280
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="components/ambient-ui/src/lib/preview-bridge.ts"
wc -l "$FILE"
sed -n '1,220p' "$FILE"
sed -n '220,520p' "$FILE"Repository: ambient-code/platform
Length of output: 3351
Validate the message source before updating hover highlights
window.addEventListener('message', ...) applies any ambient-hovered payload without checking MessageEvent.source, while preview-bridge.ts posts requests with '*'; another frame could spoof the hover highlight. Gate on the iframe’s contentWindow.
🛡️ Proposed guard
function handleHoverResponse(e: MessageEvent) {
if (!isHoverResponse(e.data)) return
+ if (e.source !== iframeRef.current?.contentWindow) return📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useEffect(() => { | |
| function handleHoverResponse(e: MessageEvent) { | |
| if (!isHoverResponse(e.data)) return | |
| const iframe = iframeRef.current | |
| const overlay = overlayRef.current | |
| if (!iframe || !overlay) return | |
| const hoverData = e.data as HoverResponse | |
| const iframeRect = iframe.getBoundingClientRect() | |
| const overlayRect = overlay.getBoundingClientRect() | |
| setHoverHighlight({ | |
| kind: 'element', | |
| left: Math.round(iframeRect.left - overlayRect.left + hoverData.rect.x), | |
| top: Math.round(iframeRect.top - overlayRect.top + hoverData.rect.y), | |
| width: Math.round(hoverData.rect.width), | |
| height: Math.round(hoverData.rect.height), | |
| }) | |
| } | |
| window.addEventListener('message', handleHoverResponse) | |
| return () => window.removeEventListener('message', handleHoverResponse) | |
| }, [iframeRef]) | |
| useEffect(() => { | |
| function handleHoverResponse(e: MessageEvent) { | |
| if (!isHoverResponse(e.data)) return | |
| if (e.source !== iframeRef.current?.contentWindow) return | |
| const iframe = iframeRef.current | |
| const overlay = overlayRef.current | |
| if (!iframe || !overlay) return | |
| const hoverData = e.data as HoverResponse | |
| const iframeRect = iframe.getBoundingClientRect() | |
| const overlayRect = overlay.getBoundingClientRect() | |
| setHoverHighlight({ | |
| kind: 'element', | |
| left: Math.round(iframeRect.left - overlayRect.left + hoverData.rect.x), | |
| top: Math.round(iframeRect.top - overlayRect.top + hoverData.rect.y), | |
| width: Math.round(hoverData.rect.width), | |
| height: Math.round(hoverData.rect.height), | |
| }) | |
| } | |
| window.addEventListener('message', handleHoverResponse) | |
| return () => window.removeEventListener('message', handleHoverResponse) | |
| }, [iframeRef]) |
🤖 Prompt for 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.
In `@components/ambient-ui/src/components/preview/feedback-overlay.tsx` around
lines 136 - 159, The message handler in useEffect (handleHoverResponse) does not
validate MessageEvent.source, allowing spoofed messages to set hover highlights;
before treating e.data as HoverResponse call isHoverResponse, obtain
iframeRef.current and ensure iframe.contentWindow exists and that e.source ===
iframe.contentWindow (or e.source === iframe.contentWindow?.window) and only
then proceed to compute iframeRect/overlayRect and call setHoverHighlight; keep
existing checks for overlayRef and isHoverResponse but add this source guard to
prevent cross-frame spoofing.
Comment on lines
+172
to
+181
| const handleSendAll = useCallback(() => { | ||
| if (onSendFeedback && feedback.pendingItems.length > 0) { | ||
| onSendFeedback({ | ||
| items: feedback.pendingItems, | ||
| sessionId, | ||
| previewUrl: url, | ||
| }) | ||
| feedback.markAsSent() | ||
| } | ||
| }, [onSendFeedback, feedback.pendingItems, feedback.markAsSent, sessionId, url]) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm markAsSent semantics and absence of rollback on failure
rg -nP -C6 '\bmarkAsSent\b' --type=tsRepository: ambient-code/platform
Length of output: 9287
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where onSendFeedback is wired up and how sendFeedback.mutate is called.
rg -n --hidden --no-ignore -S '\bonSendFeedback\b' components/ambient-ui/src --type=ts --type=tsx
rg -n --hidden --no-ignore -S '\bsendFeedback\b' components/ambient-ui/src --type=ts --type=tsx
# Locate session-header.tsx if present and inspect relevant parts.
ls -la components/ambient-ui/src/components/preview | head -n 50 || true
fd session-header components/ambient-ui/src --type=ts --type=tsx --type=js --type=jsx || true
rg -n --hidden --no-ignore -S 'session-header' components/ambient-ui/src --type=ts --type=tsx || trueRepository: ambient-code/platform
Length of output: 93
🏁 Script executed:
#!/bin/bash
set -euo pipefail
SEARCH_ROOT="components/ambient-ui/src"
# Find where onSendFeedback is wired up and how sendFeedback.mutate is called.
rg -nS '\bonSendFeedback\b' "$SEARCH_ROOT" -g'*.ts' -g'*.tsx' -g'*.js' -g'*.jsx'
rg -nS '\bsendFeedback\b' "$SEARCH_ROOT" -g'*.ts' -g'*.tsx' -g'*.js' -g'*.jsx'
# Find session-header usage/definition.
rg -nS 'session-header' "$SEARCH_ROOT" -g'*.ts' -g'*.tsx' -g'*.js' -g'*.jsx'
fd -t f 'session-header.*' components/ambient-ui/src || true
fd -t f '*session*header*' components/ambient-ui/src || true
# Locate the hook call site for sendFeedback.mutate (if named like this).
rg -nS 'mutate\s*\(\s*batch\s*\)|mutate\s*\(\s*['"$SEARCH_ROOT"']' "$SEARCH_ROOT" -g'*.ts' -g'*.tsx' || trueRepository: ambient-code/platform
Length of output: 2879
🏁 Script executed:
#!/bin/bash
set -euo pipefail
SRC="components/ambient-ui/src"
# Locate the implementation of useSendFeedback
fd -t f 'use-send-feedback*' "$SRC" || true
fd -t f '*send-feedback*' "$SRC" || true
rg -nS '\buseSendFeedback\b' "$SRC" -g'*.ts' -g'*.tsx'
# Show the relevant portions of session-header.tsx around onSendFeedback
sed -n '1,160p' components/ambient-ui/src/app/(dashboard)/[projectId]/fleet/[sessionId]/_components/session-header.tsx
# Show the relevant portions of preview-overlay.tsx around handleSendAll
sed -n '130,220p' components/ambient-ui/src/components/preview/preview-overlay.tsx
# Inspect the useSendFeedback hook file(s)
# (Use rg to find the mutate call and any onSuccess/onError handlers.)
rg -nS 'useSendFeedback|onSuccess|onError|useMutation|mutate\(' "$SRC" -g'*.ts' -g'*.tsx' -g'*.js' -g'*.jsx'Repository: ambient-code/platform
Length of output: 2335
🏁 Script executed:
#!/bin/bash
set -euo pipefail
SESSION_HEADER="components/ambient-ui/src/app/(dashboard)/[projectId]/fleet/[sessionId]/_components/session-header.tsx"
PREVIEW_OVERLAY="components/ambient-ui/src/components/preview/preview-overlay.tsx"
USE_SEND_FEEDBACK="components/ambient-ui/src/queries/use-send-feedback.ts"
echo "---- use-send-feedback.ts (lines 1-220) ----"
nl -ba "$USE_SEND_FEEDBACK" | sed -n '1,220p'
echo "---- session-header.tsx (lines 1-220) ----"
nl -ba "$SESSION_HEADER" | sed -n '1,220p'
echo "---- preview-overlay.tsx (lines 130-210) ----"
nl -ba "$PREVIEW_OVERLAY" | sed -n '130,210p'
echo "---- look for markAsSent usage in overlay ----"
rg -n "markAsSent" "$PREVIEW_OVERLAY" -S
echo "---- look for pending/sent state behavior in use-feedback ----"
nl -ba components/ambient-ui/src/hooks/use-feedback.ts | sed -n '1,130p'Repository: ambient-code/platform
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
SESSION_HEADER="components/ambient-ui/src/app/(dashboard)/[projectId]/fleet/[sessionId]/_components/session-header.tsx"
PREVIEW_OVERLAY="components/ambient-ui/src/components/preview/preview-overlay.tsx"
USE_SEND_FEEDBACK="components/ambient-ui/src/queries/use-send-feedback.ts"
echo "---- use-send-feedback.ts (1-240) ----"
cat -n "$USE_SEND_FEEDBACK" | sed -n '1,240p'
echo "---- session-header.tsx (1-180) ----"
cat -n "$SESSION_HEADER" | sed -n '1,180p'
echo "---- preview-overlay.tsx (130-220) ----"
cat -n "$PREVIEW_OVERLAY" | sed -n '130,220p'
echo "---- look for markAsSent usage in overlay ----"
rg -n "markAsSent" "$PREVIEW_OVERLAY" -S
echo "---- look for pending/sent behavior in use-feedback ----"
cat -n components/ambient-ui/src/hooks/use-feedback.ts | sed -n '1,120p'Repository: ambient-code/platform
Length of output: 13403
🏁 Script executed:
#!/bin/bash
set -euo pipefail
SRC="components/ambient-ui/src"
# Find domainProbe definition and feedbackDeliveryFailed implementation.
rg -nS '\bfeedbackDeliveryFailed\b' components/ambient-ui/src -g'*.ts' -g'*.tsx'
rg -nS '\bdomainProbe\b' components/ambient-ui/src -g'*.ts' -g'*.tsx'
# Also inspect observability lib path if present.
fd -t f 'observability*' components/ambient-ui/src/lib || true
fd -t f '*observability*' components/ambient-ui/src/lib || true
fd -t f 'observability*' components/ambient-ui/src -g'*.ts' -g'*.tsx' || true
# Print domainProbe file(s).
ls -R components/ambient-ui/src/lib | head -n 50 || trueRepository: ambient-code/platform
Length of output: 2577
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect observability implementation
cat -n components/ambient-ui/src/lib/observability.ts | sed -n '1,140p'
# Inspect more of preview-overlay around the send button/handlers (look for feedback delivery UI)
cat -n components/ambient-ui/src/components/preview/preview-overlay.tsx | sed -n '1,260p'
# Inspect use-send-feedback tests for onError expectations (UI/rollback vs probe-only)
cat -n components/ambient-ui/src/queries/__tests__/use-send-feedback.test.ts | sed -n '1,260p'Repository: ambient-code/platform
Length of output: 19404
Gate feedback.markAsSent() on successful feedback delivery
- In
components/ambient-ui/src/components/preview/preview-overlay.tsx(handleSendAll), we callonSendFeedback(...)and then immediatelyfeedback.markAsSent(). SessionHeaderwiresonSendFeedbackassendFeedback.mutate(batch)(fire-and-forget), anduseSendFeedbackonly logs viadomainProbe.feedbackDeliveryFailed(...)on error—there’s no rollback or user-visible error state. Result: failed sends can still move items frompendingItemstosentItems.
Tie the state transition to the mutation’s success (e.g., mutateAsync/await before markAsSent, or call markAsSent from useSendFeedback’s onSuccess and show an error on onError).
🤖 Prompt for 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.
In `@components/ambient-ui/src/components/preview/preview-overlay.tsx` around
lines 172 - 181, The current handleSendAll calls feedback.markAsSent immediately
after invoking onSendFeedback, causing items to move to sent even if delivery
fails; change handleSendAll to await the feedback mutation's success before
calling feedback.markAsSent (either call mutateAsync/await the Promise returned
by sendFeedback or have useSendFeedback invoke feedback.markAsSent in its
onSuccess and handle errors in onError), and ensure you only call
feedback.markAsSent when the mutation resolves successfully (referencing
handleSendAll, onSendFeedback, feedback.markAsSent, and the sendFeedback
mutation).
Comment on lines
+282
to
+290
| <iframe | ||
| ref={iframeRef} | ||
| src={buildProxyUrl(url)} | ||
| sandbox={SANDBOX_POLICY} | ||
| referrerPolicy="no-referrer" | ||
| title={displayTitle} | ||
| className="h-full w-full rounded border border-border bg-white" | ||
| onLoad={handleIframeLoad} | ||
| /> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect proxy route + CSP to confirm same-origin serving of external content
rg -nP -C3 'sandbox|allow-same-origin|frame-src|preview-proxy' --type=ts --type=jsRepository: ambient-code/platform
Length of output: 18638
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the iframe sandbox policy definition + usage
sed -n '1,120p' components/ambient-ui/src/components/preview/preview-overlay.tsx
sed -n '240,330p' components/ambient-ui/src/components/preview/preview-overlay.tsx
# Inspect what preview-proxy returns (sanitization / CSP / script handling)
sed -n '1,220p' components/ambient-ui/src/app/api/preview-proxy/route.ts
sed -n '1,220p' components/ambient-ui/src/lib/preview-proxy.ts
# Check if there is any per-proxied-document CSP that could mitigate script execution
rg -n "Content-Security-Policy|script-src|sandbox" components/ambient-ui/src/app/api/preview-proxy/route.ts components/ambient-ui/src/lib/preview-proxy.tsRepository: ambient-code/platform
Length of output: 16540
Fix iframe sandbox to prevent app-origin script execution for preview-proxy.
components/ambient-ui/src/components/preview/preview-overlay.tsx sets SANDBOX_POLICY = 'allow-scripts allow-same-origin allow-forms' on an iframe loading same-origin /api/preview-proxy?.... The proxy (components/ambient-ui/src/app/api/preview-proxy/route.ts + src/lib/preview-proxy.ts) only strips frame-blocking headers (X-Frame-Options / CSP frame-ancestors) and injects a <base> + navigation interceptor, but does not add a restrictive per-document CSP or otherwise neutralize scripts. With allow-same-origin, any scripts in the proxied HTML run with the parent app’s origin (cookies/localStorage and same-origin API access), turning an allowlisted preview into app-origin XSS.
Serve the proxy from an isolated origin/subdomain (so allow-same-origin can’t collapse isolation), and/or remove allow-same-origin from the iframe sandbox and use the bridge for required communication; add defense-in-depth via a restrictive CSP for proxied HTML.
🤖 Prompt for 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.
In `@components/ambient-ui/src/components/preview/preview-overlay.tsx` around
lines 282 - 290, The iframe sandbox currently allows same-origin scripts which
lets proxied pages run with the app origin; remove "allow-same-origin" from the
SANDBOX_POLICY used in preview-overlay.tsx (the constant referenced by the
iframe props) so the iframe remains origin-isolated, and instead implement
explicit communication via the existing bridge; additionally, harden the
preview-proxy server (preview-proxy/route.ts and src/lib/preview-proxy.ts) to
serve a restrictive Content-Security-Policy (and ideally serve from an isolated
subdomain) that prevents execution of inline or remote scripts as
defense-in-depth.
Comment on lines
+31
to
32
| - --client-id=ambient-frontend | ||
| - --client-secret-file=/etc/oauth/config/client-secret |
Contributor
There was a problem hiding this comment.
❓ Verification inconclusive
🏁 Script executed:
#!/bin/bash
# Look for OAuthClient definitions and the secret backing the proxy
rg -nP 'kind:\s*OAuthClient' -A8 -g '*.yaml' -g '*.yml'
echo "---- references to client ids / secret ----"
rg -nP 'ambient-frontend|ambient-ui-oauth-config|client-id=' -g '*.yaml' -g '*.yml'Repository: ambient-code/platform
Length of output: 1669
Confirm oauth-proxy client-id/secret + OAuthClient redirect wiring (ambient-ui-oauth-patch.yaml)
components/manifests/overlays/production/ambient-ui-oauth-patch.yaml sets --client-id=ambient-frontend while mounting the ambient-ui-oauth-config Secret for client-secret (and cookie_secret). Also, this repo contains no kind: OAuthClient manifests, so the presence/redirectURIs of an ambient-frontend OAuthClient can’t be checked here—auth will fail in prod if the mounted client-secret (and redirect URI) doesn’t match the cluster’s registered OAuthClient.
🤖 Prompt for 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.
In `@components/manifests/overlays/production/ambient-ui-oauth-patch.yaml` around
lines 31 - 32, The manifest sets --client-id=ambient-frontend and mounts the
ambient-ui-oauth-config Secret for client-secret/cookie_secret, but there is no
OAuthClient in this repo to verify redirectURIs or the registered client secret;
confirm the cluster's OAuthClient named "ambient-frontend" exists and its
clientSecret and redirectURIs match the mounted secret, or make them consistent
by either updating --client-id to the actual registered OAuthClient name or
updating the mounted Secret (ambient-ui-oauth-config) to hold the registered
client secret and cookie_secret; optionally add a kind: OAuthClient manifest for
"ambient-frontend" with the correct redirectURIs to this repo or document the
required redirectURIs so deployment will not fail.
CodeRabbit fixes:
- Scope postMessage replies to e.origin instead of '*' (security)
- Use el.getAttribute('class') for SVG className compatibility
- Fix impure markAsSent updater that risked duplicate sentItems
in React StrictMode
- Accept portless localhost URLs when allowlist has localhost:*
Docker build fix:
- Copy SDK to /ambient-sdk/ts-sdk (where file:../ambient-sdk/ts-sdk
resolves from WORKDIR /app) instead of symlinking
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The ambient-sdk ts-sdk dist/ is gitignored — CI has no pre-built output. Install SDK deps and run tsc in the deps stage so dist/ exists when npm ci resolves the file: dependency and webpack can find the entry point. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Merge Queue Status
This pull request spent 32 seconds in the queue, including 7 seconds running CI. Required conditions to merge |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
preview-bridge.jsfor cross-origin HTML capture with hover highlightingArchitecture
Port/adapter pattern: SessionMessagesPort + direct-fetch adapter (bypasses broken SDK SessionMessageAPI). Domain types only in components — no SDK type leakage.
167 tests across 13 test files. Fakes over mocks per workflow standards.
Security
allow-scripts allow-same-origin allow-forms(no top-navigation, no popups)referrerPolicy="no-referrer"on iframemaxLength=2000on comment textarearedirect: "manual") to prevent off-allowlist redirect chainsTest plan
ambient-code.io/ui/preview-urlannotationCor click "Comment" to enter feedback modecd components/ambient-ui && npx vitest run🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Configuration