fix: harden dbt review action version lookup and credential write#911
Conversation
Follow-up hardening for the `github/review` composite action from the #900 review. Stacks on #900 (refines its new semver version step). - Authenticate the release-version lookup with `${{ github.token }}` (lifts the unauthenticated 60 req/hr IP limit to 1,000 req/hr) so busy runners aren't throttled into the `latest` fallback. - Skip the binary cache when the version resolves to `latest` (`if: steps.version.outputs.version != 'latest'`), so one rate-limited/offline lookup can't pin a stale binary across all later runs. - Read the hosted API key from the environment inside the `jq` program (`$ENV.IN_ALT_KEY`) instead of passing it via `--arg`, keeping it out of `argv` (visible to other processes; printed under `ACTIONS_STEP_DEBUG`). - Add 4 adversarial tests: auth header present with a token, omitted+safe without one (bash-3.2 empty-array idiom), cache gated on `!= 'latest'`, and the API key absent from the `jq` argv. Closes #909 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
Disabled knowledge base sources:
📝 WalkthroughWalkthroughThe PR hardens the altimate-code GitHub action for v0.8.5 by adding Bearer token authentication to the release lookup API call (lifting rate limits when ChangesGitHub Release Action Security & Resilience v0.8.5
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested labels
Poem
✨ 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 |
What does this PR do?
Re-lands the
github/reviewcomposite-action hardening ontomain. These three follow-ups were surfaced by the multi-model review of #900 and were originally opened as #910 — but #910 was accidentally merged into #900's feature branch (fix/v0.8.4-dbt-review-launch) after #900 had already squash-merged tomain, so the changes never reachedmain. This PR targetsmaindirectly and supersedes #910.github.action_ref— a branch, SHA, ormain) callsreleases/latestunauthenticated (IP-limited to 60 req/hr). It now passes${{ github.token }}, lifting the limit to 1,000 req/hr so busy runners aren't throttled into thelatestfallback.latest. When the version resolved tolatestit was used as a static cache key, so one rate-limited/offline lookup pinned that binary forever. The cache step is now gated withif: steps.version.outputs.version != 'latest'; a resolved semver caches normally,latestfalls through to a fresh install.jqargv. The credential write read the key via--arg key "$IN_ALT_KEY"(visible inargv; printed underACTIONS_STEP_DEBUG→set -x). It now reads from the environment inside the jq program ($ENV.IN_ALT_KEY).None of these are regressions; the golden
@vX.Y.Zpath never hits the release-API lookup and is unaffected.Type of change
Issue for this PR
Closes #909
How did you verify your code works?
Four adversarial tests in
release-v0.8.5-adversarial.test.ts(run the action's real shell with fakecurl/jqonPATH):Authorization: Bearer <token>whenGITHUB_TOKENis present;${AUTH[@]+"${AUTH[@]}"}empty-array idiom is safe underset -u, back to bash 3.2);Cache altimate-codestep is gated onsteps.version.outputs.version != 'latest';jqargv (asserts the secret is absent from captured args and the program uses$ENV.IN_ALT_KEY).Local checks (all green on this branch, cherry-picked cleanly onto current
main):bun test test/skill/release-v0.8.5-adversarial.test.ts→ 13 pass.bun run script/upstream/analyze.ts --markers --base main --strict→ ok.prettier --checkon the changed test file → clean. Action YAML parses.Checklist
🤖 Generated with Claude Code
Summary by cubic
Hardens the
github/reviewcomposite action to reduce rate-limit failures, prevent stale binary caching, and keep the hosted API key out of process args. Improves reliability for busy runners and avoids secret exposure.releases/latestlookup with${{ github.token }}to lift limits to 1,000 req/hr.'latest'; semver versions still cache normally.jq($ENV.IN_ALT_KEY) instead of--argto keep it out ofargvand debug logs.vX.Y.Z) continue to bypass the API lookup and behave as before.Written for commit 981d238. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Security