Repository navigation
fix(security): bound upstream responses, scrub credentials and pin media origins - #245
Conversation
# Conflicts: # internal/client/client.go
|
Run failed. View the logs →
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
 |
The health probes now build their URLs with client.BaseURLPath, so ensureScheme has no production callers left and TestEnsureScheme only covered dead code. Delete both. Found by the MiMo v2.6 Flash review.
Escaped quoted keys look back for their opening quote as far as the key budget at every delimiter, so even the linear scan runs a few tens of times slower than its benign twin there. The old bound of ten times the benign scan plus 20ms left little headroom, and the test failed on a loaded machine. Allow twenty times plus 150ms and take the fastest of five runs: the quadratic scan still fails every case by a wide margin. Follow-up to the GPT-6.1 Sol review finding fixed in 1bcd002.
|
Droid encountered an error —— View job |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one minor cleanup suggestion inline. The security boundaries in the new commits hold up under a full pass:
go build ./...andgo test ./...pass, and every bearer/redaction boundary in the delta is pinned by tests asserting exact outcomes.
Reviewed changes
- Preserved CDN signing parameters on unconfigured origins and redirects —
withholdBearerFromURL/stripBearerQueryParamsnow remove only query parameters that actually carry the bearer, soSignature,sig,key,Expiresand friends reach the CDN byte-for-byte; tokenless requests forwardLocationqueries the waynet/httpwould, and a bearer split across query pairs still fails closed. - Stopped reporting unconfigured-origin refusals as login failures — a 401/403 from an origin outside
mediaOrigins(requested without the bearer) now surfaces as*UnconfiguredMediaOriginErrorthrough the downloader, pipeline, playback stream andsanitizeUpstreamErr, telling the user to extendmediaOriginsinstead of re-authenticating; authentication failures still win when both occur. - Scrubbed submitted credentials from login errors —
secrets.SanitizeErrorWithCredentialsremoves the username and password (longest first, before the generic scrub) from transport/read/decode failures, covering an upstream that echoes the request body; oversized credentials fail closed toREDACTEDrather than skipping replacement. - Redacted whole quoted assignment values — open-value patterns (
auth=, bare=/:assignments) now extend through the closing quote viaquotedCredentialEnd, sopassword="pw-one pw-two"no longer leaves the tail visible after the first delimiter, while unrelated context (next=1) is kept. - Bounded the canonical assignment scan to linear time —
breakCursormemoizes break-free spans,quotedAssignmentKeyBoundscaps the opening-quote lookback, and scheme-word scans stop after the longest known scheme; an adversarial test pins both the linearity bound and that redaction output is unchanged. - Raised
maxPlaylistSegmentsto 50000 counting both views together — a dual-view lecture now fits under the cap, with coverage moved up fromparsePlaylisttogetPlaylist. - Rejected dot path segments in
baseUrlat config-validation time —./..segments now fail where the config is checked, matching the client'scanonicalBaseURLboundary. - Rebuilt health probes on the shared boundary —
ensureSchemeand string concatenation are gone; probes construct the profile URL viaclient.BaseURLPath(invalid config fails closed) and issue an origin-scoped authenticated GET, withupstreamTCPHostderiving the dial address from the validated URL. - Documented which event fields
Writer.Emitscrubs —Erroronly, sinceDetailsand friends carry no upstream text for current callers.
glm-5.3 ｜ 𝕏
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| if err != nil || !bearerWithheld { | ||
| return resp, err | ||
| } | ||
| return unconfiguredOriginRefusal(resp, parsedURL) |
There was a problem hiding this comment.
[SUGGESTION]: Refusal misattributes the refusing origin after a redirect to a configured origin
A bearerWithheld request starts on an unconfigured origin A, but allowsRedirect (internal/client/media_origin.go:125) still permits a cross-origin redirect to a configured origin B. When B answers 401/403, unconfiguredOriginRefusal(resp, parsedURL) computes the origin from the initial parsedURL, so the error reports "media origin A returned HTTP 401 ... it is not in mediaOrigins" even though A never answered - B refused the request. The remedial hint then points the user at the wrong host, and allowlisting A will not stop B from refusing.
Derive the origin from resp.Request.URL (the final request that produced this response) inside unconfiguredOriginRefusal, keeping the initial URL only as a fallback, so the reported origin matches the host that actually returned the status.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
A quoted value is replaced through its closing quote, past the end of its regex match. When the next key followed that quote directly, as in password="pw-one pw-two"sig="sig-one sig-two", the closing quote was the next match's boundary, so the match began inside the replaced text and was skipped, and part or all of the second value reached errors and events. Search again from the end of each replacement instead, so a key right after the quote is found and a key inside a quoted value is still never treated as an assignment. The encoded paths already redacted these shapes; the test now covers them along with SanitizeError. Found by the GPT-6.1 Sol review.
An unconfigured media origin can redirect to another origin, even one in mediaOrigins, and the 401 or 403 then comes from the destination. The error said the starting origin returned that status. Keep naming the starting origin as the one to add to mediaOrigins, and when the response came from a different origin, say that the request ended in HTTP N from that origin. Both are scheme://host[:port] only. Found by the GPT-6.1 Sol and GLM 5.3 reviews.
|
Droid encountered an error —— View job |
There was a problem hiding this comment.
ℹ️ No new issues in the two new commits — both fixes verified,
go vet ./...,go build ./...andgo test -race -count=1 ./...all pass. The review is non-approving only because the prior cleanup suggestion (leftover debug block ininternal/secrets/secrets_test.go:823) is still open and untouched by this delta; nothing in the new commits needs fixing.
Reviewed changes
- Redacted adjacent quoted assignments with no separator —
replaceCredentialValuesnow resumes its search from the end of each replacement rather than the end of each regex match, so a key that directly follows a quoted value's closing quote (password="pw-one pw-two"sig="sig-one sig-two") is redacted instead of skipped, while a key inside a quoted value remains part of that value. The committed test fails against the parent commit with real leaks (sig-one/sig-twofragments), and a broader probe (escaped quotes, mixed quote styles, multibyte values, colon and JSON forms) stays clean; the residual adjacent-shape over-redaction (cookie="…"authorization="…"swallows its tail) is identical at the parent, fail-closed, and pre-existing. - Named the origin that refused a redirected media request —
UnconfiguredMediaOriginErrorgainsRespondingOrigin, derived fromresp.Request.URLand origin-normalized, so a tokenless request that starts at an unconfigured origin and is redirected elsewhere reports where the 401/403 actually came from while still naming the starting origin as the one to configure; same-origin refusals keep the single-origin message, and no downstream consumer pinned the old wording.
Posted via Pullfrog · View workflow run
glm-5.3 ｜ 𝕏
The encoded-authorization case dumped parser internals through t.Logf and asserted nothing; TestScrub_RedactsEncodedFreeFormAssignmentsWithoutKnownToken already pins that behaviour. (Kilo Code and Pullfrog review bots)
|
Droid encountered an error —— View job |
There was a problem hiding this comment.
✅ No new issues found. The one open thread from earlier reviews — the leftover debug block in
internal/secrets/secrets_test.go— is addressed by this delta, so nothing remains blocking.go vet ./...,go build ./...andgo test -count=1 ./...all pass.
Reviewed changes
- Removed the leftover debug logging from the scrub table test — the
t.Logfscaffolding (decoded view dump, credential ranges, regex-match values, step pointers) guarding theencoded authorizationcase is deleted; zerot.Logfcalls remain insecrets_test.go, andTestScrub_RedactsEncodedFreeFormAssignmentsWithoutKnownTokencontinues to pin that behavior with exactwantstrings.
glm-5.3 ｜ 𝕏
| // boundary. Any other match ends before a byte that cannot start a key, so | ||
| // resuming there finds the same matches as one search over the whole text. | ||
| for last < len(value) { | ||
| index := step.expression.FindStringSubmatchIndex(value[last:]) |
There was a problem hiding this comment.
[SUGGESTION]: Replacement rescan loop is worst-case quadratic on credential-dense input
The rewrite searches value[last:] once per replacement, so total scan cost is O(n × matches), where the previous single FindAllStringSubmatchIndex pass was linear. Text shaped like repeated auth=1& pairs advances last only ~7 bytes per iteration while each rescan walks an O(n) suffix, so a dense adversarial diagnostic reaching Scrub — the self-declared defense-in-depth boundary for logs, terminal output and durable error summaries, with no length cap in the function itself — costs ~n². This package already bounds its other scans for exactly this reason (the bounded quoted-key lookback in quotedAssignmentKeyBounds, the bounded scheme-word scan via maxCredentialSchemeBytes).
Smallest credible fix: bound each rescan window to a maxCredentialKeyRunes-plus-scheme lookback past last, mirroring those bounded lookbacks, or document the assumed input-size bound at the loop.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Lands the security-hardening work that was sitting on unpushed local branches since v0.1.29. Three branches stack into this one (each contains the one before it):
baseUrlorigin and to an explicitmediaOrigins/IMPARTUS_MEDIA_ORIGINSallowlist. Requests to other hosts go without the token.baseUrlis rejected.main.Base. The branch starts at the v0.1.29 release merge (
59a0c60) and merges into currentmainwithout conflicts.Checks.
go build ./...andgo test ./...pass on the branch as-is and on the merge withmain(go 1.26.5).*.example.test,example.comand documentation IP ranges.Behaviour change for API consumers. A consumer whose segments or keys are served from a host other than
baseUrlmust list that host inmediaOrigins/IMPARTUS_MEDIA_ORIGINS.Left out.
fix/authenticated-media-origin-boundaryis an earlier version of item 3. All of its tests are included here.feat/manifest-media-sha256overlaps with fix(cli): persist SHA-256 when recording artifacts #236, which is already onmain.