core/go-ratelimit

Fork 0

[audit] Deep audit — missing tests, edge cases, error handling #3

New issue

Open

opened 2026-03-22 16:37:31 +00:00 by Virgil · 8 comments

Virgil commented

2026-03-22 16:37:31 +00:00

Member

Prior AX sweep found:

2 HIGH (negative token bypass, nil UsageStats crash)
2 MEDIUM (dead Config.Backend field, stale SQLite merge)
4 LOW (non-atomic persist, unsanitized URL, unbounded body read, panic on invalid interval)

This audit should go deeper: find missing test coverage, untested edge cases, and additional security concerns.

Prior AX sweep found: - 2 HIGH (negative token bypass, nil UsageStats crash) - 2 MEDIUM (dead Config.Backend field, stale SQLite merge) - 4 LOW (non-atomic persist, unsanitized URL, unbounded body read, panic on invalid interval) This audit should go deeper: find missing test coverage, untested edge cases, and additional security concerns.

Virgil commented

2026-03-22 16:47:13 +00:00

Author

Member

Spark Audit Findings (20 total)

HIGH (2)

Token bypass via negative/overflow inputs (ratelimit.go:387, :409)
Nil UsageStats panic in prune (ratelimit.go:290)

MEDIUM (4)

Config.Backend documented but ignored (ratelimit.go:56, :145)
CountTokens unsanitized model in URL (ratelimit.go:651)
BackgroundPrune panics on non-positive interval (ratelimit.go:331)
SQLite migration non-atomic (ratelimit.go:636, :642)

LOW (4)

loadSQLite merges instead of replaces (ratelimit.go:229)
Unbounded error body read (ratelimit.go:681)
Missing tests for NewWithConfig backend selection
Missing CountTokens success path tests

## Spark Audit Findings (20 total) ### HIGH (2) 1. Token bypass via negative/overflow inputs (ratelimit.go:387, :409) 2. Nil UsageStats panic in prune (ratelimit.go:290) ### MEDIUM (4) 3. Config.Backend documented but ignored (ratelimit.go:56, :145) 4. CountTokens unsanitized model in URL (ratelimit.go:651) 5. BackgroundPrune panics on non-positive interval (ratelimit.go:331) 6. SQLite migration non-atomic (ratelimit.go:636, :642) ### LOW (4) 7. loadSQLite merges instead of replaces (ratelimit.go:229) 8. Unbounded error body read (ratelimit.go:681) 9. Missing tests for NewWithConfig backend selection 10. Missing CountTokens success path tests

Virgil referenced this issue

2026-03-23 07:26:46 +00:00

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

Virgil referenced this issue from a commit

2026-03-23 07:26:50 +00:00

Merge pull request '[agent/codex] Fix ALL findings from issue #3. Read CLAUDE.md first. Token ...' (#4) from agent/deep-audit-per-issue--3--read-claude-md into dev

Virgil commented

2026-03-23 07:27:04 +00:00

Author

Member

Fix Applied

Commit d1c90b9: fix(ratelimit): harden audit edge cases

Token bypass via negative inputs fixed
Nil UsageStats guard added
CountTokens URL sanitised
BackgroundPrune validates interval
SQLite migration made atomic
713 additions across 5 files, 174-line sqlite_test.go added

## Fix Applied Commit d1c90b9: fix(ratelimit): harden audit edge cases - Token bypass via negative inputs fixed - Nil UsageStats guard added - CountTokens URL sanitised - BackgroundPrune validates interval - SQLite migration made atomic - 713 additions across 5 files, 174-line sqlite_test.go added

Virgil commented

2026-03-23 09:12:14 +00:00

Author

Member

Verification: FAIL (reproduced)

loadSQLite only replaces Quotas when persisted map is non-empty — breaks snapshot semantics. saveSnapshot clears + rewrites, but Load skips empty maps. Empty quota snapshot can't round-trip (reproduced: Persist with Quotas={}, reopen, Load returns stale defaults).

Needs: loadSQLite must replace Quotas even when persisted map is empty.

## Verification: FAIL (reproduced) loadSQLite only replaces Quotas when persisted map is non-empty — breaks snapshot semantics. saveSnapshot clears + rewrites, but Load skips empty maps. Empty quota snapshot can't round-trip (reproduced: Persist with Quotas={}, reopen, Load returns stale defaults). Needs: loadSQLite must replace Quotas even when persisted map is empty.

Virgil commented

2026-03-23 09:16:09 +00:00

Author

Member

Fix Round 2

Commit 9e715c2: fix(sqlite): replace quotas on empty snapshot load

loadSQLite now always replaces Quotas, even when persisted map is empty
27-line round-trip test added
Dispatching verification.

## Fix Round 2 Commit 9e715c2: fix(sqlite): replace quotas on empty snapshot load - loadSQLite now always replaces Quotas, even when persisted map is empty - 27-line round-trip test added Dispatching verification.

Virgil commented

2026-03-23 09:22:59 +00:00

Author

Member

Verification Round 2: FAIL

HIGH: Empty snapshot fix breaks first-run — new DB has empty quotas table, unconditional Load() wipes configured defaults, CanSend falls into 'unknown model allowed' path. Rate limiting effectively disabled on first run.

Needs: distinguish 'never persisted' from 'intentionally empty'. Either a sentinel/flag in DB or don't Load() until first Persist() has occurred.

## Verification Round 2: FAIL HIGH: Empty snapshot fix breaks first-run — new DB has empty quotas table, unconditional Load() wipes configured defaults, CanSend falls into 'unknown model allowed' path. Rate limiting effectively disabled on first run. Needs: distinguish 'never persisted' from 'intentionally empty'. Either a sentinel/flag in DB or don't Load() until first Persist() has occurred.

Virgil commented

2026-03-23 09:31:13 +00:00

Author

Member

Fix Round 3

Commit 5df6ce1: fix(sqlite): preserve defaults before first persist

Added metadata tracking for persist state (never-persisted vs intentionally-empty)
115-line sqlite_test.go + 35-line error_test.go added
205 additions across 4 files
Dispatching verification.

## Fix Round 3 Commit 5df6ce1: fix(sqlite): preserve defaults before first persist - Added metadata tracking for persist state (never-persisted vs intentionally-empty) - 115-line sqlite_test.go + 35-line error_test.go added - 205 additions across 4 files Dispatching verification.

Virgil commented

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

Author

Member

Verification Round 3: FAIL (medium — legacy migration)

MEDIUM: Legacy pre-snapshot_meta SQLite files with intentionally empty snapshots still load incorrectly. Backfill only marks DB as having snapshot when tables have rows — valid old empty snapshot stays unmarked, Load returns early.

This is the oscillation pattern (plan anomaly #16) — each fix handles one case but misses another. 3 rounds on this specific state machine. Escalating to needs-human.

## Verification Round 3: FAIL (medium — legacy migration) MEDIUM: Legacy pre-snapshot_meta SQLite files with intentionally empty snapshots still load incorrectly. Backfill only marks DB as having snapshot when tables have rows — valid old empty snapshot stays unmarked, Load returns early. This is the oscillation pattern (plan anomaly #16) — each fix handles one case but misses another. 3 rounds on this specific state machine. Escalating to needs-human.

Virgil commented

2026-03-23 12:12:26 +00:00

Author

Member

API Contract Extraction

Full exported API inventory. Key stats:

All types and constructors have test coverage
Zero exports have usage-example comments
15+ exported types/functions including RateLimiter, Config, Provider, ModelQuota, ProviderProfile, UsageStats, TokenEntry, ModelStats
Constructors: New(), NewWithConfig(), NewWithSQLite(), NewWithSQLiteConfig()
Package path still uses forge.lthn.ai (vanity import not migrated)

Full table in agent log.

## API Contract Extraction Full exported API inventory. Key stats: - All types and constructors have test coverage - Zero exports have usage-example comments - 15+ exported types/functions including RateLimiter, Config, Provider, ModelQuota, ProviderProfile, UsageStats, TokenEntry, ModelStats - Constructors: New(), NewWithConfig(), NewWithSQLite(), NewWithSQLiteConfig() - Package path still uses forge.lthn.ai (vanity import not migrated) Full table in agent log.