validate DoIds before filesystem access

[justin-layerv]

Summary

Validate wire-supplied DHP DoId values before any server or DB code incorporates them into ztdo filenames.

Root cause

DRGMsg.DoId, DARMsg.DoId, DAVMsg.DoId, and DWRMsg.DoId cross the wire and ultimately reach paths such as data-<DoId>.json and data-key-<DoId>.json. The server and DB helpers concatenated those values without a strict component policy. Values containing enough path separators and parent components could escape the intended etc/ztdo directory, enabling an authenticated or otherwise handler-capable attacker to read, overwrite, or delete files writable by the OpenNHP process.

Raw filesystem errors and unescaped DoIds could also disclose deployment paths or inject control characters into operator logs.

Impact

DoIds are now limited to 1–64 ASCII letters, digits, underscores, and hyphens—a superset of OpenNHP's UUID-shaped production identifiers. Traversal strings are rejected before any filesystem access, and the rejection uses a fixed sentinel that does not reflect attacker input onto the wire. This protects ztdo configuration and data-key storage without changing valid UUID workflows.

Changes

• Add shared common.ValidateDoID and common.ErrInvalidDoID primitives.
• Validate at all server config read/write and DB data-key load/save/delete boundaries.
• Return fixed errors across protocol-facing boundaries while retaining detailed, %q-escaped operator logs.
• Correct DB key-store directory creation to use the same absolute ExeDirPath tree as the subsequent file operation.
• Close DB key-store reads and reject malformed stored JSON instead of returning a partial object.
• Add allowlist/traversal tables, filesystem-no-write assertions, valid UUID round trips, and error-scrubbing tests.

Validation

• cd nhp && go test ./...
• cd nhp && go test -race ./common -run TestValidateDoID -count=1
• cd endpoints && go test ./db
• cd endpoints && go test -race ./db -run 'TestDataPrivateKeyStore' -count=1
• cd endpoints && go test -c ./server -o <temp test binary>
• cd endpoints && go build ./...
• cd nhp && golangci-lint run ./common
• cd endpoints && golangci-lint run ./db ./server
• git diff --check

Executing the server test binary is blocked in this macOS environment by existing package initialization that attempts to create KBS key material under /opt/confidential-containers and fails with permission denied. The server tests compile, the complete endpoints module builds, and CI will run them in its normal Linux environment.

Attribution

Adapted for current OpenNHP main from LayerV's security fix: layervai/nhp#1189 (fe65b027).

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 37 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
endpoints/server/msghandler.go	37.50%	17 Missing and 3 partials ⚠️
endpoints/db/utils.go	55.26%	14 Missing and 3 partials ⚠️

@@            Coverage Diff             @@
##             main    #1637      +/-   ##
==========================================
+ Coverage   12.54%   13.01%   +0.47%     
==========================================
  Files          96       97       +1     
  Lines       14526    14566      +40     
==========================================
+ Hits         1822     1896      +74     
+ Misses      12526    12474      -52     
- Partials      178      196      +18     

Flag	Coverage Δ
unittests	13.01% <50.00%> (+0.47%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
nhp/common/doid.go	100.00% <100.00%> (ø)
endpoints/db/utils.go	22.34% <55.26%> (+22.34%)	⬆️
endpoints/server/msghandler.go	12.78% <37.50%> (+4.59%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

✅ Looks Good - Code looks good

This is a well-scoped, correct path-traversal fix. The ValidateDoID allowlist (^[a-zA-Z0-9_-]{1,64}$) is a strict superset of production UUID DoIds and rejects every dangerous filename component — path separators, .., dots, null bytes, Unicode separators, whitespace, and shell metacharacters — before any os.Open/os.Create/os.Remove. I verified the fix is applied at every boundary where wire-supplied DoIds reach the filesystem:

• server.SaveZdtoConfig (DRG → data-<DoId>.json)
• server.ReadZdtoConfig — covers both HandleDHPDARMessage (msghandler.go:498) and HandleDHPDAVMessage (msghandler.go:549)
• db.NewDataPrivateKeyStoreWith (DWR → data-key-<DoId>.json, udpdevice.go:834)
• db.DataPrivateKeyStore.Save / .Delete

The previous behavior was genuinely exploitable: filepath.Join(ExeDirPath, "etc", "ztdo", "data-"+doId+".json") cleans embedded ../ sequences and escapes etc/ztdo, and the DAR/DAV handlers reflected the raw filesystem error back to the agent via dsaMsg.ErrMsg/dagMsg.ErrMsg. Both are now closed.

Things I confirmed

• Error scrubbing is correct. ReadZdtoConfig/SaveZdtoConfig/DB store now return fixed sentinels (errReadConfigFailed, errSaveConfigFailed, ErrInvalidDoID, ErrDataPrivateKeyStore), so the wire-facing ErrMsg no longer leaks deployment paths, while detailed context stays in %q-escaped operator logs (log-injection safe).
• errors.Is compatibility. ErrDataPrivateKeyStore is a *common.Error pointer returned by identity, and ErrInvalidDoID is a plain errors.New, so the tests' errors.Is assertions are sound.
• Pre-existing bug fixed. db.Save previously called os.MkdirAll("etc/ztdo") (relative to CWD) while creating the file under common.ExeDirPath/etc/ztdo — broken unless CWD == ExeDirPath. Now both use the same absolute tree. Good catch.
• Resource/robustness improvements. File handles are now closed in NewDataPrivateKeyStoreWith, and malformed stored JSON is rejected instead of silently returning a partial object (the old _ = d.fromJson(...)).
• No compile fallout. fmt and errors remain used in both edited files; log is correctly imported in db/utils.go; all log.* signatures match.
• No breaking change for real deployments. DoIds originate from uuid.New() (ztdo.GetObjectID()), which validates cleanly. No DoId literals exist in shipped configs.

Minor, non-blocking observations

1. The DB CLI --ztdo-id flag (db/main/main.go:358 → Save) is now subject to the same 64-char allowlist. This is desirable hardening, but operators who previously used custom identifiers with dots or >64 chars would now be rejected — worth a note in release docs if that path is user-facing.
2. Out of scope for this PR, but the uuid path segment in httpstorage.go:201 (filepath.Join(ExeDirPath, uploadDir, uuid, filename)) isn't obviously constrained the way filename is (line 196) — a future pass could apply the same rigor there.

Test coverage is thorough: allowlist/traversal tables, filesystem-no-write assertions, valid-UUID round trips, and an explicit error-scrubbing test. Nice work.