fix: batch 2 — 9 verified bugs, SECURITY.md compaction + verification, 3 audit-gap closures - #141
Merged
Merged
Conversation
shell sandbox kill follow-up has no deadline (wedged tool call), session search paginates against a pre-filtered window (matches silently dropped), prompt-cancel registry cross-deletion kills live prompt's cancel func, REPL advertises unimplemented slash commands, telegram session TTL expiry is dead code, skill context injected after the user message, heartbeat SignalHandler invoked concurrently from parallel watchdogs (-race canary), base64 decode-from-file returned outside the untrusted-content wrapper, FirstSentence picks a later sentence when !/? precedes .
Validated with failing RED tests (previous commit) that now pass. Full
suite, -race on changed packages, golangci-lint all green.
cmd/odek:
- shell: the sandbox kill follow-up now runs under a 10s deadline — a hung
Docker daemon can no longer wedge the tool call forever after its own
timeout fired
- serve_api: /api/sessions search scans the whole index before paginating;
matches deeper in recency order than limit+offset were silently dropped
- serve: prompt-cancel registrations are generation-guarded — when two
prompts run on one session, the first finisher no longer deletes the
live prompt's cancel func (registerPromptCancel returns an unregister
closure; legacy helper kept)
- repl: tab-completion list is now the single source of truth synced with
handleREPLCommand (/sandbox /model /session were advertised but unknown)
- subagent: task errors exit 1 and timeouts exit 2 per docs/EXTENSIONS.md;
previously every outcome exited 0 after startup succeeded (typed
subagentRunError mapped in dispatch; failure line replaces the ✅ banner)
internal/loop:
- skill/episode/extended-memory blocks are injected right BEFORE the
latest user message as documented; the old scan skipped index 0 and
appended after it. headLen gains ctxLeadDroppableFrom so injected blocks
stay trimmable (oversized injection still dropped pre-call, pinned by
TestTrimContext_PostInjectionBudget) while base system + memory remain
protected for prompt caching
- emitSignal serializes handler invocation: parallel tool heartbeats fire
from separate goroutines, and SignalHandler consumers are only promised
non-blocking, never concurrent-safe
internal/render:
- FirstSentence picks the EARLIEST sentence boundary instead of iterating
separator types ('Done! Next step.' returned the second sentence)
internal/telegram:
- GetOrCreate evicts an expired cache entry before Load; TTL expiry was
dead code because Load returned the same stale pointer unchecked
Hardening (no dedicated RED):
- events/jsonl: OpenJSONLSink opens with O_NOFOLLOW, closing the
Lstat-then-open symlink swap race
- mcpclient: child stderr is actually inherited (os.Stderr); nil connects
to /dev/null, contradicting the comment and hiding server diagnostics
Investigated and rejected: base64 decode-of-inline-string returning
unwrapped content — decode input is model-supplied tool args, not fresh
external data; there is also no decode-from-path feature to wrap.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
odek | 10dca7e | Commit Preview URL Branch Preview URL |
Aug 22 2026, 12:44 PM |
…l integrity, O_NOFOLLOW event sink SECURITY.md §75 records the five batch-2 follow-ups in the mitigation-log style; EXTENSIONS.md's sink contract now mentions the O_NOFOLLOW open that closes the Lstat→open symlink-swap window.
Replace the append-only fix log (§16-§75, 'previously X / now Y' entries, duplicate §18b/§25/§26 numbering, dangling internal § references) with present-tense documentation grouped by subsystem: named sections, one resource-bounds table, a deduplicated attack-vector matrix, and a single pointer to the regression-bar tests. Per-fix history remains in the git log and the named TestAudit_*/TestRED_* suites. 1028 lines / 121 KB -> 566 lines / 78 KB.
…ure missing defenses Cross-checked every claim against the code (file:line evidence) and corrected eight factual errors: - WS concurrency rejection surfaces as HTTP 403 (x/net/websocket maps handshake errors to StatusForbidden), not 503 - trust-anchor list: project_sandbox_approvals.json is NOT in the code's anchor list (removed from doc); telegram.pid IS (added) - MCP per-tool approval runs for every server, not just project ones; only operator-set auto_approve / env bypass / persisted approvals skip - git -c is code_execution only for exec-capable keys (alias.* !, core.pager, core.fsmonitor, credential.helper); git config always - sandbox: host network coerced to none with warning (not hard reject); setuid wording -> --security-opt no-new-privileges + noexec tmpfs - group/world-readable secrets.env is refused outright (config.json only warns) - forged trust responses: WS coerces to one approve, Telegram denies, TTY re-prompts (was blanket 'coerced to single approve') - episode/skill promotion is no longer CLI-only (operator-authenticated REST endpoints exist); still never an agent tool Added undocumented state of the art: external session refs (validated, deduped, never dereferenced), static security headers + no-store on the token-bearing page, serve input caps (WS 8 MiB, prompts 1 MiB, REST bodies, attachments), model-ID validation, client-side envelope hardening, mcp_servers.*.auto_approve trust rules (+ schedules.dangerous / maintenance / trusted_proxies / tools.enabled / sandbox:false rejections), MCP server mode surface, Extended Memory quarantine, skill import SSRF guard + symlink refusal, compaction-digest untrusted wrapping, sandbox wrapper positional-arg quoting defense, artifact Render CR/LF flattening, upgrade checksum verification, and three missing redaction families + the known-value registry.
…EEN) - ~/.odek/project_sandbox_approvals.json is now a trust anchor in both danger.isOdekTrustAnchor and confineToCWD's protected list: file tools previously wrote it as local_write, letting a prompt-injected agent pre-approve its own project's sandbox overrides. - skill_patch runs the patched body through internal/redact before writing (same bar as WriteSkill): the two direct os.WriteFile calls bypassed the secret scan every other SKILL.md write path applies, and a needed redaction now also counts as a review flag in the response. - stale comments in internal/memory (scan.go, memory.go) claimed FactLooksUnsafe was auto-extract-only; it also guards agent-driven memory add/replace via AddFact/ReplaceFact. RED first: TestRED_ClassifyPath_ProjectSandboxApprovalsAnchor, TestRED_ConfinesProjectSandboxApprovals, TestRED_SkillPatchRedactsSecrets. All four touched packages green: danger, skills, memory, cmd/odek. docs/SECURITY.md trust-anchor list and skill-redaction wording updated.
jkyberneees
changed the title
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
Follow-up to #140: a second RED/GREEN batch resolving 9 further verified bugs from the original hunt, plus 2 hardening fixes. Each bug is pinned by a failing test (commit 1) that passes after the fixes (commit 2).
fix/batch2-red-green· Commits:7c260f5(RED) →fd7b195(fixes)The bugs
cmd/odek
docker execkill ran with no context; a hung dockerd wedged the tool call forever. Now bounded by a 10s deadline. (RED: fake docker on PATH wedged 20s)/api/sessions?q=…filtered the newestlimit+offsetentries only; matches deeper in recency order silently returnedcount=0. Search now scans the full index (an index read) before paginating./api/cancela silent no-op. Registrations are now generation-guarded (registerPromptCancelreturns an unregister closure)./sandbox /model /session, all yielding "Unknown command".replCommandsis now the single source of truth synced with the handler."status":"error"JSON but exited 0, breaking the documented 0/1/2/3 contract in docs/EXTENSIONS.md. TypedsubagentRunErrormaps through dispatch to exit 2 (timeout) / 1 (task error); failure banner replaces the ✅ line.internal/loop
headLengained actxLeadDroppableFromboundary: injected blocks stay droppable while base system + memory remain protected for prompt caching. (Validated against both new RED and existingTestTrimContext_PostInjectionBudget.)emitSignalnow serializes invocations. (RED: overlap detector under-race)internal/render
!/?preceded any later.— it iterated separator types, not boundary positions. Now picks the earliest boundary.internal/telegram
GetOrCreateevicted nothing on expiry andLoadreturned the same stale cached pointer unchecked. Expired entries are now evicted before reload.Hardening (no dedicated RED)
events/jsonl:OpenJSONLSinkopens withO_NOFOLLOW, closing the Lstat→open symlink-swap race on the event stream.mcpclient: child stderr is genuinely inherited (os.Stderr) —nilconnected to/dev/null, contradicting the comment and hiding MCP server diagnostics.Investigated & rejected
Test plan
-raceconcurrency detector)go test ./... -count=1greengo test -racegreen on all changed packagesgolangci-lint run: 0 issues; gofmt clean on all touched filesTestTrimContext_PostInjectionBudget,TestTrimContext_OverBudget, prompt-cancel legacy tests)Addendum — follow-up commits on this branch
Three additional commits landed on this branch after the batch-2 fixes:
docs(security): SECURITY.md compaction + implementation verification (
85f50c8,832d826)git -csemantics; sandbox setuid wording; secrets.env refusal; forged-trust behavior per approver; promote no longer CLI-only) and ~15 undocumented defenses added (external session refs, static security headers, serve input caps, MCP server mode, Extended Memory quarantine, skill-import SSRF guard, compaction-digest wrapping, upgrade checksums, redaction families, …).fix: close 3 audit gaps found during that verification (
10dca7e, RED/GREEN)~/.odek/project_sandbox_approvals.jsonmissing from trust anchors — file tools could write it aslocal_write, letting a prompt-injected agent pre-approve its own project's sandbox overrides. Now an anchor in bothdanger.isOdekTrustAnchorandconfineToCWD's protected list.skill_patchbypassed secret redaction — its two directos.WriteFilecalls skipped theinternal/redactpass every other SKILL.md write path applies. Patched bodies are now redacted (same bar asWriteSkill); a triggered redaction also counts as a review flag.internal/memorycomments claimedFactLooksUnsafewas auto-extract-only; it also guards agent-drivenmemory add/replace.RED tests:
TestRED_ClassifyPath_ProjectSandboxApprovalsAnchor,TestRED_ConfinesProjectSandboxApprovals,TestRED_SkillPatchRedactsSecrets— all confirmed failing before, green after (danger, skills, memory, cmd/odek suites).