fix(checkver): Harden github checkver#6641
Conversation
Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
|
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:
WalkthroughRefactors GitHub checkver in Changes
Sequence Diagram(s)sequenceDiagram
participant Script as Checkver Script
participant Validator as URL Validator
participant Token as GitHubToken Checker
participant API as GitHub API (api.github.com)
participant Site as GitHub Site (github.com)
Script->>Validator: receive `checkver.github` (string or object)
Validator-->>Script: validate as `github.com` or `api.github.com/repos/...`
alt non-API URL
Script->>Script: normalize to `/releases/latest`
end
Script->>Token: is `$GitHubToken` present?
alt token present
Script->>API: rewrite to `api.github.com/repos/...` and call releases/latest (with Authorization)
API-->>Script: return release metadata
else no token
Script->>Site: fetch `/releases/latest` from github.com (no API rewrite)
Site-->>Script: return release page data
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/checkver.ps1`:
- Around line 169-176: The validation logs an error when $inputGithubUrl does
not match $githubUrlPattern but continues processing, causing the malformed $url
to be used (later passed to DownloadDataAsync inside the ForEach-Object loop);
after the existing error call for invalid $inputGithubUrl, add an explicit
return to skip the current iteration so the code does not trim/append
'/releases/latest' or queue DownloadDataAsync for an invalid URL.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/checkver.ps1`:
- Around line 159-177: The GitHub API repo URL case isn't normalized: when
$inputGithubUrl is an api.github.com/repos/<owner>/<repo> root URL it gets left
as-is and returns repo metadata the default $regex can't match; update the
normalization logic around $url (after computing $inputGithubUrl and trimming)
to detect api.github.com repo root URLs and append the releases endpoint (e.g.
add '/releases/latest') when the path is just /repos/<owner>/<repo> (i.e. $url
-like 'https://api.github.com/repos/*' but not containing an extra path
segment), so that $url always points to a releases endpoint that the default
$regex can parse.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/checkver.ps1`:
- Around line 259-261: The code increments $in_progress before calling
$wc.DownloadDataAsync, which can throw synchronously and leave the
downloadDataCompleted event never firing; move the $in_progress++ to after a
successful call to $wc.DownloadDataAsync (i.e., call Register-ObjectEvent $wc
downloadDataCompleted, then call $wc.DownloadDataAsync($url, $state), and only
then increment $in_progress), or alternatively capture the subscription (assign
Register-ObjectEvent to a variable), wrap the DownloadDataAsync call in
try/catch, and on exception unregister the event/subscription and avoid
incrementing (or decrement $in_progress) so the Wait-Event loop cannot deadlock.
- Around line 178-181: Update the replacement that transforms GitHub URLs when
$GitHubToken is set by escaping the dot in the regex so it matches a literal
period (use '(www\.)?github\.com' instead of '(www\.)?github.com'), and add a
short inline comment above the if ($GitHubToken) block explaining the dual-mode
behavior: when a token is present the script switches to the API endpoint and
adds the Authorization header via $wc.Headers.Add, otherwise it intentionally
falls back to unauthenticated HTML scraping to leverage GitHub's more permissive
HTML rate limits as a hardening fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
|
I noticed more rate limit errors in the Excavator workflow today. P.S. The workflows for the main/extras/versions bucket are all using the develop branch (ScoopInstaller/Extras#17065).
Even though the URL indicates the GitHub API is being used, the requests are still missing a token in the header. Is this expected behavior? e.g., "checkver": {
"url": "https://api.github.com/repos/FiloSottile/age/releases",
"regex": "/age-v([\\w.-]+)-windows"
}, |
|
See the PR note, it's expected breaking. You'll have to use github: instead of url: now. |
Okay, got it. Sorry, I skimmed through it at first and didn't fully understand the intent of your note. |
|
@z-Fng You might be interested in reviewing the other changes in my checkver refinement series since you made relevant PRs before. |
OK, I will. These changes are great. The code is much more readable and more secure than before. However, to some extent, breaking changes could cause a lot of trouble for bucket maintainers, especially since some of our community buckets have tens of thousands of manifests. Furthermore, currently, we can only set |
2 participants
Description
NOTE: With this change, one has to use
"github":instead of"url":to specify checkver urls in github url format, explicitly. This could be considered a breaking change and is by intention, to narrow down the use of GitHub token to only when the checkver is explicitly set togithubmode.Motivation and Context
Closes #XXXX
Relates to #XXXX
How Has This Been Tested?
Run
checkver.ps1over local buckets.Checklist:
developbranch.