-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix: pin bun runtime config and improve log hygiene #1174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| # Intentionally minimal. action.yml pins --config to this file so bun resolves | ||
| # its runtime config from the action directory rather than the workspace. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,9 @@ const SENSITIVE_PATHS = [ | |
| ".claude.json", | ||
| ".gitmodules", | ||
| ".ripgreprc", | ||
| "CLAUDE.md", | ||
| "CLAUDE.local.md", | ||
| ".husky", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In our repo, we do commit the .husky files, but not the hook setup in themself. |
||
| ]; | ||
|
|
||
| /** | ||
|
claude[bot] marked this conversation as resolved.
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -145,10 +145,11 @@ | |
| () => exchangeForAppToken(oidcToken, permissions), | ||
| { | ||
| shouldRetry: (error) => !(error instanceof WorkflowValidationSkipError), | ||
| }, | ||
| ); | ||
| console.log("App token successfully obtained"); | ||
| core.setSecret(appToken); | ||
|
|
||
| console.log("Using GITHUB_TOKEN from OIDC"); | ||
| return appToken; | ||
|
Check failure on line 154 in src/github/token.ts
|
||
|
Comment on lines
148
to
154
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 The OIDC token obtained via Extended reasoning...What the bug is: The The specific code path: The OIDC token flows from Why existing code does not prevent it: The current code does not log the OIDC token directly — What the impact would be: While not an active leak in current code, the OIDC token is still a valid bearer credential (short-lived but non-trivially scoped to the action's audience). More importantly, the PR explicitly establishes the pattern of calling How to fix it: Add Step-by-step proof of the inconsistency: (1) |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟣 The positional entrypoint path argument is unquoted in all three bun run steps: run ${GITHUB_ACTION_PATH}/src/entrypoints/run.ts — if GITHUB_ACTION_PATH contains spaces (possible on self-hosted runners), bash word-splitting will break the command. This is a pre-existing issue that this PR makes more visible by adding properly double-quoted --config and --tsconfig-override flags alongside the unquoted path; fix by wrapping the path: run "${GITHUB_ACTION_PATH}/src/entrypoints/run.ts".
Extended reasoning...
The unquoted shell expansion
run ${GITHUB_ACTION_PATH}/src/entrypoints/run.tsis present in all three bun entrypoint steps (lines ~232, ~332, ~346 of action.yml). When bash processes this command, it performs word-splitting on unquoted variable expansions. If GITHUB_ACTION_PATH contains a space — for example/my runner/_actions/org/repo/v1on a self-hosted runner — bash will split it into separate tokens, and bun will receiverun,/my,runner/...as distinct arguments rather than a single file path, causing a file-not-found error.The code path that triggers the bug: any self-hosted GitHub Actions runner whose configured path contains whitespace. The environment variable GITHUB_ACTION_PATH is set by the Actions runner to the directory of the checked-out action. While GitHub-hosted runners use /home/runner/work/_actions/... (no spaces), enterprise self-hosted runners can be configured with arbitrary paths.
The existing code fails to prevent this because the --config and --tsconfig-override flags added by this PR are correctly quoted (--config="${GITHUB_ACTION_PATH}/bunfig.toml"), demonstrating that the author was aware of proper quoting, but the final positional argument was overlooked. The old single-line invocation had the same issue and was never fixed.
The practical impact is low: GitHub-hosted runners never have spaces in GITHUB_ACTION_PATH, and most self-hosted runners follow similar conventions. However, the inconsistency is now more prominent — three properly-quoted flags sit right above an unquoted path argument, which could mislead reviewers into thinking the path is safe.
This is a pre-existing issue not introduced by this PR. The original
bun run ${GITHUB_ACTION_PATH}/src/entrypoints/run.tshad the same unquoted expansion. The PR touched these lines and could have fixed it incidentally when reformatting to multi-line, but did not introduce the problem.Step-by-step proof: (1) A self-hosted runner is configured with a path containing a space, e.g. /my runners/actions. (2) The runner sets GITHUB_ACTION_PATH=/my runners/actions/org/repo@v1. (3) bash executes
bun --no-env-file --config="..." --tsconfig-override="..." run ${GITHUB_ACTION_PATH}/src/entrypoints/run.ts. (4) After word-splitting, bun receives: arg0=run, arg1=/my, arg2=runners/actions/org/repo@v1/src/entrypoints/run.ts. (5) Bun tries to find a file named /my — it does not exist — and exits with an error. Fix: change torun "${GITHUB_ACTION_PATH}/src/entrypoints/run.ts"in all three steps.