fix(update): authenticate GitHub API requests to fix 403 Forbidden#504
fix(update): authenticate GitHub API requests to fix 403 Forbidden#504PatrickNoFilter wants to merge 6 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe update checker now prefers ChangesUpdate check token selection
Ignore sandbox path
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant fetchRelease
participant Env
participant GitHubAPI
fetchRelease->>Env: read ZERO_GITHUB_TOKEN
alt ZERO_GITHUB_TOKEN non-empty
Env-->>fetchRelease: token value
else fallback
fetchRelease->>Env: read GITHUB_TOKEN
Env-->>fetchRelease: token value
end
fetchRelease->>fetchRelease: require https and api.github.com
fetchRelease->>GitHubAPI: request with Authorization header when allowed
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/tui/mouse.go (1)
15-23: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
CONTAINERcheck is overly broad for a Termux-specific fallback.Podman/toolbox sets
container=podman/ociand systemd-nspawn setscontainer=...on plain desktop/CI Linux boxes that have fully workingAllMotionsupport. Gating the hover-highlight feature off for every container runtime (not just Termux/proot on Android) degrades UX for unrelated users. Consider requiringPROOT_CWD(or an Android indicator) alongsideCONTAINER, rather than treatingCONTAINERalone as sufficient.💡 Possible tightening
- // Termux on Android — touch gestures send wheel events through proot - // unreliably with AllMotion (1003 tracking). Drop to CellMotion. - if term != "" || proot != "" || container != "" { - return tea.MouseModeCellMotion - } + // Termux on Android — touch gestures send wheel events through proot + // unreliably with AllMotion (1003 tracking). Drop to CellMotion. + // Require proot together with a generic "container" signal so ordinary + // desktop/CI containers (podman/toolbox/systemd-nspawn) aren't affected. + if term != "" || (proot != "" && container != "") { + return tea.MouseModeCellMotion + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/mouse.go` around lines 15 - 23, The fallback in mouse mode detection is too broad because the CONTAINER check alone disables AllMotion for unrelated container runtimes. Update the logic in the mouse mode helper to treat CONTAINER as sufficient only when paired with Termux/proot/Android evidence, and keep the fallback targeted to the Termux-specific path in the function that reads TERMUX_VERSION, PROOT_CWD, and CONTAINER. Ensure desktop and CI container environments still use AllMotion unless the Termux/proot indicators are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/update/update.go`:
- Around line 257-261: The Authorization header logic in the update request path
is too broad and can send ZERO_GITHUB_TOKEN/GITHUB_TOKEN to non-GitHub
endpoints. Update the request-building code in the update flow to only set the
Bearer token when the target host is a GitHub API host, and skip attaching
credentials for custom mirrors or other full URLs such as
ZERO_UPDATE_RELEASE_URL. Use the existing request/endpoint handling around the
token lookup in update.go to add a host check before setting the header.
---
Nitpick comments:
In `@internal/tui/mouse.go`:
- Around line 15-23: The fallback in mouse mode detection is too broad because
the CONTAINER check alone disables AllMotion for unrelated container runtimes.
Update the logic in the mouse mode helper to treat CONTAINER as sufficient only
when paired with Termux/proot/Android evidence, and keep the fallback targeted
to the Termux-specific path in the function that reads TERMUX_VERSION,
PROOT_CWD, and CONTAINER. Ensure desktop and CI container environments still use
AllMotion unless the Termux/proot indicators are present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 21d7794a-d97d-4a37-9a2d-29c39b9e20c4
📒 Files selected for processing (4)
internal/tui/keybinding_help.gointernal/tui/model.gointernal/tui/mouse.gointernal/update/update.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Fix the failing smoke and review checks first
Smoke (ubuntu-latest) / Smoke (macos-latest) / Zero Review
The current PR checks are still not green: Ubuntu smoke and macOS smoke are failing, and the Zero Review gate is failing. Windows smoke was still pending at the last refresh. Please fix the failing checks, or update the PR with evidence that they are unrelated, before treating this as ready. -
[P2] Remove the unrelated TUI scroll commit from this update-auth PR
internal/tui/mouse.go:14
This branch still includes the Termux touch-scroll commit and changesinternal/tui/keybinding_help.go,internal/tui/model.go, andinternal/tui/mouse.go, while the PR title/body and stated testing are for GitHub update authentication. There is already a dedicated TUI PR (#503) and issue (#505) for that work, so merging the same TUI patch here would bypass that focused review path and keep this PR's scope/body inaccurate. Please rebase #504 so it contains only the update-auth change, leaving the TUI files and the unrelatedzero-linux-sandboxignore entry to the appropriate PR.
PatrickNoFilter
force-pushed
the
fix/update-auth
branch
2 times, most recently
from
61a0beb to
c5f13b7
Compare
|
Thanks @jatmn! Both findings addressed: [P2] Unrelated TUI scroll removed ✅ [P1] Smoke checks 🔄 |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found issues that still need to be addressed.
Findings
-
[P2] Link an approved parent issue before continuing this community PR
CONTRIBUTING.md:19
This PR is from a community contributor, but the body does not link any parent issue and the PR has noissue-approvedcontext. The contribution policy says community PRs must be tied to an existing issue that has already been reviewed and approved by the core team, and that PRs opened before the related issue has theissue-approvedlabel may be closed without review. Please open/link the parent issue and wait for theissue-approvedlabel, or get explicit maintainer approval recorded on this PR before continuing the implementation review. -
[P2] Remove the remaining unrelated sandbox ignore entry
.gitignore:45
The branch no longer includes the TUI scroll files, but it still addszero-linux-sandboxto.gitignore. My previous review asked for this update-auth PR to contain only the GitHub update-auth change and to leave both the TUI files and the unrelatedzero-linux-sandboxignore entry to the appropriate PR. Please drop this.gitignorechange so the PR scope/body match what will merge. -
[P2] Add regression coverage for the update auth header behavior
internal/update/update.go:257
The new credential path is security-sensitive, but the PR does not add tests for it:internal/update/update_test.gois unchanged, so there is no assertion thatZERO_GITHUB_TOKENwins overGITHUB_TOKEN, thatGITHUB_TOKENis used as the fallback, or that neither token is sent to a customZERO_UPDATE_RELEASE_URL/Options.Endpointhost. Please add a smallhttptest-based update test aroundfetchReleaseorCheckso this does not regress back into either unauthenticated GitHub calls or credential leakage to custom endpoints. -
[P2] Document the token env vars where users configure updates
docs/UPDATE.md:35
The implementation adds the only workaround users need for rate-limited update checks, but the update docs andzero update --helpstill only mention endpoint/target configuration. After this change, a user who hits the 403 path has no project documentation telling them to setZERO_GITHUB_TOKEN, that it takes precedence overGITHUB_TOKEN, or that tokens are only sent toapi.github.com. Please document the new env vars in the update docs/help so the feature is discoverable and the custom-endpoint credential boundary is clear.
PatrickNoFilter
force-pushed
the
fix/update-auth
branch
from
c5f13b7 to
3698a51
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tui/model.go`:
- Around line 1549-1602: Shift+Up/Shift+Down are being matched by the plain
arrow key cases in the input handler, so the Termux line-scroll shortcuts never
reliably win. Update the key handling in model.go by moving the shift-specific
branches in the main switch ahead of the generic tea.KeyUp/tea.KeyDown cases (or
otherwise making the plain cases exclude shifted input) so the
Shift+Up/Shift+Down behavior is handled before the subchat exit and normal arrow
navigation logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 06af8f5a-c461-4466-8f27-c89a12df0fb4
📒 Files selected for processing (5)
.gitignoreinternal/tui/keybinding_help.gointernal/tui/model.gointernal/tui/mouse.gointernal/update/update.go
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- internal/tui/keybinding_help.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/update/update.go
| case keyShift(msg) && keyIs(msg, tea.KeyUp): | ||
| if m.transcriptDetailed { | ||
| return m, nil | ||
| } | ||
| if m.pendingPermission != nil { | ||
| return m.movePermissionCursor(-1), nil | ||
| } | ||
| if m.pendingAskUser != nil { | ||
| return m.moveAskUserCursor(-1), nil | ||
| } | ||
| if m.providerWizard != nil { | ||
| return m.handleProviderWizardKey(msg) | ||
| } | ||
| if m.mcpAddWizard != nil { | ||
| return m.handleMCPAddWizardKey(msg) | ||
| } | ||
| if m.mcpManager != nil { | ||
| return m.handleMCPManagerKey(msg) | ||
| } | ||
| if m.picker != nil { | ||
| break | ||
| } | ||
| if m.suggestionsActive() { | ||
| break | ||
| } | ||
| m = m.clearHover() | ||
| return m.scrollChat(1), nil | ||
| case keyShift(msg) && keyIs(msg, tea.KeyDown): | ||
| if m.transcriptDetailed { | ||
| return m, nil | ||
| } | ||
| if m.pendingPermission != nil { | ||
| return m.movePermissionCursor(1), nil | ||
| } | ||
| if m.pendingAskUser != nil { | ||
| return m.moveAskUserCursor(1), nil | ||
| } | ||
| if m.providerWizard != nil { | ||
| return m.handleProviderWizardKey(msg) | ||
| } | ||
| if m.mcpAddWizard != nil { | ||
| return m.handleMCPAddWizardKey(msg) | ||
| } | ||
| if m.mcpManager != nil { | ||
| return m.handleMCPManagerKey(msg) | ||
| } | ||
| if m.picker != nil { | ||
| break | ||
| } | ||
| if m.suggestionsActive() { | ||
| break | ||
| } | ||
| m = m.clearHover() | ||
| return m.scrollChat(-1), nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant key helpers and the switch cases in internal/tui/model.go
rg -n "func keyIs|func keyShift|case keyIs\\(msg, tea.KeyUp\\)|case keyIs\\(msg, tea.KeyDown\\)|case keyShift\\(msg\\) && keyIs\\(msg, tea.KeyUp\\)|case keyShift\\(msg\\) && keyIs\\(msg, tea.KeyDown\\)|case keyShift\\(msg\\) && keyIs\\(msg, tea.KeyTab\\)|case keyIs\\(msg, tea.KeyTab\\)" internal/tui/model.go
echo
echo "---- key helpers / nearby context ----"
# Read the helper definitions and the surrounding switch sections with line numbers
sed -n '1,220p' internal/tui/model.go | cat -n | sed -n '1,220p'
echo
echo "---- switch region around arrow keys ----"
sed -n '1380,1615p' internal/tui/model.go | cat -nRepository: Gitlawb/zero
Length of output: 20535
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the key helper definitions if they live elsewhere too.
rg -n "func keyIs|func keyShift" internal -g '*.go'
# Inspect Bubble Tea key handling usage for modifiers in this repo.
rg -n "ModShift|tea\\.KeyUp|tea\\.KeyDown|keyShift\\(" internal -g '*.go'Repository: Gitlawb/zero
Length of output: 4506
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- internal/tui/input_compat.go ----"
sed -n '1,120p' internal/tui/input_compat.go | cat -n
echo
echo "---- relevant tests around shifted keys / bindings ----"
sed -n '1,220p' internal/tui/input_compat_test.go | cat -n
echo
echo "---- nearby keybinding translation code ----"
sed -n '1,240p' internal/tui/keybindings.go | cat -nRepository: Gitlawb/zero
Length of output: 13286
Shift+Up/Shift+Down are shadowed by the plain arrow handlers
Because keyIs() only checks the key code, case keyIs(msg, tea.KeyUp) / case keyIs(msg, tea.KeyDown) will match shifted arrow presses first. Move the shifted bindings above the plain arrow cases if the Termux line-scroll shortcuts are intended to work; otherwise Shift+Up can still trigger the subchat exit path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui/model.go` around lines 1549 - 1602, Shift+Up/Shift+Down are
being matched by the plain arrow key cases in the input handler, so the Termux
line-scroll shortcuts never reliably win. Update the key handling in model.go by
moving the shift-specific branches in the main switch ahead of the generic
tea.KeyUp/tea.KeyDown cases (or otherwise making the plain cases exclude shifted
input) so the Shift+Up/Shift+Down behavior is handled before the subchat exit
and normal arrow navigation logic.
Add Authorization header support to fetchRelease() to fix 403 on shared IPs (Termux/PRoot, CI runners). Reads ZERO_GITHUB_TOKEN env var first, falls back to GITHUB_TOKEN, then unauthenticated. Token is only sent when request.URL.Hostname() == "api.github.com" to prevent leaking credentials to custom mirrors.
PatrickNoFilter
force-pushed
the
fix/update-auth
branch
from
3698a51 to
afa7bed
Compare
|
Thanks @jatmn, both findings addressed: [P2] Unrelated TUI files removed ✅ [P1] Smoke tests 🔄 |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found issues that still need to be addressed.
Findings
-
[P1] Do not send GitHub tokens over plaintext HTTP endpoints
internal/update/update.go:257
--endpointandZERO_UPDATE_RELEASE_URLaccept full URLs, andresolveEndpointonly requires a scheme, sohttp://api.github.com/repos/.../releases/latestreachesfetchRelease. The new guard checks onlyrequest.URL.Hostname() == "api.github.com", so that HTTP endpoint now getsAuthorization: Bearer ...before any redirect can move the request to HTTPS. Please require HTTPS before attachingZERO_GITHUB_TOKEN/GITHUB_TOKENto the GitHub API request, or reject insecureapi.github.comendpoints, so a typo or copied HTTP URL cannot expose the token on the wire. -
[P2] Link an approved parent issue before continuing this community PR
CONTRIBUTING.md:19
This community PR still does not link a parent issue with theissue-approvedlabel, and I do not see explicit maintainer approval recorded on the PR that bypasses that policy. The contribution policy says community PRs must be tied to an existing issue that has already been reviewed and approved by the core team before implementation review continues. Please open/link the parent issue and wait forissue-approved, or get the maintainer exception recorded on this PR. -
[P2] Add regression coverage for the update auth header behavior
internal/update/update.go:257
The security-sensitive credential path is still untested:internal/update/update_test.gohas HTTP endpoint tests, but no assertion thatZERO_GITHUB_TOKENwins overGITHUB_TOKEN, thatGITHUB_TOKENis used as the fallback, that neither token is sent to a customZERO_UPDATE_RELEASE_URL/Options.Endpointhost, or that insecurehttp://api.github.comendpoints do not receive credentials. Please add a smallhttptest-based regression test aroundfetchReleaseorCheckso this does not regress back into unauthenticated GitHub calls or credential leakage to custom/insecure endpoints. -
[P2] Document the token env vars where users configure updates
docs/UPDATE.md:35
The implementation adds the workaround users need when GitHub rate-limits unauthenticated update checks, but the update docs andzero update --helpstill only mention endpoint, repository, timeout, and target configuration. A user hitting the 403 path has no project documentation telling them to setZERO_GITHUB_TOKEN, that it takes precedence overGITHUB_TOKEN, or that tokens are only sent toapi.github.com. Please document the new env vars in the update docs/help so the feature is discoverable and the custom-endpoint credential boundary is clear.
- Require scheme==https in addition to hostname==api.github.com before attaching Authorization header (P1 review finding) - Add auth test suite: 5 tests covering precedence, fallback, custom endpoint, HTTP endpoint, and no-token scenarios - Document ZERO_GITHUB_TOKEN and GITHUB_TOKEN in docs/UPDATE.md - Add env var documentation to zero update --help
|
Thanks @jatmn, all findings addressed in the latest push (e930918): [P1] HTTPS guard added - Before attaching Authorization, the code now checks request.URL.Scheme == https AND hostname == api.github.com. An http://api.github.com endpoint will not receive credentials. [P2] Regression tests added - 5 new tests in internal/update/auth_test.go via custom http.RoundTripper:
[P2] Docs added - New Authentication section in docs/UPDATE.md documenting both env vars, precedence, and the custom-endpoint security boundary. zero update --help now lists ZERO_GITHUB_TOKEN and ZERO_UPDATE_RELEASE_URL in an Environment section. [P2] Parent issue - Issue #505 is open and needs the issue-approved label from a maintainer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/update/auth_test.go (1)
43-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer the exported
EnvUpdateTokenconstant over hardcoded string literals.Tests hardcode
"ZERO_GITHUB_TOKEN"instead of referencingupdate.EnvUpdateToken(exported per the PR objective) exported fromupdate.go. If the constant's value ever changes, these tests would silently test the wrong variable.Also applies to: 62-62, 80-81, 99-99
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/update/auth_test.go` around lines 43 - 44, The auth tests are hardcoding the ZERO_GITHUB_TOKEN environment variable name instead of using the exported update.EnvUpdateToken constant, which can drift if the constant changes. Update the affected test cases in auth_test.go to reference update.EnvUpdateToken wherever that string literal appears, while keeping the GITHUB_TOKEN fallback setup unchanged. Use the existing update.EnvUpdateToken symbol from update.go to locate and replace all occurrences in the listed test blocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/update/auth_test.go`:
- Around line 55-124: The auth tests are still affected by ambient environment
variables, so they can pass or fail depending on the parent shell. In
TestFetchReleaseFallsBackToGithubToken and
TestFetchReleaseNoAuthWhenTokensNotSet, explicitly clear the unused token env
vars with t.Setenv before calling fetchRelease, using the existing test helpers
and authTransport setup to keep the assertions deterministic.
---
Nitpick comments:
In `@internal/update/auth_test.go`:
- Around line 43-44: The auth tests are hardcoding the ZERO_GITHUB_TOKEN
environment variable name instead of using the exported update.EnvUpdateToken
constant, which can drift if the constant changes. Update the affected test
cases in auth_test.go to reference update.EnvUpdateToken wherever that string
literal appears, while keeping the GITHUB_TOKEN fallback setup unchanged. Use
the existing update.EnvUpdateToken symbol from update.go to locate and replace
all occurrences in the listed test blocks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 360e24d4-c4a8-49e0-9142-23c2faae8ba6
📒 Files selected for processing (5)
docs/UPDATE.mdinternal/cli/app_test.gointernal/cli/update.gointernal/update/auth_test.gointernal/update/update.go
✅ Files skipped from review due to trivial changes (2)
- internal/cli/update.go
- internal/cli/app_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/update/update.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found issues that still need to be addressed.
Findings
-
[P2] Link an approved parent issue before continuing this community PR
CONTRIBUTING.md:19
This community PR still does not link a parent issue with theissue-approvedlabel, and I do not see explicit maintainer approval recorded on the PR that bypasses that policy. The contribution policy says community PRs must be tied to an existing issue that has already been reviewed and approved by the core team before implementation review continues. Please open/link the parent issue and wait forissue-approved, or get the maintainer exception recorded on this PR. -
[P2] Complete CodeRabbit's request to isolate the auth tests from ambient token env vars
internal/update/auth_test.go:62
CodeRabbit's current review item is still valid:TestFetchReleaseFallsBackToGithubTokensetsGITHUB_TOKENbut does not clearZERO_GITHUB_TOKEN, so any maintainer shell or CI job that already exportsZERO_GITHUB_TOKENwill exercise the precedence path and fail the fallback assertion. The same leak exists inTestFetchReleaseNoAuthWhenTokensNotSet, which clears neither token before expecting noAuthorizationheader on anapi.github.comrequest. Please clear the unused token variables witht.Setenvin those tests so the regression coverage is deterministic. -
[P3] Normalize the GitHub API host before deciding whether to authenticate
internal/update/update.go:257
--endpointandZERO_UPDATE_RELEASE_URLaccept full URLs, but the new guard comparesrequest.URL.Hostname()to the lowercase literalapi.github.com. Host names are case-insensitive, so a valid endpoint override such ashttps://API.GITHUB.COM/repos/Gitlawb/zero/releases/latestwould skip the token and keep failing with the unauthenticated rate-limit behavior this PR is trying to avoid. Please normalize or use a case-insensitive comparison for the GitHub host before deciding whether to attach the token.
- TestFetchReleaseFallsBackToGithubToken: clear ZERO_GITHUB_TOKEN with t.Setenv so ambient env doesn't break the fallback assertion - TestFetchReleaseNoAuthWhenTokensNotSet: clear both tokens so ambient env in CI/maintainer shell doesn't inject auth - Use strings.EqualFold for api.github.com hostname comparison so case-insensitive hostnames like API.GITHUB.COM are recognised
|
Thanks @jatmn, findings addressed in commit 6230a58 (pushed to fix/update-auth branch): Auth tests isolated from ambient tokens ✅
Case-insensitive hostname comparison ✅
Parent issue (#503) ❓ |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
@PatrickNoFilter please fix smoke issues
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving.
Read the auth guard closely since it's credential-handling, and it's sound: the token only attaches when the scheme is https AND the host is api.github.com (case-insensitive), so it never goes over plaintext or to a custom --endpoint / ZERO_UPDATE_RELEASE_URL mirror. ZERO_GITHUB_TOKEN takes precedence over GITHUB_TOKEN, and auth_test.go locks in all five paths (precedence, fallback, custom-host, HTTP-scheme, no-token) with t.Setenv isolation so ambient CI env can't flip the assertions. Docs and --help now cover the new vars and the api.github.com-only boundary, and CI is green.
That's every one of your findings addressed — the HTTPS guard, the case-insensitive host, the regression coverage, and the docs.
On the policy gate: the 403-on-shared-IPs bug (Termux/PRoot, CI runners) is a real one worth fixing, so I've opened #507 and marked it issue-approved, which clears the issue-first requirement here.
Good to merge — over to kevin.
3 participants
Problem
Running
zero update --check(orzero upgrade) fails with:The update check at
internal/update/fetchRelease()sends an unauthenticated request toapi.github.com/repos/Gitlawb/zero/releases/latest. GitHub's unauthenticated API is rate-limited to 60 requests/hour per IP — on shared environments (Termux/PRoot, CI runners, VPS, etc.) this limit is hit almost immediately.Fix
Added
Authorization: Bearer <token>header to the GitHub API request. The token is read from two env vars in priority order:ZERO_GITHUB_TOKEN(Zero-specific, takes precedence)GITHUB_TOKEN(common GitHub convention, fallback)If neither variable is set, the request remains unauthenticated (existing behavior, may still 403 on rate-limited IPs).
Testing
With token:
Without token (same IP, already rate-limited):
Changes
internal/update/update.go— +8 lines:EnvUpdateTokenconstant + conditionalAuthorizationheader infetchRelease().Summary by CodeRabbit
ZERO_GITHUB_TOKENsupport for update authentication (preferred overGITHUB_TOKEN).https://api.github.com/...; it’s omitted for other hosts and for plainhttp, even if tokens are set.ZERO_UPDATE_RELEASE_URLto override the release API URL.zero update --check/--jsondocs and CLI help with environment variable details and rate-limit guidance.Authorizationheader behavior across token/host/protocol scenarios.