feat: reject cross-host redirects and HTTPS→HTTP downgrades (#95) #111

Merged
aweiker merged 2 commits from review-bot-issue-95 into main 2026-05-13 16:58:49 +00:00
Owner

Summary

Reject cross-host redirects and HTTPS→HTTP downgrades entirely in both GitHub and Gitea HTTP clients, rather than just stripping auth headers.

Changes

github/client.go

  • Add defaultCheckRedirect function that rejects:
    • HTTPS→HTTP protocol downgrades (returns error)
    • Cross-host redirects entirely (returns error)
    • More than 10 total redirects (returns error)
  • Same-host, same-or-upgraded-scheme redirects remain allowed
  • Wire defaultCheckRedirect into NewClient
  • Update SetHTTPClient(nil) to restore the policy

gitea/client.go

  • Add identical defaultCheckRedirect function
  • Wire into NewClient
  • Update SetHTTPClient(nil) to restore the policy

Tests (github/client_test.go, gitea/client_test.go)

  • TestNewClient_HasCheckRedirect — verify policy is wired
  • TestDefaultCheckRedirect_RejectsHTTPSToHTTP — downgrade rejected
  • TestDefaultCheckRedirect_RejectsCrossHost — cross-host rejected
  • TestDefaultCheckRedirect_AllowsSameHost — same-host allowed, auth preserved
  • TestDefaultCheckRedirect_AllowsSameHostHTTPToHTTP — no false positive on HTTP→HTTP same-host
  • TestDefaultCheckRedirect_RejectsTooManyRedirects — 10+ redirects rejected
  • TestDefaultCheckRedirect_EmptyViaAllowed — defensive nil/empty guard
  • TestSetHTTPClient_NilRestoresDefault — nil restore includes policy

Security Rationale

Previously, cross-host redirects only stripped the Authorization header but still followed the redirect. This meant the client could consume responses from untrusted endpoints, which could be exploited for response manipulation. Now the client fails fast with a clear error message.

Behavioral Change

This is intentionally more restrictive. The review-bot uses JSON-based API endpoints (contents API, PR API, review API) that should never redirect cross-host in normal operation. If a legitimate redirect flow is needed in the future, callers can provide a custom http.Client via SetHTTPClient.

Closes #95

## Summary Reject cross-host redirects and HTTPS→HTTP downgrades entirely in both GitHub and Gitea HTTP clients, rather than just stripping auth headers. ## Changes ### `github/client.go` - Add `defaultCheckRedirect` function that rejects: - HTTPS→HTTP protocol downgrades (returns error) - Cross-host redirects entirely (returns error) - More than 10 total redirects (returns error) - Same-host, same-or-upgraded-scheme redirects remain allowed - Wire `defaultCheckRedirect` into `NewClient` - Update `SetHTTPClient(nil)` to restore the policy ### `gitea/client.go` - Add identical `defaultCheckRedirect` function - Wire into `NewClient` - Update `SetHTTPClient(nil)` to restore the policy ### Tests (`github/client_test.go`, `gitea/client_test.go`) - `TestNewClient_HasCheckRedirect` — verify policy is wired - `TestDefaultCheckRedirect_RejectsHTTPSToHTTP` — downgrade rejected - `TestDefaultCheckRedirect_RejectsCrossHost` — cross-host rejected - `TestDefaultCheckRedirect_AllowsSameHost` — same-host allowed, auth preserved - `TestDefaultCheckRedirect_AllowsSameHostHTTPToHTTP` — no false positive on HTTP→HTTP same-host - `TestDefaultCheckRedirect_RejectsTooManyRedirects` — 10+ redirects rejected - `TestDefaultCheckRedirect_EmptyViaAllowed` — defensive nil/empty guard - `TestSetHTTPClient_NilRestoresDefault` — nil restore includes policy ## Security Rationale Previously, cross-host redirects only stripped the Authorization header but still followed the redirect. This meant the client could consume responses from untrusted endpoints, which could be exploited for response manipulation. Now the client fails fast with a clear error message. ## Behavioral Change This is intentionally more restrictive. The review-bot uses JSON-based API endpoints (contents API, PR API, review API) that should never redirect cross-host in normal operation. If a legitimate redirect flow is needed in the future, callers can provide a custom `http.Client` via `SetHTTPClient`. Closes #95
rodin added 1 commit 2026-05-13 15:51:55 +00:00
feat: reject cross-host redirects and HTTPS→HTTP downgrades (#95)
CI / test (pull_request) Successful in 16s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 37s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m19s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m47s
1e0959b077
Add defaultCheckRedirect to both GitHub and Gitea clients that rejects:
- HTTPS→HTTP protocol downgrades (prevents plaintext leakage)
- Cross-host redirects entirely (prevents consuming untrusted responses)

Same-host, same-or-upgraded-scheme redirects remain allowed.

Both NewClient constructors wire the policy, and SetHTTPClient(nil)
restores it. Callers providing a non-nil client are responsible for
configuring their own safe redirect policy.

Closes #95
sonnet-review-bot approved these changes 2026-05-13 15:52:37 +00:00
sonnet-review-bot left a comment
First-time contributor

Original review

Supersededsee current review for up-to-date findings.

Previous findings (commit 1e0959b0)

Sonnet Review

Summary

The PR correctly implements a restrictive redirect policy for both GitHub and Gitea HTTP clients, rejecting HTTPS→HTTP downgrades and cross-host redirects. The implementation is correct, well-documented, and thoroughly tested. CI passes.

Findings

# Severity File Line Finding
1 [MINOR] gitea/client.go 88 The defaultCheckRedirect function is duplicated verbatim between gitea/client.go and github/client.go. Since these are separate packages this is unavoidable without introducing a shared internal package, but it means any future change must be made in both places. A comment noting the duplication and pointing to the sibling package would help future maintainers. Alternatively, an internal/httppolicy package could house the shared function — though that adds structural complexity for a single function.
2 [NIT] gitea/client.go 93 The comment 'Guard: net/http guarantees len(via) >= 1 but defend against zero-length to avoid panic on index out of range' is accurate but slightly misleading: net/http does guarantee len(via) >= 1 when CheckRedirect is called during an actual redirect, but the test TestDefaultCheckRedirect_EmptyViaAllowed calls the function directly with nil. The guard is appropriate for testability and defensive coding; the comment could be tightened to say 'Guard for direct invocation in tests and any future callers'.

Recommendation

APPROVE — Approve. The security improvement is correct and intentional. The defaultCheckRedirect function logic is sound: the 10-redirect limit check is evaluated before the len(via) == 0 guard (so a 10-element via slice correctly returns an error before the zero-length check runs), the HTTPS→HTTP downgrade check correctly uses prev not req, and the cross-host check uses req.URL.Host != prev.URL.Host which correctly handles ports as part of the host string. Tests are comprehensive, covering all code paths including the defensive nil/empty via case. The duplicate implementation across packages is a minor maintenance concern but acceptable given the project's package structure.


Review by sonnet


Evaluated against 1e0959b0

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/111#pullrequestreview-3305) for up-to-date findings. <details><summary>Previous findings (commit 1e0959b0)</summary> # Sonnet Review ## Summary The PR correctly implements a restrictive redirect policy for both GitHub and Gitea HTTP clients, rejecting HTTPS→HTTP downgrades and cross-host redirects. The implementation is correct, well-documented, and thoroughly tested. CI passes. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `gitea/client.go` | 88 | The `defaultCheckRedirect` function is duplicated verbatim between `gitea/client.go` and `github/client.go`. Since these are separate packages this is unavoidable without introducing a shared internal package, but it means any future change must be made in both places. A comment noting the duplication and pointing to the sibling package would help future maintainers. Alternatively, an `internal/httppolicy` package could house the shared function — though that adds structural complexity for a single function. | | 2 | [NIT] | `gitea/client.go` | 93 | The comment 'Guard: net/http guarantees len(via) >= 1 but defend against zero-length to avoid panic on index out of range' is accurate but slightly misleading: net/http does guarantee `len(via) >= 1` when `CheckRedirect` is called during an actual redirect, but the test `TestDefaultCheckRedirect_EmptyViaAllowed` calls the function directly with `nil`. The guard is appropriate for testability and defensive coding; the comment could be tightened to say 'Guard for direct invocation in tests and any future callers'. | ## Recommendation **APPROVE** — Approve. The security improvement is correct and intentional. The `defaultCheckRedirect` function logic is sound: the 10-redirect limit check is evaluated before the `len(via) == 0` guard (so a 10-element via slice correctly returns an error before the zero-length check runs), the HTTPS→HTTP downgrade check correctly uses `prev` not `req`, and the cross-host check uses `req.URL.Host != prev.URL.Host` which correctly handles ports as part of the host string. Tests are comprehensive, covering all code paths including the defensive nil/empty via case. The duplicate implementation across packages is a minor maintenance concern but acceptable given the project's package structure. --- *Review by sonnet* <!-- review-bot:sonnet --> --- *Evaluated against 1e0959b0* </details> <!-- review-bot:sonnet -->
@@ -81,0 +85,4 @@
func defaultCheckRedirect(req *http.Request, via []*http.Request) error {
if len(via) >= 10 {
return fmt.Errorf("stopped after 10 redirects")
}
First-time contributor

[MINOR] The defaultCheckRedirect function is duplicated verbatim between gitea/client.go and github/client.go. Since these are separate packages this is unavoidable without introducing a shared internal package, but it means any future change must be made in both places. A comment noting the duplication and pointing to the sibling package would help future maintainers. Alternatively, an internal/httppolicy package could house the shared function — though that adds structural complexity for a single function.

**[MINOR]** The `defaultCheckRedirect` function is duplicated verbatim between `gitea/client.go` and `github/client.go`. Since these are separate packages this is unavoidable without introducing a shared internal package, but it means any future change must be made in both places. A comment noting the duplication and pointing to the sibling package would help future maintainers. Alternatively, an `internal/httppolicy` package could house the shared function — though that adds structural complexity for a single function.
@@ -81,0 +90,4 @@
// zero-length to avoid panic on index out of range.
if len(via) == 0 {
return nil
}
First-time contributor

[NIT] The comment 'Guard: net/http guarantees len(via) >= 1 but defend against zero-length to avoid panic on index out of range' is accurate but slightly misleading: net/http does guarantee len(via) >= 1 when CheckRedirect is called during an actual redirect, but the test TestDefaultCheckRedirect_EmptyViaAllowed calls the function directly with nil. The guard is appropriate for testability and defensive coding; the comment could be tightened to say 'Guard for direct invocation in tests and any future callers'.

**[NIT]** The comment 'Guard: net/http guarantees len(via) >= 1 but defend against zero-length to avoid panic on index out of range' is accurate but slightly misleading: net/http does guarantee `len(via) >= 1` when `CheckRedirect` is called during an actual redirect, but the test `TestDefaultCheckRedirect_EmptyViaAllowed` calls the function directly with `nil`. The guard is appropriate for testability and defensive coding; the comment could be tightened to say 'Guard for direct invocation in tests and any future callers'.
gpt-review-bot approved these changes 2026-05-13 15:53:32 +00:00
gpt-review-bot left a comment
First-time contributor

Original review

Supersededsee current review for up-to-date findings.

Previous findings (commit 1e0959b0)

Gpt Review

Summary

The changes correctly add a strict redirect policy to both GitHub and Gitea clients, wire it into constructors and SetHTTPClient(nil), and include comprehensive tests. The implementation follows idiomatic Go patterns and repository conventions, and CI passed.

Findings

# Severity File Line Finding
1 [MINOR] gitea/client.go 78 defaultCheckRedirect duplicates identical logic in github/client.go. Consider consolidating shared redirect policy in an internal package to avoid duplication and keep behavior consistent (see internal/ packages pattern).
2 [NIT] gitea/client.go 98 Error messages include a Unicode arrow ("→"). While harmless, consider using ASCII ("->") for environments that may not render Unicode consistently in logs.
3 [MINOR] github/client.go 107 defaultCheckRedirect duplicates identical logic in gitea/client.go. A shared helper under internal/ could reduce duplication and ensure consistent policy across clients.
4 [NIT] github/client.go 123 Error messages use a Unicode arrow ("→"). Consider ASCII arrows ("->") to maximize compatibility with plain-text log consumers.

Recommendation

APPROVE — Overall, this is a solid, security-improving change that aligns with Go patterns and repository conventions. The redirect policy is clearly defined, properly wired into NewClient and SetHTTPClient(nil), and thoroughly tested, including downgrade, cross-host, same-host allowances, maximum redirect limits, and default restoration. CI has passed. As a small improvement, consider moving the identical defaultCheckRedirect implementation to a shared internal package to avoid duplication and keep behavior consistent across clients. Also consider using ASCII arrows in error messages for broader log compatibility. With these minor suggestions, the PR is good to merge.


Review by gpt


Evaluated against 1e0959b0

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/111#pullrequestreview-3308) for up-to-date findings. <details><summary>Previous findings (commit 1e0959b0)</summary> # Gpt Review ## Summary The changes correctly add a strict redirect policy to both GitHub and Gitea clients, wire it into constructors and SetHTTPClient(nil), and include comprehensive tests. The implementation follows idiomatic Go patterns and repository conventions, and CI passed. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `gitea/client.go` | 78 | defaultCheckRedirect duplicates identical logic in github/client.go. Consider consolidating shared redirect policy in an internal package to avoid duplication and keep behavior consistent (see internal/ packages pattern). | | 2 | [NIT] | `gitea/client.go` | 98 | Error messages include a Unicode arrow ("→"). While harmless, consider using ASCII ("->") for environments that may not render Unicode consistently in logs. | | 3 | [MINOR] | `github/client.go` | 107 | defaultCheckRedirect duplicates identical logic in gitea/client.go. A shared helper under internal/ could reduce duplication and ensure consistent policy across clients. | | 4 | [NIT] | `github/client.go` | 123 | Error messages use a Unicode arrow ("→"). Consider ASCII arrows ("->") to maximize compatibility with plain-text log consumers. | ## Recommendation **APPROVE** — Overall, this is a solid, security-improving change that aligns with Go patterns and repository conventions. The redirect policy is clearly defined, properly wired into NewClient and SetHTTPClient(nil), and thoroughly tested, including downgrade, cross-host, same-host allowances, maximum redirect limits, and default restoration. CI has passed. As a small improvement, consider moving the identical defaultCheckRedirect implementation to a shared internal package to avoid duplication and keep behavior consistent across clients. Also consider using ASCII arrows in error messages for broader log compatibility. With these minor suggestions, the PR is good to merge. --- *Review by gpt* <!-- review-bot:gpt --> --- *Evaluated against 1e0959b0* </details> <!-- review-bot:gpt -->
@@ -78,18 +78,61 @@ type Client struct {
MaxDiffSize int64
First-time contributor

[MINOR] defaultCheckRedirect duplicates identical logic in github/client.go. Consider consolidating shared redirect policy in an internal package to avoid duplication and keep behavior consistent (see internal/ packages pattern).

**[MINOR]** defaultCheckRedirect duplicates identical logic in github/client.go. Consider consolidating shared redirect policy in an internal package to avoid duplication and keep behavior consistent (see internal/ packages pattern).
@@ -81,0 +95,4 @@
// Reject protocol downgrade: HTTPS→HTTP leaks request metadata over plaintext.
if prev.URL.Scheme == "https" && req.URL.Scheme == "http" {
return fmt.Errorf("refusing redirect: HTTPS to HTTP downgrade (%s → %s)", prev.URL.Host, req.URL.Host)
}
First-time contributor

[NIT] Error messages include a Unicode arrow ("→"). While harmless, consider using ASCII ("->") for environments that may not render Unicode consistently in logs.

**[NIT]** Error messages include a Unicode arrow ("→"). While harmless, consider using ASCII ("->") for environments that may not render Unicode consistently in logs.
@@ -107,6 +107,32 @@ type Client struct {
now func() time.Time
First-time contributor

[MINOR] defaultCheckRedirect duplicates identical logic in gitea/client.go. A shared helper under internal/ could reduce duplication and ensure consistent policy across clients.

**[MINOR]** defaultCheckRedirect duplicates identical logic in gitea/client.go. A shared helper under internal/ could reduce duplication and ensure consistent policy across clients.
@@ -110,0 +120,4 @@
if len(via) == 0 {
return nil
}
prev := via[len(via)-1]
First-time contributor

[NIT] Error messages use a Unicode arrow ("→"). Consider ASCII arrows ("->") to maximize compatibility with plain-text log consumers.

**[NIT]** Error messages use a Unicode arrow ("→"). Consider ASCII arrows ("->") to maximize compatibility with plain-text log consumers.
security-review-bot requested review from security-review-bot 2026-05-13 15:54:00 +00:00
security-review-bot approved these changes 2026-05-13 15:54:00 +00:00
Dismissed
security-review-bot left a comment
Collaborator

Original review

Supersededsee current review for up-to-date findings.

Previous findings (commit 1e0959b0)

Security Review

Summary

Security posture is improved by rejecting cross-host redirects and HTTPS→HTTP downgrades, and by ensuring the default policy is restored when resetting the HTTP client. Tests robustly cover the new behavior. One minor hardening opportunity exists around case-insensitive scheme comparison.

Findings

# Severity File Line Finding
1 [MINOR] gitea/client.go 93 Scheme comparison is case-sensitive (prev.URL.Scheme == "https" && req.URL.Scheme == "http"). If a server returns a Location with an uppercase or mixed-case scheme (e.g., "HTTP"), this may bypass the downgrade check depending on parsing behavior. Use strings.EqualFold or normalize schemes to lowercase before comparison.
2 [MINOR] github/client.go 122 Scheme comparison is case-sensitive (prev.URL.Scheme == "https" && req.URL.Scheme == "http"). A mixed/uppercase scheme in a redirect could bypass the downgrade check. Use strings.EqualFold or normalize schemes to lowercase before comparison.

Recommendation

APPROVE — Approve this change. It meaningfully strengthens redirect handling by rejecting cross-host redirects and HTTPS→HTTP downgrades, reducing the risk of credential leakage and untrusted response consumption. For additional hardening, normalize or compare URL schemes case-insensitively (e.g., strings.EqualFold or strings.ToLower) in the downgrade check to avoid edge cases with mixed/uppercase schemes in Location headers. Optionally consider normalizing hosts before comparison for consistency, though the current strictness is safe from a security perspective.


Review by security


Evaluated against 1e0959b0

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/111#pullrequestreview-3307) for up-to-date findings. <details><summary>Previous findings (commit 1e0959b0)</summary> # Security Review ## Summary Security posture is improved by rejecting cross-host redirects and HTTPS→HTTP downgrades, and by ensuring the default policy is restored when resetting the HTTP client. Tests robustly cover the new behavior. One minor hardening opportunity exists around case-insensitive scheme comparison. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `gitea/client.go` | 93 | Scheme comparison is case-sensitive (prev.URL.Scheme == "https" && req.URL.Scheme == "http"). If a server returns a Location with an uppercase or mixed-case scheme (e.g., "HTTP"), this may bypass the downgrade check depending on parsing behavior. Use strings.EqualFold or normalize schemes to lowercase before comparison. | | 2 | [MINOR] | `github/client.go` | 122 | Scheme comparison is case-sensitive (prev.URL.Scheme == "https" && req.URL.Scheme == "http"). A mixed/uppercase scheme in a redirect could bypass the downgrade check. Use strings.EqualFold or normalize schemes to lowercase before comparison. | ## Recommendation **APPROVE** — Approve this change. It meaningfully strengthens redirect handling by rejecting cross-host redirects and HTTPS→HTTP downgrades, reducing the risk of credential leakage and untrusted response consumption. For additional hardening, normalize or compare URL schemes case-insensitively (e.g., strings.EqualFold or strings.ToLower) in the downgrade check to avoid edge cases with mixed/uppercase schemes in Location headers. Optionally consider normalizing hosts before comparison for consistency, though the current strictness is safe from a security perspective. --- *Review by security* <!-- review-bot:security --> --- *Evaluated against 1e0959b0* </details> <!-- review-bot:security -->
@@ -81,0 +90,4 @@
// zero-length to avoid panic on index out of range.
if len(via) == 0 {
return nil
}
Collaborator

[MINOR] Scheme comparison is case-sensitive (prev.URL.Scheme == "https" && req.URL.Scheme == "http"). If a server returns a Location with an uppercase or mixed-case scheme (e.g., "HTTP"), this may bypass the downgrade check depending on parsing behavior. Use strings.EqualFold or normalize schemes to lowercase before comparison.

**[MINOR]** Scheme comparison is case-sensitive (prev.URL.Scheme == "https" && req.URL.Scheme == "http"). If a server returns a Location with an uppercase or mixed-case scheme (e.g., "HTTP"), this may bypass the downgrade check depending on parsing behavior. Use strings.EqualFold or normalize schemes to lowercase before comparison.
security-review-bot marked this conversation as resolved
@@ -110,0 +119,4 @@
// zero-length to avoid panic on index out of range.
if len(via) == 0 {
return nil
}
Collaborator

[MINOR] Scheme comparison is case-sensitive (prev.URL.Scheme == "https" && req.URL.Scheme == "http"). A mixed/uppercase scheme in a redirect could bypass the downgrade check. Use strings.EqualFold or normalize schemes to lowercase before comparison.

**[MINOR]** Scheme comparison is case-sensitive (prev.URL.Scheme == "https" && req.URL.Scheme == "http"). A mixed/uppercase scheme in a redirect could bypass the downgrade check. Use strings.EqualFold or normalize schemes to lowercase before comparison.
security-review-bot marked this conversation as resolved
rodin added the wip label 2026-05-13 16:04:39 +00:00
Author
Owner

Self-Review: PR #111

Self-review against 1e0959b077

Phase 1: Independent Findings

None — diff looks clean. The implementation is correct, well-structured, and thoroughly tested. The defaultCheckRedirect function logic is sound (max redirects → empty-via guard → downgrade check → cross-host check), and the SetHTTPClient(nil) restoration path properly reinstates the secure defaults. Tests cover all branches.

Phase 2: Prior Review Verification

Finding Reviewer Status Notes
Duplicated defaultCheckRedirect across packages sonnet-review, gpt-review STILL PRESENT Accepted by all reviewers — unavoidable without an internal/ package, reasonable for a single function
Comment wording re: len(via) >= 1 guarantee sonnet-review STILL PRESENT NIT — cosmetic comment improvement suggestion
Unicode arrow (→) in error messages gpt-review STILL PRESENT NIT — net/http errors are rarely piped to ASCII-only consumers in this codebase
Case-sensitive scheme comparison security-review STILL PRESENT Go's net/http normalizes URL schemes to lowercase during parsing (url.Parse always lowercases scheme), so this is safe in practice. Hardening with strings.EqualFold would be belt-and-suspenders.

Assessment: Clean

All three reviewers (sonnet, gpt, security) approved the PR. The outstanding findings are all acknowledged NITs or minor hardening suggestions that don't affect correctness or security in practice. The core security improvement (rejecting cross-host redirects and HTTPS→HTTP downgrades) is properly implemented and tested. Ready to proceed.

## Self-Review: PR #111 Self-review against 1e0959b077bd0b06807ea984ba17dd137b8023f9 ### Phase 1: Independent Findings None — diff looks clean. The implementation is correct, well-structured, and thoroughly tested. The `defaultCheckRedirect` function logic is sound (max redirects → empty-via guard → downgrade check → cross-host check), and the `SetHTTPClient(nil)` restoration path properly reinstates the secure defaults. Tests cover all branches. ### Phase 2: Prior Review Verification | Finding | Reviewer | Status | Notes | |---------|----------|--------|-------| | Duplicated `defaultCheckRedirect` across packages | sonnet-review, gpt-review | STILL PRESENT | Accepted by all reviewers — unavoidable without an `internal/` package, reasonable for a single function | | Comment wording re: `len(via) >= 1` guarantee | sonnet-review | STILL PRESENT | NIT — cosmetic comment improvement suggestion | | Unicode arrow (→) in error messages | gpt-review | STILL PRESENT | NIT — `net/http` errors are rarely piped to ASCII-only consumers in this codebase | | Case-sensitive scheme comparison | security-review | STILL PRESENT | Go's `net/http` normalizes URL schemes to lowercase during parsing (`url.Parse` always lowercases scheme), so this is safe in practice. Hardening with `strings.EqualFold` would be belt-and-suspenders. | ### Assessment: ✅ Clean All three reviewers (sonnet, gpt, security) approved the PR. The outstanding findings are all acknowledged NITs or minor hardening suggestions that don't affect correctness or security in practice. The core security improvement (rejecting cross-host redirects and HTTPS→HTTP downgrades) is properly implemented and tested. Ready to proceed.
rodin removed the wip label 2026-05-13 16:06:01 +00:00
rodin added the wip label 2026-05-13 16:25:29 +00:00
Author
Owner

Fix Plan against 1e0959b077:

Findings & Actions

# Finding Reviewer(s) Action
1 defaultCheckRedirect duplicated across packages sonnet-review (#19618), gpt-review (#19625, #19627) PUSH BACK + minor improvement: add cross-reference comment pointing to sibling package
2 Comment wording re: len(via) >= 1 guard sonnet-review (#19619) FIX: update comment to "Guard for direct invocation in tests and any future callers"
3 Case-sensitive scheme comparison security-review (#19631, #19632) PUSH BACK: verified Go's url.Parse normalizes schemes to lowercase (tested with Go 1.26.2 — "HTTP://..." parses to scheme "http"). The comparison is safe.
4 Unicode arrow (→) in error messages gpt-review (#19626, #19628) FIX: replace with -> in both packages

Implementation Details

Fix #2 (comment wording): In both gitea/client.go and github/client.go, change:

// Guard: net/http guarantees len(via) >= 1 but defend against
// zero-length to avoid panic on index out of range.

to:

// Guard for direct invocation in tests and any future callers;
// net/http guarantees len(via) >= 1 during actual redirects.

Fix #4 (Unicode arrows): In both packages, replace with -> in error format strings and comments.

Push-back #1 (duplication): Reply explaining that for a single ~25-line function, an internal/ package adds structural complexity not warranted here. Add a comment noting the sibling implementation.

Push-back #3 (scheme case): Reply with verification that Go's url.Parse normalizes schemes to lowercase, making strings.EqualFold unnecessary.

## Fix Plan against 1e0959b077bd0b06807ea984ba17dd137b8023f9: ### Findings & Actions | # | Finding | Reviewer(s) | Action | |---|---------|-------------|--------| | 1 | `defaultCheckRedirect` duplicated across packages | sonnet-review (#19618), gpt-review (#19625, #19627) | PUSH BACK + minor improvement: add cross-reference comment pointing to sibling package | | 2 | Comment wording re: `len(via) >= 1` guard | sonnet-review (#19619) | FIX: update comment to "Guard for direct invocation in tests and any future callers" | | 3 | Case-sensitive scheme comparison | security-review (#19631, #19632) | PUSH BACK: verified Go's `url.Parse` normalizes schemes to lowercase (tested with Go 1.26.2 — `"HTTP://..."` parses to scheme `"http"`). The comparison is safe. | | 4 | Unicode arrow (→) in error messages | gpt-review (#19626, #19628) | FIX: replace `→` with `->` in both packages | ### Implementation Details **Fix #2 (comment wording):** In both `gitea/client.go` and `github/client.go`, change: ``` // Guard: net/http guarantees len(via) >= 1 but defend against // zero-length to avoid panic on index out of range. ``` to: ``` // Guard for direct invocation in tests and any future callers; // net/http guarantees len(via) >= 1 during actual redirects. ``` **Fix #4 (Unicode arrows):** In both packages, replace `→` with `->` in error format strings and comments. **Push-back #1 (duplication):** Reply explaining that for a single ~25-line function, an `internal/` package adds structural complexity not warranted here. Add a comment noting the sibling implementation. **Push-back #3 (scheme case):** Reply with verification that Go's `url.Parse` normalizes schemes to lowercase, making `strings.EqualFold` unnecessary.
Author
Owner

Acknowledged — I've added cross-reference comments pointing to the sibling implementation in both files. An internal/ package would add structural overhead for a single function (~25 lines), so I'm keeping the duplication with clear comments noting it must be kept in sync. If this function grows or we add more shared HTTP policy, we can extract then.

Acknowledged — I've added cross-reference comments pointing to the sibling implementation in both files. An `internal/` package would add structural overhead for a single function (~25 lines), so I'm keeping the duplication with clear comments noting it must be kept in sync. If this function grows or we add more shared HTTP policy, we can extract then.
Author
Owner

Verified: Go's url.Parse normalizes schemes to lowercase. Tested with Go 1.26.2:

Input: "HTTP://example.com"   -> Parsed scheme: "http"
Input: "HTTPS://example.com"  -> Parsed scheme: "https"
Input: "Http://example.com"   -> Parsed scheme: "http"

Since net/http's redirect handling parses the Location header through url.Parse, the scheme will always be lowercase by the time CheckRedirect is called. The case-sensitive comparison is safe.

Adding strings.EqualFold here would be misleading — it would suggest the normalization doesn't happen, when it does.

Verified: Go's `url.Parse` normalizes schemes to lowercase. Tested with Go 1.26.2: ``` Input: "HTTP://example.com" -> Parsed scheme: "http" Input: "HTTPS://example.com" -> Parsed scheme: "https" Input: "Http://example.com" -> Parsed scheme: "http" ``` Since `net/http`'s redirect handling parses the Location header through `url.Parse`, the scheme will always be lowercase by the time `CheckRedirect` is called. The case-sensitive comparison is safe. Adding `strings.EqualFold` here would be misleading — it would suggest the normalization doesn't happen, when it does.
rodin added 1 commit 2026-05-13 16:28:51 +00:00
fix: address review feedback on redirect policy
PR Ready Gate / clear-labels (pull_request) Successful in 1s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 31s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 52s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m24s
7de6fdd9ec
- Replace Unicode arrows (→) with ASCII (->) in error messages and
  comments for log compatibility (gpt-review NITs #19626, #19628)
- Improve guard comment to clarify it exists for testability, not
  runtime safety (sonnet-review NIT #19619)
- Add cross-reference comments noting intentional duplication between
  gitea/client.go and github/client.go (sonnet-review #19618,
  gpt-review #19625, #19627)

Pushed back on:
- internal/ package for dedup: structural overhead not warranted for
  a single ~25-line function
- strings.EqualFold for scheme: Go's url.Parse normalizes schemes to
  lowercase, making case-insensitive comparison unnecessary
rodin removed the wip label 2026-05-13 16:28:55 +00:00
sonnet-review-bot approved these changes 2026-05-13 16:29:39 +00:00
sonnet-review-bot left a comment
First-time contributor

Sonnet Review

Summary

This PR correctly implements a secure redirect policy for both the GitHub and Gitea HTTP clients. The logic is sound, the duplication is explicitly documented and justified, and the test coverage is thorough. CI passes.

Findings

# Severity File Line Finding
1 [NIT] gitea/client.go 96 The defaultCheckRedirect function uses fmt.Errorf for all error paths, but none of these errors wrap an underlying cause (no %w). Per the error-handling patterns, fmt.Errorf without %w is fine here since these are newly constructed errors with no upstream cause to preserve. This is correct — just noting that errors.New could be used instead for the static messages (e.g. stopped after 10 redirects) since there's no format interpolation needed there, though fmt.Errorf is not wrong.
2 [NIT] gitea/client.go 83 The duplication comment says Changes here must be mirrored there but there is no enforcement mechanism (no shared test, no lint rule, no CI check). This is a maintenance risk. A follow-up to extract this into a shared internal/ package would eliminate the drift risk entirely. The PR description acknowledges this is intentional due to package separation, but the internal/ pattern exists precisely for this use case.

Recommendation

APPROVE — Approve. The security fix is correct and necessary — cross-host redirects were previously only stripping auth headers rather than refusing outright, which is the right hardening. The defaultCheckRedirect logic correctly handles all edge cases: HTTPS→HTTP downgrade, cross-host, too many redirects, and the empty-via guard. Tests are comprehensive and idiomatic. The duplication between packages is a mild design concern but is explicitly documented and the packages are intentionally separate. A future refactor to extract this into an internal/redirectpolicy package would be cleaner, but is not required for this PR to be safe and correct.


Review by sonnet


Evaluated against 7de6fdd9

# Sonnet Review ## Summary This PR correctly implements a secure redirect policy for both the GitHub and Gitea HTTP clients. The logic is sound, the duplication is explicitly documented and justified, and the test coverage is thorough. CI passes. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [NIT] | `gitea/client.go` | 96 | The `defaultCheckRedirect` function uses `fmt.Errorf` for all error paths, but none of these errors wrap an underlying cause (no `%w`). Per the error-handling patterns, `fmt.Errorf` without `%w` is fine here since these are newly constructed errors with no upstream cause to preserve. This is correct — just noting that `errors.New` could be used instead for the static messages (e.g. `stopped after 10 redirects`) since there's no format interpolation needed there, though `fmt.Errorf` is not wrong. | | 2 | [NIT] | `gitea/client.go` | 83 | The duplication comment says `Changes here must be mirrored there` but there is no enforcement mechanism (no shared test, no lint rule, no CI check). This is a maintenance risk. A follow-up to extract this into a shared `internal/` package would eliminate the drift risk entirely. The PR description acknowledges this is intentional due to package separation, but the `internal/` pattern exists precisely for this use case. | ## Recommendation **APPROVE** — Approve. The security fix is correct and necessary — cross-host redirects were previously only stripping auth headers rather than refusing outright, which is the right hardening. The `defaultCheckRedirect` logic correctly handles all edge cases: HTTPS→HTTP downgrade, cross-host, too many redirects, and the empty-via guard. Tests are comprehensive and idiomatic. The duplication between packages is a mild design concern but is explicitly documented and the packages are intentionally separate. A future refactor to extract this into an `internal/redirectpolicy` package would be cleaner, but is not required for this PR to be safe and correct. --- *Review by sonnet* <!-- review-bot:sonnet --> --- *Evaluated against 7de6fdd9*
@@ -80,1 +80,4 @@
// defaultCheckRedirect is the redirect policy used by NewClient.
// NOTE: This function is intentionally duplicated in github/client.go (and vice versa)
// because the packages are separate. Changes here must be mirrored there.
First-time contributor

[NIT] The duplication comment says Changes here must be mirrored there but there is no enforcement mechanism (no shared test, no lint rule, no CI check). This is a maintenance risk. A follow-up to extract this into a shared internal/ package would eliminate the drift risk entirely. The PR description acknowledges this is intentional due to package separation, but the internal/ pattern exists precisely for this use case.

**[NIT]** The duplication comment says `Changes here must be mirrored there` but there is no enforcement mechanism (no shared test, no lint rule, no CI check). This is a maintenance risk. A follow-up to extract this into a shared `internal/` package would eliminate the drift risk entirely. The PR description acknowledges this is intentional due to package separation, but the `internal/` pattern exists precisely for this use case.
@@ -81,0 +93,4 @@
if len(via) == 0 {
return nil
}
prev := via[len(via)-1]
First-time contributor

[NIT] The defaultCheckRedirect function uses fmt.Errorf for all error paths, but none of these errors wrap an underlying cause (no %w). Per the error-handling patterns, fmt.Errorf without %w is fine here since these are newly constructed errors with no upstream cause to preserve. This is correct — just noting that errors.New could be used instead for the static messages (e.g. stopped after 10 redirects) since there's no format interpolation needed there, though fmt.Errorf is not wrong.

**[NIT]** The `defaultCheckRedirect` function uses `fmt.Errorf` for all error paths, but none of these errors wrap an underlying cause (no `%w`). Per the error-handling patterns, `fmt.Errorf` without `%w` is fine here since these are newly constructed errors with no upstream cause to preserve. This is correct — just noting that `errors.New` could be used instead for the static messages (e.g. `stopped after 10 redirects`) since there's no format interpolation needed there, though `fmt.Errorf` is not wrong.
security-review-bot requested review from security-review-bot 2026-05-13 16:30:01 +00:00
security-review-bot approved these changes 2026-05-13 16:30:02 +00:00
security-review-bot left a comment
Collaborator

Security Review

Summary

Security posture is improved by enforcing a strict redirect policy (rejecting cross-host redirects and HTTPS→HTTP downgrades) in both GitHub and Gitea clients. The implementation is correct, follows secure defaults, and includes thorough tests.

Recommendation

APPROVE — Approve as-is. The new CheckRedirect policy prevents credential leakage and SSRF-like redirect abuse by failing fast on cross-host and HTTPS→HTTP redirects, while retaining safe same-host behavior and a redirect cap. Tests cover key scenarios, and CI has passed.


Review by security


Evaluated against 7de6fdd9

# Security Review ## Summary Security posture is improved by enforcing a strict redirect policy (rejecting cross-host redirects and HTTPS→HTTP downgrades) in both GitHub and Gitea clients. The implementation is correct, follows secure defaults, and includes thorough tests. ## Recommendation **APPROVE** — Approve as-is. The new CheckRedirect policy prevents credential leakage and SSRF-like redirect abuse by failing fast on cross-host and HTTPS→HTTP redirects, while retaining safe same-host behavior and a redirect cap. Tests cover key scenarios, and CI has passed. --- *Review by security* <!-- review-bot:security --> --- *Evaluated against 7de6fdd9*
gpt-review-bot approved these changes 2026-05-13 16:30:33 +00:00
gpt-review-bot left a comment
First-time contributor

Gpt Review

Summary

The changes introduce a strict, security-minded redirect policy for both GitHub and Gitea clients and wire it into constructors and SetHTTPClient(nil). The implementation is correct, idiomatic, covered by thorough tests, and aligns with the stated security rationale. CI passed.

Findings

# Severity File Line Finding
1 [NIT] gitea/client.go 78 defaultCheckRedirect compares URL schemes and hosts with case-sensitive equality. While practical, schemes and hosts are case-insensitive by spec; using strings.EqualFold for scheme/host comparisons would make this more robust in edge cases.
2 [NIT] github/client.go 107 defaultCheckRedirect uses case-sensitive comparisons for scheme and host. Consider strings.EqualFold to be resilient to unusual case in redirect URLs.

Recommendation

APPROVE — Overall, this PR is well-implemented and improves security by rejecting cross-host redirects and HTTPS→HTTP downgrades. The redirect policy is correctly applied in both clients and restored in SetHTTPClient(nil), and tests comprehensively verify behaviors. As a minor robustness improvement, consider using case-insensitive comparisons (strings.EqualFold) for URL scheme and host in defaultCheckRedirect to handle atypical casing in Location headers. Otherwise, this is ready to merge.


Review by gpt


Evaluated against 7de6fdd9

# Gpt Review ## Summary The changes introduce a strict, security-minded redirect policy for both GitHub and Gitea clients and wire it into constructors and SetHTTPClient(nil). The implementation is correct, idiomatic, covered by thorough tests, and aligns with the stated security rationale. CI passed. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [NIT] | `gitea/client.go` | 78 | defaultCheckRedirect compares URL schemes and hosts with case-sensitive equality. While practical, schemes and hosts are case-insensitive by spec; using strings.EqualFold for scheme/host comparisons would make this more robust in edge cases. | | 2 | [NIT] | `github/client.go` | 107 | defaultCheckRedirect uses case-sensitive comparisons for scheme and host. Consider strings.EqualFold to be resilient to unusual case in redirect URLs. | ## Recommendation **APPROVE** — Overall, this PR is well-implemented and improves security by rejecting cross-host redirects and HTTPS→HTTP downgrades. The redirect policy is correctly applied in both clients and restored in SetHTTPClient(nil), and tests comprehensively verify behaviors. As a minor robustness improvement, consider using case-insensitive comparisons (strings.EqualFold) for URL scheme and host in defaultCheckRedirect to handle atypical casing in Location headers. Otherwise, this is ready to merge. --- *Review by gpt* <!-- review-bot:gpt --> --- *Evaluated against 7de6fdd9*
@@ -78,18 +78,63 @@ type Client struct {
MaxDiffSize int64
First-time contributor

[NIT] defaultCheckRedirect compares URL schemes and hosts with case-sensitive equality. While practical, schemes and hosts are case-insensitive by spec; using strings.EqualFold for scheme/host comparisons would make this more robust in edge cases.

**[NIT]** defaultCheckRedirect compares URL schemes and hosts with case-sensitive equality. While practical, schemes and hosts are case-insensitive by spec; using strings.EqualFold for scheme/host comparisons would make this more robust in edge cases.
@@ -107,6 +107,34 @@ type Client struct {
now func() time.Time
First-time contributor

[NIT] defaultCheckRedirect uses case-sensitive comparisons for scheme and host. Consider strings.EqualFold to be resilient to unusual case in redirect URLs.

**[NIT]** defaultCheckRedirect uses case-sensitive comparisons for scheme and host. Consider strings.EqualFold to be resilient to unusual case in redirect URLs.
rodin added the wip label 2026-05-13 16:35:06 +00:00
Author
Owner

Self-Review: PR #111

Self-review against 7de6fdd9ec

Phase 1: Independent Findings

None — diff looks clean. The defaultCheckRedirect implementation is correct and well-structured:

  • Max-redirects check (10) fires before the empty-via guard, so it works properly
  • HTTPS→HTTP downgrade check correctly uses prev.URL.Scheme vs req.URL.Scheme
  • Cross-host check uses req.URL.Host != prev.URL.Host which handles ports correctly
  • SetHTTPClient(nil) properly restores secure defaults with both timeout and CheckRedirect
  • Tests cover all code paths including edge cases (empty via, max redirects, same-host allowance)
  • Comments cross-reference the sibling package implementation

Phase 2: Prior Review Verification

Finding Reviewer Status Notes
Duplicated defaultCheckRedirect across packages sonnet-review (round 1), gpt-review (round 1) RESOLVED Cross-reference comments added, push-back reply posted explaining rationale
Comment wording re: len(via) >= 1 guard sonnet-review (round 1) RESOLVED Comment now reads "Guard for direct invocation in tests and any future callers; net/http guarantees len(via) >= 1 during actual redirects"
Unicode arrow (→) in error messages gpt-review (round 1) RESOLVED Error messages now use ASCII ->
Case-sensitive scheme comparison security-review (round 1), gpt-review (round 2) RESOLVED Pushed back with verification that Go's url.Parse normalizes schemes to lowercase; security-review dismissed
errors.New for static messages sonnet-review (round 2) RESOLVED (NIT accepted) Using fmt.Errorf for consistency across all error paths is fine; no action needed
No enforcement mechanism for duplication sync sonnet-review (round 2) RESOLVED (NIT accepted) Acknowledged maintenance risk; comments serve as sufficient guidance for this small function

Assessment: Clean

All reviewers approved on current HEAD (7de6fdd9). All actionable findings from the first round have been addressed (comment wording fix, Unicode→ASCII arrows, cross-reference comments). Remaining findings from the second round are NITs that don't affect correctness or security. The PR is ready.

## Self-Review: PR #111 Self-review against 7de6fdd9ec7db7441bc1bbd58dafabc6a4ad4aad ### Phase 1: Independent Findings None — diff looks clean. The `defaultCheckRedirect` implementation is correct and well-structured: - Max-redirects check (10) fires before the empty-via guard, so it works properly - HTTPS→HTTP downgrade check correctly uses `prev.URL.Scheme` vs `req.URL.Scheme` - Cross-host check uses `req.URL.Host != prev.URL.Host` which handles ports correctly - `SetHTTPClient(nil)` properly restores secure defaults with both timeout and CheckRedirect - Tests cover all code paths including edge cases (empty via, max redirects, same-host allowance) - Comments cross-reference the sibling package implementation ### Phase 2: Prior Review Verification | Finding | Reviewer | Status | Notes | |---------|----------|--------|-------| | Duplicated `defaultCheckRedirect` across packages | sonnet-review (round 1), gpt-review (round 1) | RESOLVED | Cross-reference comments added, push-back reply posted explaining rationale | | Comment wording re: `len(via) >= 1` guard | sonnet-review (round 1) | RESOLVED | Comment now reads "Guard for direct invocation in tests and any future callers; net/http guarantees len(via) >= 1 during actual redirects" | | Unicode arrow (→) in error messages | gpt-review (round 1) | RESOLVED | Error messages now use ASCII `->` | | Case-sensitive scheme comparison | security-review (round 1), gpt-review (round 2) | RESOLVED | Pushed back with verification that Go's url.Parse normalizes schemes to lowercase; security-review dismissed | | `errors.New` for static messages | sonnet-review (round 2) | RESOLVED (NIT accepted) | Using `fmt.Errorf` for consistency across all error paths is fine; no action needed | | No enforcement mechanism for duplication sync | sonnet-review (round 2) | RESOLVED (NIT accepted) | Acknowledged maintenance risk; comments serve as sufficient guidance for this small function | ### Assessment: ✅ Clean All reviewers approved on current HEAD (7de6fdd9). All actionable findings from the first round have been addressed (comment wording fix, Unicode→ASCII arrows, cross-reference comments). Remaining findings from the second round are NITs that don't affect correctness or security. The PR is ready.
rodin removed the wip label 2026-05-13 16:36:26 +00:00
aweiker was assigned by rodin 2026-05-13 16:56:34 +00:00
rodin added the ready label 2026-05-13 16:56:34 +00:00
aweiker approved these changes 2026-05-13 16:58:46 +00:00
aweiker merged commit dc2e1ca5de into main 2026-05-13 16:58:49 +00:00
Sign in to join this conversation.
5 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: rodin/review-bot#111