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.
## 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
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
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.
~~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 -->
[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.
[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
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.
~~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 -->
[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).
[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.
[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.
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.
~~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 -->
[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
[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
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
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.
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.
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.
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.
- 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
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.
# 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*
[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.
[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 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.
# 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
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.
# 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*
[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.
[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
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 rodin2026-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 main2026-05-13 16:58:49 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
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.godefaultCheckRedirectfunction that rejects:defaultCheckRedirectintoNewClientSetHTTPClient(nil)to restore the policygitea/client.godefaultCheckRedirectfunctionNewClientSetHTTPClient(nil)to restore the policyTests (
github/client_test.go,gitea/client_test.go)TestNewClient_HasCheckRedirect— verify policy is wiredTestDefaultCheckRedirect_RejectsHTTPSToHTTP— downgrade rejectedTestDefaultCheckRedirect_RejectsCrossHost— cross-host rejectedTestDefaultCheckRedirect_AllowsSameHost— same-host allowed, auth preservedTestDefaultCheckRedirect_AllowsSameHostHTTPToHTTP— no false positive on HTTP→HTTP same-hostTestDefaultCheckRedirect_RejectsTooManyRedirects— 10+ redirects rejectedTestDefaultCheckRedirect_EmptyViaAllowed— defensive nil/empty guardTestSetHTTPClient_NilRestoresDefault— nil restore includes policySecurity 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.ClientviaSetHTTPClient.Closes #95
Original reviewSuperseded — see 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
gitea/client.godefaultCheckRedirectfunction is duplicated verbatim betweengitea/client.goandgithub/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, aninternal/httppolicypackage could house the shared function — though that adds structural complexity for a single function.gitea/client.golen(via) >= 1whenCheckRedirectis called during an actual redirect, but the testTestDefaultCheckRedirect_EmptyViaAllowedcalls the function directly withnil. 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
defaultCheckRedirectfunction logic is sound: the 10-redirect limit check is evaluated before thelen(via) == 0guard (so a 10-element via slice correctly returns an error before the zero-length check runs), the HTTPS→HTTP downgrade check correctly usesprevnotreq, and the cross-host check usesreq.URL.Host != prev.URL.Hostwhich 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@@ -81,0 +85,4 @@func defaultCheckRedirect(req *http.Request, via []*http.Request) error {if len(via) >= 10 {return fmt.Errorf("stopped after 10 redirects")}[MINOR] The
defaultCheckRedirectfunction is duplicated verbatim betweengitea/client.goandgithub/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, aninternal/httppolicypackage 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}[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) >= 1whenCheckRedirectis called during an actual redirect, but the testTestDefaultCheckRedirect_EmptyViaAllowedcalls the function directly withnil. 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'.Original reviewSuperseded — see 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
gitea/client.gogitea/client.gogithub/client.gogithub/client.goRecommendation
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@@ -78,18 +78,61 @@ type Client struct {MaxDiffSize int64[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)}[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[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][NIT] Error messages use a Unicode arrow ("→"). Consider ASCII arrows ("->") to maximize compatibility with plain-text log consumers.
Original reviewSuperseded — see 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
gitea/client.gogithub/client.goRecommendation
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@@ -81,0 +90,4 @@// zero-length to avoid panic on index out of range.if len(via) == 0 {return nil}[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.
@@ -110,0 +119,4 @@// zero-length to avoid panic on index out of range.if len(via) == 0 {return nil}[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.
Self-Review: PR #111
Self-review against
1e0959b077Phase 1: Independent Findings
None — diff looks clean. The implementation is correct, well-structured, and thoroughly tested. The
defaultCheckRedirectfunction logic is sound (max redirects → empty-via guard → downgrade check → cross-host check), and theSetHTTPClient(nil)restoration path properly reinstates the secure defaults. Tests cover all branches.Phase 2: Prior Review Verification
defaultCheckRedirectacross packagesinternal/package, reasonable for a single functionlen(via) >= 1guaranteenet/httperrors are rarely piped to ASCII-only consumers in this codebasenet/httpnormalizes URL schemes to lowercase during parsing (url.Parsealways lowercases scheme), so this is safe in practice. Hardening withstrings.EqualFoldwould 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.
Fix Plan against
1e0959b077:Findings & Actions
defaultCheckRedirectduplicated across packageslen(via) >= 1guardurl.Parsenormalizes schemes to lowercase (tested with Go 1.26.2 —"HTTP://..."parses to scheme"http"). The comparison is safe.→with->in both packagesImplementation Details
Fix #2 (comment wording): In both
gitea/client.goandgithub/client.go, change:to:
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.Parsenormalizes schemes to lowercase, makingstrings.EqualFoldunnecessary.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.Verified: Go's
url.Parsenormalizes schemes to lowercase. Tested with Go 1.26.2:Since
net/http's redirect handling parses the Location header throughurl.Parse, the scheme will always be lowercase by the timeCheckRedirectis called. The case-sensitive comparison is safe.Adding
strings.EqualFoldhere would be misleading — it would suggest the normalization doesn't happen, when it does.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
gitea/client.godefaultCheckRedirectfunction usesfmt.Errorffor all error paths, but none of these errors wrap an underlying cause (no%w). Per the error-handling patterns,fmt.Errorfwithout%wis fine here since these are newly constructed errors with no upstream cause to preserve. This is correct — just noting thaterrors.Newcould be used instead for the static messages (e.g.stopped after 10 redirects) since there's no format interpolation needed there, thoughfmt.Errorfis not wrong.gitea/client.goChanges here must be mirrored therebut 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 sharedinternal/package would eliminate the drift risk entirely. The PR description acknowledges this is intentional due to package separation, but theinternal/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
defaultCheckRedirectlogic 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 aninternal/redirectpolicypackage would be cleaner, but is not required for this PR to be safe and correct.Review by 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.[NIT] The duplication comment says
Changes here must be mirrored therebut 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 sharedinternal/package would eliminate the drift risk entirely. The PR description acknowledges this is intentional due to package separation, but theinternal/pattern exists precisely for this use case.@@ -81,0 +93,4 @@if len(via) == 0 {return nil}prev := via[len(via)-1][NIT] The
defaultCheckRedirectfunction usesfmt.Errorffor all error paths, but none of these errors wrap an underlying cause (no%w). Per the error-handling patterns,fmt.Errorfwithout%wis fine here since these are newly constructed errors with no upstream cause to preserve. This is correct — just noting thaterrors.Newcould be used instead for the static messages (e.g.stopped after 10 redirects) since there's no format interpolation needed there, thoughfmt.Errorfis not wrong.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
7de6fdd9Gpt 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
gitea/client.gogithub/client.goRecommendation
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@@ -78,18 +78,63 @@ type Client struct {MaxDiffSize int64[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[NIT] defaultCheckRedirect uses case-sensitive comparisons for scheme and host. Consider strings.EqualFold to be resilient to unusual case in redirect URLs.
Self-Review: PR #111
Self-review against
7de6fdd9ecPhase 1: Independent Findings
None — diff looks clean. The
defaultCheckRedirectimplementation is correct and well-structured:prev.URL.Schemevsreq.URL.Schemereq.URL.Host != prev.URL.Hostwhich handles ports correctlySetHTTPClient(nil)properly restores secure defaults with both timeout and CheckRedirectPhase 2: Prior Review Verification
defaultCheckRedirectacross packageslen(via) >= 1guard->errors.Newfor static messagesfmt.Errorffor consistency across all error paths is fine; no action neededAssessment: ✅ 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.