Skip to content

Commit dd87f27

Browse files
Crash0v3rrid3claude
andcommitted
fix(review): make no-secrets path truly skip, harden scripts-lint
Code-review follow-ups on the smoke workflow: - Graceful degradation was only claimed, not real. GitHub exposes an unset secret as an empty string (present, not nil), and the reused test skips only on `env[...] != nil`, so a repo without the secrets would run the scan with empty creds and fail (script's `:?` under set -euo pipefail), not skip. Guard the scan step on the secrets being non-empty so it is skipped when absent and the job stays green on the build step. Fix the header comment to match. - Correct the scripts-lint comment: the launchers' shebang is `#!/usr/bin/env bash -il`, not `#!/usr/bin/env bash`. - Drop the fork-controllable filename from the `::notice/::error file=` workflow commands (workflow-command injection vector on fork PRs); log plain lines instead. bash -n still prints the real error location. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 69d5925 commit dd87f27

1 file changed

Lines changed: 14 additions & 5 deletions

File tree

.github/workflows/spm-smoke-test.yml

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,9 @@
77
# The scan downloads the BrowserStack CLI and makes authenticated network calls,
88
# so it needs BROWSERSTACK_USERNAME / BROWSERSTACK_ACCESS_KEY repo secrets. Those
99
# secrets are never exposed to fork PRs, so that job is gated to same-repo PRs
10-
# (and manual dispatch); fork PRs skip it. If the secrets are not configured yet,
11-
# the end-to-end test XCTSkips and the job still passes (build + unit test only).
10+
# (and manual dispatch); fork PRs skip it. The scan step is itself guarded on the
11+
# secrets being present, so if they are not configured the scan is skipped and the
12+
# job still passes on the build step alone.
1213
#
1314
# A second job (scripts-lint) syntax-checks every launcher script under scripts/.
1415
# It needs no secrets, so it runs on all PRs including forks.
@@ -54,7 +55,12 @@ jobs:
5455
working-directory: tests/spm
5556
run: swift build
5657

58+
# Guarded on the secrets actually being set: GitHub exposes an unset secret
59+
# as an empty string (present, not nil), so without this guard the scan would
60+
# run with empty credentials and fail. When the secrets are absent this step
61+
# is skipped and the job stays green on the build step alone.
5762
- name: End-to-end scan smoke (tests/spm)
63+
if: env.BROWSERSTACK_USERNAME != '' && env.BROWSERSTACK_ACCESS_KEY != ''
5864
working-directory: tests/spm
5965
run: swift test
6066

@@ -67,7 +73,7 @@ jobs:
6773
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
6874

6975
# Every script under scripts/ — the bash, zsh and fish variants alike — is
70-
# a bash script (`#!/usr/bin/env bash`); the variants differ only in which
76+
# a bash script (`#!/usr/bin/env bash -il`); the variants differ only in which
7177
# login shell they source BrowserStack creds from. So all of them are
7278
# syntax-checked with `bash -n`. The scripts self-update, register git
7379
# hooks and need credentials, so they are not executed here — this is a
@@ -84,10 +90,13 @@ jobs:
8490
fi
8591
status=0
8692
for script in "${scripts[@]}"; do
93+
# Plain log lines, not ::notice file=/::error file= workflow commands:
94+
# scripts/ filenames are attacker-controllable on fork PRs, and
95+
# interpolating them into a workflow command is an injection vector.
8796
if bash -n "$script"; then
88-
echo "::notice file=${script}::bash -n OK"
97+
echo "OK $script"
8998
else
90-
echo "::error file=${script}::bash -n failed — fix the syntax error above."
99+
echo "FAILED $script (bash -n syntax error above)"
91100
status=1
92101
fi
93102
done

0 commit comments

Comments
 (0)