Skip to content

fix(security): resolve Phase 3 review findings #2 (git auth off-argv) and #5 (admin-only credential lists) - #7

Merged
tyler-rich merged 1 commit into
mainfrom
claude/phase-3-security-review-8xmikc
Jul 3, 2026
Merged

fix(security): resolve Phase 3 review findings #2 (git auth off-argv) and #5 (admin-only credential lists)#7
tyler-rich merged 1 commit into
mainfrom
claude/phase-3-security-review-8xmikc

Conversation

@tyler-rich

@tyler-rich tyler-rich commented Jul 3, 2026

Copy link
Copy Markdown
Owner

Resolves two findings from the Phase 3 security review. Review-only pass identified these; this PR fixes them. No other Phase 3 behavior changes.

Finding #2 — generic-host git credential no longer visible in the process list

Trivy clones repos with go-git, which never shells out to the system git binary and therefore ignores GIT_ASKPASS, .netrc, and credential helpers. For generic (non-GitHub/GitLab) private hosts, Trivy's only credential channel is the clone URL — so the old code embedded username:token in the URL passed as a positional argv to trivy repo, making the credential visible via /proc/<pid>/cmdline to any process in the container's PID namespace.

Per the committed decision spec (docs/reviews/phase3-finding2-resolution.md, Option 1), generic HTTPS hosts are now cloned ourselves:

  • Clone into a fresh tmpfs working dir with the system git binary.
  • The credential is delivered through a transient GIT_ASKPASS helper (mode 0700) that echoes it from the clone subprocess's own environment — never in argv, never in the parent process env, never in the script file, never persisted.
  • The requested branch/commit/tag is checked out; Trivy then scans the local checkout (no URL credential anywhere near Trivy).
  • The helper and checkout are shredded/removed in a finally on success, failure, or cancellation.
  • git is added to the runtime image.
  • GitHub/GitLab are unchanged — they keep Trivy's native GITHUB_TOKEN/GITLAB_TOKEN env path (already off-argv).

Clone failures raise an operator-safe error that never echoes git's stderr (which can contain the request URL).

Finding #5 — credential metadata is now admin-only

GET /api/registries and GET /api/git-credentials (full metadata — host, username, provider) are now admin-only; operators previously had read access. Because launching a scan still needs an operator to pick a credential by name, two new operator endpoints return only {id, name}:

  • GET /api/registries/options (enabled registries)
  • GET /api/git-credentials/options

The New Scan page's credential pickers now use these option endpoints. No host, username, provider, auth type, or secret is exposed to operators.

Two adaptations from the spec (both preserve its security mechanics)

The spec assumed Alpine/apk + sync subprocess; Scrye is Debian/apt + an async worker. Logged in docs/PLAN.md §14:

  1. Askpass helper is 0700, not 0600 — git execs it, so a non-executable helper fails with EACCES. Still owner-only.
  2. Clone runs through the existing async run_command seam on the container's tmpfs /tmp (matching docker_config_env), not a blocking subprocess.run or a bespoke mount — so it doesn't block the event loop.

Tests

  • New test_credentials.py cases assert the credential never appears in the clone argv, is delivered only via env, the askpass file is 0700 at clone time, the workspace is shredded on both success and scan-exception, token-only auth maps correctly, commit checkout is issued, and a clone-failure message leaks no credential.
  • test_targets_api.py updated: operators get 403 on the full lists and 200 on the /options endpoints, which expose only {id, name}.
  • Full backend suite: 148 passed, 3 skipped (skips are pre-existing "frontend not built"). ruff/black/.env.example sync clean. Frontend eslint/prettier/build clean.

See docs/PLAN.md § Deviations for the two dated entries.

Finding #2 — generic-host git credential no longer rides in the process
argv. Trivy clones with go-git, which ignores GIT_ASKPASS/.netrc/credential
helpers, so generic (non-GitHub/GitLab) private repos previously embedded
username:token in the clone URL passed to `trivy repo` — visible via
/proc/<pid>/cmdline. Generic HTTPS hosts are now cloned into tmpfs with the
system `git` binary via a transient GIT_ASKPASS helper (mode 0700; echoes the
credential from the clone subprocess's own environment, never argv, never the
parent env, never the script file, never persisted), the requested ref is
checked out, and Trivy then scans the local checkout. The helper and checkout
are shredded/removed in a finally block on success, failure, or cancellation.
`git` is added to the runtime image. GitHub/GitLab keep Trivy's native
GITHUB_TOKEN/GITLAB_TOKEN env path (already off-argv).

Finding #5 — registry and git-credential list endpoints (full metadata) are
now admin-only; operators previously had read access to host/username/provider.
Two new operator endpoints (GET /registries/options, GET /git-credentials/
options) return only {id, name} for scan-launch selection, exposing no
credential metadata. The New Scan page uses these option endpoints.

Implements docs/reviews/phase3-finding2-resolution.md (Option 1). See
docs/PLAN.md §14 for both dated deviation entries and the two spec adaptations
(askpass 0700 not 0600 since git execs it; async run_command on the existing
tmpfs seam instead of sync subprocess).
@tyler-rich
tyler-rich merged commit b34d9a5 into main Jul 3, 2026
2 checks passed
tyler-rich added a commit that referenced this pull request Jul 3, 2026
…sion)

Revises the locked stack decision from Python 3.12 to 3.13 to resolve the
Grype-flagged CPython interpreter CVEs (CVE-2026-7210, -6100, -4224, -3298,
-3644, -9669, -4786, ...) whose fixes exist only in Python 3.13+; the 3.12 base
image was already the latest, so moving off 3.12 is the only real fix. Chose 3.13
over 3.14 for dependency-ecosystem maturity.

- Dockerfile: base image python:3.12-slim-bookworm -> python:3.13-slim-bookworm
  (digest-pinned) for both the venv-builder and runtime stages.
- pyproject.toml: requires-python >=3.13; black/ruff target-version py313.
- CI: backend job runs on Python 3.13.
- Remove the ci/grype.yaml ignore for the CPython interpreter binary so it is
  scanned and gated like everything else (the CI dogfood scan now confirms the
  interpreter CVEs are actually gone on 3.13, not suppressed).
- Update the locked decision in CLAUDE.md and docs/PLAN.md (§0 #7, §2), mark the
  prior interpreter-exclusion deviation superseded, and add a dated deviation
  entry with the 3.13-over-3.14 rationale.

Verified on Python 3.13.12: clean install of all deps (incl. cryptography and
other C-extension/Rust wheels), full 325-test suite passing, a full Alembic
upgrade/downgrade/upgrade cycle, and a clean app import, with no new 3.13-specific
warnings (only the pre-existing Starlette HTTP_422 deprecation).
@tyler-rich
tyler-rich deleted the claude/phase-3-security-review-8xmikc branch July 6, 2026 06:43
tyler-rich added a commit that referenced this pull request Jul 9, 2026
…8xmikc

fix(security): resolve Phase 3 review findings #2 (git auth off-argv) and #5 (admin-only credential lists)
tyler-rich added a commit that referenced this pull request Jul 9, 2026
…sion)

Revises the locked stack decision from Python 3.12 to 3.13 to resolve the
Grype-flagged CPython interpreter CVEs (CVE-2026-7210, -6100, -4224, -3298,
-3644, -9669, -4786, ...) whose fixes exist only in Python 3.13+; the 3.12 base
image was already the latest, so moving off 3.12 is the only real fix. Chose 3.13
over 3.14 for dependency-ecosystem maturity.

- Dockerfile: base image python:3.12-slim-bookworm -> python:3.13-slim-bookworm
  (digest-pinned) for both the venv-builder and runtime stages.
- pyproject.toml: requires-python >=3.13; black/ruff target-version py313.
- CI: backend job runs on Python 3.13.
- Remove the ci/grype.yaml ignore for the CPython interpreter binary so it is
  scanned and gated like everything else (the CI dogfood scan now confirms the
  interpreter CVEs are actually gone on 3.13, not suppressed).
- Update the locked decision in CLAUDE.md and docs/PLAN.md (§0 #7, §2), mark the
  prior interpreter-exclusion deviation superseded, and add a dated deviation
  entry with the 3.13-over-3.14 rationale.

Verified on Python 3.13.12: clean install of all deps (incl. cryptography and
other C-extension/Rust wheels), full 325-test suite passing, a full Alembic
upgrade/downgrade/upgrade cycle, and a clean app import, with no new 3.13-specific
warnings (only the pre-existing Starlette HTTP_422 deprecation).
tyler-rich added a commit that referenced this pull request Jul 9, 2026
…8xmikc

fix(security): resolve Phase 3 review findings #2 (git auth off-argv) and #5 (admin-only credential lists)
tyler-rich added a commit that referenced this pull request Jul 9, 2026
…sion)

Revises the locked stack decision from Python 3.12 to 3.13 to resolve the
Grype-flagged CPython interpreter CVEs (CVE-2026-7210, -6100, -4224, -3298,
-3644, -9669, -4786, ...) whose fixes exist only in Python 3.13+; the 3.12 base
image was already the latest, so moving off 3.12 is the only real fix. Chose 3.13
over 3.14 for dependency-ecosystem maturity.

- Dockerfile: base image python:3.12-slim-bookworm -> python:3.13-slim-bookworm
  (digest-pinned) for both the venv-builder and runtime stages.
- pyproject.toml: requires-python >=3.13; black/ruff target-version py313.
- CI: backend job runs on Python 3.13.
- Remove the ci/grype.yaml ignore for the CPython interpreter binary so it is
  scanned and gated like everything else (the CI dogfood scan now confirms the
  interpreter CVEs are actually gone on 3.13, not suppressed).
- Update the locked decision in CLAUDE.md and docs/PLAN.md (§0 #7, §2), mark the
  prior interpreter-exclusion deviation superseded, and add a dated deviation
  entry with the 3.13-over-3.14 rationale.

Verified on Python 3.13.12: clean install of all deps (incl. cryptography and
other C-extension/Rust wheels), full 325-test suite passing, a full Alembic
upgrade/downgrade/upgrade cycle, and a clean app import, with no new 3.13-specific
warnings (only the pre-existing Starlette HTTP_422 deprecation).
tyler-rich added a commit that referenced this pull request Jul 9, 2026
…8xmikc

fix(security): resolve Phase 3 review findings #2 (git auth off-argv) and #5 (admin-only credential lists)
tyler-rich added a commit that referenced this pull request Jul 9, 2026
…sion)

Revises the locked stack decision from Python 3.12 to 3.13 to resolve the
Grype-flagged CPython interpreter CVEs (CVE-2026-7210, -6100, -4224, -3298,
-3644, -9669, -4786, ...) whose fixes exist only in Python 3.13+; the 3.12 base
image was already the latest, so moving off 3.12 is the only real fix. Chose 3.13
over 3.14 for dependency-ecosystem maturity.

- Dockerfile: base image python:3.12-slim-bookworm -> python:3.13-slim-bookworm
  (digest-pinned) for both the venv-builder and runtime stages.
- pyproject.toml: requires-python >=3.13; black/ruff target-version py313.
- CI: backend job runs on Python 3.13.
- Remove the ci/grype.yaml ignore for the CPython interpreter binary so it is
  scanned and gated like everything else (the CI dogfood scan now confirms the
  interpreter CVEs are actually gone on 3.13, not suppressed).
- Update the locked decision in CLAUDE.md and docs/PLAN.md (§0 #7, §2), mark the
  prior interpreter-exclusion deviation superseded, and add a dated deviation
  entry with the 3.13-over-3.14 rationale.

Verified on Python 3.13.12: clean install of all deps (incl. cryptography and
other C-extension/Rust wheels), full 325-test suite passing, a full Alembic
upgrade/downgrade/upgrade cycle, and a clean app import, with no new 3.13-specific
warnings (only the pre-existing Starlette HTTP_422 deprecation).
tyler-rich added a commit that referenced this pull request Jul 26, 2026
…s structure, correct a false CVE claim in the CHANGELOG (#102)

* docs(archive): make §14 contiguous, add a newest-first index, and add the finding-ID decoder

Twelve dated §14 entries — every one from 2026-07-09 onward, including all the
recent work — sat underneath `## Build performance` rather than under §14, so
anyone scrolling §14 to the end stopped short of them. Moved the Build
performance section to the end of the file instead of re-parenting the entries:
it is self-contained and cross-referenced by heading name (from CLAUDE.md and
four workflows), not by position, so nothing breaks. All 104 dated entries are
now under §14.

Added a newest-first index at the top of §14, one anchored line per entry. The
entries themselves are deliberately NOT reordered: sixteen of them refer to each
other relatively ("the entry below", "superseded by the entry above"), and a
sort would silently invert every one. The three ordering regimes are documented
instead, and the index is sorted by date regardless of physical position, so
lookup no longer depends on the scroll order.

Added §15, a finding-ID index: one row per SEC/SC/APIR/CON/P1-P3/D/R/QUA/INF/
FE/API/FEAT/DOC/SCN id with a one-line description and its resolving PR. §14
cites these ids bare and never re-explains them; this is the decoder that
replaces the docs/reviews/ reports. It also records the SEC-* prefix collision
between the two reviews that reused it.

Corrected §0 locked decision #7: it said CVE-2025-15366 and CVE-2025-15367 are
both unfixable on 3.14. That is true of released 3.14.6 but not of the 3.14
line — the imaplib backport landed on the maintenance branch and closes on
3.14.7 (issue #98). Only the poplib CVE remains 3.15-only (#52).

* docs(changelog): correct the CVE-2025-15366 claim under [Unreleased]

The Python 3.14 entry said all four waived CPython CVEs remain unfixable until
3.15 because upstream declined the backport to 3.10-3.14, and pointed at issue
#52 for all of them. Both halves are false, and this text ships verbatim as the
next release's notes.

Checked against ci/grype.yaml and the two 2026-07-26 §14 entries: the imaplib
backport for CVE-2025-15366 merged onto the CPython 3.14 branch on 2026-07-07 —
18 days before the entry was written — so it closes on 3.14.7, not 3.15, and it
was regrouped into Group A alongside CVE-2026-15308 and CVE-2026-12003, tracked
in issue #98. Only CVE-2025-15367 (poplib) is genuinely 3.15-only and still
tracked in #52.

What was true and is kept: released 3.14.6 carries neither guard, so the upgrade
cleared nothing at the pinned version.

* docs: strike completed roadmap items, surface the settings-level work, add two process rules

ROADMAP:
- Struck "Pin GitHub Actions to commit SHAs" (done in #57 — ci.yml has 8
  SHA-pinned uses:, dev-nightly 3, publish 3, rescan 2) and "Frontend test
  runner" (done in #78 — vitest 3.2.7, "test": "vitest run", 20 test files).
- Rewrote "Row-bound secret AAD", which was false as stated: row binding is
  implemented (secret_store.py row_aad(), L1/SEC-7, #64) and every write is
  row-bound. What remains is only the bulk re-encryption of legacy column-only
  ciphertext so the read fallback can be dropped, so it is folded into the
  existing "Admin bulk secret re-encryption" item.
- Extended the public-repo governance checklist with five settings-level items
  that existed only in §14 prose and were therefore invisible: Actions workflow
  permissions -> read-only, confirm GHCR package visibility is public (the
  original check asked for Private and its premise inverted when the repo went
  public), delete the unused DOCKERHUB_* secrets, set the GitHub profile display
  name to tyler-rich, and confirm Dependabot security alerts are on.

CLAUDE.md § Git & PR conventions gains two rules learned the hard way: a stacked
child PR retargeted after its parent was squash-merged needs git rebase --onto
(flipping the base in the UI re-computes the merge base and balloons the diff),
and on: pull_request does not fire on 'edited', so a base change never re-runs
CI and the green check you are looking at is from the old base.

CONTRIBUTING § Releasing gains a "Before you tag" checklist — CHANGELOG
[Unreleased] reviewed (it ships verbatim), THIRD_PARTY_LICENSES verified against
the versions actually pulled, Dependabot PRs triaged, requirements.lock
regenerated — plus the two after-tag steps: back-merge main into dev, and re-run
rescan.yml.

* docs: delete docs/reviews and docs/upgrades, sweep every inbound reference

The twelve review reports and the Python 3.14 handoff doc held only closed
findings, and sat at the same directory level as the two live documents. They
are removed rather than moved to a docs/history/ subtree: the only real argument
against deleting them was that §14 cites their finding ids bare and never
re-explains them, and §15 (previous commit) answers that directly. Git is the
archive for the rest. docs/ now contains exactly ARCHIVE.md, ROADMAP.md, and
screenshots/.

The originals stay retrievable — the pre-deletion commit is
0780b07, and the §14 entry records the
git show incantation. Nothing was rewritten before deletion.

Swept every inbound reference the audit enumerated, plus the ones it did not:
CONTRIBUTING § Project layout (both directories dropped, screenshots/ added),
CONTRIBUTING § API conventions, CHANGELOG's L13/APIR-8 citation, and both
dependabot.yml D3 comments now point at ARCHIVE §15. Inside §14, 59
docs/reviews/ and 7 docs/upgrades/ path prefixes were stripped so the entries
name the reports as documents rather than as paths that no longer resolve, with
a note at the top of §14 sending the reader to §15. Fixed the dead
claude-md-compliance-review.md link (a filename that never existed). Left one
docs/upgrades/ mention deliberately: the 2026-07-25 entry's record of what
CONTRIBUTING's layout listing omitted is a statement about that date, marked
'(as it then was)'.

Verified no workflow, test, or source file referenced either directory, and that
the four '§ Build performance' cross-references are by heading name and survive
that section's move.

Added the dated §14 entry recording all of it, including that docs/history/ was
considered and rejected, and two corrections to the audit: twelve entries were
misfiled under Build performance (not fourteen — the other two are that
section's own sub-headings), and there are 42 remote branches with 36 prunable
(not 39/33).
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.

1 participant