-
Notifications
You must be signed in to change notification settings - Fork 178
fix(update): authenticate GitHub API requests to fix 403 Forbidden #504
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
0579627
d9faab6
8852aec
73eaf70
57161c7
7c6aa92
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,128 @@ | ||
| package update | ||
|
|
||
| import ( | ||
| "context" | ||
| "io" | ||
| "net/http" | ||
| "strings" | ||
| "testing" | ||
| ) | ||
|
|
||
| // authTransport intercepts HTTP requests and records the Authorization header | ||
| // so tests can verify whether fetchRelease sent credentials. | ||
| type authTransport struct { | ||
| t *testing.T | ||
| receivedAuth string | ||
| responseBody string | ||
| responseCode int | ||
| } | ||
|
|
||
| func (at *authTransport) RoundTrip(req *http.Request) (*http.Response, error) { | ||
| at.receivedAuth = req.Header.Get("Authorization") | ||
| at.t.Logf("authTransport: %s %s → Authorization: %q", req.Method, req.URL.String(), at.receivedAuth) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win Redact
Proposed redaction- at.t.Logf("authTransport: %s %s → Authorization: %q", req.Method, req.URL.String(), at.receivedAuth)
+ at.t.Logf("authTransport: %s %s → authorization_present=%t", req.Method, req.URL.String(), at.receivedAuth != "")As per path instructions, Also applies to: 51-51, 70-70, 89-89, 107-107, 126-126 🤖 Prompt for AI AgentsSource: Path instructions |
||
| if at.responseBody == "" { | ||
| at.responseBody = `{"tag_name":"v0.2.0","html_url":"https://example.test/release","assets":[{"name":"zero-v0.2.0-linux-x64.tar.gz","browser_download_url":"https://example.test/zero-v0.2.0-linux-x64.tar.gz"},{"name":"zero-v0.2.0-linux-x64.tar.gz.sha256","browser_download_url":"https://example.test/zero-v0.2.0-linux-x64.tar.gz.sha256"}]}` | ||
| } | ||
| if at.responseCode == 0 { | ||
| at.responseCode = 200 | ||
| } | ||
| return &http.Response{ | ||
| StatusCode: at.responseCode, | ||
| Body: io.NopCloser(strings.NewReader(at.responseBody)), | ||
| Header: make(http.Header), | ||
| }, nil | ||
| } | ||
|
|
||
| func TestFetchReleaseSendsAuthToHttpsGithub(t *testing.T) { | ||
| // ZERO_GITHUB_TOKEN → sent to https://api.github.com | ||
| at := &authTransport{t: t} | ||
| oldClient := http.DefaultClient | ||
| http.DefaultClient = &http.Client{Transport: at} | ||
| t.Cleanup(func() { http.DefaultClient = oldClient }) | ||
|
|
||
| t.Setenv(EnvUpdateToken, "zero_token") | ||
| t.Setenv("GITHUB_TOKEN", "github_fallback") | ||
|
|
||
| _, err := fetchRelease(context.Background(), "https://api.github.com/repos/Gitlawb/zero/releases/latest") | ||
| if err != nil { | ||
| t.Logf("fetchRelease returned error (expected for fake response): %v", err) | ||
| } | ||
| if at.receivedAuth != "Bearer zero_token" { | ||
| t.Fatalf("ZERO_GITHUB_TOKEN should take precedence: got Authorization %q, want %q", at.receivedAuth, "Bearer zero_token") | ||
| } | ||
| } | ||
|
|
||
| func TestFetchReleaseFallsBackToGithubToken(t *testing.T) { | ||
| // Only GITHUB_TOKEN set → sent to https://api.github.com | ||
| at := &authTransport{t: t} | ||
| oldClient := http.DefaultClient | ||
| http.DefaultClient = &http.Client{Transport: at} | ||
| t.Cleanup(func() { http.DefaultClient = oldClient }) | ||
|
|
||
| t.Setenv("GITHUB_TOKEN", "fallback_token") | ||
| t.Setenv(EnvUpdateToken, "") // clear ambient precedence var | ||
|
|
||
| _, err := fetchRelease(context.Background(), "https://api.github.com/repos/Gitlawb/zero/releases/latest") | ||
| if err != nil { | ||
| t.Logf("fetchRelease returned error (expected for fake response): %v", err) | ||
| } | ||
| if at.receivedAuth != "Bearer fallback_token" { | ||
| t.Fatalf("GITHUB_TOKEN should be used as fallback: got Authorization %q, want %q", at.receivedAuth, "Bearer fallback_token") | ||
| } | ||
| } | ||
|
|
||
| func TestFetchReleaseNoAuthToCustomEndpoint(t *testing.T) { | ||
| // Token set but endpoint is custom host → no auth sent | ||
| at := &authTransport{t: t} | ||
| oldClient := http.DefaultClient | ||
| http.DefaultClient = &http.Client{Transport: at} | ||
| t.Cleanup(func() { http.DefaultClient = oldClient }) | ||
|
|
||
| t.Setenv(EnvUpdateToken, "secret") | ||
| t.Setenv("GITHUB_TOKEN", "fallback") | ||
|
|
||
| _, err := fetchRelease(context.Background(), "https://internal.mirror.example.com/releases/latest") | ||
| if err != nil { | ||
| t.Logf("fetchRelease returned error (expected for fake response): %v", err) | ||
| } | ||
| if at.receivedAuth != "" { | ||
| t.Fatalf("auth should not be sent to custom endpoint: got Authorization %q", at.receivedAuth) | ||
| } | ||
| } | ||
|
|
||
| func TestFetchReleaseNoAuthToHttpGithub(t *testing.T) { | ||
| // Token set but scheme is HTTP → no auth sent even to api.github.com | ||
| at := &authTransport{t: t} | ||
| oldClient := http.DefaultClient | ||
| http.DefaultClient = &http.Client{Transport: at} | ||
| t.Cleanup(func() { http.DefaultClient = oldClient }) | ||
|
|
||
| t.Setenv(EnvUpdateToken, "secret") | ||
|
|
||
| _, err := fetchRelease(context.Background(), "http://api.github.com/repos/Gitlawb/zero/releases/latest") | ||
| if err != nil { | ||
| t.Logf("fetchRelease returned error (expected for fake response): %v", err) | ||
| } | ||
| if at.receivedAuth != "" { | ||
| t.Fatalf("auth should not be sent over HTTP: got Authorization %q", at.receivedAuth) | ||
| } | ||
| } | ||
|
|
||
| func TestFetchReleaseNoAuthWhenTokensNotSet(t *testing.T) { | ||
| // No env vars set → no auth sent (authenticated fallback) | ||
| at := &authTransport{t: t} | ||
| oldClient := http.DefaultClient | ||
| http.DefaultClient = &http.Client{Transport: at} | ||
| t.Cleanup(func() { http.DefaultClient = oldClient }) | ||
|
|
||
| t.Setenv(EnvUpdateToken, "") | ||
| t.Setenv("GITHUB_TOKEN", "") | ||
|
|
||
| _, err := fetchRelease(context.Background(), "https://api.github.com/repos/Gitlawb/zero/releases/latest") | ||
| if err != nil { | ||
| t.Logf("fetchRelease returned error (expected for fake response): %v", err) | ||
| } | ||
| if at.receivedAuth != "" { | ||
| t.Fatalf("no auth should be sent when no tokens set: got Authorization %q", at.receivedAuth) | ||
| } | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert
ZERO_UPDATE_RELEASE_URLin the help regression.The help text adds this environment variable, but the assertion list does not check it. Add
ZERO_UPDATE_RELEASE_URLso the test detects removal of the documented release URL override.Proposed test update
As per coding guidelines,
**/*_test.gorequires a regression test for every behavior or security-boundary change.📝 Committable suggestion
🤖 Prompt for AI Agents
Source: Coding guidelines