Files

532 lines
25 KiB
Plaintext

================================================================================
F2 TOFU-CORE CODE QUALITY AUDIT — FINAL VERDICT
================================================================================
Date: 2026-08-08
Reviewer: Sisyphus-Junior (independent agent)
Target: tofu-core Dlang codebase (24 .d files, 11,018 LOC)
Branch: current HEAD
Build: dmd 2.112 (debug, warningsAsErrors), ldc2 (release)
================================================================================
VERDICT: REJECTED
================================================================================
Reason: 1 BLOCKER (C10 — path traversal in fetch.d), 1 MAJOR (C5 — unsafe
cast(int) in config.d), 2 MINOR findings.
See per-check table below for details.
================================================================================
PER-CHECK SUMMARY TABLE
───────────────────────
Check | Name | Result | Severity (if fail)
───────┼─────────────────────────┼──────────┼───────────────────
C1 | dub build (warnings=err)| PASS | —
C2 | @safe/@system audit | PASS | —
C3 | Hardcoded paths | PASS | —
C4 | Exception handling | PASS | —
C5 | cast() audit | FAIL | MAJOR (1), MINOR (1)
C6 | Error message quality | PASS* | MINOR (2 non-actionable)
C7 | Unbounded memory growth | PASS | —
C8 | Concurrency (spinner) | PASS | —
C9 | Module structure (<1KL) | PASS | —
C10 | Security (sandbox+path) | FAIL | BLOCKER (1)
───────────────────────────────────────────────────────────────────────────────
================================================================================
DETAILED EVIDENCE PER CHECK
================================================================================
─── C1: dub build with warnings-as-errors ────────────────────────────────────
Build 1: dmd debug (default)
Command: dub build
Result: PASS
Evidence:
Warning: only DUB-level deprecation about "warningsAsErrors" buildOption
(DUB recommends "buildRequirements" instead — cosmetic, not a
code defect). Zero compiler warnings, zero errors.
Output:
Starting Performing "debug" build using /usr/bin/dmd for x86_64.
Up-to-date toml 1.0.0: target for configuration [library] is up to date.
Up-to-date tofu ~main: target for configuration [application] is up to date.
Finished
Build 2: ldc2 release (-b release --compiler=ldc2)
Result: PASS
Evidence:
Starting Performing "release" build using ldc2 for x86_64.
Building toml 1.0.0: building configuration [library]
Building tofu ~main: building configuration [application]
Linking tofu
Zero warnings, zero errors.
Build 3: dmd --force (clean rebuild)
Command: dub build --force
Result: PASS
Only linker messages, zero compiler warnings:
Building toml 1.0.0: building configuration [library]
Building tofu ~main: building configuration [application]
Linking tofu
Sub-verdict: PASS
─── C2: @safe audit ──────────────────────────────────────────────────────────
Check: grep -rn '@system' src/
Result: PASS — zero @system function annotations anywhere in the codebase
Evidence:
All 5 grep hits are documentation comments explaining WHY a particular
section uses @trusted (e.g. "stdin/stdout are @system in dmd 2.112").
No actual @system annotation on any function.
$ grep -rn '@system' src/
src/tofu/log.d:49:/// `@trusted`: `stdout` itself is `@system` to access in dmd 2.112.
src/tofu/http.d:5:/// `@safe`; the `@system` curl internals are isolated behind `@trusted`
src/tofu/http.d:74:// @trusted helpers — all @system curl calls are isolated here
src/tofu/state.d:36:// ─── @trusted wrappers (fs + json — @system in Phobos) ──────
src/tofu/commands/upgrade.d:36:// ─── @trusted I/O wrappers (stdout/stdin are @system in DMD 2.112) ───
Annotations count:
- @safe functions/blocks: 242
- @trusted functions/blocks: 28
- Ratio: ~8.6:1 safe-to-trusted
- @system blocks (`@system:`): ZERO
Public API coverage:
All public API functions are either @safe or @trusted with documented
rationale in module-level doc comments. Filesystem, process, and socket
operations are isolated behind @trusted helpers with explicit comments.
Sub-verdict: PASS
─── C3: Hardcoded paths audit ────────────────────────────────────────────────
Check: grep -rn '"/tmp/\|~/\|"/usr/\|home/' src/ --include='*.d'
Result: PASS — all hits are in test code or documentation
Evidence:
src/tofu/config.d:117: // doc comment: "~/.config/tofu/config.toml"
src/tofu/config.d:130: cfg.cacheDir = expandTilde("~/.cache/tofu"); ← DEFAULT, env-overridable
src/tofu/config.d:279: test assertion — assert(cfg.cacheDir == expandTilde("~/.cache/tofu"))
src/tofu/config.d:300-302: test TOML fixture string content
src/tofu/config.d:308-310: test assertions on TOML fixture
src/tofu/config.d:356-362: test assertions with env override
src/tofu/config.d:424-431: test assertions with env override
src/tofu/build.d:142,181: error messages mentioning config file path (informational)
src/tofu/build.d:500: test assertion — assert(pkgNameFromPath("/tmp/...") == "build")
src/tofu/build.d:585: test fixture — cfg.cacheDir = "/tmp/dummy"
src/tofu/errors.d:287: comment — "On a fresh system ~/.cache/tofu may not exist yet."
Analysis:
- All hardcoded paths are in test assertions, test fixture strings, or
informational error messages.
- The config module (config.d:130) is the sole owner of path defaults
via `expandTilde("~/.cache/tofu")` — overridable by TOML or env var.
- No production code path uses hardcoded absolute paths.
Sub-verdict: PASS
─── C4: Exception handling ───────────────────────────────────────────────────
Check 4a: main.d has try/catch around dispatch
Result: PASS
Evidence:
src/main.d lines 101-127:
try {
final switch (pa.cmd) {
case Command.help: ... return 0;
case Command.install: return installCommand(...);
case Command.search: return searchCommand(...);
case Command.upgrade: return upgradeCommand(...);
case Command.remove_: return removeCommand(...);
case Command.info: return infoCommand(...);
}
} catch (Exception e) {
int ec = exitCodeFor(e);
logError("%s", e.msg);
return ec;
}
Note: catches `Exception`, not `Throwable` — correct D idiom (Errors
like OutOfMemoryError should NOT be caught). PASS.
Lock management: `scope(exit) releaseLock(cacheDir)` at line 81,
`scope(failure) releaseLock(cacheDir)` at line 82 — correct.
Check 4b: grep 'catch.*Throwable'
Result: PASS — all occurrences in unittest cleanup only
Evidence:
src/tofu/http.d:230: catch (Throwable) {} ← oneShotResponder test util
src/tofu/index.d:403: catch (Throwable) {} ← oneShotResponder test util
src/tofu/binary.d:252: catch (Throwable) {} ← oneShotResponder test util
src/tofu/fetch.d:368: catch (Throwable) ← waitForPort test util
src/tofu/fetch.d:381: catch (Throwable) {} ← killServer test util
src/tofu/fetch.d:412: catch (Throwable) {} ← removeDir test util
All 6 occurrences are inside `version(unittest)` blocks — test helpers
where silently discarding errors during cleanup is acceptable (one-shot
TCP responders, wait loops, server teardown). No production code path
swallows Throwable.
Sub-verdict: PASS
─── C5: cast() audit ─────────────────────────────────────────────────────────
Check: grep -rn 'cast(' src/
Result: FAIL — one MAJOR finding, one MINOR finding
Findings:
[MAJOR] src/tofu/config.d:227 — unsafe cast(int) tJobs
──────────────────────────────────────────────────────────
Line: cfg.defaultJobs = cast(int) tJobs;
Context: tJobs is `long.min` sentinel or a TOML integer (long).
`cfg.defaultJobs` is `int` (32-bit signed).
Issue: `cast(int)` silently truncates large values.
e.g. TOML `default_jobs = 5000000000` wraps to negative int.
Impact: Low (absurd job counts not realistic), but violates
defensive coding. The parsePositiveInt in cli.d correctly
validates via `to!int` (which throws on overflow); the
config path should similarly validate.
Fix: Replace `cast(int) tJobs` with `to!int(tJobs)` or clamp
the value. (NOT fixing — reporting only.)
[MINOR] src/tofu/commands/install.d:367 — cast(PackageIndex[])[] in test
──────────────────────────────────────────────────────────
Line: delegate () @safe { return cast(PackageIndex[])[]; }
Context: Test delegate to inject an empty index for testing
"package not found" (exit code 2).
Issue: Raw cast of `[]` literal. Idiom would be
`(PackageIndex[]).init` or `new PackageIndex[0]`.
Impact: Test-only, zero runtime consequence.
Severity: MINOR
PASS items (all in @trusted or test context):
- errors.d:80-139: cast(TofuError/HttpException/...) — safe downcast
- errors.d:216: cast(pid_t) — safe integer cast in POSIX-specific code
- http.d:98: cast(string) data — in @trusted getImpl, ubyte[]→string
- http.d:223,242: cast()/cast(shared) listener — test-only TCP server
- vercmp.d:113: cast(string) buf — in @trusted lambda, char[]→string
- index.d:278: cast() entries — in @trusted block, extracting parsed.array
- index.d:396,416: cast()/cast(shared) listener — test-only TCP server
- binary.d:245,265: cast()/cast(shared) listener — test-only TCP server
- fetch.d:350: cast(InternetAddress) — test-only findFreePort
- commands/info.d:83,151: cast(string) — in @trusted lambdas
Sub-verdict: FAIL (MAJOR: config.d:227)
─── C6: Error message quality ────────────────────────────────────────────────
Check: spot-check 5 throw sites across modules — each must name WHAT
failed and suggest HOW to fix.
Site 1: src/tofu/build.d:141-144
Message: "zeta-makepkg not found. Install zeta-toolchain or set
TOFU_ZETA_TOOLCHAIN_PATH in ~/.config/tofu/config.toml"
WHAT: zeta-makepkg not found ✓
HOW: install zeta-toolchain OR set config path ✓
Verdict: PASS — fully actionable
Site 2: src/tofu/fetch.d:277-279
Message: "build script '%s' not found for package '%s'"
WHAT: which script, which package ✓
HOW: no suggestion — user doesn't know HOW to resolve
Verdict: MINOR — names WHAT clearly but lacks a suggestion
(e.g. "check the recipe's build_script field")
Site 3: src/tofu/http.d:113-114
Message: "cannot reach ZUUR at <url>: timeout after 120s"
WHAT: which URL timed out ✓
HOW: no suggestion — user doesn't know HOW to fix network issues
Verdict: MINOR — names WHAT clearly but no suggestion
(e.g. "check your network connection or TOFU_ZUUR_URL")
Site 4: src/tofu/index.d:223-224
Message: "ZUUR index is invalid: lua command not found on PATH"
WHAT: lua not found ✓
HOW: implies "install lua" ✓
Verdict: PASS — actionable
Site 5: src/tofu/cli.d:213-214
Message: "unknown option: -Z (run 'tofu --help')"
WHAT: which option is unknown ✓
HOW: run --help for valid options ✓
Verdict: PASS — fully actionable
Summary: 3 of 5 fully actionable, 2 lack HOW guidance. Minor room for
improvement but not a rejection-level issue.
Sub-verdict: PASS (with 2 MINOR notes)
─── C7: Unbounded memory growth in hot paths ─────────────────────────────────
Check 7a: http.d onReceive string accumulator
Evidence: http.d:96-99
http.onReceive = (ubyte[] data) {
content ~= cast(string) data;
return data.length;
};
Analysis: The `get()` function accumulates the full response body into a
string. Used for fetching the ZUUR index (index.lua) — typically
a few KB. Also used for fetching binary package.lua manifests
(a few hundred bytes). Acceptable for these use cases.
The `downloadFile()` function writes to disk, not memory.
Verdict: PASS
Check 7b: stderr ring buffer bound
Evidence: build.d:46-63,75
private final class StderrRing {
string[] _lines;
size_t cap;
...
void add(string line) {
_lines ~= line;
if (_lines.length > cap)
_lines = _lines[1 .. $]; // drop oldest, keep cap
}
}
auto ring = new StderrRing(20); // ← confirmed: 20-line bound
Analysis: Ring buffer initialized at 20 lines (line 75). `add()` drops
oldest line when exceeding capacity. Reader thread joins before
`ring.get()` is called, ensuring happens-before.
Verdict: PASS
Check 7c: spinner thread join
Evidence: ui.d:95-106 — `_thread.join()` wrapped in try/catch
Verdict: PASS (deferred to C8)
Sub-verdict: PASS
─── C8: Concurrency — ui.d spinner thread ────────────────────────────────────
Check: spinner clean stop — no join hang, shared bool flag
Evidence: ui.d:31-119
shared bool _running; // line 35 — shared flag
...
void threadFn() {
while (_running) { // line 50 — checked every iteration
... // 100ms sleep per iteration
}
}
void stop() {
_running = false; // line 93 — signal to stop
if (_thread !is null) {
try {
_thread.join(); // line 99 — join, max wait ~100ms
} catch (Exception) {
// double-join or thread failure — harmless
}
_thread = null; // prevent double-join
}
}
Analysis:
- `_running` is `shared bool` — correct for cross-thread communication
- Thread loop checks `_running` every 100ms (Thread.sleep at line 55)
- `stop()` sets the flag, then joins — max join delay ~100ms
- Double-join prevented by `_thread = null` after join
- Exception during join caught and discarded (benign)
- No hang risk — flag is simple bool, no mutex contention
- Non-TTY mode skips thread entirely (lines 89-90)
Sub-verdict: PASS
─── C9: Module structure ─────────────────────────────────────────────────────
Check: wc -l src/tofu/*.d src/tofu/commands/*.d
Result: PASS — all modules under 1000 lines
Lines Module
───── ──────────────────────
851 src/tofu/resolve.d ← largest
708 src/tofu/install.d
695 src/tofu/build.d
694 src/tofu/index.d
644 src/tofu/fetch.d
600 src/tofu/errors.d
554 src/tofu/commands/install.d
545 src/tofu/commands/upgrade.d
541 src/tofu/types.d
472 src/tofu/recipeparse.d
462 src/tofu/commands/info.d
435 src/tofu/config.d
435 src/tofu/cache.d
390 src/tofu/binary.d
376 src/tofu/commands/search.d
366 src/tofu/state.d
363 src/tofu/cli.d
356 src/tofu/http.d
346 src/tofu/vercmp.d
334 src/tofu/deps.d
330 src/tofu/commands/remove.d
275 src/tofu/ui.d
242 src/tofu/log.d
4 src/tofu/package.d ← module re-export only
─────
11018 TOTAL
Largest module (resolve.d, 851 lines) is well under the 1000-line
ceiling. No file requires an override justification.
Sub-verdict: PASS
─── C10: Security ────────────────────────────────────────────────────────────
Check 10a: index.d Lua sandbox whitelist
Result: PASS
Evidence:
- Whitelist defined at index.d:70-91 (Lua sandboxLuaScript constant)
- Stripped globals: io, os, require, dofile, loadfile, loadstring,
package, debug
- Test 2 (line 471): os.execute("rm -rf /") → IndexException caught,
sentinel file survives → sandbox works
- Test 3 (line 514): io.open("/etc/shadow") → IndexException caught
- ZETA lib/sandbox.lua port, Lua 5.1/5.2+ compatible
- Confirmed: hunkered-down whitelist, NOT a blacklist
Check 10b: fetch.d path traversal
Result: FAIL — BLOCKER
Evidence:
├─ src/tofu/fetch.d:253-258:
│
│ auto buildScriptPath = extractBuildScript(recipeContent);
│ if (buildScriptPath.length > 0) {
│ logDetail("custom build system: fetching %s", buildScriptPath);
│ auto bsUrl = cfg.recipesUrl(name) ~ "/" ~ buildScriptPath;
│ auto bsDest = cacheDir ~ "/" ~ buildScriptPath; // ← UNSANITIZED
│
│ () @trusted {
│ auto bsDir = bsDest.dirName;
│ if (bsDir.length > 0 && !exists(bsDir))
│ mkdirRecurse(bsDir);
│ }();
│ ...
│ downloadFile(bsUrl, bsDest); // writes to escaped path
│ }
│
└─ PROBLEM: No sanitization of `buildScriptPath`.
ATTACK VECTOR:
1. Malicious recipe in ZUUR includes:
build_system = "custom"
build_script = "../../../.bashrc"
2. extractBuildScript returns "../../../.bashrc"
3. bsDest = "~/.cache/tofu/../../../.bashrc"
→ resolves to "$HOME/.bashrc"
4. downloadFile creates $HOME/.bashrc.part, then renames to $HOME/.bashrc
5. Result: user's .bashrc is overwritten with attacker-controlled content
Confirmations:
- Zero occurrences of ".." sanitization anywhere in src/ (confirmed
via grep for indexOf/canFind/contains/startsWith of "..")
- No expandTilde, buildPath, normalizePath, or absolutePath applied
to buildScriptPath or bsDest in the build_script code path
- mkdirRecurse and exists() resolve ".." through the OS kernel
Severity: BLOCKER
The .recipe file content comes from the remote ZUUR repository.
A malicious maintainer (or compromised repository) can write
arbitrary files anywhere the user has write access.
Required fix (not implemented — reporting only):
- Reject buildScriptPath containing ".." before constructing bsDest
- OR resolve bsDest to absolute canonical path and verify it starts
with cacheDir
Sub-verdict: FAIL (BLOCKER: path traversal in fetch.d)
================================================================================
FINDINGS SUMMARY TABLE
================================================================================
# | Check | Severity | Location | Description
───┼───────┼──────────┼────────────────────┼─────────────────────────────────
1 | C10 | BLOCKER | fetch.d:253-258 | Path traversal: buildScriptPath
| | | | from .recipe used unsanitized in
| | | | bsDest, can escape cache dir via
| | | | "../" in build_script field
───┼───────┼──────────┼────────────────────┼─────────────────────────────────
2 | C5 | MAJOR | config.d:227 | cast(int) tJobs silently truncates
| | | | large TOML default_jobs values
───┼───────┼──────────┼────────────────────┼─────────────────────────────────
3 | C6 | MINOR | fetch.d:277-279 | Error message lacks HOW guidance
| | | | for missing build script
4 | C6 | MINOR | http.d:113-114 | Error message lacks HOW guidance
| | | | for timeout/network errors
5 | C5 | MINOR | install.d:367 | cast(PackageIndex[])[] in test —
| | | | raw cast of empty array literal
───┴───────┴──────────┴────────────────────┴─────────────────────────────────
================================================================================
PASS COUNTS
================================================================================
8 of 10 checks PASS: C1, C2, C3, C4, C6*, C7, C8, C9
1 of 10 checks FAIL (non-blocker): C5 (MAJOR + MINOR)
1 of 10 checks FAIL (blocker): C10 (BLOCKER)
──
*C6 PASS but with 2 MINOR non-actionable error message notes
================================================================================
OVERALL VERDICT: REJECTED
================================================================================
The codebase is well-structured, compiles cleanly, handles concurrency
correctly, and has no hardcoded paths or unsafe exception swallowing.
The Lua sandbox is properly implemented. However, the path traversal
vulnerability in fetch.d (C10) is a BLOCKER that could allow a malicious
ZUUR recipe to write files outside the cache directory — this MUST be
fixed before approval.
Recommended fix (not applied — audit only):
In fetch.d, after line 254, add:
if (buildScriptPath.indexOf("..") >= 0)
throw new FetchException(
"build_script path contains '..': " ~ buildScriptPath);
This is a one-line fix. Re-audit of C10 after the fix is applied should
be trivial.
================================================================================
================================================================================
F2 FIX APPLIED — build_script path traversal + default_jobs truncation
================================================================================
Date: 2026-08-08
FINDING 1 (BLOCKER, fetch.d:253-258) — RESOLVED
src/tofu/fetch.d: added private @safe validateBuildScriptPath(string)
- rejects ".." segments, leading '/' (absolute), and '\' (backslash)
with: throw new FetchException("build_script path is unsafe: <path>")
- wired into fetchRecipe step 4 BEFORE building bsUrl/bsDest
- defense in depth: after joining, throws if
!bsDest.startsWith(cacheDir ~ "/")
- valid relative paths unchanged (build.sh, scripts/build.sh pass)
FINDING 2 (MAJOR, config.d:227) — RESOLVED
src/tofu/config.d: default_jobs from TOML now range-validated (1..1024)
- tJobs < 1 || tJobs > 1024 -> stderr warning
"Warning: invalid default_jobs <n> in config, using default 1"
and cfg.defaultJobs = 1 (no silent cast(int) truncation)
- else cfg.defaultJobs = cast(int) tJobs
TESTS ADDED
fetch.d: direct validateBuildScriptPath unittests (evil paths rejected,
valid paths pass) + integration Test 6: recipe with
build_script="../../evil.sh" -> FetchException, nothing escapes cache
config.d: default_jobs = 5000000000 -> 1, 1024 -> 1024, 2048 -> 1,
-3 -> 1
VERIFICATION
dub build : PASS (warningsAsErrors, no warnings)
dub test : 23 modules passed unittests
smoketest : PASS 16 checks passed