santiagomed commented on code in PR #3588:
URL: https://github.com/apache/thrift/pull/3588#discussion_r3376292602
##########
.github/workflows/release_rust.yml:
##########
@@ -33,28 +32,34 @@ on:
jobs:
publish:
runs-on: ubuntu-latest
- if: false # currently broken and no maintainers around -> see
THRIFT-5917
+ environment: release
Review Comment:
**[suggestion]** The single `publish` job carries `environment: release`
(line 35) and `id-token: write` (line 38) but still runs under the
`pull_request` trigger and `workflow_dispatch`. The sibling release workflows
in this repo (`pypi.yml`, `release_ruby.yml`) deliberately trigger their
publish jobs only on `release` and have no `pull_request` trigger. Because this
is `pull_request` (not `pull_request_target`), fork PRs get a read-only token
and the auth/publish steps are additionally gated by `github.event_name ==
'release'`, so this is not directly exploitable. However, it means every PR
that modifies this workflow file enters the protected `release` environment
(potentially queuing a deployment / required-reviewer approval as noise) and is
granted `id-token: write` for a context where it is never needed.
**Suggestion:** Split the PR/dispatch validation (`cargo publish --dry-run`)
into its own job with no `environment` and minimal `permissions: { contents:
read }`, and keep a separate `publish` job that is the only one carrying
`environment: release` + `id-token: write` and is scoped to the `release` event
(e.g. `if: github.event_name == 'release'` at the job level, mirroring
`pypi.yml`/`release_ruby.yml`).
##########
.github/workflows/release_rust.yml:
##########
@@ -33,28 +32,34 @@ on:
jobs:
publish:
runs-on: ubuntu-latest
- if: false # currently broken and no maintainers around -> see
THRIFT-5917
+ environment: release
permissions:
contents: read
id-token: write
steps:
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd #
v6.0.2
with:
persist-credentials: false
- - name: Dryrun
+
+ - name: Dry run
working-directory: lib/rs
+ # Use stable toolchain: the repo-pinned 1.83 predates edition2024
support
+ # required by transitive dependencies (getrandom >= 0.4).
run: cargo publish --dry-run
+ env:
+ RUSTUP_TOOLCHAIN: stable
Review Comment:
**[nit]** `RUSTUP_TOOLCHAIN: stable` (lines 50 and 64) floats the toolchain
to whatever "stable" resolves to at run time. This makes both the dry-run
validation and the actual published build non-reproducible, and means the
artifact pushed to crates.io may be built against a different compiler than the
rest of the repo's CI (which pins 1.83, per the added comment). A floating
channel can silently change behavior between a passing dry-run and a later
publish.
**Suggestion:** Consider pinning a specific stable version (e.g.
`RUSTUP_TOOLCHAIN: 1.XX.0`) so the dry-run and publish use an identical,
reproducible toolchain, and bump it intentionally.
##########
.github/workflows/release_rust.yml:
##########
@@ -33,28 +32,34 @@ on:
jobs:
publish:
runs-on: ubuntu-latest
- if: false # currently broken and no maintainers around -> see
THRIFT-5917
+ environment: release
permissions:
contents: read
id-token: write
steps:
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd #
v6.0.2
with:
persist-credentials: false
- - name: Dryrun
+
+ - name: Dry run
working-directory: lib/rs
+ # Use stable toolchain: the repo-pinned 1.83 predates edition2024
support
+ # required by transitive dependencies (getrandom >= 0.4).
run: cargo publish --dry-run
+ env:
+ RUSTUP_TOOLCHAIN: stable
- name: Authenticate to crates.io
- # Only publish if it's a tag and the tag is not a pre-release
- if: ${{ startsWith(github.ref, 'refs/tags/') && !contains(github.ref,
'-') }}
+ # Only publish on a non-prerelease GitHub Release event
+ if: ${{ github.event_name == 'release' &&
!github.event.release.prerelease }}
id: crates-io-auth
uses:
rust-lang/crates-io-auth-action@bbd81622f20ce9e2dd9622e3218b975523e45bbe #
v1.0.4
- name: Publish
+ # Only publish on a non-prerelease GitHub Release event
+ if: ${{ github.event_name == 'release' &&
!github.event.release.prerelease }}
Review Comment:
**[nit]** The publish path (gated on `github.event_name == 'release' &&
!github.event.release.prerelease`) does not verify that the released tag/ref
corresponds to the version declared in `lib/rs/Cargo.toml`. The prerelease
guard is correct and draft releases are implicitly handled (a published release
is never a draft). But if a maintainer publishes a Release whose tag does not
match the `Cargo.toml` version, `cargo publish` will either republish/fail on
an existing version or publish an unexpected version. Low risk (cargo rejects
duplicate versions) but there is no early, explicit consistency check.
**Suggestion:** Optionally add a guard step that asserts the `Cargo.toml`
`version` matches `github.event.release.tag_name` (stripping any leading `v`)
before authenticating/publishing, failing fast on mismatch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
