fix(media): constrain attachment redirects#134
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75d6dc51f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if len(via) >= 3 || !isAllowedAttachmentURL(req.URL.String()) { | ||
| return errors.New("attachment redirect denied") |
There was a problem hiding this comment.
If an attachment legitimately needs three allowed Discord CDN redirects, this guard fails it before issuing the third redirected request: net/http calls CheckRedirect with req as the upcoming request and via as the requests already made, so len(via) == 3 is the third redirect target (initial request plus two followed redirects), not a request after three redirect hops. This caps successful fetches at two redirects and records those otherwise-allowed attachments as failures.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🦐 gold shrimp
|
Codex review: needs changes before merge. Reviewed July 17, 2026, 3:37 AM ET / 07:37 UTC. Summary Reproducibility: yes. for the proposed-patch defect: three consecutive allowlisted redirects reach Review metrics: 2 noteworthy metrics.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge
Security Review findings
Review detailsBest possible solution: Preserve the CDN allowlist and final-response defense, allow exactly three redirect hops, reject the fourth, add exact boundary coverage, and revalidate the refreshed branch against current main. Do we have a high-confidence way to reproduce the issue? Yes for the proposed-patch defect: three consecutive allowlisted redirects reach Is this the best way to solve the issue? No as written; the defense-in-depth structure is appropriate, but the hop-count guard must match the stated three-hop contract and be revalidated after updating the branch. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: unclear because the file could not be read completely. Codex review notes: model internal, reasoning high; reviewed against 91aa80d2f826. Label changesLabel changes:
Label justifications:
Evidence reviewedAcceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
Co-authored-by: Brendan <brendan@grant.lan>
75d6dc5 to
ea55e2a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea55e2a626
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| defer func() { _ = resp.Body.Close() }() | ||
| // Injected clients can supply their own redirect policy, so validate the | ||
| // final response URL at the fetch boundary as well. | ||
| if resp.Request == nil || resp.Request.URL == nil || !isAllowedAttachmentURL(resp.Request.URL.String()) { |
There was a problem hiding this comment.
When FetchOptions.HTTPClient is backed by a custom RoundTripper, net/http does not populate Response.Request for the transport; fakes and test transports often omit it. This guard now turns an otherwise successful response for the already-validated attachment URL into attachment response URL denied before status/body handling whenever the transport leaves Request nil, so injected clients that worked before fail even with no redirect involved.
Useful? React with 👍 / 👎.
1 participant
Summary
Extracted from the generic attachment hardening in #131. Thanks @GrantTheAssistant; contributor credit is preserved in the commit and changelog.
Proof
go test ./... -count=1go test -count=1 -race ./internal/mediagolangci-lint run ./...go vet ./...staticcheck ./...gosec -exclude=G101,G115,G202,G301,G304 ./...govulncheck ./...go mod verifygo mod tidy -diffThe integration regression drives the production
fetchURLboundary through a realhttp.Client/RoundTripper: an allowlisted Discord request returns a final link-local response URL, and Discrawl rejects it before reading or writing the response body.