Code Debt & Improvements

Technical debt and improvement opportunities discovered during development. Items here are not blocking but worth addressing in future passes.

Security Hardening

containsDotDot duplicated across packages

handler.go and store.go both define identical containsDotDot functions. Both only split on /, which is correct for the Mark Protocol but would miss \ on Windows. Consider extracting to a shared internal/pathutil package and splitting on both separators.

parseVersionPath uses filepath.Split on protocol paths

handler.go:parseVersionPath uses filepath.Split, which uses the OS separator. On Windows, this would silently break version path parsing (e.g., FETCH /doc.md/v3). Should use path.Split instead since protocol paths always use forward slashes.

findVersions and VerifyChain bypass resolve()

Both findVersions and VerifyChain construct filesystem paths from request paths using filepath.Join without routing through resolve(). They rely on callers having already validated the path, but CurrentVersion and VerifyChain are exported methods that accept arbitrary strings. A future caller could bypass path validation. Consider adding resolve() calls or making these methods unexported.

Resilience

Stale .tmp files visible in directory listings

If the server crashes between os.WriteFile and os.Rename in Store.Archive, a .tmp file persists and would appear in directory listings via ListDir. Consider filtering names ending in .tmp in ListDir, alongside the existing versions and dot-file filters.

handleVersions accesses versions[0] without length guard

Store.Versions returns os.ErrNotExist for empty version lists, so handleVersions never receives an empty slice today. But the contract doesn't guarantee this — a defensive len(versions) == 0 check before accessing versions[0] would be safer.

Code Quality

Handler Logger nil fallback

Handler.logger() silently falls back to slog.Default() when Logger is nil. This masks misconfiguration and can cause noisy test output. Consider requiring Logger at construction time or initializing it in a constructor.

Performance

Store.Write reads previous version file twice

Write reads the previous version file for the no-op content check, then buildVersionFile reads the same file again to compute previous-hash. Consider passing the already-read data (or its hash) into buildVersionFile to avoid the duplicate I/O on every write after v1.

Store.Write returns shared metadata map reference

Write returns the same meta map instance passed in, both on success and ErrNotModified. A caller mutating the returned Document.Metadata would also mutate the input. Low risk today since no caller does this, but a defensive copy would be cleaner. Also, nil vs empty-map inconsistency with Get (which returns nil for no metadata).