core/go-rag

Fork 0

[audit] Security, AX compliance, missing tests, error handling #4

New issue

Open

opened 2026-03-22 16:41:02 +00:00 by Virgil · 5 comments

Virgil commented

2026-03-22 16:41:02 +00:00

Member

Full audit:

Security: path traversal, injection, panics on untrusted input, race conditions
AX compliance: os.Getenv → core.Env, filepath.* → core.Path*, fmt.Sprintf → core.Sprintf, strings.* → core.*, errors.New/fmt.Errorf → core.E
Missing tests: exported functions without test coverage
Error handling: silently dropped errors, bare panics, missing nil checks
UK English: American spellings in comments/docs
Missing usage-example comments on exported identifiers
Missing SPDX licence headers

Report all findings with severity and file:line. Do NOT fix.

Full audit: 1. Security: path traversal, injection, panics on untrusted input, race conditions 2. AX compliance: os.Getenv → core.Env, filepath.* → core.Path*, fmt.Sprintf → core.Sprintf, strings.* → core.*, errors.New/fmt.Errorf → core.E 3. Missing tests: exported functions without test coverage 4. Error handling: silently dropped errors, bare panics, missing nil checks 5. UK English: American spellings in comments/docs 6. Missing usage-example comments on exported identifiers 7. Missing SPDX licence headers Report all findings with severity and file:line. Do NOT fix.

Virgil referenced this issue

2026-03-22 19:27:34 +00:00

[agent/codex] Full audit per issue #4. Read CLAUDE.md. Report ALL findings... #5

Virgil referenced this issue from a commit

2026-03-22 19:27:37 +00:00

Merge pull request '[agent/codex] Full audit per issue #4. Read CLAUDE.md. Report ALL findings...' (#5) from agent/full-audit-per-issue--4--read-claude-md into dev

Virgil commented

2026-03-22 19:27:53 +00:00

Author

Member

Codex Audit Findings

HIGH (1)

Symlink traversal in ingest — crafted docs tree reads/embeds files outside requested root (ingest.go:90, :116)

MEDIUM (4)

AddRAGSubcommands non-idempotent, panics on repeat registration (cmd_commands.go:16, cmd_rag.go:33)
Public API panics on nil dependencies instead of returning errors (query.go:54, ingest.go:69)
Ingest silently drops per-file/per-chunk failures (ingest.go:111, :117, :133)
Zero test coverage on exported integration boundaries (helpers.go:46, ollama.go)

## Codex Audit Findings ### HIGH (1) 1. Symlink traversal in ingest — crafted docs tree reads/embeds files outside requested root (ingest.go:90, :116) ### MEDIUM (4) 2. AddRAGSubcommands non-idempotent, panics on repeat registration (cmd_commands.go:16, cmd_rag.go:33) 3. Public API panics on nil dependencies instead of returning errors (query.go:54, ingest.go:69) 4. Ingest silently drops per-file/per-chunk failures (ingest.go:111, :117, :133) 5. Zero test coverage on exported integration boundaries (helpers.go:46, ollama.go)

Virgil referenced this issue

2026-03-23 07:21:48 +00:00

[agent/codex] Fix ALL findings from issue #4. Read CLAUDE.md first. Symlin... #6

Virgil commented

2026-03-23 07:22:07 +00:00

Author

Member

Fix Applied

Commit 2e8bc5a: fix(rag): harden ingest and command registration

Symlink traversal fixed in ingest
AddRAGSubcommands idempotent (no repeat panic)
Nil dependency guards with new dependencies.go
Silently dropped failures now surfaced
525 additions across 13 files, comprehensive tests added

## Fix Applied Commit 2e8bc5a: fix(rag): harden ingest and command registration - Symlink traversal fixed in ingest - AddRAGSubcommands idempotent (no repeat panic) - Nil dependency guards with new dependencies.go - Silently dropped failures now surfaced - 525 additions across 13 files, comprehensive tests added

Virgil commented

2026-03-23 09:07:36 +00:00

Author

Member

Verification: FAIL (reproduced)

MEDIUM: Symlinked root dirs don't ingest — Ingest resolves real root into rootDir but WalkDir starts from absDir. filepath.WalkDir doesn't traverse symlink root. Needs: walk from rootDir not absDir (ingest.go:76/:104).

## Verification: FAIL (reproduced) MEDIUM: Symlinked root dirs don't ingest — Ingest resolves real root into rootDir but WalkDir starts from absDir. filepath.WalkDir doesn't traverse symlink root. Needs: walk from rootDir not absDir (ingest.go:76/:104).

Virgil commented

2026-03-23 09:11:32 +00:00

Author

Member

Fix Round 2

Commit 993a804: fix(ingest): walk resolved root dir

WalkDir now uses rootDir (resolved symlinks) not absDir
30-line symlink root test added
Dispatching verification.

## Fix Round 2 Commit 993a804: fix(ingest): walk resolved root dir - WalkDir now uses rootDir (resolved symlinks) not absDir - 30-line symlink root test added Dispatching verification.

Virgil commented

2026-03-23 09:17:29 +00:00

Author

Member

Verification Round 2: FAIL

HIGH: TOCTOU race in Ingest — validates path during walk (ingest.go:104) but reopens cached pathname later with coreio.Local.Read (ingest.go:152). File can be swapped to symlink between check and read.

This is a fundamental TOCTOU — needs open-then-validate (open file first, then check fd's real path) or O_NOFOLLOW. Round 3 fix may not be sufficient for automated resolution — escalating to needs-human.

## Verification Round 2: FAIL HIGH: TOCTOU race in Ingest — validates path during walk (ingest.go:104) but reopens cached pathname later with coreio.Local.Read (ingest.go:152). File can be swapped to symlink between check and read. This is a fundamental TOCTOU — needs open-then-validate (open file first, then check fd's real path) or O_NOFOLLOW. Round 3 fix may not be sufficient for automated resolution — escalating to needs-human.

Virgil referenced this issue from a commit

2026-04-27 13:47:41 +00:00

fix(rag): address CodeRabbit findings on PR #4

Virgil referenced this issue from a commit

2026-04-27 16:19:24 +00:00

fix(rag): r2 — AX-2 docstrings + remaining residual findings on PR #4