Skip to content

[Security hardening] Add automated security audit workflow#2442

Open
PascalThuet wants to merge 9 commits intogithub:mainfrom
PascalThuet:codex/add-security-audit-workflow
Open

[Security hardening] Add automated security audit workflow#2442
PascalThuet wants to merge 9 commits intogithub:mainfrom
PascalThuet:codex/add-security-audit-workflow

Conversation

@PascalThuet
Copy link
Copy Markdown
Contributor

@PascalThuet PascalThuet commented May 2, 2026

Summary

  • Add a dedicated Security Audit workflow for dependency and static-analysis checks.
  • Audit committed hashed requirements on push, pull request, and manual dispatch so ordinary CI stays deterministic.
  • Verify committed audit requirements are refreshed when dependency inputs change, while keeping live dependency resolution for the scheduled audit across the supported Python and OS matrix.
  • Run Bandit for high-severity findings with a scoped baseline for the accepted shell-step B602 finding.
  • Pin all repository GitHub Actions to full commit SHAs.
  • Harden extension, preset, integration, and workflow downloads with bounded reads, strict HTTPS redirects, optional sha256 verification, bounded ZIP extraction, and source-path traversal guards.

Security context

This creates a repeatable CI baseline for dependency advisories and high-severity static-analysis issues while preserving a scheduled live-resolution signal for newly published dependency problems. It also closes similar supply-chain and archive-handling gaps found during follow-up review.

Closes #2438

Validation

  • git diff --check
  • uvx ruff check on all changed Python surfaces and tests
  • uv run python -m pytest tests/test_download_security.py tests/test_extensions.py tests/test_presets.py tests/integrations/test_integration_catalog.py tests/test_security_workflow.py tests/test_upgrade.py -q
  • uv run python -m pytest -q
  • uv pip compile pyproject.toml --extra test --universal --generate-hashes --quiet --no-header --output-file , compared with .github/security-audit-requirements.txt
  • uvx --from pip-audit==2.10.0 pip-audit --disable-pip --require-hashes -r .github/security-audit-requirements.txt --progress-spinner off
  • uvx --from bandit==1.9.4 bandit -r src -lll --baseline .github/bandit-baseline.json

@PascalThuet PascalThuet marked this pull request as ready for review May 2, 2026 06:45
@PascalThuet PascalThuet requested a review from mnriem as a code owner May 2, 2026 06:45
@mnriem mnriem requested a review from Copilot May 4, 2026 12:39
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new GitHub Actions workflow to introduce automated security checks for the Python codebase, aiming to catch dependency advisories and high-severity static-analysis findings in CI alongside the existing test/lint workflows.

Changes:

  • Add .github/workflows/security.yml with a dedicated Security Audit workflow.
  • Run pip-audit on pushes to main, pull requests, a weekly cron, and manual dispatch.
  • Run Bandit against src/, temporarily skipping B602 pending the shell-step hardening tracked in #2440.
Show a summary per file
File Description
.github/workflows/security.yml New CI workflow that adds dependency-audit and static-analysis jobs for the Python project.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 1/1 changed files
  • Comments generated: 5

Comment thread .github/workflows/security.yml Outdated
Comment thread .github/workflows/security.yml Outdated
Comment thread .github/workflows/security.yml Outdated
Comment thread .github/workflows/security.yml Outdated
Comment thread .github/workflows/security.yml Outdated
Copy link
Copy Markdown
Collaborator

@mnriem mnriem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address Copilot feedback. If not applicable, please explain why

@PascalThuet
Copy link
Copy Markdown
Contributor Author

Addressed the Copilot feedback in the latest push:

  • Dependency auditing now exports the locked runtime + test extra dependency set with uv export --extra test --frozen --no-emit-project, then audits the generated requirements file.
  • pip-audit and Bandit are pinned via uvx --from (pip-audit==2.10.0, bandit==1.9.4).
  • Removed the global Bandit --skip B602; the existing shell execution call sites now use targeted # nosec B602 suppressions with local context.
  • Added CONTRIBUTING.md security-check commands so contributors can reproduce the workflow locally.

Local validation:

uv export --quiet --extra test --frozen --format requirements.txt --no-emit-project --output-file /tmp/spec-kit-audit-requirements.txt
uvx --from pip-audit==2.10.0 pip-audit -r /tmp/spec-kit-audit-requirements.txt --progress-spinner off
uvx --from bandit==1.9.4 bandit -r src -lll
git diff --check

Results: pip-audit reported no known vulnerabilities, Bandit reported no issues, and git diff --check passed.

@PascalThuet
Copy link
Copy Markdown
Contributor Author

Added automated regression coverage for the security workflow in tests/test_security_workflow.py.

The new tests statically verify that:

  • dependency auditing exports the locked runtime + test extra requirements before running pip-audit;
  • pip-audit and Bandit remain pinned via uvx --from;
  • Bandit does not use a global --skip B602;
  • CONTRIBUTING.md documents the same security commands.

Validation after this commit:

uv run python -m pytest tests/test_security_workflow.py -q
uv export --quiet --extra test --frozen --format requirements.txt --no-emit-project --output-file /tmp/spec-kit-audit-requirements.txt
uvx --from pip-audit==2.10.0 pip-audit -r /tmp/spec-kit-audit-requirements.txt --progress-spinner off
uvx --from bandit==1.9.4 bandit -r src -lll
git diff --check

All passed locally.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (1)

src/specify_cli/init.py:414

  • This second # nosec B602 suppresses Bandit for the non-capturing branch as well, so neither path through run_command will ever raise B602 again. If the intent is only to defer the current finding until #2440, the skip needs to stay in the workflow/configuration layer rather than permanently muting this call site in source.
            # shell=True is only available to callers that opt in explicitly.
            subprocess.run(cmd, check=check_return, shell=shell)  # nosec B602
  • Files reviewed: 5/5 changed files
  • Comments generated: 5

Comment thread .github/workflows/security.yml Outdated
Comment thread CONTRIBUTING.md Outdated
Comment thread tests/test_security_workflow.py Outdated
Comment thread src/specify_cli/workflows/steps/shell/__init__.py Outdated
Comment thread src/specify_cli/__init__.py Outdated
@mnriem
Copy link
Copy Markdown
Collaborator

mnriem commented May 5, 2026

Please address Copilot feedback. If not applicable, please explain why. Note the shell step should indeed be ignored

Copy link
Copy Markdown
Collaborator

@mnriem mnriem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments above

@PascalThuet
Copy link
Copy Markdown
Contributor Author

Addressed the follow-up review in e1e8051:

  • Removed --frozen from the dependency export because this repo intentionally does not commit uv.lock; the workflow and contributor docs now use the same clean-checkout-safe command.
  • Replaced source-level # nosec B602 suppressions with a CI-level Bandit baseline at .github/bandit-baseline.json.
  • Kept that baseline scoped to exactly one accepted finding: B602 in ShellStep, matching the maintainer note that the shell step should be ignored.
  • Hardened the generic run_command helper so shell=True is rejected instead of suppressed, which removes the similar non-shell-step B602 sink entirely.
  • Expanded tests/test_security_workflow.py to catch these regressions: no lockfile flags, pinned tools, no global --skip B602, one-entry Bandit baseline, no source # nosec B602, docs in sync, and run_command(shell=True) rejection.

Validation:

uv export --quiet --extra test --format requirements.txt --no-emit-project --output-file /tmp/spec-kit-audit-requirements.txt
uvx --from pip-audit==2.10.0 pip-audit -r /tmp/spec-kit-audit-requirements.txt --progress-spinner off
uvx --from bandit==1.9.4 bandit -r src -lll --baseline .github/bandit-baseline.json
uv run python -m pytest tests/test_security_workflow.py tests/test_workflows.py -q
uvx ruff check src/
git diff --check

I also verified the uv export command in a temporary clean directory without uv.lock; it succeeds before running the audit.

@PascalThuet
Copy link
Copy Markdown
Contributor Author

One more small cleanup after re-review: pushed 6f1da27 to use uv pip compile pyproject.toml --extra test for the audit requirements instead of uv export.

Reason: it still audits the runtime + test extra dependency set, but it avoids both requiring and generating a project uv.lock, which makes the CI and CONTRIBUTING.md command cleaner for this repo.

Revalidated locally:

uv pip compile pyproject.toml --extra test --quiet --output-file /tmp/spec-kit-audit-requirements.txt
uvx --from pip-audit==2.10.0 pip-audit -r /tmp/spec-kit-audit-requirements.txt --progress-spinner off
uvx --from bandit==1.9.4 bandit -r src -lll --baseline .github/bandit-baseline.json
uv run python -m pytest tests/test_security_workflow.py -q
git diff --check

All passed.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (1)

tests/test_security_workflow.py:91

  • Bandit doesn't require the exact literal # nosec B602 to suppress this finding; plain # nosec, #nosec, and multi-ID forms also disable B602. This check leaves those suppression paths untested, so a future source-level bypass can slip in without failing CI.
        )

    def test_b602_is_not_suppressed_in_source(self):
        source_text = "\n".join(
            path.read_text(encoding="utf-8")
            for path in (REPO_ROOT / "src").rglob("*.py")
        )
  • Files reviewed: 5/5 changed files
  • Comments generated: 3

Comment thread .github/workflows/security.yml Outdated
Comment thread CONTRIBUTING.md Outdated
Comment thread tests/test_security_workflow.py Outdated
@mnriem
Copy link
Copy Markdown
Collaborator

mnriem commented May 5, 2026

Please address Copilot feedback

@PascalThuet
Copy link
Copy Markdown
Contributor Author

Addressed the latest Copilot review in 129d19e:

  • Dependency audit now runs on the supported OS/Python matrix: ubuntu-latest and windows-latest across Python 3.11, 3.12, and 3.13.
  • The requirements output path in CI now uses ${{ runner.temp }}, and the contributor docs use a relative spec-kit-audit-requirements.txt path instead of /tmp, so the commands are Windows-compatible.
  • The security workflow regression test now verifies GitHub Actions are pinned to full 40-character commit SHAs.
  • The Bandit suppression guard now catches any # nosec form, not only the exact # nosec B602 spelling.
  • The dependency audit now compiles hashed requirements and runs pip-audit --disable-pip --require-hashes, avoiding a second pip resolution step.

Validation:

uv run python -m pytest tests/test_security_workflow.py -q
uv pip compile pyproject.toml --extra test --python-version 3.11 --generate-hashes --quiet --output-file /tmp/spec-kit-audit-py311.txt
uv pip compile pyproject.toml --extra test --python-version 3.12 --generate-hashes --quiet --output-file /tmp/spec-kit-audit-py312.txt
uv pip compile pyproject.toml --extra test --python-version 3.13 --generate-hashes --quiet --output-file /tmp/spec-kit-audit-py313.txt
uvx --from pip-audit==2.10.0 pip-audit --disable-pip --require-hashes -r /tmp/spec-kit-audit-py311.txt --progress-spinner off
uvx --from pip-audit==2.10.0 pip-audit --disable-pip --require-hashes -r /tmp/spec-kit-audit-py312.txt --progress-spinner off
uvx --from pip-audit==2.10.0 pip-audit --disable-pip --require-hashes -r /tmp/spec-kit-audit-py313.txt --progress-spinner off
uvx --from bandit==1.9.4 bandit -r src -lll --baseline .github/bandit-baseline.json
git diff --check

All passed locally.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 5/5 changed files
  • Comments generated: 5

Comment thread .github/workflows/security.yml Outdated
Comment thread CONTRIBUTING.md Outdated
Comment thread tests/test_security_workflow.py
Comment thread src/specify_cli/__init__.py Outdated
Comment thread .github/workflows/security.yml
Copy link
Copy Markdown
Collaborator

@mnriem mnriem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address Copilot feedback

@PascalThuet
Copy link
Copy Markdown
Contributor Author

Addressed the latest Copilot review round in commit aa02062:

  • Push/PR/manual dependency audits now use committed hashed requirements at .github/security-audit-requirements.txt for deterministic CI.
  • Scheduled audits still perform live resolution across the supported Python/OS matrix.
  • CONTRIBUTING now runs pip-audit from the committed requirements file and documents how to refresh it without creating a root-level artifact.
  • The security workflow tests now assert triggers, deterministic audit behavior, action SHA pins, hashed committed requirements, and the absence of run_command(shell=...).
  • PR validation notes now match the Bandit baseline command used by CI.

Local validation passed:

  • git diff --check
  • uv run python -m pytest tests/test_security_workflow.py -q
  • uv run python -m pytest -q
  • uvx ruff check src tests/test_security_workflow.py
  • uvx --from pip-audit==2.10.0 pip-audit --disable-pip --require-hashes -r .github/security-audit-requirements.txt --progress-spinner off
  • uvx --from bandit==1.9.4 bandit -r src -lll --baseline .github/bandit-baseline.json

The new GitHub workflow runs are currently waiting for fork PR approval (action_required).

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 6/6 changed files
  • Comments generated: 2

Comment thread .github/workflows/security.yml
Comment thread tests/test_security_workflow.py
@mnriem
Copy link
Copy Markdown
Collaborator

mnriem commented May 5, 2026

Please address Copilot feedback

@PascalThuet
Copy link
Copy Markdown
Contributor Author

Added a follow-up hardening commit: 1f85b92.

This addresses the similar risk surfaces we discussed after the Copilot review:

  • All repository workflows now pin third-party actions to full commit SHAs, guarded by tests/test_github_workflows.py.
  • Extension/preset downloads now use bounded reads and optional catalog sha256 verification.
  • Extension/preset ZIP installs now use a shared safe extractor with traversal, Windows-drive, symlink, file-count, per-file-size, and total-size checks.
  • Extension/preset/workflow catalog fetches now use strict HTTPS redirect handling for download/catalog paths.
  • Extension command source files and manifest-declared paths now reject traversal before registration.

Local validation passed:

  • git diff --check
  • uvx ruff check src tests/test_download_security.py tests/test_github_workflows.py tests/test_registrar_path_traversal.py tests/test_extensions.py tests/test_presets.py tests/test_security_workflow.py
  • uv run python -m pytest tests/test_download_security.py tests/test_github_workflows.py tests/test_registrar_path_traversal.py tests/test_extensions.py tests/test_presets.py tests/test_workflows.py -q
  • uv run python -m pytest tests/test_security_workflow.py -q
  • uv run python -m pytest -q (2781 passed, 34 skipped)
  • uvx --from pip-audit==2.10.0 pip-audit --disable-pip --require-hashes -r .github/security-audit-requirements.txt --progress-spinner off
  • uvx --from bandit==1.9.4 bandit -r src -lll --baseline .github/bandit-baseline.json

The latest GitHub workflow runs are again waiting for fork PR approval (action_required).

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 24/24 changed files
  • Comments generated: 5

Comment thread src/specify_cli/presets.py
Comment thread src/specify_cli/extensions.py
Comment thread src/specify_cli/presets.py
Comment thread .github/workflows/security.yml
Comment thread src/specify_cli/_download_security.py
@mnriem
Copy link
Copy Markdown
Collaborator

mnriem commented May 5, 2026

Please address Copilot feedback. Thank you once again for working through all these reviews!

@PascalThuet
Copy link
Copy Markdown
Contributor Author

Addressed the latest Copilot review feedback in commit 4599155.

What changed:

  • Reject Windows drive-relative paths such as C:secret.md in preset/extension command paths and ZIP member names.
  • Use bounded reads for catalog JSON in extension, preset, and integration catalogs, plus remaining CLI download surfaces that still had raw response reads.
  • Add a committed requirements sync check for PR/push runs when dependency inputs change, using GitHub-provided diff SHAs with a conservative fallback.
  • Regenerate .github/security-audit-requirements.txt without the uv header so the workflow sync comparison is stable.
  • Add regression coverage for max ZIP entries, total uncompressed ZIP size, bounded catalog reads, stale audit requirements checks, and a source scan preventing new bare resp.read()/response.read() calls under src/specify_cli.

Validation run locally:

  • git diff --check
  • uvx ruff check on all changed Python surfaces and new tests
  • uv run python -m pytest tests/test_download_security.py tests/test_extensions.py tests/test_presets.py tests/integrations/test_integration_catalog.py tests/test_security_workflow.py tests/test_upgrade.py -q: 629 passed
  • uv run python -m pytest -q: 2797 passed, 34 skipped
  • uv pip compile ... --no-header compared cleanly against .github/security-audit-requirements.txt
  • pip-audit on the committed requirements: no known vulnerabilities found
  • Bandit with the committed baseline: no issues identified

I attempted to resolve the five Copilot threads directly, but the GitHub integration returned Resource not accessible by integration, so they will need to be marked resolved by someone with repository permissions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Security hardening] Add automated security audit checks for Python dependencies and static analysis

3 participants