fix(analysis): audit all non-exempt top-level builtins/ files#261
Conversation
Closes vuln-hunt F-1 (2026-05-18-initial-audit / builtin-import-allowlist): the collector helper had an unconditional subdir-only filter that shadowed the skipTopLevel callback, silently excluding builtins/proc_provider.go and builtins/features.go from the symbol allowlist. A maintainer could land a banned import (os/exec, net/*, plugin, reflect) in either file undetected. Rename collectSubdirGoFiles -> collectGoFilesRecursive and drop the shadowing filter so skipTopLevel solely controls top-level exemption. builtins.go remains exempt via the existing callback; proc_provider.go and features.go are now audited (current imports already comply). Add TestVerificationBuiltinsChecksNonExemptTopLevel as the regression guard, and update the stale F-1 reference in the signal-handling vuln-hunt fallback test.
|
|
@codex review this PR Please use the following severity format for all findings:
Prefix each finding title with its priority label, e.g. "P0: ...", "P1: ...", etc. |
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Self-Review (iteration 1)
Scope: Closes vuln-hunt F-1 (2026-05-18-initial-audit / builtin-import-allowlist). Four-file change in analysis/:
symbols_test.go— rename helper, drop the shadowing subdir-only filter.symbols_builtins_test.go— call-site updates + tightened comment.symbols_builtins_verification_test.go— new regression test.signal_handling_vuln_hunt_test.go— stale F-1 reference updated.
Overall assessment: safe to merge. The fix is surgical: it removes a filter that shadowed an intentional callback, restores the original design (skipTopLevel controls top-level exemption), and adds a regression test that fails if the subdir-only filter is reintroduced. No new attack surface; this strictly tightens analyzer coverage.
Findings
| # | Priority | File | Finding |
|---|
(no P0/P1/P2/P3 findings raised in this iteration)
Verification performed
make fmtclean.go test ./analysis/... -count=1passes.TestVerificationBuiltinsChecksNonExemptTopLevelconfirmed to fail when the pre-fixstrings.Contains(rel, separator)filter is reintroduced (the regression guard works in both directions).- Confirmed no remaining references to the old
collectSubdirGoFilesname in the tree. - Confirmed
internalCheckConfig'snil, nilcall now nominally audits top-level files inbuiltins/internal/, but no such files exist today — benign contract widening.
Positive observations
- Root-cause fix, not a workaround. The shadowing filter is removed rather than papered over with a second collector or a parallel test entry point. The contract change (
skipTopLevelis now load-bearing) matches what any reader ofbuiltinsCheckConfigwould already assume — which is exactly the misreading that hid the original bug. - Test names the threat.
TestVerificationBuiltinsChecksNonExemptTopLeveldirectly invertsTestVerificationBuiltinsSkipsTopLeveland references the vuln-hunt finding in its docstring, giving future maintainers a clear chain back to the audit. - Belt-and-braces kept. The signal-handling vuln-hunt fallback (
TestVulnHuntSubsystemSignalHandling_NoSignalNotifyInProductionCode) is preserved as defense-in-depth — its comment is updated to note the original gap is closed, rather than removing the test outright. - Rename improves discoverability.
collectGoFilesRecursivecorrectly describes the new contract; the prior name (collectSubdirGoFiles) was the exact misnomer that hid the bug.
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Iteration 1 self-review result: P0=0, P1=0, P2=0, P3=0 — no findings raised. Review URL: #261 (review) |
2 participants
Summary
collectSubdirGoFileshad an unconditional subdir-only filter that shadowed theskipTopLevelcallback, silently excludingbuiltins/proc_provider.goandbuiltins/features.gofrom the symbol allowlist. Banned imports (os/exec,net/*,plugin,reflect) could land in either file undetected.collectSubdirGoFiles→collectGoFilesRecursive, drop the shadowing filter, soskipTopLevelsolely controls top-level exemption.builtins.gostays exempt via the existing callback;proc_provider.goandfeatures.goare now audited (their current imports already comply).TestVerificationBuiltinsChecksNonExemptTopLevelregression test that injectsos/execintoproc_provider.goand asserts the analyzer flags it. Update the stale F-1 reference comment insignal_handling_vuln_hunt_test.go(kept as defense-in-depth).Test plan
go test ./analysis/... -count=1TestBuiltinAllowedSymbols,TestBuiltinPerCommandSymbols,TestVerificationBuiltinsCleanPass,TestVerificationBuiltinsSkipsTopLevel,TestVerificationBuiltinsChecksNonExemptTopLevelall passmake fmtclean