chore(ci): medium/low findings from the 2026-07-28 full review [platform part] #349

Split from LeanerCloud/cloud-commitments-cli#1594 (cross-component issue, monorepo split)

This issue carries the findings that only apply to files now owned by this repo (terraform/, iac/, arm/, cloudformation/, Dockerfile*, docker-compose*.yml, Makefile.terraform, .env.example, .trivyignore, .snyk, .tflint.hcl, .hadolint.yaml, .dockerignore, and the deploy/migration/rollback workflows), plus this repo's own copy of the Makefile.

Re-verified against the current state of this repo (2026-09-28) before filing: several items from the original 2026-07-28 review have already been fixed since (ci-success treating a skipped job as success, persist-credentials on pre-commit.yml and the deploy workflows, the postgres:16-alpine digest pin in ci.yml and docker-compose.test.yml, and the unquoted-heredoc JSON payloads in deploy-aws-lambda.yml / deploy-aws-fargate.yml / rollback.yml, the last one closed as #1542). Those are dropped from the list below; only items still reproducing against the current tree are kept, with their line numbers re-checked.

The three findings about shared tooling files that are copied identically into all four split repos (pre-commit hook repos pinned to mutable tags, check-yaml excluding workflow files, and the .golangci.yml gocyclo threshold mismatch) are filed as this repo's own item within the same list, since this repo's copy independently reproduces them. cloud-commitments-go has a fourth item (its own Makefile still has the pre-split silent-pass bug that this repo's Makefile does not).

MEDIUM

• AWS Terraform IaC misconfigurations are ungated end to end

  • Where: .pre-commit-config.yaml:203 (entry: bash -c 'trivy config ... --skip-dirs terraform/environments/aws ...'); .github/workflows/ci.yml Trivy IaC scan (scan-type: 'config') still documented inline as using "the default exit-code 0 so misconfig findings are reported [to the Security tab, not gating]".
  • What: the local and CI pre-commit gate skips the entire AWS environment directory, and CI's Trivy IaC scan reports to the Security tab without failing the job.
  • Failure scenario: a CRITICAL misconfiguration introduced under terraform/environments/aws/ (a public S3 bucket, 0.0.0.0/0 ingress, an unencrypted RDS instance) fails no gate anywhere. .trivyignore additionally blanket-accepts 15 AVD rule IDs repo-wide rather than per-resource.
  • Fix direction: drop --skip-dirs terraform/environments/aws from the pre-commit trivy invocation (re-verify the Trivy version installed no longer panics on that directory), and give the CI IaC step exit-code: 1 for CRITICAL now that .trivyignore documents the accepted set.

• Makefile security and lint targets pass silently when the tool is absent, and never fail on findings

  • Where: Makefile lint (~:111-116), security-scan-go (~:157-166), security-scan-docker (~:168-176), security-scan-terraform (~:177-184), security-scan-snyk (~:230-239). All still @if command -v <tool> > /dev/null; then <tool> ...; else echo "<tool> not installed"; fi, and each then branch's exit status is the trailing success echo, not the tool's.
  • What: even when the tool runs and finds issues, the target exits 0; and when the tool is absent it exits 0 too.
  • Failure scenario: make security-scan prints success both on a machine with no gosec installed and on a machine where gosec found ten CRITICAL issues.
  • Fix direction: else echo "..."; exit 1; fi in each target, and chain the tool invocation with && (or drop the trailing success echo) so the tool's exit status propagates. security-scan-terraform (tfsec) should also be retired, since tfsec was removed from CI in favour of Trivy.

• make ci and make terraform-validate are broken and cannot serve as a local pre-flight

  • Where: Makefile terraform-validate loops for dir in terraform/environments/*/dev, invoked by ci.
  • What: terraform/environments/{aws,azure,gcp} are flat directories with .tf files at the top level (confirmed: terraform/environments/aws/ has no dev/ subdirectory today). The glob does not expand, cd fails, || exit 1 fails the target.
  • Failure scenario: the documented local CI pre-flight (make ci) cannot be used, so nobody runs it, so the never-failing security-scan target above it is never exercised locally either.
  • Fix direction: loop terraform/environments/* and skip non-directories, matching what ci.yml's own terraform-validate job already does correctly.

• Snyk is continue-on-error and outside the ci-success gate

  • Where: ci.yml snyk-scan job (continue-on-error: true), not present in ci-success's needs: list.
  • What: ci-success's skipped-job handling was already fixed (it now allowlists on result == "success" rather than denylisting failure/cancelled), but Snyk itself is still soft-fail and not a required job, so a CRITICAL dependency finding from Snyk never turns CI red.
  • Failure scenario: Snyk finds a CRITICAL vulnerability; the job step fails but continue-on-error: true marks the job green, and ci-success never looks at it since it isn't in needs. The PR merges.
  • Fix direction: add snyk-scan to ci-success's needs: and drop continue-on-error: true (or make the pass/fail explicit via the job's own outcome rather than swallowing it).

• Trivy filesystem scan and the coverage threshold are advisory only

  • Where: ci.yml Trivy scan-type: fs step (comment: "This scan uses the default exit-code 0 ... misconfig findings are reported"); coverage check still emits ::warning::Coverage is below 80% and never fails.
  • What: both appear as gates in the job list and neither is one.
  • Failure scenario: a CRITICAL dependency CVE surfaced by Trivy lands in the Security tab and CI stays green; the same PR drops backend coverage 20 points and CI is green on both counts.
  • Fix direction: set exit-code: '1' on the Trivy fs step (.trivyignore already exists to record accepted findings), and decide whether the 80% coverage line is a gate or documentation; if a gate, exit 1.

• docker-build's smoke test still rebuilds the image and cannot fail

  • Where: ci.yml docker-build job: docker/build-push-action now builds once with load: true and GHA caching (an improvement from the reviewed commit), but the "Test Docker image" step still runs a second, uncached docker build -t cudly:test . instead of reusing the cudly:${{ github.sha }} tag the previous step already loaded, and both smoke commands still end in || true.
  • What: the step passes even if the entrypoint crashes on start, and duplicates the most expensive step in the job.
  • Failure scenario: a change to scripts/entrypoint.sh makes every container exit immediately; docker-build still reports success.
  • Fix direction: reuse the already-loaded cudly:${{ github.sha }} tag in the smoke-test step instead of rebuilding, and drop || true so a non-zero --version / --help fails the job.

• Get migration version writes untrusted command output into $GITHUB_ENV

  • Where: .github/workflows/database-migration.yml "Get migration version" step: VERSION=$(migrate ... version 2>&1 || echo "unknown") then echo "MIGRATION_VERSION=$VERSION" >> "$GITHUB_ENV".
  • What: 2>&1 folds stderr into $VERSION. golang-migrate/pq error text is multi-line and can echo connection details; writing a multi-line value to $GITHUB_ENV in KEY=VALUE form either fails the step with "Invalid format" or, if the text contains a newline followed by NAME=value, injects an arbitrary environment variable into every subsequent step.
  • Failure scenario: a connection failure produces a multi-line error; the step fails with a confusing "Invalid format" instead of the real DB error, or an error string carrying attacker-influenced text sets an environment variable for later steps. Nothing reads MIGRATION_VERSION afterwards, so the step earns nothing in exchange.
  • Fix direction: capture stdout only, use the heredoc delimiter form for $GITHUB_ENV, or delete the step since nothing consumes the value.

• No Dependabot or Renovate configuration, and no CODEOWNERS

  • Where: .github/ contains only runbooks/ and workflows/, confirmed on the current tree; no dependabot.yml, no renovate.json, no CODEOWNERS.
  • What: this repo SHA-pins every Action, digest-pins every Docker base image, and version-pins every CI tool. That is good hygiene, and it becomes a liability without update automation.
  • Failure scenario: a CVE lands in a pinned base image or Action and nothing notices; without CODEOWNERS, workflow changes also require no security review.
  • Fix direction: add .github/dependabot.yml covering github-actions, docker, gomod and npm (frontend/); add CODEOWNERS covering .github/workflows/** and terraform/**. (This repo's copy of the finding; cloud-commitments-go, -cli and -mcp each need their own equivalent config since Dependabot/Renovate configs are per-repo.)

• Terraform version still pinned to divergent values across the pipeline

  • Where: ci.yml and pre-commit.yml pin 1.10.5; the six deploy/cleanup/destroy workflows now share a single TF_VERSION: '1.10.0' env var each (an improvement over hardcoding it per-step, but still one patch behind ci.yml); rollback.yml pins TF_VERSION: '1.6.0', four minors behind.
  • What: terraform validate runs on 1.10.5 in CI while terraform apply runs on 1.10.0 (or 1.6.0 for rollback), so a behavioural or provider-resolution difference can pass CI and still break at apply time.
  • Fix direction: define the version once (a repo-level variable or a .terraform-version file) and reference it from every workflow, including rollback.yml.

LOW

• .env.example ships a working admin password and API key

  • Where: .env.example:69 (ADMIN_PASSWORD_DEV=LocalDev!Pass123) and :71 (ADMIN_API_KEY_DEV=cudly-local-dev-api-key-not-for-prod).
  • What: clearly labelled dev placeholders, but they are functional bootstrap credentials, and the file's own header instructs readers to copy it to .env.local.
  • Failure scenario: an operator copies .env.example, fills in the cloud settings, misses these two lines, and deploys with a publicly known admin password.
  • Fix direction: replace both values with obviously non-functional placeholders (<set-me>) so the app fails loudly rather than starting with a known credential.

• deploy-all.yml failure check ignores cancelled child deployments

  • Where: .github/workflows/deploy-all.yml still tests only == "failure" for each of the four child deploy job results; the surrounding comments show this asymmetry with ci.yml (which does handle cancelled) is currently a deliberate, but undocumented-as-accepted, gap.
  • What: a cancelled reusable-workflow result passes the check and the job still prints "All deployments completed successfully!".
  • Failure scenario: a production multi-cloud deploy is cancelled mid-terraform apply, leaving infrastructure half-applied, and the summary reports success.
  • Fix direction: treat cancelled (and skipped-when-expected) as failure, matching ci.yml's own handling.

• .dockerignore omits the test compose/Dockerfile pair it excludes for the dev pair

  • Where: .dockerignore excludes docker-compose.yml and Dockerfile.dev but still not docker-compose.test.yml or Dockerfile.test.
  • What: both are COPY . .-ed into the builder stage. No secrets are involved, but it is unnecessary build context and an inconsistency in a file whose entire purpose is this.
  • Fix direction: add both to the "Docker compose dev files" block.

• Makefile.terraform clean-locks deletes provider lock files with no confirmation

  • Where: Makefile.terraform clean-locks target: find terraform/environments -name ".terraform.lock.hcl" -type f -delete.
  • What: .terraform.lock.hcl is the committed provider checksum pin. Deleting it and re-running init re-resolves providers to whatever the ~> constraint currently admits. The adjacent clean target is correctly scoped and its help text says "preserves .terraform.lock.hcl", so the intent is understood elsewhere in the same file.
  • Failure scenario: an operator runs clean-locks to clear a resolution problem, commits the regenerated lock, and the repo's provider supply-chain pinning silently moves to a newer, unreviewed provider build with no diff that reads as a supply-chain change.
  • Fix direction: require an explicit confirmation variable, and print a warning that the regenerated lock must be reviewed as a supply-chain change.

Findings shared with the other three split repos (this repo's own copy still reproduces them)

• Third-party pre-commit hook repos are pinned to mutable tags, not commit SHAs

  • Where: .pre-commit-config.yaml - rev: v1.105.0 (antonbabenko/pre-commit-terraform), rev: v6.0.0 (pre-commit/pre-commit-hooks), rev: v0.47.0 (igorshubovych/markdownlint-cli).
  • What: every GitHub Action in this repo is SHA-pinned with a version comment; the pre-commit hook repos, which execute arbitrary code in the same CI job, are pinned only to Git tags, which the upstream owner can move.
  • Fix direction: pin each to a full commit SHA with the version kept as a trailing comment, matching the Actions convention.

• check-yaml pre-commit hook excludes every workflow file

  • Where: .pre-commit-config.yaml check-yaml hook: exclude: '^(docker-compose.*\.yml|\.github/workflows/.*|cloudformation/.*|iac/.*/cloudformation/.*)$'.
  • What: the only YAML syntax check in the pipeline skips all workflow files, and there is no actionlint step anywhere.
  • Failure scenario: a malformed workflow is committed; GitHub silently refuses to run it (no failing check, no red signal).
  • Fix direction: drop \.github/workflows/.* from the exclusion and add an actionlint hook.

• .golangci.yml gocyclo threshold (15) differs from the enforcing gates (10)

  • Where: .golangci.yml gocyclo: min-complexity: 15 versus ci.yml's standalone gocyclo -over 10 -ignore "_test\.go" step and the Makefile's gocyclo -over 10 in complexity.
  • What: golangci-lint reports nothing for functions at complexity 11-15; the standalone gocyclo steps fail them.
  • Failure scenario: a function lands at complexity 11-15, passes make lint, and reddens Lint Code / pre-commit --all-files, blocking every other PR until fixed (a recorded regression pattern, tracked previously as fix(frontend): stop Enter on a column-filter button from sorting #1388).
  • Fix direction: set min-complexity: 10 so all tools agree.

Findings from the 2026-09-02 codebase audit

Re-verified against the current tree; all five are still current.

A13-013 (medium)

A correction to the .trivyignore AVD-AWS-0013 suppression. Its text claims TLS 1.0 "was tightened in a prior PR" and that the finding only trips because the value is "inherited rather than set explicitly". It is set explicitly: terraform/modules/frontend/aws/main.tf:113 is still minimum_protocol_version = var.acm_certificate_arn != null ? "TLSv1.2_2021" : "TLSv1", and the null branch is the default for every environment. The finding is live rather than an artifact; it is unexploitable only because enable_cdn = false in every shipped tfvars, which is a different reason from the one written down, so the suppression should be rewritten to that reason or the branch dropped. Related to #1627's minimum_protocol_version item, which tracks the code side. Audit finding A13-013.

A14-034 (medium)

The make ci / make terraform-validate item has a larger sibling: scripts/tf-deploy.sh:62 still builds ENV_DIR="${PROJECT_ROOT}/terraform/environments/${PROVIDER}/${PROFILE}", a path this repo does not have (environments are single flat roots selected by -var-file). The script then creates that directory and symlinks only main.tf into it, leaving a root with no variables.tf or backend.tf, and runs terraform init with no -backend-config. Makefile:deploy and every target in Makefile.terraform route through this script, so the documented local deployment path is inoperable in the same way the local CI pre-flight is. Fixing this alongside terraform-validate keeps one directory assumption rather than two. Audit finding A14-034.

A13-016 (low)

Two corrections to the suppression files. First, .trivyignore's GCP-0015 entry says the GCP database module "exposes require_ssl as a variable" and is "Tracked as a hardening follow-up to set require_ssl = true in the module default". No require_ssl variable exists anywhere under terraform/modules/database/gcp/; both ip_configuration blocks hardcode ssl_mode = "ENCRYPTED_ONLY", which already requires TLS. The entry is either stale against a fixed finding or masking a different one. Second, .snyk declares two keys the Snyk policy format does not read: failOnSeverity: high and a license allow/deny map. Both real enforcement points pass the threshold on the command line instead (Makefile and ci.yml both --severity-threshold=high), and nothing reads the license map, so a GPL-3.0 dependency would be admitted despite the file appearing to forbid it. Deleting the two inert blocks, or moving the license policy to a real enforcement point with a comment saying where it lives, removes the false assurance. Audit findings A13-016 and A13-017.

A13-021 (low)

Two more mutable-tag container references besides the postgres:16-alpine ones (which are now digest-pinned in both ci.yml and docker-compose.test.yml). docker-compose.yml's nginx:alpine floats a whole major, and dpage/pgadmin4:9.2 is minor-pinned but not digest-pinned. Scope is the local dev stack only, and the nginx half matters most because scripts/nginx.conf depends on the reverse-proxy behaviour. Worth pinning both in the same pass. Audit finding A13-021.

A14-037 (low)

The .golangci.yml gocyclo item covers the threshold axis; there is a second axis at a third site. Makefile's complexity target still runs gocyclo -over 10 . 2>&1 || true with no -ignore flag, while ci.yml passes -ignore "_test\.go" and .golangci.yml excludes _test\.go. So the local target fails on table-driven tests that the merge gate accepts. Adding -ignore "_test\.go" to the Makefile target belongs in the same change as setting min-complexity: 10. Separately, the 2>&1 || true capture on that line folds a gocyclo tool error into the "complexity issues" branch, so a missing binary reads as findings. Audit finding A14-037. (cloud-commitments-go's Makefile has the identical gap; see the sibling issue there.)

A14-041 (low)

A third gate belongs alongside the Trivy fs scan and the coverage threshold. ci.yml's tfvars format checks still end with terraform fmt -check "github-${env}.tfvars" || echo "Note: github-${env}.tfvars may need formatting", so nothing they find can fail the step, in a step named "Validate environment tfvars files". terraform fmt -check returns non-zero on a genuine parse error as well as on formatting, so a broken github-prod.tfvars reaches the deploy workflows with the validating job green. Simplest fix is to let the non-zero exit propagate, or fold these files into the existing terraform fmt -check -recursive step. Audit finding A14-041.

Acceptance criteria

• Each checklist item above is fixed (or explicitly re-triaged with a documented reason) with evidence that the specific gate now actually fails on the described scenario.
• The three shared-tooling items (mutable-tag pre-commit pins, check-yaml workflow exclusion, gocyclo threshold mismatch) are tracked and fixed consistently with the sibling copies of the same finding in cloud-commitments-cli, cloud-commitments-go, and cloud-commitments-mcp.