fix: security and correctness hardening across credentials, API, storage, and tooling#45
Merged
Merged
Conversation
…rage
Critical:
- audit.go: tighten JSONLFileSink file perms 0o644 -> 0o600 (metadata)
- api/server.go: store and call bgCancel in Shutdown (context leak);
add validateAuthConfig loopback auth gate on ListenAndServe
- guardrails.go: add AddRuleSafe/NewGuardrailsSafe (error-returning
variants) for untrusted rule sources; fix ApplyRedactions to use
FindAllStringIndex positions so the correct match instance is
redacted instead of the first occurrence via strings.Index
- cache/backend.go: bound RESP bulk-string allocation at 64 MB to
prevent memory-exhaustion DoS from a malicious Redis
- budgets.go: chmod budget DB to 0o600 (stores plaintext API keys)
High:
- conversation/engine.go: defer-close the final StreamResult on all
exit paths; add nil-provider guard in streamAndSave
- client/stream.go: sort OpenAI tool calls by Index before emitting
(was non-deterministic map iteration); nil args -> {_raw} fallback
- credentials/keyring_platform.go: only map not-found errors to
ErrNotFound; real backend errors now pass through so LookupSecret
logs them at Warn instead of silently classifying as 'no secret'
- openai_proxy.go: surface streaming errors as SSE data event before
[DONE]; raise body limit to 10 MiB for multi-turn conversations
Medium:
- provider_registry.go: bound resolveEnvSecret with 10s timeout ctx
- retry.go: time.After -> time.NewTimer + Stop (avoid timer leak)
- sqlite.go: escape backslash in GetNodeByPrefix LIKE query
- config/provider_env.go: ValidateBaseURL uses url.Parse instead of
os.Stat; config dir perms 0o755 -> 0o700
…server - ratelimit.go: replace time.After with time.NewTimer+Stop in both token-bucket wait paths (minInterval enforcement and token-deficit wait) to prevent runtime timer leaks on ctx cancellation - adaptive_ratelimit.go: same fix in RPM and TPM throttle paths - internal/api/server.go: wrap handler chain with securityHeaders middleware (X-Content-Type-Options, X-Frame-Options, Cache-Control) matching the hawk daemon's security posture
- router/retry.go: replace afterFunc = time.After with newTimer = time.NewTimer to avoid leaking the timer in the runtime when ctx is cancelled before the delay elapses - router/router.go: update call site to use newTimer + timer.Stop()
- internal/httputil: new package with canonical ConstantTimeEqual, DecodeJSONBody, WriteJSON, SecurityHeaders, IsLoopbackHost, ValidateAuthConfig, ExtractBearerToken — eliminates duplication across internal/api/server.go (previously had local copies) - internal/api/server.go: delegate to httputil; remove duplicated constantTimeEqual, decodeJSONBody, writeJSON, securityHeaders, validateAuthConfig, isLoopbackHost - internal/api/auth_test.go: update to use httputil.ConstantTimeEqual - config/runtime.go: bound envValue keyring lookup with 5s timeout (was unbounded context.Background()) - runtime/runtime.go: bound CredentialTargets keyring probes with 5s timeout each (was unbounded context.Background())
Brings in 4 commits covering critical/high/medium fixes and the centralized internal/httputil package. See branch history for details.
- budgets.go: chmod WAL/SHM sidecar files to 0o600 in addition to the main DB file (WAL holds uncheckpointed pages including plaintext keys) - httputil.go: make ExtractBearerToken case-insensitive per RFC 7235 and adopt it in Server.auth (was dead code with duplicated logic) - server.go: use httputil.ExtractBearerToken instead of inline duplicate
Contributor
Author
Code Review — Findings & FixesReviewed all 23 changed files. Found 2 issues (1 P1, 1 P3), both fixed in commit [P1] storage/budgets.go: chmod 0o600 misses WAL/SHM sidecar files
[P3] internal/httputil/httputil.go: ExtractBearerToken was dead code
Verified cleanConstantTimeEqual (crypto/subtle), IsLoopbackHost (IPv4/IPv6/mapped), ValidateAuthConfig gate, bgCancel lifecycle in Shutdown, guardrails FindAllStringIndex redaction (sorted desc, right-to-left), LIKE backslash escaping, cache RESP 64MB bound, keyring error mapping, bounded keyring lookups (no goroutine leak), conversation/engine StreamResult closed on all paths, OpenAI tool call sort by Index, timer.Stop() on all paths. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Comprehensive security and correctness hardening across eyrie, covering 4 commits and 21 files.
Critical fixes
High fixes
Medium fixes
Refactor
Verification
go build ./...go test -race -count=1 ./...— all passgo vet ./...go fmtclean