PostReview, DeleteReview, and RequestReviewer were calling c.httpClient.Do
directly, bypassing the scheme check in doRequest that rejects http:// URLs
unless AllowInsecureHTTP is explicitly enabled.
Introduce doRequestWithBody(ctx, method, url, body) with the same HTTPS guard,
and refactor all three write methods to use it. This ensures tokens are never
sent over plaintext regardless of which API path is exercised.
Add scheme validation tests for each method.
Implement the higher-level GitHub API methods that were TODO since
issue #120. The github package now provides:
- GetPullRequest: PR metadata (title, body, head SHA/ref, draft)
- GetPullRequestDiff: unified diff via Accept: application/vnd.github.diff
- GetPullRequestFiles: changed files list (paginated, 100/page)
- GetCommitStatuses: CI statuses (GitHub uses 'state' field, normalized)
- GetFileContent: file content with base64 decode (strips embedded newlines)
- GetFileContentRef: file at a specific ref
- ListContents: directory listing or single-file normalization
- GetAllFilesInPath: recursive file fetching
- PostReview: submit review with event/body/commit/inline comments
- ListReviews: list PR reviews (paginated)
- DeleteReview: delete review (GitHub only allows PENDING deletion)
- GetAuthenticatedUser: returns login of the authed user
- RequestReviewer: add a user as requested reviewer
API types added: PullRequest, CommitStatus, ChangedFile, ReviewComment,
Review, ContentEntry.
Notable edge cases handled:
- GitHub embeds newlines in base64 content; stripped before decode
- GetFileContent returns error for non-file paths (type=dir)
- ListContents normalizes single-file response to a slice
- DeleteReview documents GitHub's PENDING-only constraint
Removes TODO comment from baseURL field (now consumed by all methods).
Closes part of #130.
Co-authored-by: Rodin <rodin@forgedthought.ai>
- Fix AllowInsecureHTTP doc comment: say '_test.go file in the same
package' instead of 'export_test.go' (MINOR #1)
- Remove dead u.Fragment = "" from redactURL: HTTP requests never
carry fragments over the wire per RFC 7230 §5.1 (MINOR #2)
- Use 127.0.0.1:1 in scheme-rejection tests to make intent clearer
that no network call should occur (NIT #3)
- Restore timer.Stop() no-op in case <-timer.C for symmetry with ctx.Done
- Add missing blank line between TestNoInsecureOption_RejectsHTTP and
TestNoInsecureOption_RejectsUppercaseHTTP
- Remove double blank line before TestAllowInsecureHTTP_WithoutEnvVar_Rejected
Resolves review comments from sonnet-review-bot on PR #113.
Address review feedback on PR #113:
- MAJOR (both reviews): Replace strings.HasPrefix(reqURL, "http://")
with url.Parse + strings.EqualFold for case-insensitive scheme
comparison per RFC 3986. Prevents bypass via HTTP:// or Http://.
- MINOR (security): Enhance redactURL to strip userinfo component
(user:pass@host) in addition to query params, preventing credential
leakage in error messages and logs.
- NIT (gpt): Remove redundant timer.Stop() after timer.C fires —
it's a no-op and the comment was misleading.
- Add tests for uppercase/mixed-case HTTP scheme rejection and
userinfo redaction.
Three-layer defense for the AllowInsecureHTTP client option:
1. Environment gate: AllowInsecureHTTP() requires REVIEW_BOT_ALLOW_INSECURE=1
env var. Without it, the option is silently ignored with a slog.Warn.
2. Warning log on activation: When the env gate IS satisfied, a slog.Warn
fires at client construction time so operators notice in production logs.
3. Test bypass: AllowInsecureHTTPForTest() skips the env gate entirely,
keeping test code clean (no t.Setenv needed in every test).
Additionally, doRequest now rejects HTTP URLs unless allowInsecureHTTP is set
on the client, providing defense-in-depth against credential leakage.
Closes#96
- 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
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
- Add timer.Stop() to the timer.C branch to prevent timer leak on the
normal path (previously only called in ctx.Done branch)
- Rename struct field 'http' to 'httpClient' to avoid shadowing the
net/http import
Addresses self-review findings on PR #110.
- Add TODO comment on unused baseURL field explaining its intended
future use by higher-level exported methods
- Rewrite TestDoRequest_ContextCanceled to actually exercise the
timer-cancel path in the retry select (goroutine cancels context
while client is blocked in timer.C select)
- Change Retry-After: 1 to Retry-After: 0 in IntegerSeconds test
to avoid unnecessary 1s sleep during test runs
- Add doc note on SetRetryBackoff explaining that an empty non-nil
slice silently drops Retry-After delays
- Fix data race: copy retryBackoff slice per-request to prevent
concurrent doRequest calls from racing on shared state
- Fix parseRetryAfter: trim whitespace before parsing for robustness
- Fix parseRetryAfter: treat delta-seconds of 0 as valid per RFC 7231
- Add bounded read on success path (10 MB limit) for defense-in-depth
- Clean up TestDoRequest_429_RetryAfter_IntegerSeconds: remove dead
code block and commented-out reasoning
- Fix import ordering: context before errors (goimports compliance)
Implement the github package client with Retry-After header parsing that
supports both integer seconds (e.g. "Retry-After: 120") and HTTP-date
format (e.g. "Retry-After: Thu, 01 Dec 2025 16:00:00 GMT") per RFC 7231
§7.1.3.
Key design decisions:
- Use http.ParseTime which handles RFC 1123, RFC 850, and ASCTIME formats
- Cap maximum retry delay at 60s (maxRetryAfter) to prevent stalling
- If HTTP-date is in the past, use delay of 0 (retry immediately)
- Inject time.Now via c.now field for deterministic testing
Closes#94
claw
Rodin