fix(hardening): concurrency, durability, and security hardening - #185
Merged
Conversation
Security: - Cap daemon autonomy server-side: clients can no longer request full/yolo autonomy; operators opt in via Config.MaxAutonomy (--autonomy / HAWK_DAEMON_AUTONOMY), default capped at semi. - Refuse non-loopback daemon binds without TLS (API key and session transcripts would otherwise travel in plaintext); enforce TLS 1.2+ and MaxHeaderBytes on the daemon HTTP server. - Harden POST /v1/review: validate concerns/model against a printable charset with a length cap and bound concurrent review subprocesses. - Cap HTTP decision-hook response bodies (64 KiB) and warn loudly when a guardrail hook is configured fail-open. Reliability: - Propagate previously swallowed review-store errors so reviews can no longer get stuck in "running" or silently re-review corrupt stores. - Surface credential-gate symlink flip failures to the user instead of silently leaving the container without the approved credential. - Replace init-time panics in storage path resolution with safe temp-dir fallbacks. - Route yaad bridge diagnostics through slog so they survive TUI mode. - Use typed engine.WorkMode constants instead of raw string compares. - Make alert queue drain responsive to Stop() without dropping alerts. Tests: - Enable the previously build-tagged sqlite store tests by default. - Fix the golden test to actually diff against the golden file (regenerated for the current root help output). - Harden weak assertions in config/providers/review tests and add BM25 unit tests for the previously untested scoring package.
Reliability: - Deregister the SIGHUP forwarder after first use so repeated runChat() invocations no longer leak signal handlers and goroutines. - Cancel any prior in-flight stream in startStream before overwriting the cancel func, preventing an orphaned stream goroutine. - Route async hook execution errors through slog instead of discarding them silently. - Log and requeue langfuse batches on transient flush failures (bounded at 2x flush size) instead of silently dropping telemetry. - Record the background TaskRunner.Run error and expose it via RunErr() so callers can observe a failed background run. Tests: - Add dependency-boundary tests for internal/token (forwards to tok). - Add tests for internal/home (Dir/Expand and env handling). - Add drift-detection tests for cmd/compat-test (pin freshness logic). Chore: - Refresh GitNexus symbol counts in AGENTS.md after re-index.
Daemon middleware: - Test clientIP, generateRequestID, isCORSSettingAllowed, and stripSlackMention pure helpers. - Test request-ID propagation (generated and client-supplied), security header emission (incl. HSTS gating and Cache-Control on POST), CORS preflight and disallowed-origin behavior, responseWriter status capture, and the full installed middleware stack over HTTP. Review engine: - Add ListAll (incl. limit), Get-on-missing, empty-diff→passed lifecycle, WAL-checkpoint Close, and splitReviewStatements tests. - Add printReviewSummary (no-findings and with-findings) and silentErr (background suppresses, foreground wraps) tests.
- Return tabwriter and json.MarshalIndent errors in `hawk trust list` and `hawk plugin status` instead of discarding them. - Propagate the review-schema version scan error during migration so a corrupt store surfaces loudly instead of silently re-migrating. - Log auto-init failures via slog instead of swallowing them. - Route codegraph sync warnings through slog so they survive TUI mode (where the stdlib logger is discarded). - Convert WAL batch flush and settings-parse warnings from hand-rolled stderr prints to slog for consistent structured diagnostics. Tests: - Add internal/startup phase timing tests (mark/end, noop end, latest- open-close semantics, Reset, copy-on-read, TotalTime).
Security: - Netproxy handleHTTP now enforces policy on r.URL.Host (the actual dial target) instead of only the Host header. An absolute-form request URI whose URL host differs from a benign Host header can no longer bypass the allow/deny and private-network rules. Stats/logs also record the enforced target. - Marketplace installs reject scp-style (git@host:...) repo URLs, which bypass the HTTPS transport and cannot be pinned or verified. - Marketplace installs now fail closed when the cloned plugin has no plugin.json manifest, instead of only scanning when the file happens to exist. Tests: - Regression test proving an absolute-URI target pointing at a blocked domain is refused even when the Host header names an allowed domain. - Test that scp-style marketplace repo URLs are rejected.
S1 — Terminal escape injection:
- Add sanitizeDisplay() which strips ANSI CSI/OSC sequences, C0 controls,
and DEL from untrusted content while preserving newlines/tabs.
- Apply it at the render choke points (renderDisplayMessage and the
stream tail) so tool results, model output, thinking, system messages,
and the permission box body can no longer forge prompts or hijack the
terminal.
S2 — Secret display:
- Tool output is now redacted before the user-facing tool_result stream
event, not just before the model copy. Wired via a redactOutput
callback on toolExecutionDeps (falls back to unchanged when absent).
S3 — Dangerous-mode confirmation:
- --dangerously-skip-permissions now requires typing the literal token
"dangerous-skip-permissions" instead of a single y.
- Selecting YOLO ("Autonomous") in the autonomy picker now requires
typing "continue" to confirm; anything else cancels.
S4 — Folder trust parity:
- Print/repl/watch paths now enforce the same folder-trust gate as the
TUI, so project-scoped hooks/MCP/plugins are uniformly blocked in
untrusted directories.
…n exit
R1 — Panic safety:
- runChat now registers a panicSaveFn closure that persists the active
session and stops the container on an unexpected panic, so a crash
loses at most the in-flight message and never leaves a zombie Docker
sandbox. RunWithPanicRecovery invokes it instead of nil.
R2 — Incremental durability:
- Wire the existing (previously unused) BatchedWAL into the TUI session
log so per-message fsync is timer-batched instead of stalling the UI
thread.
- Persist completed tool results to the WAL as they happen (previously
only written at turn end), so a crash mid-turn keeps them.
- The model's WAL field is now a small sessionWAL interface (Append/
Remove/Close) satisfied by both WAL and BatchedWAL.
R3 — Unified quit:
- /quit and /exit now use the canonical quitModel() sequence (cancel
stream -> save -> stop watcher/parallel/bg -> stop container) instead
of a hand-rolled duplicate that orphaned the in-flight stream. Sleep
inhibitor is cancelled first.
R4 — Visible persistence failures:
- WAL append errors now surface once as a persistent inline banner
("session persistence is failing") instead of being silently dropped.
R5 — No false crash reports:
- Drop the SIGTERM goroutine-dump handler: SIGTERM is Bubble Tea's
graceful-quit signal, so a normal `kill <pid>` no longer writes a
spurious crash-signal-SIGTERM report. SIGQUIT dumps remain.
P1 — Deduplicate the registry build: - runChat now passes the registry already built by its startup goroutine into newChatModelWithRegistry instead of rebuilding it (which re-ran MCP server startup a second time). Removed the now-unused wrapper. P2 — Defer the pre-paint DNS lookup: - The provider DNS reachability check moved out of validateStartup (which runs before first paint) into the post-paint ui-cache-warm background goroutine, so an offline machine no longer stalls TUI startup for seconds. validateStartup keeps only the local checks (API key, sessions dir writability). P3 — Cache cwd in the status bar refresh: - refreshStatusBarLeft now short-circuits on a cached value before the os.Getwd() syscall, so per-keystroke updates skip the syscall entirely. P4 — Non-blocking MCP startup: - MCP tools load asynchronously after the registry is built (up to 1.5s per server no longer blocks first paint); CLI tool filters still apply. Loader functions are captured by value so the async goroutine cannot race with tests that swap the package-level loader vars. P5 — Offload session save off the UI thread: - The turn-end session JSONL write now runs as a background tea.Cmd via saveSessionCmd (atomic tmp+rename makes this safe; WAL is removed only after a successful save). Quit keeps the synchronous save since the process is exiting. WAL fsync batching landed with BatchedWAL in the reliability batch.
- C1: atomic tryConsumeApproval to fix approval N-count TOCTOU race - C2: Wait/WaitIDs don't hold mutex across cond.Wait; timer guards timeout - C3+C4: document WaitAsync drain; track async hook goroutine on asyncWG - C5: Cron Stop() drains in-flight jobs via inFlightWG - C6: FlushOTel flushes meter provider too (was dropping metrics) - C7: dedupe Ctrl+C quit logic through quitModel() for cancel cleanup - M1: LSP readLoop parses Content-Length framing with io.ReadFull - M2: LSP acquire() uses initDone channel to prevent duplicate servers - M3: EndSession/session-end hook use context.WithoutCancel - M4: remove glob-collapsing loop that broke detection - M10: retry only retryable url.Error, not permanent client errors - m7: strengthen dangerously-skip-permissions confirmation token
Submodule sync: external/trace advanced to port unified checkpoint architecture. Bumps trace require and its transitive dependencies (go-git v6 alpha.5, auth-go v0.5.2, etc.) per MVS selection.
taskruntime.Wait/WaitIDs: replace the one-shot time.AfterFunc broadcast with a timer goroutine that repeats the broadcast after the deadline. A single broadcast could be lost in the microsecond window between loop iterations while the waiter is outside cond.Wait(), letting the wait block past its deadline. Repeating guarantees the deadline is observed; a stop channel terminates the goroutine when the wait returns so it cannot leak. chat_update.quitModel: flush input history here so every quit path (/quit, SIGTERM, SIGINT, Ctrl+C) persists it. Previously saveInputHistory was only called on the config-view double-Ctrl+C path, so history was silently lost on all other exits.
Advances external/trace and bumps go.mod/go.sum requires to match the new gitlink. MVS pulls in transitive upgrades from the trace release.
An earlier commit added the 'entire' command and removed 'trace' from the hawk command list but did not regenerate the golden file. Update it to match the current help output so the golden test passes.
markdownlint MD025 flags multiple top-level headings. The GitNexus block injected a second # heading; demote it to ## so it reads as a section within the document. The document's single real title remains '# Extending hawk'.
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
Deep-review hardening pass across the codebase, fixing concurrency races, goroutine leaks, durability gaps, and security weaknesses.
Critical fixes
Medium fixes
Security
Verified