Skip to content

ci: keep tokens and expressions out of workflow shell steps - #211

Open
Jasonvdb wants to merge 5 commits into
masterfrom
fix/harden-android-publish-job
Open

Jasonvdb wants to merge 5 commits into
masterfrom
fix/harden-android-publish-job

Conversation

@Jasonvdb

@Jasonvdb Jasonvdb commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

Hardens the workflows that hold tokens: the publish job validates its version and stops leaving the token on disk, and no workflow interpolates ${{ }} expressions into shell scripts any more.

Description

gradle-publish.yml (runs on release and manual dispatch, with contents: write and packages: write):

  • persist-credentials: false on checkout, so the write token isn't left in .git/config while build_android.sh, Cargo build scripts and Gradle run. Nothing later needs it: gh gets GH_TOKEN and Gradle publish gets GITHUB_TOKEN through env, and the only git fetch is a public cargo install --git.
  • Every ${{ }} moved out of run: scripts into step env: (the version input, the version output, the NDK path), so a value is never parsed as shell.
  • The version (dispatch input, or the release tag) is checked against ^v?[0-9]+\.[0-9]+\.[0-9]+(-[0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*)?$ before anything uses it. All 146 existing tags pass. The check now runs right after checkout, so a bad value fails in seconds instead of after the SDK and NDK setup.
  • Removed server-id and settings-path from setup-java. The build is Gradle-only and never read that Maven settings.xml; setup-java now writes its default one to ~/.m2 instead of the workspace.

Other workflows:

  • android-bindgen.yml, ios-bindgen.yml: persist-credentials: false on checkout, since these run build scripts from pull requests. android-bindgen.yml also passes the NDK path through env.
  • claude-code-review.yml: persist-credentials: false on checkout, so the checkout's GITHUB_TOKEN is no longer in .git/config. This does not leave the agent without a token: claude-code-action sets its own token in the origin remote URL before the agent runs, and the agent can already call gh api. Also uses $GITHUB_REPOSITORY, and passes pr_number through env with a check that it is a plain number.

Behaviour is unchanged for valid inputs. This merges cleanly with #210, which pins the actions to commit SHAs.

Follow-ups, not in this PR:

  • Split the publish job so the build runs with a read-only token and only a final step gets packages: write.
  • Work out the release tag once and reuse it for the checkout ref, the version and the release upload.
  • persist-credentials: false on the remaining test workflows' checkouts (left out here to avoid conflicts with ci: pin workflow actions to commit SHAs and add Dependabot #210).
  • claude.yml: its checkout still persists a contents: write token. Check that the action's own git auth covers its pushes, then add persist-credentials: false.
  • Claude review job: limit what the agent can reach with the action's token (the origin remote URL and gh api).

Preview

No UI change.

QA Notes

  • Every workflow parses as YAML, and no run: script contains a ${{ }} expression.
  • The version step was run locally: v0.8.0-rc15, 0.8.0-swaps.11, v0.8.0-rc11.1, v1.0.0-rc-1, an empty input falling back to the tag, and all 146 existing tags pass; a trailing space, an embedded newline, v1.2.3-.. and $(touch PWNED) are rejected and run nothing.
  • The pr_number check was run locally with gh stubbed: 207 passes, 1; touch PWNED and an empty value are rejected.
  • PR CI: Android Bindgen and iOS Bindgen run on this PR and cover their checkout and NDK changes.
  • gradle-publish.yml and claude-code-review.yml only run on release or manual dispatch, so they were checked statically; the next release exercises the publish job.

@Jasonvdb
Jasonvdb marked this pull request as ready for review November 12, 2026 18:04

@coreyphillips coreyphillips left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two independent reviews, nothing blocking a merge.

worth doing, does not block

  • Review job still has a write token on disk: the action writes it to the remote URL (.github/workflows/claude-code-review.yml:29)

nits

  • claude.yml checkout still persists a contents:write token and is not in the follow-ups (.github/workflows/claude.yml:21-24). claude.yml keeps the token from its checkout, even though the job has contents: write, pull-requests: write and issues: write and runs an agent. The description says this PR hardens the workflows that hold tokens. Its follow-up list only covers the remaining test workflows, all of which have contents: read. claude.yml is in neither group. Adding persist-credentials: false there may well be safe, because the action sets up its own git auth for pushes, but nobody has checked that yet. It is worth adding to the follow-up list so it is not missed.

@talosmachina talosmachina left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No findings. Moves every ${{ }} out of run: scripts into step env:, validates the publish version before use, and sets persist-credentials: false on the checkouts of the publish, bindgen and review workflows. Reviewed 43458b9, full tier (CI workflows).

What I checked, and 4 candidates I ruled out

Read in full: gradle-publish.yml, claude-code-review.yml, android-bindgen.yml, ios-bindgen.yml at the head.

Build: not built. The change is workflow-only, and a Bitkit-track auto review does not run the PR's code here. CI is green: Android Bindgen, iOS Bindgen, validation, USDT, wake and Paykit route.

Checks run

  • A scan of every run: block in .github/workflows/ at the head finds no ${{ }}. On master it finds 9.
  • I read the version step, then ran a copy with bash. v0.8.0-rc15, 0.8.0-swaps.11 and an empty input (falling back to the ref name) pass. A trailing space, an embedded newline, v1.2.3-.. and $(touch PWNED) exit 1 and write no output; nothing was created. All 28 tags in my clone match the pattern.
  • git merge-tree of #210 and #211 merges cleanly.

Ruled out

  • Publish needs the persisted token: Gradle publish reads GITHUB_TOKEN and GITHUB_ACTOR from env (bindings/android/lib/build.gradle.kts:251-252), and gh release upload gets GH_TOKEN. The only git fetches in the build are public: cargo install --git in build_android.sh:39, and the git dependencies swap-common and jade-client-rs in Cargo.toml.
  • Dropping server-id/settings-path breaks publish: the publish goes through Gradle's maven { url = "https://maven.pkg.github.com/$repo" } with its own credentials, never through a Maven settings.xml.
  • The review agent needs git credentials: that workflow only allows gh commands, which read GH_TOKEN, plus read-only git commands, and the action sets up its own git auth.
  • A non-numeric pr_number: the input is type: number, and the added regex closes the shell path anyway. The prompt interpolation is not a run: script.

Merge confidence: 4/5. gradle-publish.yml and claude-code-review.yml only run on release or dispatch, so no CI run covers their changes.

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: ✅ Approve

Findings: None.

QA for 43458b9:

Check: workflow syntax and shell expressions ✅ actionlint and shell syntax checks passed.

Ran actionlint 1.7.12 across all workflows. Parsed all nine workflow files and checked every run: block with bash -n; none contains an Actions expression.

NDK paths remain literal shell data ✅ both export scripts preserved the path.

Executed both workflow scripts with a synthetic NDK path containing spaces and shell metacharacters, then compared the exported variables with the original path. The publish script also rejected a mismatched NDK revision.

Gradle receives the selected version as one argument ✅ build and publish arguments stayed literal.

Executed the workflow's build and publish shell commands with an argument-recording Gradle stub. Each received one literal -Pversion argument; this did not build or publish a package.

Check: publish versions and review numbers are validated ✅ covered by existing evidence at this head.

Relied on the PR's QA notes and the prior review at 43458b9 for accepted versions, tag fallback and rejected malformed inputs. Those cases were not repeated.

Check: Android and iOS bindings build in CI ✅ both exact-head jobs passed.

Verified the commit and successful completion of both runs:

No app build or device run: this changes only workflows and has no device QA case. Release publishing and the Claude manual-dispatch workflow were not triggered; their live execution remains unverified.

Full review of the diff, four files, including the build and publication paths they reach. The resolved token discussion is already reflected in the description and follow-ups.


Reviewed by gpt-6.1-sol-high

@ovitrif ovitrif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utAck

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants