Audit: unquoted config in remote shell, unvalidated environment names, fence epoch fails open, and naming-contract violations

[vishr]

Findings from a full-repo audit at ed5f1c4. Baseline is clean: go vet, golangci-lint run (repo config), go test -race ./..., govulncheck, and osv-scanner all pass. Everything below came from extra linters and from reading the paths that construct remote commands, derive names, and manage fencing.

Status

Each item is tracked as a sub-issue, so the progress bar above this body is the
real state rather than something that has to be kept in sync by hand. Severity
is this audit's own assessment, revised where later work changed it.

#	Item	Severity	State
#53	1 — Unquoted config value reaches a remote shell	high	fixed in #52 (81d0ead)
#54	2 — Environment names are never validated	low	fixed in #73
#55	3 — Fence epoch fails open on an unreadable epoch file	high impact, low probability	fixed in #85 (95c6fd7)
#56	4 — Four derived names use hyphen joins the naming contract forbids	low-moderate	open
#57	5 — Stale timer sweep skips removal when disable fails	moderate	open
#58	6 — Runtime names carry no environment	high	fixed in #71
#59	7 — internal/engine/protection_units.go has no production implementation	dead code	removed in #51 (f9ef6bc)
#60	8 — Exported symbols with no references	dead code	removed in #51 (f9ef6bc)
#61	9 — Smaller items	low	open
#62	10 — Linters worth adding to the gate	low	open — unused enabled in #51; five others outstanding

The sections below are the original audit, left as written so the reasoning that
produced each item stays readable. Two assessments have since changed and are
corrected in place at the items concerned: item 1's payload constraints, and
item 2's severity.

The remaining audit work is #56, #57, #61 and #62. Of those, #57 is the
highest-impact behavior defect; the others are lower-priority naming,
correctness-cleanup and tooling work.

---

1. Unquoted config value reaches a remote shell

Fixed in #52. One correction to what follows: gURLPath also refuses
spaces, which this write-up does not mention, so a payload has to be
space-free. /health;id below works; a touch /tmp/x variant would not. That
narrows the shape of an attack and changes nothing about whether one exists.
The sibling in generate.go was also worse than rated here — see the comment
thread.

internal/engine/verify.go:303

cres, err := e.T.Run(ctx, fmt.Sprintf("curl -fsS -m 5 http://%s:%d%s", ip, port, chk.HTTP))

chk.HTTP is interpolated without quoting. Its grammar, gURLPath in internal/app/constraints.go:68, rejects ' " $ `, space, and control characters, but permits ;, |, &, ( and ). A value of /health;id passes validation and the trailing command runs on the target through the deploy transport.

Two things suggest this is an omission rather than a decision:

• ip is guarded against metacharacters three lines above (verify.go:300), so the surrounding code is aware of the risk on this line.
• The adjacent chk.Exec branch at verify.go:311 does quote, via q(chk.Exec).

Suggested fix: quote the whole constructed URL rather than the path alone.

cres, err := e.T.Run(ctx, "curl -fsS -m 5 "+q(fmt.Sprintf("http://%s:%d%s", ip, port, chk.HTTP)))

Same class, smaller blast radius because it executes inside the container rather than on the host: internal/app/generate.go:576 builds a CMD-SHELL healthcheck from the same unquoted URL shape.

2. Environment names are never validated

Downgraded to low after #51. This item was rated on the environment name
reaching systemd unit filenames. That path was protection_units.go, which
#51 deleted as dead code. ProtectionTimerForEnvironment now has one caller,
internal/onebox/protected_identity.go:58, which seals the derived names into
a JSON identity record rather than executing them. The name still reaches
notification payloads, journals and error strings — no shell, no filename. The
validation gap is real and worth closing for consistency; it is no longer a
security finding.

internal/app/validate.go:20-24

Every other map key in the spec is checked against gIdent:

Block	Check
backup_targets	validate.go:26
external_services	validate.go:34
workloads	validate.go:42
services	validate.go:50
environments	none

The loop over p.Environments validates each environment's contents but never its key. That key reaches systemd unit filenames through Names.ProtectionTimerForEnvironment, the X-Onebox-Environment= metadata in renderProtectionServiceUnit, and the ownership prefix compared in ReconcileProtectionUnits.

Suggested fix: add gIdent.check("environments."+name, name) alongside the existing validateEnvironment call.

3. Fence epoch fails open on an unreadable epoch file

internal/engine/lock.go:56-61, and the same shape in internal/engine/protection_lock.go:162-167

eres, err := e.T.Run(ctx, "cat "+q(e.epochPath())+" 2>/dev/null || echo 0")
...
prev, _ := strconv.Atoi(strings.TrimSpace(eres.Stdout))
epoch := prev + 1

An epoch file that is missing, unreadable, truncated, or non-numeric collapses to prev = 0, and the next epoch becomes 1. The comment at lock.go:52-55 states that fencing depends on a strictly increasing epoch, so this silently reissues an epoch a stale runner may still hold in its fence value.

This is also the inconsistent half of a pair. lockAgeCmd directly below (lock.go:176-194) goes to considerable length to fail closed for exactly this threat, including the "absence has to be established, not assumed" arm.

Two sub-parts:

• Distinguish "no epoch file yet" from "epoch file unreadable or malformed". The first is legitimately 0; the second should refuse.
• lock.go:77 persists the epoch with echo N > file, which is neither atomic nor durable. A partial write leaves an empty file, which then reads back as 0. internal/release/secrets.go:217 already uses mktemp + mv for the same kind of write.

4. Four derived names use hyphen joins the naming contract forbids

internal/app/names.go

The header comment at names.go:15-20 explains that hyphen joins are ambiguous — ob-<app>-<service> maps both (a-b, c) and (a, b-c) to ob-a-b-c — and that this is why persistent names use join (underscore) and runtime names use runtimeName (authored - escaped to --).

Four functions bypass both:

• names.go:176 ProtectionTimer
• names.go:180 ProtectionTimerForEnvironment
• names.go:103 ProtectionCredentialFile
• names.go:123 ProtectionEnvelopePath

gIdent permits hyphens (^[a-z]([a-z0-9-]{0,38}[a-z0-9])?$), so the collision is reachable. Systemd unit names are host-global, which makes the two timer functions the ones that matter most.

ProtectionTimer has no callers and can simply be deleted; see item 8.

5. Stale timer sweep skips removal when disable fails

internal/engine/schedule.go:93

"systemctl disable --now %s.timer >/dev/null 2>&1 && rm -f /etc/systemd/system/%s.timer /etc/systemd/system/%s.service"

The && ties removal to a successful disable. A unit systemd cannot disable leaves both files on disk, and because the command runs under mutateChecked the non-zero status aborts the deploy — so the sweep neither completes nor leaves the host in the state the next run expects. The disable and the removal want separate handling.

---

6. Runtime names carry no environment, and nothing guards two environments on one host

ComposeProject() is the bare application name (names.go:56), Container(workload, replica) is <app>-<workload>-<n> (names.go:187), and WorkloadVolume is ob_<app>_<workload>_<volume> (names.go:152). None carry the environment.

Meanwhile Environment.BasePath (types.go:71) is a per-environment override, and the host owner record is application-scoped (internal/engine/host_owner.go:69) — it compares owner != e.Spec.Name and nothing else. So pointing staging at production's server passes ownership, and passes preflight too, because preflight looks for foreign resources and these names are the application's own. The result is staging adopting production's containers and volumes while writing releases under a different base path.

The model is also inconsistent with itself: ProtectionTimerForEnvironment does carry the environment, and ProtectionTimer (the variant without it) is dead.

Either record the environment alongside the application in the host owner record and refuse a mismatch, or state one-environment-per-host as a documented constraint. Right now it is neither enforced nor written down.

---

7. internal/engine/protection_units.go has no production implementation

The file is 274 lines. ProtectionUnitTarget (:61), ReconcileProtectionUnits (:124), and InspectProtectionUnits (:203) are referenced only from protection_units_test.go, which supplies an in-memory memoryProtectionSystemd double. There is no implementation of the interface outside tests and no caller in cmd/ or internal/.

Live scheduling goes through internal/engine/schedule.go instead, which writes units directly and has its own ownership convention.

Worth resolving one way or the other, since the two files encode different rules for the same host resource: protection_units.go embeds X-Onebox-* ownership metadata and refuses to overwrite foreign units, while schedule.go matches on filename prefix alone.

8. Exported symbols with no references

Zero references anywhere in internal/, cmd/, or e2e/:

Symbol	Location
release.Activate	internal/release/release.go:202
Spec.ComposeRefsOf	internal/app/compose.go:356
InspectProtectionUnits	internal/engine/protection_units.go:203
LoadMigrationBackupOverride	internal/onebox/backup_evidence.go:928
transport.NewSSH	internal/transport/ssh.go:43
Names.ProtectionTimer	internal/app/names.go:176
Rendered.Runnable	internal/app/generate.go:36
SaveScheduledOperationEnvelope	internal/onebox/scheduled_envelope.go:388
ScheduledArtifactsAsProtectionResources	internal/onebox/scheduled_install.go:125
ActiveVolumeRecord.Selection	internal/onebox/active_volume.go:146
SortedRouteKeys	internal/app/generate.go:739

SortedRouteKeys carries the comment "exported for tests that assert label determinism", but no test calls it. release.Activate is the surprising entry — presumably superseded by the manifest activation path.

These are invisible to the unused linter because they are exported, so they sit alongside the 13 unexported findings that .golangci.yml already documents as held back.

---

9. Smaller items

• internal/notify/notify.go:97 uses http.NewRequest rather than NewRequestWithContext. The webhook ignores operation cancellation; only the 5s client timeout bounds it. Line 85 also assigns body, contentType := []byte(nil), "application/json" and then reassigns both in each branch.
• internal/transport/ssh.go:252 type-asserts err.(*ssh.ExitError) instead of using errors.As. A wrapped exit error would be reported as a transport failure rather than surfacing its exit code — which matters here, because several call sites branch on res.ExitCode for expected non-zero statuses. Same shape at cmd/ob/preview.go:135 for *app.Error.
• %v where %w is intended, so callers cannot errors.Is through: internal/engine/bootstrap.go:60, internal/engine/gate.go:260, internal/engine/lock.go:79.
• cmd/ob-scheduled-runner/main.go:77 calls os.Exit(130), skipping defer stop().

10. Linters worth adding to the gate

.golangci.yml states that the gate should be green from the first run and should name what it does not yet check. These five are close to that bar today:

Linter	Findings	Non-test
errorlint	6	4
noctx	9	3
copyloopvar	1	1
unconvert	3	3
intrange	4	2

errorlint and noctx overlap with items 1, 9, and the *ssh.ExitError assertion above, so they would have caught real findings rather than style. Reproduce with:

golangci-lint run --no-config --default=none \
  --enable=errorlint,noctx,copyloopvar,unconvert,intrange ./...

contextcheck, gocritic, and prealloc were also run; their findings are style or false positives against this codebase's conventions and are not included here.

[vishr]

Items 7 and 8 are done on chore/remove-dead-code (0350d6c, plus review fixes in 4ed08a7). 868 deletions across 23 files. Items 1–6, 9 and 10 are untouched and still open.

What was removed

Item 7 — internal/engine/protection_units.go. Deleted along with its test, 527 lines. ProtectionUnitTarget had no implementation outside the in-memory memoryProtectionSystemd double, and nothing in cmd/ or internal/ reached GenerateProtectionUnits, ReconcileProtectionUnits, or InspectProtectionUnits. Live systemd unit management stays with internal/engine/schedule.go, which is reachable and covered. Recoverable from history if the work resumes — worth noting that the two files encoded different ownership rules for the same host resource, and the deleted one had the stronger scheme (X-Onebox-* metadata plus a foreign-unit refusal, versus schedule.go matching on filename prefix alone). That difference is item 5's territory, not this cleanup's.

Item 8 — all 11 exported symbols, plus the 13 unexported ones unused reported, including the status-output sanitisation layer in internal/onebox/status.go.

Enforcement so this does not recur

unused is now enabled. .golangci.yml had held it back with a note saying the findings were staged work and that deciding their fate "is not this gate's call" — that call has now been made, so the note and the exemption both go.

Verified it actually runs rather than assuming the config change took effect: adding func zzDeadProbe() int { return 1 } to internal/engine/plan.go and running golangci-lint run ./internal/engine/... reports

internal/engine/plan.go:529:6: func zzDeadProbe is unused (unused)

unused does not see exported identifiers, which is the gap that let all 11 accumulate. just dead-exports covers that half and currently reports checked 362 exported identifiers, 0 unreferenced. It is deliberately outside check and ci: an exported helper may legitimately land before its caller, and the match is on identifier name alone, so two types with a same-named method count as one. A report that under-counts is a fine prompt to go and look and a bad rule to fail a build on.

Two things left behind on purpose

DeploymentProposal.Preconditions (internal/onebox/types.go:134) is still declared. proposalPreconditions was the only thing that could have populated it and it was dead, so the field has always serialised as its zero value. It is part of the published proposal schema, which makes removing it a schema change rather than a cleanup. Worth a decision, but not this branch's.

statusIssueCodes, canonicalStatus, statusDigest survive in status.go because tests reference them. With sanitizeStatus and proposalPreconditions gone they have no production caller, so status.go is now a test-only island of 95 lines. unused counts them used and will not flag this; deleting the tests to force the issue seemed like the wrong call to make silently.

Verification

Gate	Result
just check (fmt, vet, test, docs-generate-check, site-build)	All checks passed
golangci-lint config verify + run ./...	0 issues
go test -race -count=1 ./...	pass
just vuln	0 vulnerabilities in reachable code
just dead-exports	0 unreferenced

Not pushed and no PR opened yet.

[vishr]

Item 1 is fixed in #52 (8444786, review fixes in fa63de1). Items 2–6, 9 and 10 remain open. Items 7 and 8 landed in #51, merged as f9ef6bc.

Confirmed, with a correction to the original report

Reproduced end-to-end through the real loader rather than a struct literal, so what is proven is that a project file can carry the payload.

One correction to my report above: gURLPath also refuses spaces, which I did not mention. A payload therefore has to be space-free — /health;id from the original write-up works, /health;touch /tmp/x does not. That narrows the shape of an attack and changes nothing about whether one exists. The payload used in the tests is /healthz;id>/tmp/ob-owned.

internal/engine/verify.go emitted, verbatim:

curl -fsS -m 5 http://172.20.0.5:7500/healthz;id>/tmp/ob-owned

on the target, as whatever account deploys.

The sibling in internal/app/generate.go is worse than I rated it. I called it "lower severity, runs inside the container". That is true about the blast radius and wrong about the exposure — the rendered healthcheck is:

test:
  - CMD-SHELL
  - '[ -f /tmp/ob-drain ] && exit 1; curl -fsS http://127.0.0.1:8080/healthz;id>/tmp/ob-owned || wget -qO- ... || exit 1'

which the container runs on every health probe, for the life of the container, not once at deploy time.

The fix

Both sites quote the constructed URL as one argument. The exec forms beside them stay unquoted deliberately — those are commands the author wrote and means to run — and that distinction is now written down at both sites rather than left implicit.

Two things deliberately unchanged:

• The address check in verify.go. Quoting now covers the address too, so the check is redundant against injection. It stays because an address holding a shell metacharacter means the docker inspect returned something the code does not understand, and refusing says so where quoting would carry on and fail later as a confusing curl error.
• gURLPath itself. Narrowing a published schema pattern would reject projects that are valid today, and buys nothing once the value is quoted everywhere it lands. Item 2 is still the right place to fix validation gaps.

The fix is complete, not just local

A fix that leaves a sibling unfixed is not a fix, so every other remote command was swept for the same defect. Every other authored value reaching a shell is one of:

Category	Examples
Quoted through q()	most of internal/engine
Filtered through a regex before use	volume names via volName in ops.go:88, services.go:294
Constrained by gIdent / gSignal to characters no shell reads specially	workload names, drain signals, unit names
An authored command by design	hook run, health exec

The two sites fixed here were the only data values going in raw.

Review pass

Re-reading my own diff turned up three things, fixed in fa63de1:

1. A comment stating the opposite of the code three lines below it. It claimed the container address is "checked above rather than quoted here as well" — it is quoted here as well, inside the same URL. Worse than no comment; rewritten.
2. url as the name for a shell-quoted string. It is shell syntax, not a URL. The next person to use it outside that one command would carry the quotes into whatever they built. Renamed quotedURL.
3. An awkward test assertion. It checked the quoted form was present and separately that an unquoted form was absent. The check has two arms (curl, falling back to wget), so the property is that both carry the path quoted — now counted, which also catches a half-fix that quotes one arm and leaves the other open.

Both tests were confirmed non-vacuous by reverting each fix and re-running, not assumed:

--- FAIL: TestVerifyHTTPPathIsQuotedIntoTheRemoteCommand
    verification probe does not carry the authored path as one quoted argument:
    curl -fsS -m 5 http://172.20.0.5:7500/healthz;id>/tmp/ob-owned

--- FAIL: TestHTTPHealthPathIsQuotedInsideItsShellCheck
    the authored health path is quoted in 0 of the check's 2 arms

Operational consequence

The rendered healthcheck string changes, so three corpus digests in contract-verdicts.json move with it. The first deploy after upgrading will show a compose diff and roll any workload with an health.http probe, even with nothing else changed. Unavoidable — the old string is the bug — and it happens once. Worth a release note.

Verification

Gate	Result
just check	All checks passed
golangci-lint run ./...	0 issues
go test -race -count=1 ./...	pass
just vuln	0 vulnerabilities in reachable code
just dead-exports	0 unreferenced

just e2e was not run locally; CI runs it as its own job.

[vishr]

#52 is merged as 81d0ead, with all five checks green on fa63de1 — including the Docker end-to-end job. Item 1 is closed.

Status of this issue

Item	State
1 — unquoted config value in a remote shell	Fixed, #52 (81d0ead)
2 — environment names never validated	Open
3 — fence epoch fails open	Open
4 — hyphen-joined names break the naming contract	Open
5 — stale timer sweep skips removal on disable failure	Open
6 — runtime names carry no environment	Open — needs a design decision, not a patch
7 — protection_units.go has no production implementation	Removed, #51 (f9ef6bc)
8 — exported symbols with no references	Removed, #51 (f9ef6bc)
9 — smaller items	Open
10 — linters worth adding to the gate	Partly done: unused enabled in #51; errorlint, noctx, copyloopvar, unconvert, intrange still open

Release note needed

The generated healthcheck string changed, so any workload with a health.http probe gets a different compose digest. The first deploy after this ships will show a diff and roll that workload, with nothing else changed. One-time, and unavoidable — the old string is the bug — but it should be stated in the release notes rather than discovered.

Three corpus digests in contract-verdicts.json moved for the same reason.

Next

Item 3 — the fence epoch failing open in internal/engine/lock.go:56-61 and internal/engine/protection_lock.go:162-167. It is the other half of the pair flagged as highest-priority in the original report, and it is self-contained.