Validate GitHub PAT before use in github_dl.py - #575
Conversation
openshift-ci
Bot
added
the
approved
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe downloader validates supplied GitHub tokens through the rate-limit API, trims token input, exits when validation fails, and handles missing responses during asset and checksum retrieval. ChangesGitHub download hardening
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant main
participant validate_token
participant GitHub
participant list_assets
participant get_checksum
main->>validate_token: Validate supplied token
validate_token->>GitHub: Request rate-limit endpoint
GitHub-->>validate_token: Return rate-limit response
validate_token-->>main: Return validation result
main->>list_assets: Retrieve assets
list_assets-->>main: Return assets or empty list
main->>get_checksum: Retrieve checksum
get_checksum-->>main: Return checksum or empty string
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chamalabey, clcollins The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@utils/dockerfile_assets/github_dl.py`:
- Around line 46-56: Update validate_token() so only HTTP 401 reports the token
as invalid or expired. Handle HTTP 403 and 429 as rate-limit or temporary abuse
blocks, surfacing the response status and details without instructing users to
replace GITHUB_TOKEN; preserve the existing unexpected-response handling for
other statuses.
- Line 44: Update the GitHub probe request in the surrounding function to pass
an explicit timeout, catch requests.exceptions.RequestException, and return
False on request failures. Also revise the 403 handling so it returns False
without describing the token as rejected, since the response may indicate rate
limiting or temporary blocking.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d097dd19-f9ba-4d7b-b07c-d66afb17b175
📒 Files selected for processing (1)
utils/dockerfile_assets/github_dl.py
|
New changes are detected. LGTM label has been removed. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@utils/dockerfile_assets/github_dl.py`:
- Around line 239-249: Update token loading in the CI secret and GITHUB_TOKEN
environment-variable branches to treat stripped empty strings as absent,
preserving the existing token value or unauthenticated flow. Ensure
whitespace-only values do not overwrite a valid token or satisfy later
token-presence validation.
- Around line 62-70: Validate that remaining is an int immediately after
extracting it in the rate-limit handling block, before the remaining < 37
comparison. For None, strings, or other non-integer values, print the existing
malformed-response error and return False; preserve the current threshold
behavior for valid integers.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2684f39d-da77-4ff9-9354-d486d624ab5b
📒 Files selected for processing (1)
utils/dockerfile_assets/github_dl.py
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 `@utils/dockerfile_assets/github_dl.py`:
- Around line 239-249: Update token loading in the CI secret and GITHUB_TOKEN
environment-variable branches so stripped values are assigned to args.token only
when non-empty; preserve any previously loaded valid token when a source is
blank or whitespace-only.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 83b571d2-503f-4e29-8916-a5b6712e7397
📒 Files selected for processing (1)
utils/dockerfile_assets/github_dl.py
|
/retest |
0efc1bc to
f2f6f9d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
utils/dockerfile_assets/github_dl.py (1)
63-75: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRead
resources.core.remaininginstead of relying only on legacyrate.A missing
rateobject currently defaultsremainingto0, causing valid responses to be reported as exhausted. GitHub documentsresources.coreas the canonical field and removesratein API version2026-03-10; add aresources.corelookup with a legacy fallback and reject missing values explicitly. (docs.github.com)Proposed fix
- remaining = response.json().get("rate", {}).get("remaining", 0) + payload = response.json() + rate = payload.get("resources", {}).get("core") + if rate is None: + rate = payload.get("rate") + remaining = rate.get("remaining") if isinstance(rate, dict) else None🤖 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 `@utils/dockerfile_assets/github_dl.py` around lines 63 - 75, Update the rate-limit extraction in the response handling block to read resources.core.remaining first, falling back to legacy rate.remaining only when needed. Do not default missing fields to zero; explicitly treat a missing or non-integer remaining value as a malformed response, while preserving the existing threshold check and error behavior.Source: MCP tools
🤖 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 `@utils/dockerfile_assets/github_dl.py`:
- Line 48: Update the GitHub token-validation error handling around the existing
print statement to avoid logging the raw exception or its message. Emit only a
sanitized fixed message or the exception type, while preserving the failure
indication and surrounding control flow.
---
Nitpick comments:
In `@utils/dockerfile_assets/github_dl.py`:
- Around line 63-75: Update the rate-limit extraction in the response handling
block to read resources.core.remaining first, falling back to legacy
rate.remaining only when needed. Do not default missing fields to zero;
explicitly treat a missing or non-integer remaining value as a malformed
response, while preserving the existing threshold check and error behavior.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 47c1f61f-28d2-4013-b987-1806240ebc15
📒 Files selected for processing (1)
utils/dockerfile_assets/github_dl.py
Add upfront token validation against the GitHub /rate_limit endpoint to fail fast with a clear error message when an invalid or expired token is provided. Also fix a None dereference crash in list_assets when the API returns a non-200 response. Created with assistance from Claude 🤖 <claude@anthropic.com> Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Christopher Collins <collins.christopher@gmail.com>
- Add 10s timeout and RequestException handling to prevent stalled builds - Treat HTTP 403/429 as rate-limit/blocking, not invalid token - Check remaining API calls after validation (fail if < 37) - Handle malformed rate_limit API responses - Strip whitespace from file-read and env var tokens - Remove extraneous f-string prefix on 401 message Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Christopher Collins <collins.christopher@gmail.com>
Empty or whitespace-only secret files would produce "" after .strip(), passing the `is None` check but failing token validation. Use `or None` to fall through to unauthenticated mode instead. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Christopher Collins <collins.christopher@gmail.com>
- Add None guard to get_checksum() after get_url_with_authentication() call, matching the pattern already used in list_assets() and get_binary() - Add missing `import time` that caused NameError on retry paths in get_url_with_authentication() (pre-existing bug) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Validate remaining is an int before comparison to prevent TypeError - Use local variable pattern for token loading so empty/whitespace-only secrets don't overwrite a previously loaded valid token Created with assistance from Claude 🤖 <claude@anthropic.com> Signed-off-by: Christopher Collins <collins.christopher@gmail.com>
f2f6f9d to
ae89f81
Compare
|
/override "Red Hat Konflux / ocm-container-main-enterprise-contract / ocm-container-micro" "Red Hat Konflux / ocm-container-main-enterprise-contract / ocm-container-minimal" The EC micro/minimal checks are failing with This is NOT from PR 575's code — the EC snapshot includes the full ocm-container image from master (revision The version skew is a race condition: a 🤖 Claude claude@anthropic.com commenting on behalf of @clcollins |
|
@clcollins: Overrode contexts on behalf of clcollins: Red Hat Konflux / ocm-container-main-enterprise-contract / ocm-container-micro, Red Hat Konflux / ocm-container-main-enterprise-contract / ocm-container-minimal DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
2 participants
Summary
/rate_limitendpoint to fail fast with a clear error message when an invalid or expired token is providedNonedereference crash inlist_assets()whenget_url_with_authentication()returnsNoneon non-200 responsesNonedereference crash inget_checksum()— same pattern aslist_assets(), missed in the original fixmain(), before any download or quota logicTest plan
GITHUB_TOKEN— should succeed as beforeGITHUB_TOKEN(e.g.,GITHUB_TOKEN=bad-token) — should fail with clear error:"Error: GitHub token is invalid or expired (HTTP 401)"GITHUB_TOKEN— should proceed without authentication as before (no validation call made)Pre-existing bugs fixed
These bugs existed on
masterbefore this PR and are fixed opportunistically:import time:get_url_with_authentication()callstime.sleep()on retry (line 103) but thetimemodule was never imported, causing aNameErrorcrash on any 5xx retry pathget_checksum()None dereference:get_url_with_authentication()can returnNone(on max retries or 4xx errors) butget_checksum()accessedresponse.contentwithout a None guard, causing anAttributeErrorcrash when downloading checksum files failed🤖 Generated with Claude Code
Summary by CodeRabbit