Skip to content

Fix all code-review findings (structure, data types, bugs) - #13

Merged
chadleeshaw merged 13 commits into
mainfrom
fix/code-review-findings
Sep 29, 2026
Merged

chadleeshaw merged 13 commits into
mainfrom
fix/code-review-findings

Conversation

@chadleeshaw

Copy link
Copy Markdown
Owner

Fixes every finding from the whole-codebase review (#12), across all 10 criticals, 20+ majors, and minors.

Security

  • Auth (CR-1): real server-side sessions — crypto/rand tokens, HttpOnly/Secure/SameSite=Lax cookies; 401 JSON for API paths instead of a login redirect; password-change path locked with a mutex and empty-password rejection; loud startup warning when default admin/admin is still in use.
  • Path traversal (CR-2..CR-6, M-S2..S5): one shared safeJoin helper (join-then-check with containment) applied to every file endpoint — provision editor, file viewer/delete, upload, boot-menu submit, and TFTP RRQ/WRQ (CR-7).
  • Upload size caps (MaxBytesReader) + ValidateTFTPUpload actually wired up; JS-escaped template output; header-injection-safe filenames; BoltDB opened 0600 (was 0744).

DHCP core (CR-8..CR-10, M-C8..C15, M-C20..C21, M-T13)

  • RWMutex on the service handler map; shared allocation mutex across scan-pick-save in both AssignLease and the packet path — no more racy double-allocation.
  • Packet-path leases now get UUIDs, state, and timestamps (no more blank-key overwrites).
  • Save failure → NAK (never ACK unpersisted); expired rows deleted before replacement; server-ownership check on lease extension; idempotent Start(), non-blocking Stop().
  • Typed LeaseState; To4() normalization at boundaries; MAC secondary index (leases_by_mac) with old-DB repair; typed db.ErrNotFound; GetAll surfaces corrupt rows instead of skipping them; DeleteAllKV no longer deletes inside ForEach.

Downloads (M-C1..C7, M-C19)

  • Context-based cancellation the workers actually honor; terminal state is never overwritten by stale data; throttled progress persistence; temp-file + rename on success, partial cleanup on failure, HTTP timeouts.
  • Optional per-version SHA-256 checksum verification (ExpectedChecksum).
  • Syslinux: download → extract → verify → then clean the old version (no more deleting working PXE files first); InstallBootFiles implemented for real; unified dlstatus package with typed status constants; UUID download IDs.
  • iPXE: unique menu IDs per OS+version, empty-OS guard, configured HTTP port (no more hardcoded :8080).

Structure (M-T1..T15)

  • Deleted the dead interfaces/ package (nothing imported it); testdata/ → cli/.
  • Single DI container built once; named BoltDB field, GetDB() leak removed; graceful shutdown instead of log.Fatalf in a goroutine.
  • Templates parsed once at startup (no template.Must on the request path); unified AppError/HandleError handling — no internal error text leaked to clients; deduped IP-sort/server-view/server-lookup helpers.
  • Config: numeric HTTP-port validation, SERVER_IP env override, deep-copied catalog in Build().

Verification

go build ./..., go vet ./..., gofmt, go test ./... all green, plus -race on the concurrency-sensitive packages (dhcp, db, osimage, syslinux, handlers). New regression tests: db/fixes_test.go, dhcp/fixes_test.go, dhcp/repository_test.go, dlstatus tests, TFTP traversal tests.

Follow-ups worth a look (not blockers): syslinux delete/get-one/list-all endpoints return honest 501s — the UI may want those actions hidden; TFTP status reports "unknown" until the app layer calls the new SetTFTPRunning; per-file (vs per-version) checksums if desired.

@chadleeshaw
chadleeshaw merged commit b272f6d into main Sep 29, 2026
3 checks passed
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