feat(examples): add web-capture gateway#631
Conversation
A thin Go gateway, mirroring examples/speech-gateway, that renders any web page to video via the Servo plugin: clip.* returns a finite MP4 (oneshot), cast.* serves a live WebM stream (dynamic session). The target URL is the verbatim path suffix, with an optional comma-separated options segment (dur=, vp=) and strong 720p30 defaults. Worth a reviewer's attention: - cast owns session lifetime (the engine does not auto-stop a pipeline when its MSE viewers disconnect): one shared session per URL+viewport, viewer refcount, idle + max-lifetime reaper, and graceful-shutdown teardown. The MSE proxy closes the upstream body on client disconnect so a read parked between frames can't keep the viewer count pinned. - SSRF guard (public/URL-only): blocks loopback/private/link-local/CGNAT/ cloud-metadata targets. - Encoders are swappable profiles, software by default (runs GPU-free), hardware opt-in via --clip-encoder/--cast-encoder. - Prometheus metrics incl. active sessions/viewers and reap counters. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: streamer45 <cstcld91@gmail.com>
There was a problem hiding this comment.
📝 Info: SPDX headers on non-source files are covered by REUSE.toml
CONTRIBUTING.md/AGENTS.md require SPDX headers on all new files, but the new examples/web-capture/.gitignore, .golangci.yml, and go.mod have none. I confirmed these are not violations: REUSE.toml aggregates licensing for .gitignore, **/*.yml, and **/go.mod (see REUSE.toml config-files and **/go.mod annotations), and existing example go.mod files follow the same header-less pattern. No bug.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| func (m *sessionManager) acquire(ctx context.Context, key, yaml string) (*liveSession, error) { | ||
| m.mu.Lock() | ||
| if s := m.sessions[key]; s != nil { | ||
| s.viewers++ | ||
| s.idleSince = time.Time{} | ||
| m.updateGaugesLocked() | ||
| m.mu.Unlock() | ||
| select { | ||
| case <-s.ready: | ||
| if s.createErr != nil { | ||
| m.release(s) | ||
| return nil, s.createErr | ||
| } | ||
| return s, nil | ||
| case <-ctx.Done(): | ||
| m.release(s) | ||
| return nil, ctx.Err() | ||
| } | ||
| } | ||
| if len(m.sessions) >= m.maxSessions { | ||
| m.mu.Unlock() | ||
| return nil, errOverCapacity | ||
| } | ||
| s := &liveSession{key: key, viewers: 1, createdAt: m.now(), ready: make(chan struct{})} | ||
| m.sessions[key] = s | ||
| m.updateGaugesLocked() | ||
| m.mu.Unlock() | ||
|
|
||
| // Bound the create call independently of the (possibly long-lived) viewer ctx. | ||
| cctx, cancel := context.WithTimeout(ctx, m.createTO) | ||
| defer cancel() | ||
| id, err := m.skit.createSession(cctx, yaml) | ||
|
|
||
| m.mu.Lock() | ||
| if err != nil { | ||
| delete(m.sessions, key) | ||
| s.createErr = err | ||
| m.updateGaugesLocked() | ||
| m.mu.Unlock() | ||
| close(s.ready) | ||
| return nil, err | ||
| } | ||
| s.id = id | ||
| m.byID[id] = s | ||
| m.mu.Unlock() | ||
| close(s.ready) | ||
| log.Printf("cast: session %s created (key=%q)", id, key) | ||
| return s, nil | ||
| } |
There was a problem hiding this comment.
📝 Info: Session acquire/release accounting verified balanced across creator, waiter, error and reap paths
I traced the viewer refcounting in sessionManager.acquire/release/reap (examples/web-capture/cmd/gateway/sessions.go:151-218). Key points that initially looked risky but are correct: (1) on the error paths inside acquire (existing-session ctx.Done/createErr at 164-171) acquire itself calls release, and serveCast only defers release after checking err == nil (main.go:306-320), so there is no double-release. (2) On creator create-failure (191-197) the session is removed from the map before createErr is set and ready closed, so a later acquire for the same key always creates fresh rather than serving a stale createErr. (3) m.mu guards both acquire and reap, so the idle-clear (idleSince = time.Time{} + viewers++) and the idle-reap check (viewers==0 && !idleSince.IsZero()) cannot interleave to reap a session that just gained a viewer. (4) Reaping a session with active viewers (max_lifetime) drops the gauge correctly because updateGaugesLocked only sums sessions still in the map, and the later release of the orphaned session is a no-op on the gauge. No bug found.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| case "", "/", "/favicon.ico", "/robots.txt": | ||
| gw.handleUsage(w, r) |
There was a problem hiding this comment.
📝 Info: Router intercepts /favicon.ico and /robots.txt, blocking those as capture targets
router() (examples/web-capture/cmd/gateway/main.go:235-236) routes "", /, /favicon.ico, and /robots.txt to the usage page based on the cleaned r.URL.Path. A consequence is that a target whose path is exactly favicon.ico or robots.txt cannot be captured, and any cast/clip request to host root clip.streamkit.dev/ shows usage instead of an error. This is intentional convenience behavior and harmless, but worth noting as a minor edge case.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
Addresses code-review findings on the gateway: - SSRF guard: also block 0.0.0.0/8, 198.18.0.0/15, and the IPv4 embedded in NAT64 (64:ff9b::/96) and 6to4 (2002::/16) IPv6 addresses. - cast sessions: create on a background context so a creator disconnecting mid-create no longer fails the other deduped viewers (and the create error can't masquerade as a viewer "client gave up"); cap viewers up front with a clean 503 instead of failing downstream at max_clients; drop the reconcile step that could abandon a live session on a transient list miss. - clip: acquireClip now observes client disconnect (no unbounded queueing on a full slot channel), and clip length rounds to whole frames instead of truncating sub-second durations. - proxyMSE reuses a single retry timer; collapse the two near-identical HTTP clients into one; document the upstream-status guard (it prevents a WriteHeader panic). Fix README idle-TTL default (30, not 45). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: streamer45 <cstcld91@gmail.com>
| func (c *skitClient) createSession(ctx context.Context, yaml string) (string, error) { | ||
| payload, err := json.Marshal(struct { | ||
| YAML string `json:"yaml"` | ||
| }{yaml}) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodPost, c.baseURL+"/api/v1/sessions", bytes.NewReader(payload)) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| req.Header.Set("Content-Type", "application/json") | ||
| c.auth(req) | ||
|
|
||
| resp, err := c.client.Do(req) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| defer func() { _ = resp.Body.Close() }() | ||
| if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated { | ||
| b, _ := io.ReadAll(io.LimitReader(resp.Body, 2048)) | ||
| return "", fmt.Errorf("create session: status %d: %s", resp.StatusCode, strings.TrimSpace(string(b))) | ||
| } | ||
| var out struct { | ||
| SessionID string `json:"session_id"` | ||
| } | ||
| if err := json.NewDecoder(resp.Body).Decode(&out); err != nil { | ||
| return "", fmt.Errorf("decode session response: %w", err) | ||
| } | ||
| if out.SessionID == "" { | ||
| return "", errors.New("empty session_id in response") | ||
| } | ||
| return out.SessionID, nil | ||
| } | ||
|
|
||
| func (c *skitClient) destroySession(ctx context.Context, id string) error { | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodDelete, c.baseURL+"/api/v1/sessions/"+url.PathEscape(id), nil) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| c.auth(req) | ||
| resp, err := c.client.Do(req) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer func() { _ = resp.Body.Close() }() | ||
| _, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 4096)) | ||
| // A 404 means the session is already gone — the desired end state. | ||
| if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusNotFound { | ||
| return fmt.Errorf("destroy session: status %d", resp.StatusCode) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (c *skitClient) streamURL(id string) string { | ||
| return c.baseURL + "/mse/" + url.PathEscape(id) + "/video" | ||
| } |
There was a problem hiding this comment.
📝 Info: Backend API contracts (session create/destroy, MSE path, oneshot multipart) match the gateway
Verified the gateway's assumptions against the backend: create body {"yaml":...} matches CreateSessionRequest and the response session_id matches CreateSessionResponse (apps/skit/src/server/sessions.rs:25-37); the MSE URL "/mse/"+id+"/video" built in skitClient.streamURL matches the backend's format!("/mse/{session_id}{path_suffix}") with mse_path: /video (crates/nodes/src/transport/http_mse.rs:136) and route /mse/{*path}; the oneshot call posts a multipart whose first field is config, matching parse_config_field which rejects anything else first (apps/skit/src/server/oneshot.rs:76-79); and client.input.type: none is a valid InputType::None variant (crates/api/src/yaml/mod.rs:181-188). No mismatch found.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
Move the clip/cast pipeline skeletons out of Go string constants into cmd/gateway/pipelines/*.yml.tmpl, embedded via go:embed — they now read as standalone (commented) YAML that can be diffed and reviewed on their own. Rendering is unchanged: the same placeholder substitution (URL still strconv.Quote'd) and per-profile encoder-block injection. The small per-profile encoder snippets stay in encoders.go since they're tied to the profile metadata. Also tighten the cast tests to assert on the `frame_count:` param rather than the bare word, so a comment mentioning it can't trip them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: streamer45 <cstcld91@gmail.com>
| if _, err := io.Copy(dst, src); err != nil && !errors.Is(err, context.Canceled) { | ||
| log.Printf("%s copy response error: %v", endpoint, err) | ||
| } |
There was a problem hiding this comment.
📝 Info: Cast stream copy logs an error on every normal viewer disconnect
When a cast viewer disconnects, the goroutine at examples/web-capture/cmd/gateway/proxy.go:170-173 closes the upstream body, which makes the io.Copy in streamCopy return a read-on-closed-body error. That error is not context.Canceled, so it falls through to log.Printf("%s copy response error: %v", ...) (proxy.go:190-192). This means routine viewer disconnects produce error-level log noise. Not a correctness bug, but may be worth filtering (e.g. ignore net.ErrClosed/closed-body errors) to keep logs clean.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| switch { | ||
| case optionsSegmentRe.MatchString(headDec) && tail == "": | ||
| return "", def, errNoTarget // options given but no target followed | ||
| case optionsSegmentRe.MatchString(headDec): | ||
| opts, err = parseOptions(headDec, def, maxDur) | ||
| if err != nil { | ||
| return "", def, err | ||
| } | ||
| target = tail | ||
| default: | ||
| target = rest | ||
| } |
There was a problem hiding this comment.
📝 Info: Option-segment regex can shadow a target whose first path segment contains '='
optionsSegmentRe (examples/web-capture/cmd/gateway/capture.go:60) matches any first segment shaped like key=value. A target URL whose first path segment legitimately contains = and only lowercase letters before it (e.g. a=b.com/page) is parsed as an options segment, then rejected with errUnknownOption instead of being treated as the target. The README documents the assumption that a real host never contains =, so this is intended; the digit-leading case (30s.com/x) is correctly excluded by ^[a-z]+ and is covered by a test. Flagging only because it's a non-obvious parsing edge.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
Second review pass on the gateway: - shutdownAll no longer leaks a session whose creation is in flight at SIGTERM: it marks the manager closed and waits each session's ready channel so it has an id to DELETE; acquire rejects new requests once closed. Previously an id-less in-flight session was skipped and re-registered after the maps were cleared, so its backend pipeline was never torn down. - streamCopy only logs a copy error when the client is still connected — a normal cast disconnect (ctx canceled / closed body) is no longer logged as an error, which otherwise floods the log on a busy instance. - copyHeaders strips the full hop-by-hop set (Connection, Upgrade, TE, Trailer, Keep-Alive, Proxy-*), not just Content-Length/Transfer-Encoding. - serveCast renders the cast YAML lazily, so deduped viewers of an already- running stream don't do throwaway render work. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: streamer45 <cstcld91@gmail.com>
Leverage two just-merged backend PRs (auth #634, fMP4 MSE #637): - cast can now serve H.264 in fragmented MP4 via new h264-sw/h264-hw cast profiles — http::mse auto-detects fMP4 (#637), so a plain <video> plays the live stream in Safari/iOS. The cast muxer is chosen per profile now (WebM for VP9/AV1, fMP4 for H.264); VP9/WebM stays the default for crisp screen text on Chromium/Firefox. - Docs: the gateway no longer needs an admin token — the built-in user role now allows transport::http::mse (#634), so a user-role token covers the whole pipeline. (The demo's token minting is updated separately.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: streamer45 <cstcld91@gmail.com>
Serve both outputs from one host (web.streamkit.dev): the path is
/{clip|cast}/[options/]{url} — output first, then optional config, then the
target URL. detectMode now reads the mode from the path only (the subdomain
branch is dropped), so any host works. Usage page, README, and tests updated.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: streamer45 <cstcld91@gmail.com>
| key := fmt.Sprintf("%s|%dx%d", u.String(), opts.resW, opts.resH) | ||
| s, err := gw.sessions.acquire(r.Context(), key, func() string { | ||
| return renderCastPipeline(u.String(), opts.resW, opts.resH, captureFPS, gw.maxViewers, gw.castBitrate, gw.castEnc) | ||
| }) |
There was a problem hiding this comment.
📝 Info: Viewer count and backend max_clients are exactly equal — reconnect churn can briefly 503
The gateway admits up to maxViewers concurrent viewers per session (sessions.go:162) and the cast pipeline is rendered with max_clients = gw.maxViewers (examples/web-capture/cmd/gateway/main.go:307). Each gateway viewer maps to exactly one backend MSE connection. Because the two limits are identical, a viewer that reconnects (common with MSE) before the previous connection's release/body-close has propagated can momentarily push the backend to max_clients, causing the new connection to be rejected. proxyMSE retries for mseReadyTimeout (8s) so it self-heals, but under heavy churn this can surface as transient latency. Not a correctness bug; consider sizing backend max_clients slightly above the gateway's maxViewers.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| @@ -0,0 +1,26 @@ | |||
| version: "2" | |||
There was a problem hiding this comment.
📝 Info: SPDX/REUSE compliance for headerless config files is satisfied via REUSE.toml
The new .gitignore and .golangci.yml lack inline SPDX headers, which would appear to violate the CONTRIBUTING/AGENTS rule that all new files carry SPDX headers. I confirmed REUSE.toml at the repo root aggregates **/*.yml, .gitignore, and **/.gitignore with the MPL-2.0 license (and this matches the sibling examples/speech-gateway/.golangci.yml, which is also headerless). So these files are compliant and I did not report them as rule violations. go.sum is present on disk (just omitted from the diff), so the Dockerfile go build is not broken.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
streamer45
changed the title
| switch { | ||
| case s.viewers == 0 && !s.idleSince.IsZero() && now.Sub(s.idleSince) >= m.idleTTL: | ||
| s.reapReason = "idle" | ||
| case now.Sub(s.createdAt) >= m.maxLifetime: | ||
| s.reapReason = "max_lifetime" | ||
| default: | ||
| continue | ||
| } | ||
| delete(m.sessions, key) | ||
| delete(m.byID, s.id) | ||
| toDestroy = append(toDestroy, s) |
There was a problem hiding this comment.
📝 Info: Max-lifetime reap of a session with active viewers under-reports active_viewers briefly
When reap tears down a session via the max_lifetime branch (sessions.go:241) while it still has connected viewers, the session is removed from m.sessions and updateGaugesLocked immediately drops webcapture_active_viewers by those viewers, even though their MSE connections keep streaming until skit kills the pipeline and proxyMSE unblocks. The later release calls on the detached session are no-ops for the gauge (session not in the map). This is a transient cosmetic gauge skew, not a correctness/resource bug — flagging only so it isn't mistaken for a leak.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| func isBlockedIP(ip net.IP) bool { | ||
| if ip.IsLoopback() || | ||
| ip.IsPrivate() || | ||
| ip.IsUnspecified() || | ||
| ip.IsLinkLocalUnicast() || | ||
| ip.IsLinkLocalMulticast() || | ||
| ip.IsInterfaceLocalMulticast() || | ||
| ip.IsMulticast() { | ||
| return true | ||
| } | ||
| if v4 := ip.To4(); v4 != nil { | ||
| return blockedV4(v4) | ||
| } | ||
| // IPv6 forms that embed an internal IPv4 (NAT64, 6to4) dodge the v4 checks | ||
| // above — extract the embedded address and re-check it. | ||
| if v4 := embeddedV4(ip); v4 != nil { | ||
| return isBlockedIP(v4) | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // blockedV4 covers reserved IPv4 ranges that the net.IP predicates miss. | ||
| func blockedV4(v4 net.IP) bool { | ||
| switch { | ||
| case v4[0] == 0: // 0.0.0.0/8 "this host" — 0.0.0.1 reaches localhost on Linux | ||
| return true | ||
| case v4[0] == 100 && v4[1]&0xc0 == 64: // 100.64.0.0/10 CGNAT | ||
| return true | ||
| case v4[0] == 198 && v4[1]&0xfe == 18: // 198.18.0.0/15 benchmarking | ||
| return true | ||
| default: | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| // embeddedV4 returns the IPv4 embedded in a NAT64 (64:ff9b::/96) or 6to4 | ||
| // (2002::/16) address, or nil for anything else. | ||
| func embeddedV4(ip net.IP) net.IP { | ||
| v6 := ip.To16() | ||
| if v6 == nil || ip.To4() != nil { | ||
| return nil | ||
| } | ||
| nat64 := v6[0] == 0x00 && v6[1] == 0x64 && v6[2] == 0xff && v6[3] == 0x9b && | ||
| v6[4] == 0 && v6[5] == 0 && v6[6] == 0 && v6[7] == 0 && | ||
| v6[8] == 0 && v6[9] == 0 && v6[10] == 0 && v6[11] == 0 | ||
| if nat64 { | ||
| return net.IPv4(v6[12], v6[13], v6[14], v6[15]) | ||
| } | ||
| if v6[0] == 0x20 && v6[1] == 0x02 { // 6to4 | ||
| return net.IPv4(v6[2], v6[3], v6[4], v6[5]) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
📝 Info: SSRF IP blocklist covers IPv4-mapped/NAT64/6to4 embeddings correctly
I verified the reserved-range arithmetic in blockedV4 and embeddedV4: 100.64.0.0/10 via v4[1]&0xc0==64, 198.18.0.0/15 via v4[1]&0xfe==18, 0.0.0.0/8 via v4[0]==0, plus NAT64 64:ff9b::/96 and 6to4 2002::/16 extraction recursing through isBlockedIP. All match their documented CIDRs and the test vectors in capture_test.go:128-145. The DNS re-resolution in validateTarget is documented as best-effort (rebinding between check and Servo's fetch is an acknowledged limitation). No bug.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| #[test] | ||
| fn config_partial_role_override_deep_merges_builtin() { | ||
| figment::Jail::expect_with(|jail| { | ||
| jail.create_file( | ||
| "skit.toml", | ||
| r#"[permissions.roles.user] | ||
| allowed_nodes = ["video::*", "containers::*", "transport::http::mse", "plugin::*"] | ||
| "#, | ||
| )?; | ||
| let result = load("skit.toml").expect("load with user override"); | ||
| let user = result.config.permissions.roles.get("user").expect("user role"); | ||
| // Fields absent from the TOML must survive from the built-in default | ||
| // (deep-merge), not reset to the struct's bool default of false. | ||
| assert!(user.create_sessions, "create_sessions preserved by deep-merge"); | ||
| assert!(user.destroy_sessions, "destroy_sessions preserved by deep-merge"); | ||
| // The overridden array adds the MSE transport node. | ||
| assert!(user.is_node_allowed("transport::http::mse")); | ||
| Ok(()) | ||
| }); | ||
| } |
There was a problem hiding this comment.
📝 Info: Deep-merge config test relies on figment dict-level merge of map values
The new test config_partial_role_override_deep_merges_builtin asserts that overriding only allowed_nodes for the built-in user role preserves create_sessions/destroy_sessions from defaults. This holds because load merges Serialized::defaults(Config::default()) then the TOML (config.rs:1187-1200), and figment merges nested dicts (including HashMap<String,Permissions> values) at the Value level before struct extraction, so absent fields keep the default's true rather than serde's bool default of false (defaults defined at permissions.rs:137-138). The test reflects real existing behavior, not a regression.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| func (gw *gateway) proxyMSE(w http.ResponseWriter, r *http.Request, s *liveSession) { | ||
| ctx := r.Context() | ||
| streamURL := gw.skit.streamURL(s.id) | ||
| deadline := time.Now().Add(gw.mseReadyTimeout) | ||
| retry := time.NewTicker(200 * time.Millisecond) | ||
| defer retry.Stop() | ||
|
|
||
| var resp *http.Response | ||
| for { | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, streamURL, nil) | ||
| if err != nil { | ||
| gw.failUpstream(w, "cast", err) | ||
| return | ||
| } | ||
| gw.authReq(req) | ||
| r2, err := gw.streamClient.Do(req) | ||
| if err == nil && r2.StatusCode == http.StatusOK { | ||
| resp = r2 | ||
| break | ||
| } | ||
| if r2 != nil { | ||
| _, _ = io.Copy(io.Discard, io.LimitReader(r2.Body, 2048)) | ||
| _ = r2.Body.Close() | ||
| } | ||
| if ctx.Err() != nil { | ||
| return | ||
| } | ||
| if time.Now().After(deadline) { | ||
| if err != nil { | ||
| gw.failUpstream(w, "cast", err) | ||
| } else { | ||
| gw.failUpstream(w, "cast", fmt.Errorf("mse stream not ready (status %d)", r2.StatusCode)) | ||
| } | ||
| return | ||
| } | ||
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| case <-retry.C: | ||
| } | ||
| } | ||
| defer func() { _ = resp.Body.Close() }() | ||
|
|
||
| // Close the upstream body the instant the viewer's connection ends, so | ||
| // streamCopy's read unblocks immediately. Without this, a read parked | ||
| // between frames keeps the session's viewer count > 0 and dodges idle | ||
| // reaping until skit happens to write again. ctx is also canceled when the | ||
| // handler returns, so this goroutine never outlives the request. | ||
| go func() { | ||
| <-ctx.Done() | ||
| _ = resp.Body.Close() | ||
| }() | ||
|
|
||
| copyHeaders(w.Header(), resp.Header) | ||
| if w.Header().Get("Content-Type") == "" { | ||
| w.Header().Set("Content-Type", "video/webm") | ||
| } | ||
| w.WriteHeader(http.StatusOK) | ||
| streamCopy(ctx, w, resp.Body, "cast") | ||
| } |
There was a problem hiding this comment.
📝 Info: upstream_duration metric is never observed for the cast endpoint
webcapture_upstream_duration_seconds is declared with an endpoint label (metrics.go:72-76) and observed in proxyOneshot for clip (proxy.go:93), but proxyMSE never records it for cast. This is harmless (the histogram simply has no cast series) and arguably intentional given the retry-until-ready loop has no single 'time to headers' value, but reviewers expecting per-endpoint upstream latency should know cast is absent.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #631 +/- ##
==========================================
+ Coverage 85.22% 85.24% +0.01%
==========================================
Files 249 249
Lines 75412 75427 +15
Branches 2437 2437
==========================================
+ Hits 64272 64298 +26
+ Misses 11134 11123 -11
Partials 6 6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…t fragment fast Servo's SoftwareRenderingContext shares a surfman surface pool across instances, and spin_event_loop leaves the last-painted instance's context bound — so a freshly-opened clip/cast could read another, unrelated session's pixels (a privacy leak on the public demo). - Gate surface reads on first paint: until notify_new_frame_ready has fired for the current page, emit a transparent frame instead of reading a possibly-stale surface; re-arm the gate on URL change. - Re-bind the instance's rendering context immediately before read_to_image (after spin_event_loop) so the read always captures this instance's surface, never a concurrent node's. - Add an isolated integration test that drives two concurrent solid- colour data: pages and asserts neither leaks the other's pixels. - web-capture: tune the OpenH264 GOP to ~1s (one IDR per fps frames) so the fragmented-MP4 clip flushes its first playable fragment fast. Signed-off-by: streamkit-devin <devin@streamkit.dev>
| for { | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, streamURL, nil) | ||
| if err != nil { | ||
| gw.failUpstream(w, "cast", err) | ||
| return | ||
| } | ||
| gw.authReq(req) | ||
| r2, err := gw.streamClient.Do(req) | ||
| if err == nil && r2.StatusCode == http.StatusOK { | ||
| resp = r2 | ||
| break | ||
| } | ||
| if r2 != nil { | ||
| _, _ = io.Copy(io.Discard, io.LimitReader(r2.Body, 2048)) | ||
| _ = r2.Body.Close() | ||
| } | ||
| if ctx.Err() != nil { | ||
| return | ||
| } | ||
| if time.Now().After(deadline) { | ||
| if err != nil { | ||
| gw.failUpstream(w, "cast", err) | ||
| } else { | ||
| gw.failUpstream(w, "cast", fmt.Errorf("mse stream not ready (status %d)", r2.StatusCode)) | ||
| } | ||
| return | ||
| } | ||
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| case <-retry.C: | ||
| } | ||
| } | ||
| defer func() { _ = resp.Body.Close() }() |
There was a problem hiding this comment.
📝 Info: proxyMSE retry loop avoids nil deref and leaks responses correctly
In proxyMSE (examples/web-capture/cmd/gateway/proxy.go:130-162), r2 is only dereferenced for r2.StatusCode in the deadline branch when err == nil (guaranteeing r2 != nil), and non-200 responses are drained+closed each iteration before retrying. The success response gets a defer Close plus a goroutine that closes it on ctx.Done(); double-Close on an http.Response.Body is safe. The ticker is stopped via defer. No connection/response leak across the retry window.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
E2E test results — clean / fast / realtimeRan the plugin regression test plus a live stack ( Isolation — no cross-session leak (the headline fix)
Fast-first-frame + progressive delivery — PASS
Realtime — PASS
Blocked / not freshly run (env, not code)
Latent finding — resize re-arm gate (Comment #35)The first-paint gate re-arms on URL change but not on a viewport resize via CI: 29 checks green ( |
A resize via UpdateConfig reallocates the surfman surface, which is pooled across instances and may hold another instance's pixels until the resized page repaints. Re-arm the painted gate (as on URL change) so handle_render emits transparent frames until the post-resize paint, closing the same cross-session leak on the resize path. Signed-off-by: streamkit-devin <devin@streamkit.dev>
| switch k { | ||
| case "dur": | ||
| d, err := parseDuration(v) | ||
| if err != nil { | ||
| return def, err | ||
| } | ||
| if d > maxDur { | ||
| d = maxDur // clamp rather than reject — friendlier for a demo | ||
| } | ||
| opts.dur = d |
There was a problem hiding this comment.
📝 Info: dur option silently accepted on cast despite being clip-only
parseOptions (examples/web-capture/cmd/gateway/capture.go:124-133) parses dur regardless of mode, and serveCast ignores opts.dur. The README states "dur is clip-only". So /cast/dur=30s/example.com is accepted and silently ignored rather than rejected. The detectMode test deliberately includes /cast/dur=5s/..., indicating this is intentional permissiveness, not a defect — noting only the doc/behavior mismatch.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| func (m *sessionManager) acquire(ctx context.Context, key string, renderYAML func() string) (*liveSession, error) { | ||
| m.mu.Lock() | ||
| if m.closed { | ||
| m.mu.Unlock() | ||
| return nil, errShuttingDown | ||
| } | ||
| if s := m.sessions[key]; s != nil { | ||
| if s.viewers >= m.maxViewers { | ||
| m.mu.Unlock() | ||
| return nil, errOverCapacity // reject now rather than fail downstream at max_clients | ||
| } | ||
| s.viewers++ | ||
| s.idleSince = time.Time{} | ||
| m.updateGaugesLocked() | ||
| m.mu.Unlock() | ||
| select { | ||
| case <-s.ready: | ||
| if s.createErr != nil { | ||
| m.release(s) | ||
| return nil, s.createErr | ||
| } | ||
| return s, nil | ||
| case <-ctx.Done(): | ||
| m.release(s) | ||
| return nil, ctx.Err() | ||
| } | ||
| } | ||
| if len(m.sessions) >= m.maxSessions { | ||
| m.mu.Unlock() | ||
| return nil, errOverCapacity | ||
| } | ||
| s := &liveSession{key: key, viewers: 1, createdAt: m.now(), ready: make(chan struct{})} | ||
| m.sessions[key] = s | ||
| m.updateGaugesLocked() | ||
| m.mu.Unlock() | ||
|
|
||
| // Create on a background context, NOT the first viewer's: if that viewer | ||
| // disconnects mid-create, the pipeline must still come up for the other | ||
| // deduped waiters (and createErr must not masquerade as context.Canceled). | ||
| cctx, cancel := context.WithTimeout(context.Background(), m.createTO) | ||
| defer cancel() | ||
| id, err := m.skit.createSession(cctx, renderYAML()) | ||
|
|
||
| m.mu.Lock() | ||
| if err != nil { | ||
| delete(m.sessions, key) | ||
| s.createErr = err | ||
| m.updateGaugesLocked() | ||
| m.mu.Unlock() | ||
| close(s.ready) | ||
| return nil, err | ||
| } | ||
| s.id = id | ||
| m.byID[id] = s | ||
| m.mu.Unlock() | ||
| close(s.ready) | ||
| log.Printf("cast: session %s created (key=%q)", id, key) | ||
| return s, nil | ||
| } |
There was a problem hiding this comment.
📝 Info: Session created/destroyed lifecycle and reaper interactions reviewed
Reviewed sessionManager concurrency carefully: dedupe path increments viewers under lock then waits on ready outside the lock with proper release on ctx-cancel/createErr; creator uses a background context so a disconnecting first viewer doesn't abort creation for deduped waiters; reap deletes during range (safe in Go) and tears down max-lifetime sessions even with active viewers; shutdownAll snapshots then waits on in-flight creates (covered by TestSessionManagerShutdownDuringCreate). The double resp.Body.Close() in proxyMSE (defer + ctx-watcher goroutine) is safe for net/http bodies. No viewer-count underflow (guarded by s.viewers > 0). No bug found.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| func isBlockedIP(ip net.IP) bool { | ||
| if ip.IsLoopback() || | ||
| ip.IsPrivate() || | ||
| ip.IsUnspecified() || | ||
| ip.IsLinkLocalUnicast() || | ||
| ip.IsLinkLocalMulticast() || | ||
| ip.IsInterfaceLocalMulticast() || | ||
| ip.IsMulticast() { | ||
| return true | ||
| } | ||
| if v4 := ip.To4(); v4 != nil { | ||
| return blockedV4(v4) | ||
| } | ||
| // IPv6 forms that embed an internal IPv4 (NAT64, 6to4) dodge the v4 checks | ||
| // above — extract the embedded address and re-check it. | ||
| if v4 := embeddedV4(ip); v4 != nil { | ||
| return isBlockedIP(v4) | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // blockedV4 covers reserved IPv4 ranges that the net.IP predicates miss. | ||
| func blockedV4(v4 net.IP) bool { | ||
| switch { | ||
| case v4[0] == 0: // 0.0.0.0/8 "this host" — 0.0.0.1 reaches localhost on Linux | ||
| return true | ||
| case v4[0] == 100 && v4[1]&0xc0 == 64: // 100.64.0.0/10 CGNAT | ||
| return true | ||
| case v4[0] == 198 && v4[1]&0xfe == 18: // 198.18.0.0/15 benchmarking | ||
| return true | ||
| default: | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| // embeddedV4 returns the IPv4 embedded in a NAT64 (64:ff9b::/96) or 6to4 | ||
| // (2002::/16) address, or nil for anything else. | ||
| func embeddedV4(ip net.IP) net.IP { | ||
| v6 := ip.To16() | ||
| if v6 == nil || ip.To4() != nil { | ||
| return nil | ||
| } | ||
| nat64 := v6[0] == 0x00 && v6[1] == 0x64 && v6[2] == 0xff && v6[3] == 0x9b && | ||
| v6[4] == 0 && v6[5] == 0 && v6[6] == 0 && v6[7] == 0 && | ||
| v6[8] == 0 && v6[9] == 0 && v6[10] == 0 && v6[11] == 0 | ||
| if nat64 { | ||
| return net.IPv4(v6[12], v6[13], v6[14], v6[15]) | ||
| } | ||
| if v6[0] == 0x20 && v6[1] == 0x02 { // 6to4 | ||
| return net.IPv4(v6[2], v6[3], v6[4], v6[5]) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
📝 Info: SSRF IP-range checks verified against tests
isBlockedIP/blockedV4/embeddedV4 were checked bit-by-bit: CGNAT 100.64.0.0/10 (v4[1]&0xc0==64), benchmarking 198.18.0.0/15 (v4[1]&0xfe==18), 0.0.0.0/8, plus NAT64 (64:ff9b::/96) and 6to4 (2002::/16) embedded-v4 extraction and recursion all match the table in capture_test.go (including allowing NAT64-of-8.8.8.8). The DNS re-resolution guard is advisory since Servo re-resolves independently (documented). Logic is correct.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
…er parity Browser visits to /clip/* (Accept: text/html) now get the same autoplay HTML player page already served for /cast/*, instead of the browser's bare native media viewer. The page's <video> re-requests the same URL with Accept: */* and receives the raw progressive MP4, so CLI clients (curl/ffmpeg) are unaffected. The HTML branch runs before clip-slot acquire, so a browser visit does not consume render concurrency. Generalizes castPageTemplate/writeCastPage into playerPageTemplate/ writePlayerPage(kind) shared by both handlers. Signed-off-by: streamkit-devin <devin@streamkit.dev>
The URL-change branch of handle_update_config re-armed the first-paint gate but left state.last_good_frame holding the previous URL's frame. If read_to_image returned None on a tick after the new page painted, handle_render fell back to that stale frame. Null it on URL change, mirroring the viewport-resize branch. Signed-off-by: streamkit-devin <devin@streamkit.dev>
…xample --max-concurrency 0 sized clipSem as an unbuffered channel, deadlocking every clip render; clamp to a floor of 1 via clampConcurrency. Also fix the optionsSegmentRe comment to use the real 'res=' key instead of the non-existent 'vp='. Signed-off-by: streamkit-devin <devin@streamkit.dev>
| // Until Servo has painted the current page at least once, the surfman | ||
| // surface backing this rendering context may still contain a previous, | ||
| // unrelated capture's pixels (surfman reuses surfaces across contexts). | ||
| // Emit a fully transparent frame instead of reading stale content, so a | ||
| // freshly-opened clip/cast never leaks another session's page. | ||
| if !state.delegate.painted.get() { | ||
| let len = (state.config.width as usize) * (state.config.height as usize) * 4; | ||
| let rgba_data = vec![0u8; len]; | ||
| state.render_count += 1; | ||
| state.render_duration_sum += render_start.elapsed(); | ||
| if state.result_tx.send(ServoThreadResult::Frame { rgba_data }).is_err() { | ||
| instances.remove(node_id); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
🔍 Cross-session pixel-leak fix relies on surfman bind-before-read semantics
The fix in handle_render adds state.rendering_context.make_current() both before spin_event_loop (plugins/native/servo/src/servo_thread.rs:485) and immediately before read_to_image (:544), plus a first-paint gate (:512-521) emitting transparent frames until painted flips, and re-arms painted/clears last_good_frame on URL change (:617-620) and on viewport resize (:664). Note handle_resize (output-only dimension change, :674-696) intentionally does NOT reset painted, which is consistent with the stated rationale that only UpdateConfig reallocates the surfman surface. The correctness of the whole fix hinges on the assumption that read_to_image reads whichever surfman context is currently bound process-wide and that make_current deterministically rebinds this instance's surface — this is exercised by the new tests/cross_session_leak.rs integration test but is not otherwise verifiable from the diff. Worth confirming the assumption holds across surfman versions.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Acknowledged — this is an accurate description of the fix, not a defect. The surfman bind-before-read assumption is exactly what tests/cross_session_leak.rs exercises (two concurrent solid-color data: pages, interleaved ticks, asserting no cross-bleed), and it's pinned to the surfman version vendored via the Servo dep, so it won't silently drift across versions without that test catching it. Leaving the code as-is.
The node only detected WebM Cluster IDs to delimit the init segment and GOPs, so an H.264 cast stream (fragmented MP4: ftyp+moov init, repeating moof+mdat fragments) never flipped init_complete and zero bytes reached clients. Browsers showed nothing for the h264-sw cast encoder the demo pins. Detect the container framing from the first packet (ftyp/styp box at offset 4 => fMP4, else WebM) and drive a parallel fMP4 state machine that mirrors the WebM init+GOP replay: buffer ftyp+moov as the init segment, split off the first moof-led fragment, and replay init + latest fragment to late-joiners. The WebM path is unchanged. Adds unit tests for framing detection, MP4 box scanning (incl. 64-bit largesize), init/fragment splitting, cross-packet init buffering, and GOP buffer capping. Signed-off-by: streamkit-devin <devin@streamkit.dev>
main landed #637 (fMP4 support in transport::http::mse) independently; adopt its more robust implementation (box-walking that ignores moof bytes inside mdat, content_type-authoritative format detection, bounded-memory freeze guard) and drop this branch's duplicate fMP4 work in http_mse.rs. Signed-off-by: streamkit-devin <devin@streamkit.dev>
If http.NewRequestWithContext fails in proxyOneshot, the multipart writer goroutine parks forever on the io.Pipe whose reader is never consumed. Close the reader on the error path so the worker unwinds. Addresses Devin Review. Signed-off-by: streamkit-devin <devin@streamkit.dev>
Release-ready bump covering the cross-session pixel-leak privacy fix and the first-paint transparent-frame gate. Regenerates marketplace/official-plugins.json via scripts/marketplace/generate_official_plugins.py. Signed-off-by: streamkit-devin <devin@streamkit.dev>
![staging-devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/870844?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
A post-teardown 404 from the MSE upstream is indistinguishable by status from a pre-ready 404, so the readiness loop spun the full mseReadyTimeout and returned a misleading 502 when a session was reaped (idle/max-lifetime) just as a deduped late viewer connected. Check session liveness in the loop and return 503 immediately when the session is gone. Signed-off-by: streamkit-devin <devin@streamkit.dev>
…SRF guard ::a.b.c.d (IPv4-compatible) bypassed blockedV4 because To4() returns nil, and the RFC 8215 local-use NAT64 prefix (64:ff9b:1::/48) was not matched by embeddedV4, so ::169.254.169.254 and 64:ff9b:1::a9fe:a9fe reached the metadata IP. Extend embeddedV4 to extract the embedded v4 from both forms and re-screen it; add regression cases for the two bypasses. Signed-off-by: streamkit-devin <devin@streamkit.dev>
handleCapture ran the full validateTarget (up to a 5s DNS lookup) before the acceptsHTML short-circuit, so a browser address-bar visit paid the lookup just to render the static autoplay page. Split out parseTargetURL (scheme + literal-IP screen, no DNS) for the cheap fail-fast path and defer the hostname resolution into serveClip/serveCast after the player page is served. Also drop the dead dur<=0 re-defaulting in serveClip (opts.dur is always >0). Signed-off-by: streamkit-devin <devin@streamkit.dev>
…ent errors Extract a transparent_frame(w,h) helper and route the unpainted, panic-fallback, and read_to_image-miss paths through it, collapsing three copies of the buffer build. Move the painted-surface read into read_painted_frame and stop discarding the make_current() Result before read_to_image — a bind failure means the frame may not be this instance's surface, so log it instead of silently risking the cross-session leak the bind is meant to prevent. Signed-off-by: streamkit-devin <devin@streamkit.dev>
| for _, s := range all { | ||
| // Wait for an in-flight creation to resolve so we have an id to DELETE; | ||
| // tearing down only id-bearing sessions would leak the rest's pipelines. | ||
| select { | ||
| case <-s.ready: | ||
| case <-ctx.Done(): | ||
| return | ||
| } | ||
| if s.id != "" && s.createErr == nil { | ||
| s.reapReason = "shutdown" | ||
| m.destroy(ctx, s) | ||
| } | ||
| } |
There was a problem hiding this comment.
📝 Info: shutdownAll abandons remaining sessions on context expiry
shutdownAll (examples/web-capture/cmd/gateway/sessions.go:307-319) iterates all and, while waiting for each in-flight create to resolve, returns on ctx.Done(). This abandons any subsequent sessions in all (potentially already-ready) without DELETE, contradicting the documented invariant that shutdown 'never leaks Servo pipelines'. In practice this is benign: m.destroy derives its timeout from the same ctx, so once that budget is exhausted no destroy could succeed anyway. Worth noting only as a best-effort limitation under a single slow create blocking the shutdown budget.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| // page paints can't fall back to the previous URL's last frame. | ||
| state.last_good_frame = None; | ||
| // Reset the one-shot post-load gate so the new page gets | ||
| // its custom-CSS injection (if any) once it finishes | ||
| // loading. Render ticks will run `handle_render`'s |
There was a problem hiding this comment.
📝 Info: painted gate intentionally not reset on compositor output Resize
handle_resize (plugins/native/servo/src/servo_thread.rs:627-649) clears last_good_frame but does NOT reset delegate.painted, unlike the URL-change and viewport-resize paths in handle_update_config which both re-arm it. This is correct: Resize only changes the output (frame) dimensions and leaves the SoftwareRenderingContext/surfman surface and rc_width/rc_height untouched, so the already-painted surface is still this instance's own content and is simply re-scaled to the new output size. No stale/cross-instance pixels can surface, so no re-arm is needed.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| let rgba_data = if state.delegate.painted.get() { | ||
| read_painted_frame(state, node_id) | ||
| } else { | ||
| // Until Servo has painted the current page at least once, the surfman | ||
| // surface backing this rendering context may still contain a previous, | ||
| // unrelated capture's pixels (surfman reuses surfaces across contexts). | ||
| // Emit a fully transparent frame instead of reading stale content, so a | ||
| // freshly-opened clip/cast never leaks another session's page. | ||
| transparent_frame(state.config.width, state.config.height) | ||
| }; | ||
|
|
||
| let render_duration = render_start.elapsed(); | ||
| state.render_count += 1; | ||
| state.render_duration_sum += render_duration; | ||
|
|
||
| // Log render time periodically (every 300 frames ~ 10s at 30fps). | ||
| if state.render_count % 300 == 0 { | ||
| let avg_us = state.render_duration_sum.as_micros() / u128::from(state.render_count); | ||
| tracing::debug!( | ||
| node_id = %node_id, | ||
| frame = state.render_count, | ||
| render_us = render_duration.as_micros(), | ||
| avg_render_us = avg_us, | ||
| "Servo render metrics", | ||
| ); | ||
| } | ||
|
|
||
| if state.result_tx.send(ServoThreadResult::Frame { rgba_data }).is_err() { | ||
| instances.remove(node_id); | ||
| } |
There was a problem hiding this comment.
📝 Info: render metrics now include pre-first-paint transparent frames
In the refactored handle_render (plugins/native/servo/src/servo_thread.rs:511-540), render_count / render_duration_sum and the periodic timing log now increment for every tick, including the transparent frames emitted before the first paint. Previously the read path was always exercised. This slightly skews the average-render-time metric (pre-paint ticks are near-instant), but does not affect correctness of the emitted frames.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| func isBlockedIP(ip net.IP) bool { | ||
| if ip.IsLoopback() || | ||
| ip.IsPrivate() || | ||
| ip.IsUnspecified() || | ||
| ip.IsLinkLocalUnicast() || | ||
| ip.IsLinkLocalMulticast() || | ||
| ip.IsInterfaceLocalMulticast() || | ||
| ip.IsMulticast() { | ||
| return true | ||
| } | ||
| if v4 := ip.To4(); v4 != nil { | ||
| return blockedV4(v4) | ||
| } | ||
| // IPv6 forms that embed an internal IPv4 (NAT64, 6to4) dodge the v4 checks | ||
| // above — extract the embedded address and re-check it. | ||
| if v4 := embeddedV4(ip); v4 != nil { | ||
| return isBlockedIP(v4) | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // blockedV4 covers reserved IPv4 ranges that the net.IP predicates miss. | ||
| func blockedV4(v4 net.IP) bool { | ||
| switch { | ||
| case v4[0] == 0: // 0.0.0.0/8 "this host" — 0.0.0.1 reaches localhost on Linux | ||
| return true | ||
| case v4[0] == 100 && v4[1]&0xc0 == 64: // 100.64.0.0/10 CGNAT | ||
| return true | ||
| case v4[0] == 198 && v4[1]&0xfe == 18: // 198.18.0.0/15 benchmarking | ||
| return true | ||
| default: | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| // embeddedV4 returns the IPv4 address an IPv6 form can reach an IPv4 host | ||
| // through — so the caller can re-screen it against the internal-range block | ||
| // list. Covers NAT64 (well-known 64:ff9b::/96 and RFC 8215 local-use | ||
| // 64:ff9b:1::/48), 6to4 (2002::/16), and the deprecated IPv4-compatible | ||
| // ::a.b.c.d (::/96); returns nil for anything else. | ||
| func embeddedV4(ip net.IP) net.IP { | ||
| v6 := ip.To16() | ||
| if v6 == nil || ip.To4() != nil { | ||
| return nil | ||
| } | ||
| suffixV4 := net.IPv4(v6[12], v6[13], v6[14], v6[15]) | ||
| switch { | ||
| case bytes.HasPrefix(v6, []byte{0x00, 0x64, 0xff, 0x9b, 0, 0, 0, 0, 0, 0, 0, 0}): // NAT64 64:ff9b::/96 | ||
| return suffixV4 | ||
| case bytes.HasPrefix(v6, []byte{0x00, 0x64, 0xff, 0x9b, 0x00, 0x01}): // NAT64 64:ff9b:1::/48 (RFC 8215) | ||
| return suffixV4 | ||
| case v6[0] == 0x20 && v6[1] == 0x02: // 6to4 2002::/16 | ||
| return net.IPv4(v6[2], v6[3], v6[4], v6[5]) | ||
| case bytes.Equal(v6[:12], ipv4CompatPrefix): // ::a.b.c.d (loopback/unspecified already screened) | ||
| return suffixV4 | ||
| default: | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| // ipv4CompatPrefix is the 12 zero bytes that lead an IPv4-compatible IPv6 | ||
| // address (::a.b.c.d). | ||
| var ipv4CompatPrefix = make([]byte, 12) |
There was a problem hiding this comment.
📝 Info: SSRF IPv6-embedded-IPv4 screening verified against test matrix
The isBlockedIP/embeddedV4/blockedV4 logic (examples/web-capture/cmd/gateway/capture.go:247-309) was cross-checked against the test cases. Notably ::1 (loopback) is caught by IsLoopback() before embeddedV4 would (mis)classify it via the all-zero ipv4CompatPrefix, and ::ffff:a.b.c.d is handled by Go's To4() returning non-nil so the standard predicates apply. CGNAT (100.64.0.0/10 via &0xc0==64) and benchmarking (198.18.0.0/15 via &0xfe==18) masks are correct. No gaps found.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
… Connection-listed headers - Drop gateway.authToken/authReq; route auth through the single skitClient.auth. - Drop gateway.maxViewers; read from the session manager so the two can't drift. - copyHeaders now also strips headers named in the Connection header (RFC 7230 §6.1). Signed-off-by: streamkit-devin <devin@streamkit.dev>
…tained advisory Resolves the cargo-deny advisory failures from newly published RUSTSEC notices: - anyhow 1.0.102 -> 1.0.103 (RUSTSEC-2026-0190, downcast_mut unsoundness) - wasmtime-wasi 46.0.0 -> 46.0.1 (RUSTSEC-2026-0188, WASI FilePerms bypass) - ignore RUSTSEC-2026-0192 (ttf-parser unmaintained via fontdue; no safe upgrade, tracked in #642), matching the existing unmaintained-advisory ignores. Signed-off-by: streamkit-devin <devin@streamkit.dev>
There was a problem hiding this comment.
📝 Info: First-paint gate is not re-armed on output-only Resize, by design
handle_update_config re-arms delegate.painted = false on URL change and on viewport (rendering-context) resize because the surfman surface is reallocated (plugins/native/servo/src/servo_thread.rs:625,669-672). handle_resize (output-only hint) deliberately does NOT reset painted — it only changes config.width/height used for output scaling and clears last_good_frame, while the underlying rendering surface still holds this instance's own pixels. This asymmetry is correct (no cross-session leak risk from an output-dimension change) but is subtle; a future change that makes handle_resize touch the rendering context would need to re-arm the gate too.
(Refers to lines 700-704)
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| defer func() { | ||
| p := recover() | ||
| code := rec.code | ||
| switch { | ||
| case p != nil: | ||
| code = http.StatusInternalServerError | ||
| case code == 0: | ||
| code = codeClientClosed | ||
| } | ||
| requestDuration.WithLabelValues(endpoint).Observe(time.Since(start).Seconds()) | ||
| requestsTotal.WithLabelValues(endpoint, methodLabel(r.Method), codeLabel(code)).Inc() | ||
| inflightRequests.WithLabelValues(endpoint).Dec() | ||
| if p != nil { | ||
| panic(p) | ||
| } | ||
| }() |
There was a problem hiding this comment.
📝 Info: Panicking handler is counted as 500 even if a 200 header was already sent
In withMetrics, when the wrapped handler panics after it has already written a status (e.g. proxyMSE wrote 200 then panicked mid-stream), the deferred recorder unconditionally overrides code to 500 (examples/web-capture/cmd/gateway/metrics.go:158-163) regardless of rec.code. The client may have received a 200 plus partial body, but webcapture_requests_total records code=500. This is a minor metrics-accuracy nuance, not a functional bug, and the 500 classification is arguably the more useful signal for a panic.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
A create in flight when shutdownAll ran would re-register its id in the fresh byID map and hand the caller a session that shutdownAll was about to destroy. Now acquire re-checks closed after the create resolves and returns errShuttingDown; shutdownAll still tears the pipeline down. Signed-off-by: streamkit-devin <devin@streamkit.dev>
…der profile Store encoder content types as plain MIME strings (YAML-quoted at template render time via strconv.Quote) so proxyMSE can reuse the active cast profile's type instead of a hardcoded video/webm that would mislabel H.264/fMP4 streams if the upstream header were ever missing. Also bound the stream client's wait for upstream response headers so a hung backend can't pin a viewer request indefinitely. Signed-off-by: streamkit-devin <devin@streamkit.dev>
…Token field A max-sessions or max-viewers value < 1 would silently reject every cast; floor them at 1 with a log line, like max-concurrency. Signed-off-by: streamkit-devin <devin@streamkit.dev>
Integration tests already see the identical uuid entry from [dependencies]. Signed-off-by: streamkit-devin <devin@streamkit.dev>
…pendal No upgrade path exists: opendal 0.57 (latest) pins quick-xml 0.39, while the fixes land in quick-xml >= 0.41. Exposure is limited to XML responses from the operator-configured S3 endpoint. Removal tracked in #647. Signed-off-by: streamkit-devin <devin@streamkit.dev>
2 participants
Summary
examples/web-capture: a thin Go gateway (mirroringexamples/speech-gateway) that renders any web page to video via the Servo plugin.clip.*returns a finite MP4 (oneshot);cast.*serves a live WebM stream (dynamic session). The target URL is the verbatim path suffix, with an optional comma-separatedkey=valueoptions segment (dur=,res=).--clip-encoder/--cast-encoder(Vulkan H.264, NVENC AV1). Capture is 1:1 (render = encode, default 1080p) so text stays crisp.Update: clean, fast, progressive delivery
Three linked fixes so a capture shows the right page within ~2–4s and streams in realtime:
SoftwareRenderingContextshares a surfman surface pool across instances, andspin_event_loopleaves the last-painted instance's context bound, so a freshly-opened clip/cast could read another, unrelated session's pixels. Nowhandle_render(a) emits a transparent frame until the current page's first paint (notify_new_frame_ready), re-arming the gate on URL change, and (b)make_current()s the instance's own context immediately beforeread_to_image— guaranteeing the read captures this instance's surface, never a neighbour's.data:pages interleaved and asserts neither leaks the other's pixels. It boots the real Servo engine (software/llvmpipe) and_exit(0)s after asserting — Servo has no clean global shutdown, so normal teardown SIGSEGVs in mozjs' C++ destructors.mode: stream); the OpenH264 GOP is now ~1s (one IDR perfpsframes) so the first playable fragment lands fast instead of after a full default 2s GOP.Review & Validation
sessions.go): refcount + idle/max-lifetime reaping, and the disconnect path inproxy.go(proxyMSE).capture.govalidateTarget/isBlockedIP) — are the blocked ranges sufficient for a public instance?make_current()calls inhandle_renderand the first-paint gate — confirm a freshly-opened capture never shows a prior page (sequential + concurrent).cd plugins/native/servo && CARGO_TARGET_DIR=../../../target/plugins cargo test --release --test cross_session_leak(heavy Servo build; not in CI).clip/castreach clean playable content within ~2–4s (opening frames transparent, then the real page), then stream at ~fps.Notes
go testincl.-race, golangci-lint,reuse lint, the Rust regression test). The full render path needs a backend withplugin::native::servoloaded.cargo test --workspace(the Servo plugin is a separate crate with a heavy build); run it explicitly as above.plugin::native::servonode.clipand the fMP4castprofiles are H.264/MP4 and play everywhere (incl. Safari/iOS overtransport::http::mse); WebM/VP9castmay not play in Safari.Link to Devin session: https://staging.itsdev.in/sessions/ff7a2a8a92524906b9877a8c5c6d5009
Requested by: @streamer45
Devin Review
cf95e4a