Use GITHUB_TOKEN (PAT) for branch detection on url_pull repos#3061
Draft
ashwindasr wants to merge 1 commit into
Draft
Use GITHUB_TOKEN (PAT) for branch detection on url_pull repos#3061ashwindasr wants to merge 1 commit into
ashwindasr wants to merge 1 commit into
Conversation
When url_pull points to a personal fork, the GitHub App is not installed there and branch detection fails silently (returning no results from git ls-remote). This causes the build to error with "branch does not exist" even though the branch is present. Use PAT-based auth (GITHUB_TOKEN) for url_pull repositories, and fall back to checking the push url (where the App is installed) if the PAT check also fails. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
Walkthrough
ChangesRemote branch detection with PAT authentication and fallback
Sequence DiagramsequenceDiagram
participant Caller
participant detect_remote_source_branch
participant _get_remote_branch_ref
participant get_github_git_pat_env
participant get_github_git_auth_env
Caller->>detect_remote_source_branch: check branch with url_pull present
detect_remote_source_branch->>_get_remote_branch_ref: branch, url_pull, use_pat=True
_get_remote_branch_ref->>get_github_git_pat_env: request PAT auth environment
get_github_git_pat_env-->>_get_remote_branch_ref: PAT credentials
_get_remote_branch_ref-->>detect_remote_source_branch: None (branch not found on pull URL)
detect_remote_source_branch->>_get_remote_branch_ref: branch, url_push, use_pat=False
_get_remote_branch_ref->>get_github_git_auth_env: request GitHub App auth environment
get_github_git_auth_env-->>_get_remote_branch_ref: GitHub App credentials
_get_remote_branch_ref-->>detect_remote_source_branch: commit hash (found on push URL)
detect_remote_source_branch-->>Caller: branch exists, commit hash
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@doozer/doozerlib/source_resolver.py`:
- Around line 309-315: The current logic sets use_pat = bool(pull_url) which
enables injecting the GitHub PAT for any pull URL (including non-GitHub hosts)
and can leak credentials; change the gating so use_pat is true only when
url_pull is a GitHub HTTPS remote. Update the code around
source_details["url_pull"], source_details["url"], git_url and use_pat to detect
GitHub hosts (e.g. by validating the URL's hostname or using the same
GitHub-detection used by get_github_git_auth_env) before setting use_pat, and
leave use_pat False for non-GitHub remotes so _get_remote_branch_ref(...) will
not call get_github_git_pat_env for foreign hosts; apply the same fix to the
other occurrence at the later block (around line 392) that uses use_pat.
- Around line 321-324: _source_resolver.py currently treats "no such ref" and
"probe failed" the same because SourceResolver._get_remote_branch_ref returns
falsy for both; change its contract to return a two-tuple (ref, error) where ref
is the ref string or None when not present, and error is None on a successful
probe (even if ref is missing) or an Exception/str when the git ls-remote call
failed; then update callers (the blocks that call
SourceResolver._get_remote_branch_ref, e.g. the snippet where result =
SourceResolver._get_remote_branch_ref(git_url, stage_branch, use_pat=use_pat)
and the similar calls around the other mentions) to only attempt the retry
against push_url when error is non-None, and treat ref==None with error==None as
authoritative "branch missing on url_pull" (no retry). Ensure any call sites
that expected a single return value are adjusted to unpack the tuple and
propagate/handle errors appropriately.
🪄 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-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f341f50e-f028-4866-8108-884e2e5090f5
📒 Files selected for processing (2)
doozer/doozerlib/source_resolver.pydoozer/tests/test_source_resolver.py
Comment on lines
+309
to
+315
| pull_url = source_details.get("url_pull") | ||
| push_url = source_details["url"] | ||
| git_url = pull_url or push_url | ||
| # When url_pull points to a personal fork, the GitHub App won't have | ||
| # access. Use GITHUB_TOKEN (PAT) for branch detection against url_pull | ||
| # and fall back to the push url (where the App is installed) if needed. | ||
| use_pat = bool(pull_url) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Gate PAT auth to GitHub pull remotes only.
use_pat = bool(pull_url) is too broad here. If url_pull points at GitLab or another non-GitHub HTTPS host, _get_remote_branch_ref(..., use_pat=True) will inject GITHUB_TOKEN via get_github_git_pat_env(). Unlike get_github_git_auth_env(url=...), that helper does not suppress credentials for non-GitHub remotes, so this can leak the GitHub PAT to the foreign host during git ls-remote.
Suggested direction
- use_pat = bool(pull_url)
+ normalized_pull_url = art_util.convert_remote_git_to_https(pull_url) if pull_url else ""
+ use_pat = normalized_pull_url.startswith("https://github.com/")Also applies to: 392-392
🤖 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 `@doozer/doozerlib/source_resolver.py` around lines 309 - 315, The current
logic sets use_pat = bool(pull_url) which enables injecting the GitHub PAT for
any pull URL (including non-GitHub hosts) and can leak credentials; change the
gating so use_pat is true only when url_pull is a GitHub HTTPS remote. Update
the code around source_details["url_pull"], source_details["url"], git_url and
use_pat to detect GitHub hosts (e.g. by validating the URL's hostname or using
the same GitHub-detection used by get_github_git_auth_env) before setting
use_pat, and leave use_pat False for non-GitHub remotes so
_get_remote_branch_ref(...) will not call get_github_git_pat_env for foreign
hosts; apply the same fix to the other occurrence at the later block (around
line 392) that uses use_pat.
Comment on lines
+321
to
+324
| result = SourceResolver._get_remote_branch_ref(git_url, stage_branch, use_pat=use_pat) | ||
| if not result and pull_url: | ||
| LOGGER.info("Branch check against url_pull %s failed, retrying with push url %s", pull_url, push_url) | ||
| result = SourceResolver._get_remote_branch_ref(push_url, stage_branch) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't treat "branch missing on url_pull" the same as "couldn't query url_pull".
Right now _get_remote_branch_ref() returns None both when git ls-remote errors and when the branch genuinely does not exist. These sites retry push_url on any falsy result, so a branch that exists only on push_url can be selected even though resolve_source() will still clone from pull_url. That makes branch resolution succeed and the later clone fail. Only retry push_url when the url_pull probe itself failed; a clean "no such ref" result should stay authoritative for the fork.
Also applies to: 343-346, 354-357
🤖 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 `@doozer/doozerlib/source_resolver.py` around lines 321 - 324,
_source_resolver.py currently treats "no such ref" and "probe failed" the same
because SourceResolver._get_remote_branch_ref returns falsy for both; change its
contract to return a two-tuple (ref, error) where ref is the ref string or None
when not present, and error is None on a successful probe (even if ref is
missing) or an Exception/str when the git ls-remote call failed; then update
callers (the blocks that call SourceResolver._get_remote_branch_ref, e.g. the
snippet where result = SourceResolver._get_remote_branch_ref(git_url,
stage_branch, use_pat=use_pat) and the similar calls around the other mentions)
to only attempt the retry against push_url when error is non-None, and treat
ref==None with error==None as authoritative "branch missing on url_pull" (no
retry). Ensure any call sites that expected a single return value are adjusted
to unpack the tuple and propagate/handle errors appropriately.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
url_pullis configured in image metadata (pointing to a personal fork), branch detection viagit ls-remotefails because the GitHub App is not installed on personal accounts. The build then errors with "branch does not exist" even though the branch is present.GITHUB_TOKEN) instead of GitHub App tokens when checking branches againsturl_pullrepositories.GITHUB_TOKENis not set), it falls back to checking the pushurl(where the GitHub App is installed).Context
Build #6119 of
build/layered-productsfailed to rebaseacm-clibecause:url_pullpointed togit@github.com:dhaiducek/acm-cli.git(personal fork)GITHUB_TOKENwas not set, sogit ls-remoteran unauthenticatedRequested target branch release-2.16 does not existTest plan
test_source_resolver.pytests passtest_get_remote_branch_ref_use_patverifies PAT auth is used whenuse_pat=Truetest_detect_remote_source_branch_uses_pull_url_with_patverifies PAT auth for url_pulltest_detect_remote_source_branch_pull_url_fallback_to_push_urlverifies fallback to push url when url_pull check failsurl_pullpointing to a personal forkMade with Cursor
Summary by CodeRabbit
Release Notes
New Features
Tests