-
Notifications
You must be signed in to change notification settings - Fork 0
feat(#256): add GH_TOKEN validation to pre-retro.sh #262
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
base: main
Are you sure you want to change the base?
Changes from all commits
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,129 @@ | ||
| #!/usr/bin/env bash | ||
| # pre-retro-test.sh — Test pre-retro.sh with mock gh to verify token validation. | ||
| # | ||
| # Uses a mock gh command to capture calls without hitting GitHub. | ||
| # Run from the repo root: bash internal/scaffold/fullsend-repo/scripts/pre-retro-test.sh | ||
|
|
||
| set -euo pipefail | ||
|
|
||
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| PRE_SCRIPT="${SCRIPT_DIR}/pre-retro.sh" | ||
| FAILURES=0 | ||
|
|
||
| # Create a temp directory for mock state. | ||
| TMPDIR="$(mktemp -d)" | ||
| trap 'rm -rf "${TMPDIR}"' EXIT | ||
|
|
||
| # --- Helpers --- | ||
|
|
||
| # build_mock creates a mock gh binary that returns a preconfigured exit code | ||
| # for "gh auth status" calls. | ||
| # Arguments: | ||
| # $1 — exit code for "gh auth status" (0 = valid token, 1 = invalid token). | ||
| build_mock() { | ||
| local auth_exit="$1" | ||
| local mock_bin="${TMPDIR}/bin" | ||
|
|
||
| rm -rf "${mock_bin}" | ||
| mkdir -p "${mock_bin}" | ||
|
|
||
| cat > "${mock_bin}/gh" <<MOCKEOF | ||
| #!/usr/bin/env bash | ||
| if [[ "\$1" == "auth" && "\$2" == "status" ]]; then | ||
| exit ${auth_exit} | ||
| fi | ||
| MOCKEOF | ||
|
|
||
| chmod +x "${mock_bin}/gh" | ||
| echo "${mock_bin}" | ||
| } | ||
|
|
||
| run_test_stdout() { | ||
| local test_name="$1" | ||
| local auth_exit="$2" | ||
| local expected_stdout="$3" | ||
| local expect_exit="$4" | ||
| local extra_env="${5:-}" | ||
|
|
||
| local mock_bin | ||
| mock_bin="$(build_mock "${auth_exit}")" | ||
|
|
||
| local env_cmd=( | ||
| env -i | ||
| PATH="${mock_bin}:/usr/bin:/bin" | ||
| HOME="${TMPDIR}" | ||
| ORIGINATING_URL="https://github.com/test-org/test-repo/pull/42" | ||
| ) | ||
|
|
||
| # Add extra env vars if provided. | ||
| if [[ -n "${extra_env}" ]]; then | ||
| while IFS= read -r kv; do | ||
| [[ -n "${kv}" ]] && env_cmd+=("${kv}") | ||
| done <<< "${extra_env}" | ||
| fi | ||
|
|
||
| local exit_code=0 | ||
| "${env_cmd[@]}" bash "${PRE_SCRIPT}" > "${TMPDIR}/stdout.log" 2>&1 || exit_code=$? | ||
|
|
||
| if [[ ${exit_code} -ne ${expect_exit} ]]; then | ||
| echo "FAIL: ${test_name} — expected exit ${expect_exit}, got ${exit_code}" | ||
| cat "${TMPDIR}/stdout.log" | ||
| FAILURES=$((FAILURES + 1)) | ||
| return | ||
| fi | ||
|
|
||
| if ! grep -qF "${expected_stdout}" "${TMPDIR}/stdout.log" 2>/dev/null; then | ||
| echo "FAIL: ${test_name} — expected stdout '${expected_stdout}' not found" | ||
| echo "Actual stdout:" | ||
| cat "${TMPDIR}/stdout.log" | ||
| FAILURES=$((FAILURES + 1)) | ||
| return | ||
| fi | ||
|
|
||
| echo "PASS: ${test_name}" | ||
| } | ||
|
|
||
| # --- Test cases --- | ||
|
|
||
| # Valid token → auth status exits 0 → script succeeds. | ||
| run_test_stdout "valid-token-succeeds" \ | ||
| 0 \ | ||
| "GH_TOKEN validated successfully." \ | ||
| 0 \ | ||
| "GH_TOKEN=fake-token" | ||
|
|
||
| # Invalid token → auth status exits 1 → script fails with error annotation. | ||
| run_test_stdout "invalid-token-fails" \ | ||
| 1 \ | ||
| "::error::GH_TOKEN is invalid" \ | ||
| 1 \ | ||
| "GH_TOKEN=bad-token" | ||
|
|
||
| # Absent token → script fails with error annotation (no sandbox wasted). | ||
| run_test_stdout "absent-token-fails" \ | ||
| 0 \ | ||
| "::error::GH_TOKEN is not set" \ | ||
| 1 | ||
|
|
||
| # Valid token with retro comment → script succeeds with on-demand message. | ||
| run_test_stdout "retro-comment-on-demand" \ | ||
| 0 \ | ||
| "Retro triggered on-demand with comment." \ | ||
| 0 \ | ||
| "$(printf '%s\n%s' 'GH_TOKEN=fake-token' 'RETRO_COMMENT=look at this')" | ||
|
|
||
| # Valid token without retro comment → script succeeds with automatic message. | ||
| run_test_stdout "retro-automatic-trigger" \ | ||
| 0 \ | ||
| "Retro triggered automatically (PR close)." \ | ||
| 0 \ | ||
| "GH_TOKEN=fake-token" | ||
|
|
||
| # --- Summary --- | ||
|
|
||
| echo "" | ||
| if [[ ${FAILURES} -gt 0 ]]; then | ||
| echo "${FAILURES} test(s) failed" | ||
| exit 1 | ||
| fi | ||
| echo "All tests passed" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
| # | ||
| # Required env vars: | ||
| # ORIGINATING_URL — HTML URL of the PR or issue that triggered retro | ||
| # GH_TOKEN — GitHub token; validated before sandbox starts | ||
| # | ||
| # Optional env vars: | ||
| # RETRO_COMMENT — The /retro comment text (empty for automatic triggers) | ||
|
|
@@ -22,6 +23,23 @@ fi | |
|
|
||
| echo "::notice::Retro target: ${ORIGINATING_URL}" | ||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Validate GH_TOKEN before starting the sandbox | ||
| # --------------------------------------------------------------------------- | ||
|
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. [low] secrets-handling No ::add-mask:: call for GH_TOKEN (post-retro.sh masks it). Also, gh auth status stdout is not suppressed — auth metadata (username, token type) may appear in CI logs. Suggested fix: Add echo ::add-mask::${GH_TOKEN} before validation. Change to gh auth status >/dev/null 2>&1. |
||
| # ponytail: gh auth status validates auth, not scopes — a token with only | ||
| # contents:read would pass here but fail in post-retro.sh. Still catches the | ||
| # #256 failure mode (expired/revoked token). Scope checks if needed later. | ||
| if [[ -z "${GH_TOKEN:-}" ]]; then | ||
| echo "::error::GH_TOKEN is not set — retro agent requires GitHub API access" | ||
| exit 1 | ||
| fi | ||
| echo "::add-mask::${GH_TOKEN}" | ||
|
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. [low] workflow-command-injection echo "::add-mask::${GH_TOKEN}" is theoretically vulnerable to newline-based workflow command injection. Risk is negligible: token is OIDC-minted (infrastructure-controlled), and this pattern is already used in post-retro.sh, post-review.sh, post-fix.sh, post-code.sh, and prepare-sandbox-credentials.sh. |
||
| if ! gh auth status >/dev/null 2>&1; then | ||
|
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. [low] observability gh auth status stderr suppressed (>/dev/null 2>&1) hides diagnostics that distinguish token expiry from revocation. Consider capturing stderr for a more informative error annotation. Suggested fix: Replace with: if ! auth_err=$(gh auth status 2>&1); then echo "::error::GH_TOKEN validation failed — ${auth_err}"; exit 1; fi |
||
| echo "::error::GH_TOKEN is invalid — retro agent requires GitHub API access" | ||
| exit 1 | ||
| fi | ||
| echo "GH_TOKEN validated successfully." | ||
|
|
||
| if [[ -n "${RETRO_COMMENT:-}" ]]; then | ||
| echo "Retro triggered on-demand with comment." | ||
| else | ||
|
|
||
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.
[low] validation-gap
gh auth status validates authentication but not token scopes. A token with insufficient scopes (e.g., missing issues:write) passes validation but fails in post-retro.sh.
Suggested fix: Add a comment noting this limitation. Consider a lightweight scope probe if false passes become a problem.