security(spec-sync): harden against upstream compromise

[TexasCoding]

Summary

Spec-sync workflow no longer has a path from upstream-controlled YAML into committed/PR'd repo content. Mitigations (Option (c) from #92):

1. Dropped write permissions — permissions: reduced from contents: write + pull-requests: write to contents: read + issues: write. Workflow can no longer push branches or open PRs.
2. Removed peter-evans/create-pull-request step. No automated branch/PR creation from upstream content.
3. Removed in-CI generate.py / ruff / mypy / pytest steps. Those previously executed Python derived from attacker-controllable upstream YAML; maintainers now regenerate and test locally before opening a PR by hand.
4. Drift opens a new spec-drift-labeled issue, one per distinct fingerprint — sha256 of openapi_sha + asyncapi_sha, embedded in the issue body as <!-- fingerprint:HEX -->. Re-runs against unchanged upstream are silent; if drift recurs, the existing open issue dedups so weekly cron won't spam. Each drift gets its own lifecycle (assignable, labelable, auto-closed by Closes #N in the regen PR).
5. Issue body includes sha256 checksums of fetched specs so maintainers can verify byte-for-byte that local fetches match what the bot saw.
6. Preserved existing SHA pinning of actions/checkout and astral-sh/setup-uv.

Design pivot from earlier draft

The first version of this PR posted comments on a long-lived #113 tracking issue. After bot review feedback ("Hardcoded tracking issue number — fragile, no URL") and user feedback ("can we automatically open new issues on spec drift?"), reworked to the per-drift pattern above. #113 is now closed as superseded.

Not addressed (upstream-dependent, per #92)

• Verifying a Kalshi-published checksum/sigstore signature — blocked on upstream publishing such artifacts. SHA reporting in the issue body is a partial substitute.

Closes #92

Test plan

• YAML parses cleanly via yaml.safe_load
• First scheduled run on a drift-detecting upstream opens exactly one spec-drift-labeled issue
• Re-running the workflow against the same upstream is a no-op (existing open issue dedups)
• Regen PR with Closes #N auto-closes the drift issue on merge

[claude]

Code Review — PR #109: security(spec-sync): harden against upstream compromise

Overview

This PR implements Option (c) from issue #92, closing the supply-chain attack surface in the spec-sync workflow. The threat was real and the fix is surgical: strip write permissions, stop auto-executing upstream-controlled content in CI, and shift regeneration to a manual, human-audited step. The approach is sound.

---

Security — Correct

Permissions reduction is the right call. contents: read + issues: write means even a fully compromised upstream spec cannot write to the repo or open PRs.

Removal of peter-evans/create-pull-request eliminates the path from upstream YAML → committed Python. There is no substitute risk introduced.

Env-var injection pattern is done correctly. All ${{ }} expressions are passed as environment variables rather than interpolated inline into the shell script. This is the standard GitHub Actions injection-prevention pattern — well applied.

Heredoc safety — the cat <<EOF with ${VAR} expansion is safe here. Bash expands variables inside an unquoted heredoc but does not re-evaluate the resulting string as commands. So if ADDED_PATHS contains $(malicious), it prints literally without executing. No injection risk.

SHA-pinning on third-party actions is preserved. Good.

---

One Issue Worth Fixing Before Merge

SPEC_SYNC_TRACKING_ISSUE: "92" points to the issue this PR closes.

The PR footer says Closes #92. Once merged, #92 will be closed, but the workflow will continue posting drift comments to it. GitHub allows commenting on closed issues and subscribers still get notifications — so it won't break — but it's confusing UX and the comment will land on a closed issue titled "Security: spec-sync workflow is a supply-chain risk".

Recommendation: before merging, open a new long-lived tracking issue (e.g., "Spec-sync drift tracking") and update the env var to point to it. Or, if the intent is to reuse #92 as a running log, keep it open and remove it from Closes #92.

---

Minor Observations

• No comment posted on no-drift runs — the if: steps.diff.outputs.changed == 'true' guard on all post-diff steps means no noise when nothing changed. Good.

• sha256sum on locally-written files — the checksums are computed on the files the workflow itself fetched and wrote. They reflect "what the bot downloaded," not "what Kalshi signed." The comment text already says this clearly ("Reproduce locally and verify the same hashes before generating models"), which is the right framing.

• Removed the SHA-pinning rationale comment — the old comment at the actions/checkout line explained why SHA pinning existed. The new top-of-file security model block covers the same ground more comprehensively. Net improvement.

• set -euo pipefail in both new run: blocks. Consistent with project conventions.

• No continue-on-error on the reporting step — if gh issue comment fails, the workflow fails noisily. This is the right default (fail loudly rather than silently swallow a missed report).

---

Summary

Area	Assessment
Security model	Correct — closes the supply-chain path
Permissions	Correctly minimized
Shell safety	Safe — env-var pattern used correctly
Heredoc correctness	Valid (YAML block scalar dedent handles the EOF terminator)
Tracking issue lifecycle	Needs attention — SPEC_SYNC_TRACKING_ISSUE: "92" should point to a new open issue before closing #92
Test coverage	Workflow-level change; the checklist item "First scheduled run posts a comment" should be verified on the first Monday after merge

Fix the tracking issue target and this is good to merge.

[TexasCoding]

Addressed in latest commit. Opened #113 as the long-lived "Spec-sync drift tracking" issue and switched SPEC_SYNC_TRACKING_ISSUE from "92" → "113". Closes #92 stays — the hardening work is done; #113 is the perpetual log for future drift comments.

[claude]

Code Review — PR #109: security(spec-sync): harden against upstream compromise

Overview

This PR implements Option (c) from issue #92 (F-O-01 audit finding): the spec-sync workflow is stripped of all write permissions and can no longer auto-generate code from upstream-controlled YAML or open PRs. Drift is now reported as an issue comment on a long-lived tracking issue (#113), and a maintainer manually runs the regen pipeline. This is the correct approach for a high-severity supply-chain risk.

---

What's Good

• Permission reduction is correct and minimal. contents: write + pull-requests: write → contents: read + issues: write. The workflow can now only post a comment — it can't touch repo contents or open PRs.
• Removed execution of upstream-derived code. Dropping generate.py, ruff, mypy, and pytest from CI eliminates the path from attacker-controlled YAML to running Python in a privileged runner.
• Top-level security model comment is excellent. It explains the threat model, the chosen mitigation, and the manual process clearly. Future maintainers will understand why this is structured this way.
• SHA256 checksums for spec verification give maintainers a byte-for-byte check before running regen locally. Good partial substitute for upstream signature publishing.
• Env vars used correctly in the report step. Values from steps.meta.outputs.* are bound via env: before the shell script runs — they're not template-expanded inline into the script body (${{ ... }} in run scripts). This is the safer pattern; it avoids YAML/expression injection even if step outputs contain special characters.
• SHA pinning on third-party actions maintained. ✓
• set -euo pipefail consistently applied. ✓
• concurrency.cancel-in-progress: false prevents duplicate comments from parallel runs. ✓

---

Issues and Suggestions

1. Hardcoded tracking issue number — fragile, no URL

env:
  SPEC_SYNC_TRACKING_ISSUE: "113"

If issue #113 is ever closed, locked, or a new tracking issue is opened, someone must remember to update this. A comment with the full issue URL would help maintainers find it quickly:

env:
  # https://github.com/TexasCoding/kalshi-python-sdk/issues/113
  SPEC_SYNC_TRACKING_ISSUE: "113"

Low severity — but tracking issues do get closed/replaced.

---

2. <<EOF (unquoted heredoc) works but <<'EOF' would be clearer

The report body uses an unquoted heredoc:

body=$(cat <<EOF
## Weekly spec sync drift report
...
\`contents: read\` only
...
${OPENAPI_CHANGED}
EOF
)

The unquoted form is required here because you need ${OPENAPI_CHANGED} etc. to expand, and it's correct. The \`` escaping for backtick literals is also correct — bash does not recursively evaluate ${VAR}` expansions for nested command substitution, so there's no injection risk from attacker-controlled values in those env vars.

Minor note: the backtick escaping (\contents: read`) is non-obvious to readers. A brief comment or just using single-quoted in the Markdown ('contents: read'`) would reduce visual noise. Not a bug, just readability.

---

3. Silent drift if gh issue comment fails

With set -euo pipefail, a gh API failure (rate limit, transient 5xx) marks the job failed. That's the right behavior — at least the run goes red. But there's no retry and no alternative notification channel (no Slack/email hook). If the job fails silently during off-hours and no one watches GitHub Actions, drift could go unreported for a week.

Consider adding a continue-on-error: false note in the step comment so future maintainers know failures are intentional (they are already: set -e handles this). Alternatively, a brief on: workflow_run trigger or email notification for failed runs would close the gap. Not required for this PR, but worth tracking as a follow-up.

---

4. Removed informative comment about SHA-pinning rationale

The old comment:

# Third-party actions pinned to SHA (not tag) because this workflow holds
# contents:write + pull-requests:write. A compromised mutable tag could
# weaponize the auto-PR flow. Other workflows in this repo use bare tags
# because their permissions are narrower.

…was removed along with the old permission set. The new top-level security block mentions SHA pinning but doesn't explain the mutable-tag threat. The new version is still an improvement overall, but consider adding a one-liner to the checkout step explaining why it's pinned:

# Pinned to SHA — a compromised mutable tag would still run with issues:write
- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683  # v4.2.2

Minor documentation nit.

---

Summary

Area	Assessment
Security model	✅ Correct and well-reasoned
Permission scope	✅ Minimal and correct
Code injection risks	✅ Mitigated (env: mapping, no inline template expansion in script)
Heredoc syntax	✅ Correct; minor readability nit
Tracking issue coupling	⚠️ Hardcoded issue number, add URL comment
Failure observability	⚠️ No retry/fallback on gh API failure
Documentation	✅ Top-level security comment is excellent

This is a well-executed mitigation of a real supply-chain risk. The two ⚠️ items are non-blocking — the hardcoded issue number is the more actionable one. Approved with those minor suggestions.

[TexasCoding]

Reworked per second-pass bot feedback + user direction to "auto-open new issues on spec drift when they arise."

New design: workflow opens one spec-drift-labeled issue per distinct drift fingerprint (sha256(openapi_sha || asyncapi_sha)), embedded in the issue body as <!-- fingerprint:HEX -->. Before opening, it searches existing open spec-drift issues for the same fingerprint and no-ops if found — so re-runs against unchanged upstream are silent, and weekly cron won't spam if a drift persists across runs.

This eliminates every concern from the bot's review pass:

• 🔥 Hardcoded tracking issue number — gone; no more SPEC_SYNC_TRACKING_ISSUE env var.
• ✅ Per-drift lifecycle — each drift is assignable, labelable, and auto-closes when a regen PR lands with Closes #N.
• ✅ Permission scope unchanged — issues: write already covers gh issue create.

#113 closed as superseded. PR body updated. Ready for re-review.

[claude]

Code Review — PR #109: security(spec-sync): harden against upstream compromise

Overview

This PR closes the supply chain attack surface in the spec-sync workflow (issue #92). Previously, the workflow held contents: write + pull-requests: write and ran Python code (generate.py, ruff, mypy, pytest) derived from attacker-controllable upstream YAML. A MITM or compromised Kalshi spec could have injected code into committed repo content. The fix is correct in principle: drop write permissions, remove automated code execution, and replace the auto-PR with a drift-notification issue that maintainers act on manually.

---

Strengths

• Permission reduction is the right call. contents: read + issues: write is the minimum viable scope for a notify-only workflow.
• Fingerprint-based dedup is elegant. sha256(openapi_sha || asyncapi_sha) embedded in an HTML comment is machine-readable, stable across re-runs, and doesn't require mutable state.
• extract_spec_meta.py uses yaml.safe_load() (confirmed) — running it against upstream YAML is safe.
• scripts/sync_spec.py exists — the remediation steps in the issue body are actionable.
• SHA pins on third-party actions are preserved. The narrower permission scope makes this less critical than before, but it's still good hygiene.
• Top-level security model comment is excellent: it explains the threat model and the design decision in the file where a future editor will need to understand it.
• set -euo pipefail throughout — consistent fail-fast behavior.

---

Issues

Medium — Heredoc expands env vars; malicious spec path could trigger command substitution

body=$(cat <<EOF
...
${ADDED_PATHS}
...
EOF
)

<<EOF (unquoted) expands $VAR and $(cmd) inside the body. ADDED_PATHS/REMOVED_PATHS contain API paths sourced from upstream YAML. OpenAPI path strings are constrained (/v2/markets/{ticker}) and unlikely to contain shell metacharacters, but if upstream ever introduced a path containing $(cmd), it would execute during body construction.

The fix is low-disruption: switch to <<'EOF' (single-quoted, no expansion) and substitute variables with explicit printf/sed, or write the body to a temp file with Python and use --body-file. Given the narrow threat window, this is advisory rather than a blocker — but it's the one remaining code-execution path from upstream content, even if theoretical.

Low — Dedup search capped at 100 issues

gh issue list --limit 100 --label spec-drift

If more than 100 spec-drift issues are open simultaneously (very unlikely), a duplicate would be created. Using --limit 200 or searching closed issues would tighten this, but it's not a real concern in practice.

Low — --body "${body}" vs stdin

For large diffs (many added/removed paths), the body string grows. Shell argument lists have a practical limit (~2 MB on Linux). Piping via stdin is more robust:

echo "${body}" | gh issue create ... --body-file -

Minor, but worth noting for spec updates with large diffs.

Nit — Removed SHA-pinning comment without replacement in the right place

The old inline comment explaining why actions are SHA-pinned was removed from the uses: lines. The new top-of-file comment covers this, which is actually cleaner. The old comment said:

"Other workflows in this repo use bare tags because their permissions are narrower."

That sentence no longer applies (this workflow's permissions are now also narrow), so removing it is correct. Just noting it's intentional, not an oversight.

---

Test Plan Gap

The PR test plan marks only "YAML parses cleanly" as checked. The dedup logic is the most complex new behavior and has no automated test. Consider a one-time manual smoke test before the first scheduled run:

1. Temporarily set changed=true in the diff step output (or trigger with a spec that differs).
2. Confirm exactly one issue is created with the correct label and fingerprint comment.
3. Re-trigger — confirm no second issue is created.
4. Close the issue manually, re-trigger — confirm a new issue is created (closed issue is not deduped).

Step 4 is subtle: --state open in the dedup query means a closed drift issue won't block a new one. This is the correct behavior (regen PR merged, issue closed, future drift is a new event), but worth verifying explicitly.

---

Summary

The security model change is sound and well-executed. The one substantive finding is the unquoted heredoc expanding env vars that contain upstream-sourced content — low probability of exploit, but it's the only remaining code-execution path worth addressing. Everything else is polish. Approve with the medium finding noted for follow-up.

[TexasCoding]

Third-pass addressed:

Fixed

• 🟡 Heredoc expanded upstream-sourced env vars. Replaced cat <<EOF with scripts/render_drift_body.py using string.Template.substitute(os.environ) — literal-string substitution, no shell or template-syntax re-interpretation of values. Closes the one remaining code-execution path from upstream content. Smoke-tested locally with ADDED_PATHS='/v2/markets/$(rm -rf /)' — renders as literal text, no execution.
• 🟦 --body vs stdin — now using --body-file, sidesteps argv length limits on large diffs.
• 🟦 Dedup limit 100 → 200 — trivial bump.

Acknowledged, not changed

• The removed SHA-pinning rationale comment was intentional (bot agreed; the top-of-file security block covers it now that all permissions are narrow).
• Manual dedup test plan (open → re-run → close → re-run): can't trigger from this branch without scheduling the workflow against a real upstream change. Will validate on the first scheduled run after merge.

python3 -c "import yaml; yaml.safe_load(open('.github/workflows/spec-sync.yml'))" → clean.

[claude]

Code Review — PR #109: security(spec-sync): harden against upstream compromise

Overview

This PR closes supply-chain risk #92 by stripping the spec-sync workflow of write permissions and removing all CI steps that executed upstream-derived Python. Drift is now surfaced via a labeled, deduplicated GitHub issue; maintainers regenerate locally and open a PR by hand. The design is sound and the implementation is careful.

---

Positives worth calling out

• Permission reduction is correct. contents: read + issues: write is the minimum needed. The removed contents: write + pull-requests: write was the root exposure.
• string.Template instead of heredoc in render_drift_body.py is the right fix for the shell-expansion risk. Template values are never re-evaluated as code.
• All upstream-controlled values are gated through env: before entering the shell script. This is the canonical GitHub Actions defense against expression injection.
• set -euo pipefail in both new run: blocks — good hygiene.
• concurrency: group: spec-sync prevents the race condition where two simultaneous runs could both pass the dedup check and create duplicate issues. Already present; still correct.
• SHA pinning retained on actions/checkout and astral-sh/setup-uv.
• Removing the stale "pinned because contents:write" comment is correct — the security rationale changed and the updated top-of-file block replaces it.

---

Issues

1. spec-drift label must pre-exist — workflow silently fails if it doesn't (operational risk)

gh issue list --label spec-drift against a missing label returns an empty list (no error), so the dedup check passes silently. But gh issue create --label spec-drift fails with a non-zero exit if the label doesn't exist in the repo. With set -euo pipefail, this aborts the step — no issue is created, drift goes unreported, and the failure may be easy to miss on a weekly cron.

Recommendation: Either document that the spec-drift label must be created before first run, or add a guard step:

- name: Ensure spec-drift label exists
  env:
    GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
  run: |
    gh label create spec-drift --color 'e4e669' \
      --description 'Upstream spec changed' --force \
      --repo "${GITHUB_REPOSITORY}" 2>/dev/null || true

---

2. render_drift_body.py — substitute() raises bare KeyError on missing env var

string.Template.substitute raises KeyError if any ${VAR} in the template isn't in os.environ. If a workflow step upstream is skipped or its output is empty, the step fails with a raw Python traceback rather than an informative error message.

Recommendation: Wrap in a try/except:

try:
    sys.stdout.write(TEMPLATE.substitute(os.environ))
except KeyError as exc:
    sys.exit(f"render_drift_body.py: missing environment variable {exc}")

---

3. Minor: from __future__ import annotations is unused

render_drift_body.py imports from __future__ import annotations but has no type annotations in the file. Harmless, but unnecessary noise.

---

4. Minor: --limit 200 cap on dedup list

If more than 200 spec-drift issues are open simultaneously, the dedup check could miss an older match and create a duplicate. Extremely unlikely in practice (drift issues close when a sync PR merges with Closes #N). No change needed, just worth acknowledging.

---

5. Minor: body_file temp file is not cleaned up

body_file=$(mktemp)
python3 scripts/render_drift_body.py > "${body_file}"
# body_file never removed

Ephemeral runners make this a non-issue, but trap 'rm -f "${body_file}"' EXIT after the mktemp would be idiomatic.

---

Verdict

The security model is well thought-out and the implementation is solid. Items 1 and 2 are worth fixing before merge — the spec-drift label precondition is an operational footgun on first run, and the bare KeyError will produce a confusing CI failure if any upstream step's output is unexpectedly empty. Items 3–5 are take-it-or-leave-it.

[TexasCoding]

Fourth-pass addressed:

Fixed

• 🟡 Label precondition — added an "Ensure spec-drift label exists" step using gh label create --force (idempotent: updates if present, creates if not). Removes the silent-fail footgun where, if the label were ever deleted, weekly cron drift would go unreported.
• 🟡 Template.substitute KeyError — render_drift_body.py now catches KeyError and sys.exits with "render_drift_body.py: missing environment variable 'FOO'". CI gets a one-liner instead of a traceback. Verified locally: missing-var path exits 1 with the name in the message; happy path renders unchanged.
• 🟦 Unused from __future__ import annotations dropped.
• 🟦 Temp-file cleanup — added trap 'rm -f "${body_file}"' EXIT.

Skipped

• --limit 200 cap — bot agreed this is theoretical (drift issues close on regen-PR merge); no change.

yaml.safe_load clean. Ready for next pass.

[claude]

Code Review — PR #109: security(spec-sync): harden against upstream compromise

Overview

This PR addresses issue #92 (audit finding F-O-01): the old workflow had a path from upstream-controlled YAML into executed CI code (generate.py, ruff, mypy, pytest all ran on content derived from attacker-controllable specs). The fix is well-scoped and the design rationale is clearly documented. Overall this is solid security work.

---

What's done well

• Permissions reduction is correct. contents: write + pull-requests: write → contents: read + issues: write is the right call. The workflow no longer has any path to push branches or open PRs.
• Removing peter-evans/create-pull-request eliminates the highest-risk step: a third-party action executing attacker-influenced content with write permissions.
• Removing generate.py / ruff / mypy / pytest from CI eliminates the code-execution surface entirely. Maintainers now audit locally before opening a PR.
• render_drift_body.py using string.Template is the right tool here. Template substitution does not re-scan substituted values, so upstream-controlled content in ADDED_PATHS etc. can't inject new ${VAR} patterns. The docstring explaining why this approach was chosen is appreciated — it's security-critical code.
• Dedup fingerprint design (sha256 of sha256 sums, machine-parseable HTML comment) is clean and survives workflow re-runs correctly.
• gh label create --force is idempotent. The comment explaining the footgun it prevents is good.
• --body-file instead of --body avoids argv size limits and is the correct pattern for potentially large issue bodies.
• set -euo pipefail used consistently throughout new shell blocks.
• SHA pinning on third-party actions is preserved.

---

Issues and suggestions

Medium — Residual execution surface: extract_spec_meta.py still runs on upstream YAML

The "Extract spec metadata" step (uv run python scripts/extract_spec_meta.py) still executes Python against fetched upstream YAML. The PR test plan says "YAML parses cleanly via yaml.safe_load", which is good — yaml.safe_load blocks arbitrary Python object construction. But this step is now the only remaining code-execution surface touching upstream content.

This is a reasonable residual risk and probably acceptable, but it's worth explicitly calling out in the workflow comment (or the PR description) so future reviewers understand the trust boundary. Suggestion: add a one-line comment above the "Extract spec metadata" step noting that extract_spec_meta.py uses yaml.safe_load and only reads version/path/channel keys — no code generation.

Low — --limit 200 could miss deduplication in theory

gh issue list \
  --limit 200 \
  --json number,body \
  --jq '[.[] | select(.body | contains("fingerprint:..."))] | first | .number // empty'

If more than 200 open spec-drift issues exist (very unlikely in practice), a matching fingerprint in issue 201+ would be missed and a duplicate would be opened. A comment like # 200 is far more than any real dedup window needs would make the intent clear and head off future confusion.

Low — Issue title shell expansion of metadata values

title="Spec drift ${today}: openapi ${OLD_VERSION} → ${NEW_VERSION}"

OLD_VERSION and NEW_VERSION are sourced from upstream YAML (via extract_spec_meta.py → GITHUB_OUTPUT). Version strings are typically semver-ish (safe), but if Kalshi ever publishes something unexpected (e.g., a version containing a backtick or $()), the shell assignment could produce surprising behavior. Low practical risk, but you could harden by writing the title to a temp file too, or just accepting it as an acceptable residual.

Nit — Removed comment on SHA pinning rationale

The checkout step's SHA pinning comment was removed:

# Third-party actions pinned to SHA (not tag) because this workflow holds
# contents:write + pull-requests:write. ...

The top-of-file security model block now covers the general rationale, but the specific sentence "Other workflows in this repo use bare tags because their permissions are narrower" was useful for a reader wondering why this workflow pins but others don't. Consider folding that into the top comment.

Nit — render_drift_body.py: substitute vs safe_substitute

The try/except KeyError approach for missing env vars is correct. An alternative is TEMPLATE.safe_substitute(os.environ) which silently leaves unmatched placeholders as-is rather than raising. Given that a missing variable would produce a broken/incomplete issue body, the current hard-fail behavior is actually preferable — just noting this was a conscious choice.

---

Security assessment

The PR correctly closes the core supply chain path: upstream YAML → committed/PR'd code. The remaining execution surfaces (fetching YAML via sync_spec.py, parsing via extract_spec_meta.py with yaml.safe_load) are narrow and constrained. The fingerprint-based dedup prevents issue spam without requiring write-to-repo permissions.

No new security concerns introduced.

---

Summary

Approve with minor suggestions. The design is sound, the implementation is clean, and the documentation (top-of-file comment, docstring in render_drift_body.py) makes the security reasoning legible to future maintainers. The medium-priority item (documenting the extract_spec_meta.py trust boundary) is worth a follow-up comment or a small inline addition before merge.