feat: audience-scoped asset and filesystem guard (dseg slice 8)#208
feat: audience-scoped asset and filesystem guard (dseg slice 8)#208wizzoapp[bot] wants to merge 7 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57967c9b7c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 180d36dadc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59f54c34ab
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e18f301a5f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const key = JSON.stringify([surfaceBaseUrl, asset.surfaceCredential, asset.relativeUrl]); | ||
| const existing = surfaceBindingPromises.get(key); | ||
| if (existing !== undefined) return existing; | ||
| const binding = bindAssetSurface(surfaceBaseUrl, asset); |
There was a problem hiding this comment.
Fresh evidence after the earlier asset-auth thread: this still binds every private asset against the current app origin, but secondary bearer/SSH connections keep their own authorized.httpBaseUrl (packages/client-runtime/src/connection/resolver.ts:238-244 and 342-347), while desktop t3code://app requests are proxied to a single renderer target (apps/desktop/src/app/DesktopApp.ts:177-183, apps/desktop/src/electron/ElectronProtocol.ts:117). For a private asset minted by a non-primary environment, the POST to /api/assets/relay/surface therefore reaches the wrong backend/signing key and returns 404, so attachments/favicons/file previews still fail for those token-only environments unless the relay is environment-aware.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted as a real in-diff functional regression — with a correction to the mechanism, because the fix does not live where this finding points.
Verified independently against the code. The conclusion (private assets 404 for non-primary environments) is right. The causal path is different, and slightly worse than described:
The bind POST does not go to authorized.httpBaseUrl. It goes to the window.location origin — and so does the asset GET itself. In apps/web/src/assets/assetUrls.ts:27-45, resolveBoundAssetUrl takes the public branch to httpBaseUrl (the environment origin) but the private branch to surfaceBaseUrl (the app origin). bindAssetSurface's first parameter is named httpBaseUrl but receives surfaceBaseUrl, and it is used for both the POST target and the returned asset URL (packages/client-runtime/src/state/assets.ts:59 and :71). So for private assets both requests are retargeted to the primary origin, whose per-server random signing secret (apps/server/src/assets/AssetAccess.ts:647-648) rejects a credential minted by the secondary server → masked 404.
connection/resolver.ts is not the faulty code and is not modified by this PR. The per-connection httpBaseUrl divergence at :237-245 / :341-349 is pre-existing and correct. What is new here is the private-asset surface binding, which is what makes that divergence newly consequential. A fix aimed at resolver.ts would be aimed at the wrong file.
Reachability confirmed, not vestigial: multi-environment is first-class (Sidebar.tsx:477 branches on thread.environmentId !== primaryEnvironmentId; ConnectionsSettings.tsx:1732 exposes remote/ssh backends), and asset resolution is per-environment (ProjectFavicon.tsx:14, ChatView.tsx:2437). ASSET_SAME_ORIGIN_RELAY_V1_CAPABILITY is requested unconditionally, so secondary servers will mint surface-credentialed assets this client structurally cannot bind. There is also a secondary cost: assetUrls.ts:88-100 retries on failure with backoff capped at 30s, so each broken asset leaves a perpetual background POST.
Severity: P1 on functional-regression grounds, refuted as a security issue. The failure is fail-closed at both hops — the relay credential and the asset token are each rejected on signature by the wrong server. Nothing private is served to the wrong surface; the asset simply never renders.
Note for the fix: apps/mobile/src/state/assets.ts:38,50-53 already does this correctly, using the connection's own base URL for both the URI and the binding and carrying the credential in an x-t3-asset-surface header. That approach transfers only to programmatically-fetched assets, not to DOM-loaded ones (img src, webview navigation), which is why the cookie relay exists — so the correct shape is conditional: bind against the app origin only when it proxies to the same backend as httpBaseUrl, and use the environment origin otherwise.
Not fixed in this PR. This is being tracked with the sequencing decision on the PR itself, which is at 12 review rounds; see the maintainer comment on the PR.
| } | ||
| const assetUrl = resolveAssetUrl(input.httpBaseUrl, assetResult.value.relativeUrl); | ||
| const surfaceBaseUrl = resolveBrowserAssetSurfaceBaseUrl(window.location.href); | ||
| const assetUrl = await resolveBoundAssetUrl(input.httpBaseUrl, surfaceBaseUrl, assetResult.value); |
There was a problem hiding this comment.
On desktop, openFileInPreview binds the private asset by fetching from the renderer window before handing only the resulting URL to the integrated browser, but the preview webview is created with a separate per-environment persist:t3code-preview-* partition (apps/desktop/src/ipc/methods/preview.ts:186-190, apps/desktop/src/preview/BrowserSession.ts:96-110) and only those partitions may attach as webviews (apps/desktop/src/window/DesktopWindow.ts:410-414). The surface cookie installed by this call is therefore not present when the preview webview navigates to the asset URL, so private HTML/PDF file previews in desktop still load as masked 404 unless the credential is bound inside the preview partition or passed through that navigation path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Triaged as a deliberate deferral, not an unaddressed defect — filed as ADA-177 (High).
The finding is accurate about the mechanism: the surface credential installed from the renderer is not present in the persist:t3code-preview-* partition when the preview webview navigates, so private HTML/PDF previews load as a masked 404 on desktop.
Two reasons this ships as-is rather than growing this PR:
-
The failure is fail-closed, not fail-open. The preview receives the masking 404. No private content is served to an unauthorized partition and no unbound asset URL is minted on that path. This is degraded functionality on one surface, not a hole in the security property the PR exists to establish.
-
This surface was explicitly bounded before the round, not discovered by it. The deferral was ruled on 2026-07-19 and shipped with two artifacts, both present in this diff: a characterization test in
server.test.tspinning "preview partition lacking the relay cookie is masked 404, never an unbound private asset", and a "Deliberately deferred" section in the PR body naming this exact surface. The test is the load-bearing part — it stops a later refactor from "fixing" the degradation into a bypass.
This PR has absorbed a new integration surface on each of its review rounds (cookies, then Safari third-party blocking, then Electron partitions, then version skew). Fail-closed-and-characterized is a legitimate terminal state for a surface: it ships the security property and pins the degradation, which is what the follow-up slice starts from.
Summary:
same-origin-relay-v1on asset URL requests. New servers return a typedAssetClientUpgradeRequiredErrorto old clients instead of issuing an unbound private URL, while new clients still decode old-server results with no surface credential.Test evidence:
vp check(0 errors; one unrelated pre-existing sidebar warning)vp run typecheck(15/15 packages)vp test run(6,129 passed, 7 skipped; 682 passed files, 2 skipped files)vp run lint:mobile(passed; optional host Swift/Kotlin linters unavailable)Design doc: /home/adam/.openclaw/reports/data-segregation-design-2026-07-17.md (§4, §6 slice 8).
Sequencing note: this slice deliberately guards the filesystem/asset/VCS read handlers WITHOUT changing external reachability. The factory audience-ceiling allowlist still excludes these methods, and a regression test asserts they remain unreachable by factory
orchestration:readsessions. Making them reachable (with audience-scoped positive/negative tests and per-method disclosure) is the dedicated slice 9 follow-up, per the approved design ruling of 2026-07-18 (guarding handlers and changing reachability are different risk classes and are not bundled in one diff).Deliberately deferred
The Electron
persist:t3code-preview-*partition bind bridge is deliberately deferred to a follow-up slice. Today the app renderer's relay cookie is not present in that preview partition's separate cookie jar, so a private desktop preview load fails closed with a masking 404 and never receives an unbound private asset URL.The characterization test
characterizes desktop preview partition loads without a relay cookie as masked 404pins that fail-closed behavior. This PR does not add a desktop IPC bind operation.