docs: security audit and functionality/next-steps reports Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
8.8 KiB
8.8 KiB
Functionality and Next Steps Report
Project: mapsize v1.0.0 plus the fixes from the security audit of 2026-10-02 (audits/20261002-1102-securityAudit.md).
1. Executive Summary
mapsize is a well-structured, well-documented codebase with unusually good invariants:
- a single writer for the inventory,
- a deadlock-free bounded scheduler,
- a documented lock discipline,
- a defensive renderer.
The full test suite passes across 11 packages, go vet is clean on every target OS, and govulncheck reports no vulnerabilities.
The highest-impact opportunities:
- Make actions after the scan relative to file descriptors (scanner, duplicate hashing, reveal), so that what mapsize acts on is provably what it scanned. This is the remaining correctness and security gap when running as root.
- Exercise the macOS and Windows paths for real. Trash and reveal there are implemented but have never been run (ROADMAP 0.8). CI runs
go teston those OSes, but the platform tests are Linux-only. - Small UX and robustness fixes:
- repeated actions on already-trashed nodes,
- JSON names that cannot be mapped back to real files,
- an unbounded filter-query nesting depth.
2. Current Functional Risks
| Risk | Evidence | User impact |
|---|---|---|
| Trash and reveal on macOS and Windows are untested | ROADMAP 0.8; ops_darwin.go and ops_windows.go have no tests |
First-use failures on those platforms; MS-09 (explorer comma parsing) is unverified |
| Trashing inside an already-trashed directory | trashDone (internal/tui/actions.go) flags only the trashed node FlagDeleted; its descendants stay selectable in some views until a rescan |
Normally fails safely, because the path is gone and Lstat errors. If someone recreates the same path, the stale aggregates would be subtracted again |
| Scan redirection by symlink races | platform.ReadDir opens directories by path (MS-08) |
Wrong totals on trees other users can write to |
--duplicates reading from a hung NFS/SMB mount |
fullHash makes a blocking read(2) |
It cannot be cancelled (inherent); worth documenting |
| JSON names lossy for non-UTF-8 bytes | encoding/json replaces invalid UTF-8 with U+FFFD |
Consumers cannot locate those files |
| Filter queries with extreme nesting | Recursive descent in filter/parser.go |
A self-inflicted crash from a pathological paste |
| Copy refusal for unusual names (new) | copyPath refuses anything Sanitize would change |
Users with legitimately odd names cannot copy them; the info modal shows the escaped form instead |
3. Code Quality Improvements
- Platform trash code. After this audit,
ensurePrivateDirandrenameCheckedlive intrash_unix.go, shared by Linux and macOS. The remaining Linux/macOS differences (trash layout, the.trashinfofile) are inherent. Keep any new platform logic behindinternal/platform, as ARCHITECTURE.md requires. requestTrashcaptures identity in the confirmation closure. If inode identity is added (security finding MS-11), extendplatform.Expectrather than adding parameters.- Ignored
Writeerrors insnapshot.Saveandexport.JSON. These are safe because the finalFlushreports a sticky error. A short comment saying so would stop future "fixes" from adding noise. pprof.StartCPUProfileerror ignored incmd/mapsize/main.go. Report it, so that a profiling run does not silently produce nothing.- ARCHITECTURE.md drift. Its OS-abstraction section names
readdir_unix.go,readdir_windows.goandfs_*.go. The actual files arereaddir_bsd.go,readdir_other.go,sys_*.goandmode_unix.go, plus the newtrash_unix.goandopen_*.go. The document says that if the two disagree, one of them should be fixed.
4. Frontend Interaction and Error-Handling Improvements
(This is a terminal UI; there is no web frontend.)
- Show why an action was refused, with a next step. Trash now reports "item changed since the scan … rescan first". Consider offering
r(rescan) directly from that message. - Mark descendants of trashed directories. Either hide nodes whose ancestor carries
FlagDeletedin the list, top and duplicate views, or haverequestTrashrefuse them early with "already moved to trash". - Duplicate view while hashing. Esc cancels between files. Showing "waiting on " when one read stalls for more than N seconds would make a hung network mount visible.
- Copy refusal. Offer to copy the escaped form, clearly labelled, so that users can still reference odd names in scripts.
5. Testing Roadmap
- Platform:
- macOS: run the platform tests on the CI
macos-latestrunner. PortTestTrashFreedesktop-style tests to~/.Trashand to a volume trash on a disk image created withhdiutil. - Windows: a Recycle Bin smoke test, plus reveal with comma-bearing names (MS-09).
- macOS: run the platform tests on the CI
- Abuse cases:
- A scanner race test in which a goroutine toggles a directory and a symlink during a scan, asserting that no node falls outside the root's (dev, ino) closure. This needs MS-08 first.
- Filter nesting-depth and length limits.
- A test that
--read-onlyblocks every mutating key:d,delete, anddinside the duplicate view.
- Config: malformed JSON, out-of-range
refresh_hz, and bad colours. - Regression: keep running the four tests added in this audit and the existing hostile-snapshot and fuzz targets in CI. Consider a nightly job running
make fuzzfor longer. - Load: the cold-cache HDD benchmark already on the ROADMAP, and a snapshot load benchmark at the new node-limit boundary.
6. Recommended New Functionality
- Safer root workflow: a
--trash-owned-by-meoption, or a default that refuses to trash items in directories owned by other users when running as root. This prevents most of the TOCTOU class of bugs outright. - Lossless export: add a
name_escaped/path_escaped(or base64) field to JSON and CSV for names that are not valid UTF-8. - Snapshot provenance (optional):
--sign KEYand--verify KEYusing Ed25519, so teams that share snapshots can trust their origin. See audit finding MS-13. - Configurable keybindings (already on the ROADMAP).
- Windows allocated size through
GetCompressedFileSizeW(already on the ROADMAP).
7. Next-Level Opportunities
- Descriptor-relative scanner:
- Keep a parent fd per pending directory, or reopen with
openat2(RESOLVE_BENEATH|RESOLVE_NO_SYMLINKS)on Linux 5.6+. - Verify (dev, ino) after each open.
- Store
Inofor the nodes that need it. - This closes MS-08, MS-11 and MS-12 together, and may reduce path-building work in hot loops.
- Keep a parent fd per pending directory, or reopen with
- Memory: the name arena on the ROADMAP would also cut the per-node cost behind the snapshot amplification limit.
- Release pipeline: build releases in GitHub Actions with SHA-pinned actions, attach Sigstore signatures and SLSA provenance, and publish the Homebrew formula automatically.
- Accessibility: the
monotheme and the non-colour selection cues are good. A screen-reader-friendly--no-uiinteractive list mode could serve users who cannot use a treemap.
8. Prioritised Action Plan
| Horizon | Action | Rationale | Risk |
|---|---|---|---|
| Immediate | Done: the trash identity check, copy guard, FIFO-safe open, snapshot node cap and CI token scope | Closes the Medium findings | Low; all have tests |
| Short | Fix the ARCHITECTURE.md file list, and hide or refuse descendants of trashed nodes | Accurate documentation; avoids confusing stale entries | Very low |
| Short | Run platform tests on macOS and Windows CI; verify the explorer comma behaviour | Untested destructive code paths | Low |
| Short | Pin actions by SHA and add Dependabot | Supply chain | Very low |
| Medium | Descriptor-relative scanning and actions | Removes the remaining TOCTOU class | Medium: touches the scanner hot path, so benchmark against PERFORMANCE.md |
| Medium | Lossless JSON names | Data fidelity | Low: additive field |
| Long | Signed releases and provenance; optional signed snapshots | Trust in artifacts | Low |
9. Caveats and Validation Needed
- macOS and Windows behaviour was only cross-compiled with
go vet, never executed. The Darwin volume-trash change (strict private directory) in particular should be checked on a real external volume. If.Trashes/<uid>already exists with different permissions, trash now falls back to an error instead of using it. - The trash identity check refuses items whose owner or type changed since the scan. Users who
chownwhile browsing will see refusals until they rescan; this is intended. - The snapshot node cap assumes about 15 bytes per node (PERFORMANCE.md). Trees made of extremely repetitive names compress better. The cap allows one node per byte, about 15× headroom, but it is worth validating against the largest real snapshots before the next release.
- The concurrent commit
ab3ff7b(Homebrew template) landed during the audit and was reviewed.