PR #7172 staging CI - #98
Closed
wasimysaid wants to merge 2 commits into
Closed
wasimysaid wants to merge 2 commits into
wasimysaid wants to merge 2 commits into
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
📝 WalkthroughWalkthroughChangesThe desktop release workflow now separates signed artifact builds from GitHub Release publication, validates release notes and asset sets, generates updater metadata, and restricts write permissions to Desktop release workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Build as Build matrix
participant Artifacts as Workflow artifacts
participant Publish as publish-release
participant GitHub as GitHub Releases
Build->>Build: Stage signed release assets
Build->>Artifacts: Upload normalized assets
Publish->>Artifacts: Download signed assets
Publish->>Publish: Validate expected assets and metadata
Publish->>GitHub: Create or validate versioned release
Publish->>GitHub: Upload assets and updater metadata
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 (2)
tests/security/test_release_desktop_permissions.py (2)
16-25: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert the publishing job’s complete permission map.
The test still passes if
publish-releasegains additional scopes such asactions: writeorid-token: write. Lock the intended least-privilege boundary explicitly.Proposed assertion
workflow = _workflow() assert workflow["permissions"] == {"contents": "read"} + assert workflow["jobs"]["publish-release"]["permissions"] == { + "contents": "write" + }🤖 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 `@tests/security/test_release_desktop_permissions.py` around lines 16 - 25, Update test_only_publish_job_can_write_repository_contents to assert that the publish-release job’s complete permissions map contains only the intended contents: write scope, rejecting any additional permissions such as actions or id-token.
28-42: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCheck inherited environments for
GITHUB_TOKEN.Line 41 checks only step-local
env; a token defined at workflow or build-job scope would be inherited by every Tauri step and remain undetected.Proposed effective-environment check
- jobs = _workflow()["jobs"] + workflow = _workflow() + jobs = workflow["jobs"] build = jobs["build"] publish = jobs["publish-release"] + inherited_env = { + **workflow.get("env", {}), + **build.get("env", {}), + } assert "permissions" not in build @@ assert len(tauri_steps) == 3 for step in tauri_steps: - assert "GITHUB_TOKEN" not in step.get("env", {}) + effective_env = {**inherited_env, **step.get("env", {})} + assert "GITHUB_TOKEN" not in effective_env🤖 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 `@tests/security/test_release_desktop_permissions.py` around lines 28 - 42, Update test_build_matrix_hands_off_assets_without_release_credentials to inspect the effective environment inherited from workflow- and build-job-level env scopes, not only each Tauri step’s local env. Assert that GITHUB_TOKEN is absent from those inherited scopes and each step’s env so all three tauri-apps/tauri-action steps are verified without release credentials.
🤖 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 @.github/workflows/release-desktop.yml:
- Around line 786-813: Update the “Validate release asset set” step to reject
any files beyond the eight required release assets, rather than only checking
that each suffix occurs once. Build the allowed set from the matched
required-suffix assets, compare it with every file in asset_dir, and fail
validation when unexpected MSI, debug, metadata, or other files are present so
the later upload step publishes only the validated exact set.
---
Nitpick comments:
In `@tests/security/test_release_desktop_permissions.py`:
- Around line 16-25: Update test_only_publish_job_can_write_repository_contents
to assert that the publish-release job’s complete permissions map contains only
the intended contents: write scope, rejecting any additional permissions such as
actions or id-token.
- Around line 28-42: Update
test_build_matrix_hands_off_assets_without_release_credentials to inspect the
effective environment inherited from workflow- and build-job-level env scopes,
not only each Tauri step’s local env. Assert that GITHUB_TOKEN is absent from
those inherited scopes and each step’s env so all three tauri-apps/tauri-action
steps are verified without release credentials.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 13e7e831-ea0c-413b-914e-01fcc5063231
📒 Files selected for processing (2)
.github/workflows/release-desktop.ymltests/security/test_release_desktop_permissions.py
Comment on lines
+786
to
+813
| - name: Validate release asset set | ||
| shell: bash | ||
| run: | | ||
| set -euo pipefail | ||
| python3 <<'PY' | ||
| import pathlib | ||
| import os | ||
| import sys | ||
|
|
||
| asset_dir = pathlib.Path(os.environ['RUNNER_TEMP'], 'desktop-release-assets') | ||
| files = [path for path in asset_dir.iterdir() if path.is_file()] | ||
| required_suffixes = ( | ||
| '.dmg', | ||
| '.app.tar.gz', | ||
| '.app.tar.gz.sig', | ||
| '.deb', | ||
| '.AppImage', | ||
| '.AppImage.sig', | ||
| '-setup.exe', | ||
| '-setup.exe.sig', | ||
| ) | ||
| for suffix in required_suffixes: | ||
| matches = [path for path in files if path.name.endswith(suffix)] | ||
| if len(matches) != 1: | ||
| sys.exit(f'Expected exactly one {suffix} release asset, found {len(matches)}') | ||
| if any(path.name == 'latest.json' for path in files): | ||
| sys.exit('Build artifacts must not supply latest.json') | ||
| print('\n'.join(sorted(path.name for path in files))) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject unexpected files before publishing the release.
The validator confirms each required suffix exists, but permits arbitrary additional files; Line 867 then uploads all of them. A new Tauri output such as an MSI, debug artifact, or metadata file would therefore be published unintentionally.
Proposed exact-set validation
required_suffixes = (
'.dmg',
'.app.tar.gz',
'.app.tar.gz.sig',
'.deb',
'.AppImage',
'.AppImage.sig',
'-setup.exe',
'-setup.exe.sig',
)
+ unexpected = [
+ path.name
+ for path in files
+ if not any(path.name.endswith(suffix) for suffix in required_suffixes)
+ ]
+ if unexpected:
+ sys.exit(
+ 'Unexpected release assets: ' + ', '.join(sorted(unexpected))
+ )
for suffix in required_suffixes:Also applies to: 861-867
🤖 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 @.github/workflows/release-desktop.yml around lines 786 - 813, Update the
“Validate release asset set” step to reject any files beyond the eight required
release assets, rather than only checking that each suffix occurs once. Build
the allowed set from the matched required-suffix assets, compare it with every
file in asset_dir, and fail validation when unexpected MSI, debug, metadata, or
other files are present so the later upload step publishes only the validated
exact set.
Owner
Author
|
Fork CI completed successfully. Closing this staging-only PR. |
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.
Staging-only CI validation for unslothai#7172.
This PR exists only to run fork GitHub Actions checks and should be closed after validation.
Summary by CodeRabbit
Security
Release Management
Bug Fixes
Tests