From 55dffb5b635b5096b02d8440b11041662d4cd248 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Thu, 8 Oct 2026 11:57:04 +0200 Subject: [PATCH] docs(14.3-02): add the phase 14.3 security review for plans 01 and 02 - ASVS L1 table (V5, V6, V10, V12, V14) and T-14.3-01..19 plus T-14.3-SC with controls, evidence and the plan 04 test that will pin each row - 17 mitigated, 3 accepted (low), 0 open; refusal probe results --- .../14.3-SECURITY-REVIEW.md | 96 +++++++++++++++++++ 1 file changed, 96 insertions(+) create mode 100644 .planning/phases/14.3-one-command-app-scaffolding-summer-new-and-summercms-io-new/14.3-SECURITY-REVIEW.md diff --git a/.planning/phases/14.3-one-command-app-scaffolding-summer-new-and-summercms-io-new/14.3-SECURITY-REVIEW.md b/.planning/phases/14.3-one-command-app-scaffolding-summer-new-and-summercms-io-new/14.3-SECURITY-REVIEW.md new file mode 100644 index 0000000..a358a3e --- /dev/null +++ b/.planning/phases/14.3-one-command-app-scaffolding-summer-new-and-summercms-io-new/14.3-SECURITY-REVIEW.md @@ -0,0 +1,96 @@ +--- +phase: 14.3 +slug: one-command-app-scaffolding +status: reviewed +threats_total: 20 +threats_closed: 20 +threats_open: 0 +accepted_risks: 3 +asvs_level: 1 +created: 2026-10-08 +reviewer: gsd-executor (14.3-02 Task 3, self-performed; see Reviewer Note) +--- + +# Phase 14.3 — Security Review (plans 01 and 02) + +> `summer new` (generator), the `curl | bash` bootstrap script in +> `modules/sunrise`, the site route `GET /new` and the D-20 acceptance harness. +> Every threat registered in 14.3-01-PLAN.md (T-14.3-01..10, SC) and +> 14.3-02-PLAN.md (T-14.3-11..19, SC) is mapped below to its source-level +> control and the evidence available now. Plan 04 adds rows T-14.3-20..26 and +> fills the "Pinned by (plan 04)" column with executed test names. + +**Date:** 2026-10-08 +**Scope:** `git diff v0.1.12..HEAD -- internal/build cmd/summer modules/lagoon modules/sunrise scripts/check-phase14.3.sh` in `summercms.go` (15 files, +1520/-15), and the site plugin commit `2157bb9` (`script.go`, `plugin.go`, `routes_test.go`, `README.md`) in `sm-summercmsio-plugin`. Read alongside `../sm-summercmsio-app/deploy/nginx/summercms.io.conf`. +**Level:** ASVS L1. A high finding blocks the phase. + +## Reviewer Note + +The plan names a security-review lane; this session had no separate auditor agent, so the plan 02 executor performed the review directly against the diff. Evidence was re-executed during the review, not inherited: the Task 1 verify chain (smoke, truncation, docs tree, plugin route and the gated `TestBootstrapEndToEnd`), the plan 02 `--acceptance` container run, and a throwaway-repository probe of every refusal path of `new.sh` (results in "Refusal probe" below). + +## Method + +1. Read every changed line in the scope above, with attention to: input validation of `` in the script and in `build.ValidateAppName`; quoting of every expansion in `new.sh` (`shellcheck` clean except one deliberate SC2016 for the literal `$PATH` in the hint); behaviour under `curl | bash` (stdin, truncation, `curl -f` on HTTP errors); tag and ancestry handling; refusal paths that protect local work; write locations; `.env` handling and the first commit; compose port binding; MIME type and `nosniff` on `/new`; nginx passthrough; the acceptance harness's use of the host Docker socket. +2. Mapped each registered threat to a control (file and function) and to evidence. +3. Probed the script's refusal paths against a fake framework repository (module `git.golem15.com/golem15/summercms`, tags `v0.0.1`, `v0.0.2`). + +## Verdict + +20 threats in scope (T-14.3-01..19 and T-14.3-SC): 17 mitigated, 3 accepted (all low), **0 open**. No high finding; no code fix was needed. + +## ASVS L1 + +| ASVS | Applies | Control | Evidence | +|------|---------|---------|----------| +| V5 Input Validation | yes | `new.sh` `main`: `^[a-z][a-z0-9]*$` before any path use; `internal/build/new.go` `ValidateAppName`: same regex plus `token.IsKeyword`, `main`, `internal`; `--framework` and siblings accepted only when `go.mod` declares the framework module (`checkFramework`); `SUMMERCMS_REF` starting with `-` rejected (`target_commit`) | `TestNewAppSmoke`; refusal probe (bad name exit 2); plan 01 Task 2 rules check | +| V6 Cryptography | yes | `modules/lagoon/keygen.go` `GenerateAppKey` on `crypto/rand`, 32 bytes; `NewApp` `validateAppKey` rejects anything else | `TestGenerateAppKeyIs32ByteBase64`, `TestEncryptedKeyGeneratePrints32ByteBase64` | +| V10 Malicious Code | yes | Script embedded from the framework release (`sunrise.Script`); `curl -fsSL`; HTTPS default clone URL; release tags only; tags fetched without `--force`; ancestry check; no `eval`, no `sudo`, no dynamic code strings | `TestScriptTruncationRunsNothing`, `TestBootstrapEndToEnd`, refusal probe, `--acceptance` | +| V12 Files and Resources | yes | Script writes only `SUMMERCMS_DIR` (clone), `sm--app` (via `summer new`) and the Go install directory; parent must already exist; `NewApp` refuses an existing target (`os.Lstat`), creates with `os.Mkdir`, removes only what it created; `.env` 0600 | `TestScriptTruncationRunsNothing` (empty work dir), `--generator`, refusal probe | +| V14 Configuration | yes | Secrets only in `.env` (gitignored before `git add -A`); `.env.example` without a key; compose publishes `127.0.0.1:54320`; `/new` served `text/plain; charset=utf-8` with `X-Content-Type-Options: nosniff` | `--generator` (`git ls-files .env` empty, mode 600), `TestNewScriptRoute` | + +## Threat Register + +| Threat ID | Category | Component | Severity | Disposition | Control (file / function) | Evidence now | Status | Pinned by (plan 04) | +|-----------|----------|-----------|----------|-------------|---------------------------|--------------|--------|---------------------| +| T-14.3-01 | Tampering | app name, target path | high | mitigate | `internal/build/new.go` `ValidateAppName` before `filepath.Join(parent, appDirName(name))`; target is always `/sm--app` | `TestNewAppSmoke`; plan 01 rules check (RULES-OK) | mitigated | TestNewValidateAppName | +| T-14.3-02 | Information Disclosure | `.env`, first git commit | high | mitigate | `internal/build/new.go` `planNewApp` (`.env` mode 0600, `.gitignore` written with the files) and `initGit` (`git add -A` after `.gitignore` exists) | `scripts/check-phase14.3.sh --generator` asserts `git ls-files .env` empty and mode 600; acceptance run committed the app the same way | mitigated | TestNewEnvFileMode, TestNewGitFirstCommit | +| T-14.3-03 | Information Disclosure | app key generation | high | mitigate | `modules/lagoon/keygen.go` `GenerateAppKey` (`crypto/rand`); `internal/build/new.go` `validateAppKey` | `TestGenerateAppKeyIs32ByteBase64`; `--generator` (44-character key) | mitigated | TestGenerateAppKey, TestNewRequiresValidAppKey | +| T-14.3-04 | Tampering | existing directory, cleanup | high | mitigate | `internal/build/new.go` `NewApp`: `os.Lstat` refusal, `os.Mkdir` (not `MkdirAll`), `os.RemoveAll` only of the directory this run created; `new.sh` `main` also stops early when `sm--app` exists (`-e` or `-L`) | refusal probe: existing `sm-demo-app` stops the script before any clone, exit 1 | mitigated | TestNewRefusesExistingTarget, TestNewCleansUpOnFailure | +| T-14.3-05 | Information Disclosure | compose.yaml Postgres port | medium | mitigate | `internal/build/stubs/new.tmpl` compose: `127.0.0.1:54320:5432`, named volume only | `--generator` and `--acceptance` ran the compose file; template grep | mitigated | TestNewGolden | +| T-14.3-06 | Spoofing | framework auto-detection | medium | mitigate | `internal/build/new.go` `resolveFramework` / `checkFramework`: module path must be `git.golem15.com/golem15/summercms`; an explicit `--framework` never falls through; the script always passes `--framework` | `TestNewAppSmoke`; `--generator` (sibling detection) | mitigated | TestNewFrameworkResolution | +| T-14.3-07 | Information Disclosure | committed go.mod / go.work | low | mitigate | `internal/build/scaffold.go` `relModulePath`, relative `runGoWorkUse` | `--generator` (no absolute `use` line); `TestBootstrapEndToEnd` (relative `../summercms` replace) | mitigated | TestNewGolden | +| T-14.3-08 | Information Disclosure | compose.yaml dev credentials | low | accept | `summer`/`summer` is a documented local default for a loopback-only database; the encryption key stays in `.env` | template review | accepted | TestNewGolden | +| T-14.3-09 | Information Disclosure | `summer serve` default `:8080` | low | accept | Existing surf default listens on all interfaces; next steps point at `127.0.0.1`; out of scope for this phase | review | accepted | — | +| T-14.3-10 | Repudiation | fallback git identity | low | accept | `internal/build/new.go` `initGit` / `gitIdentityMissing`: fallback only for the missing keys, first commit only, announced with a warning | `--generator` and `--acceptance` printed the warning | accepted | TestNewGitUsesConfiguredIdentity | +| T-14.3-11 | Tampering | truncated download | high | mitigate | `modules/sunrise/new.sh`: top level holds only comments and function definitions; the last line is `main "$@"`; `main` parses arguments before any external command | `TestScriptTruncationRunsNothing` (every line-boundary prefix and every byte prefix of the final line, with stub `git`, `go`, `docker`, `curl`, `summer`; positive control proves the stubs record calls); `TestScriptShape` | mitigated | TestScriptTruncationRunsNothing, TestScriptShape | +| T-14.3-12 | Tampering | error body piped to bash | medium | mitigate | Documented one-liner uses `curl -fsSL` (no body on HTTP errors); `GET /new` answers only the embedded bytes (`script.go` `scriptHandler`); a cut-off body is covered by T-14.3-11 | `TestNewScriptRoute`; `TestBootstrapEndToEnd` runs the exact `curl -fsSL … \| bash -s -- demo` form | mitigated | TestScriptBehavior | +| T-14.3-13 | Spoofing | `/new` rendered as HTML | medium | mitigate | `sm-summercmsio-plugin` `script.go` `scriptHandler`: `Content-Type: text/plain; charset=utf-8`, `X-Content-Type-Options: nosniff` via `setCommonHeaders`; no request data reflected; nginx allows GET/HEAD only (`limit_except GET HEAD`) and gzips `text/plain` | `TestNewScriptRoute` (headers, strong ETag, 304), `TestRoutesAssemble` (`POST /new` 405) | mitigated | TestNewScriptRoute | +| T-14.3-14 | Tampering | moved upstream tag, non-fast-forward | high | mitigate | `new.sh` `checkout_framework`: `git fetch --quiet --tags origin` without `--force` (git refuses to clobber a local tag and the script stops); checkout only when `git merge-base --is-ancestor HEAD `; otherwise exit 1 with the checkout unchanged | refusal probe: moved `v0.0.2` upstream → fetch refused, local tag kept at its original commit, HEAD unchanged; non-ancestor HEAD → "not a fast-forward", exit 1, HEAD unchanged | mitigated | TestScriptBehavior | +| T-14.3-15 | Tampering | user's existing checkout | high | mitigate | `new.sh` `check_checkout`: refuses a non-directory, a missing or foreign `go.mod` module, a non-git directory or a subdirectory of another repository, tracked changes (`git status --porcelain --untracked-files=no`), a branch (`git symbolic-ref -q HEAD`); the script contains no reset, clean, stash, `--force` or delete | refusal probe: branch checkout, dirty tracked file and non-framework directory each refused with exit 1 and left untouched | mitigated | TestScriptBehavior | +| T-14.3-16 | Elevation of Privilege | shell injection via name or overrides | high | mitigate | `new.sh` `main`: name regex before use; every expansion quoted (`shellcheck` clean); overrides used only as arguments (`git clone -- "$repo" "$dir"`, `-C "$dir"`); `SUMMERCMS_REF` rejected when it starts with `-`; no `eval`, `sudo` or dynamic code strings; `summer new` re-validates the name (`ValidateAppName`) | `grep -Ecw 'sudo\|eval'` = 0; `shellcheck`; refusal probe (bad name exit 2) | mitigated | TestScriptShellcheck, TestScriptBehavior | +| T-14.3-17 | Denial of Service | credential prompt hang with piped stdin | medium | mitigate | `new.sh` `main` exports `GIT_TERMINAL_PROMPT=0`; every `git`, `go`, `docker` and `summer` child runs with ``; script run from a work directory holding `summercms`: + +| Case | Result | +|------|--------| +| checkout on branch `master` | `error: … is on branch master; this installer only moves a detached release checkout …`, exit 1, untouched | +| detached, tracked file modified | `error: … has uncommitted changes to tracked files …`, exit 1, untouched | +| upstream `v0.0.2` moved to a new commit | fetch refused, `error: git fetch … failed (a release tag may have moved upstream); the checkout was not changed`; local `v0.0.2` still at the original commit (e343b45, upstream ea6dd36); HEAD unchanged | +| HEAD has a local commit not reachable from `v0.0.2` | `error: moving … is not a fast-forward; the checkout was not changed`, exit 1, HEAD unchanged | +| `sm-demo-app` already exists | `error: … already exists`, exit 1, before any git command | +| name `Demo` | `error: invalid application name Demo …`, exit 2 | +| no argument / `--help` | usage, exit 2 / 0 | +| `summercms` exists without `go.mod` | `error: … is not a SummerCMS framework checkout (no go.mod) …`, exit 1 | + +## Notes (no finding) + +- Write locations: besides `SUMMERCMS_DIR`, `sm--app` and the Go install directory, `go install` and `summer new` write to the user's Go build and module caches (`GOCACHE`, `GOMODCACHE`). That is the Go toolchain's normal state, not a script write. +- `SUMMERCMS_REPO`, `SUMMERCMS_DIR` and `SUMMERCMS_REF` come from the invoking user's own environment; they are not a trust boundary. git's default `protocol.allow` already refuses `ext::` URLs. +- Tags are not signed; the release checkout trusts HTTPS to `git.golem15.com` plus the no-clobber fetch (trust on first use). Signed-tag verification is a possible later hardening, not an L1 requirement. +- On a fresh clone whose target cannot be resolved (no release tag, unknown `SUMMERCMS_REF`), the clone stays on its default branch and the error says to remove it and run again; a re-run would otherwise refuse it as a branch checkout. The script never deletes it itself.