Audit-pass follow-up: security/OTP/test hardening#60
Merged
Conversation
…-closed, atom-DoS, ReDoS cap, UrlGuard ranges, docs)
…async-load reply-on-crash, async_nolink absorb, claims O(1), iodata flat accumulation, drop needless ets rescue)
…receive, start_supervised!, non-tautological glob assertion, unique telemetry handler IDs+detach, membership asserts, encoded-IP SSRF cases, async path_guard)
…sions stores), safer slug-index write order
…rrect async_nolink completion-clause comment
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
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 hardening pass addressing review findings on commit #59 (the security/bug/performance audit). Implemented across 4 phases; all changes verified green. 24 files, +471/−120. No behavioral API
breaks.
Changes
Security
real_path, so file tools open the validated inode instead of re-traversing an attacker-swappable symlink (narrows TOCTOU).ensure_within/2gaineda resolved-root fallback to stay idempotent when
file_glob/file_grepre-validate wildcard results. The residual TOCTOU window (Erlang's:fileAPI has noO_NOFOLLOW) is documented.pin_connection(_, _, nil)now fails closed; no unpinned fetch path can be reintroduced viaallow_private_hosts.String.to_atom/1on YAML-controlled eval input →String.to_existing_atom/1+ fallback (eval/runner.ex,eval/evaluators/schema.ex), with a regression test proving no atomis interned.
rgpath was already safe.198.18.0.0/15+ RFC 5737 test-nets; new regression tests for decimal/hex/octal/encoded-IP SSRF bypasses.Correctness (OTP / Elixir)
get_tool_field/2:||→Map.fetch/2(falsy-safe forfalse/0/"").Process.alive?pre-check;safe_acquire/3catches:exitand fails open with a log + telemetry signal instead of crashing the agent loop.agent_serverasync load task always replies (try/rescue/catch), so a raising backend can't wedge the caller until timeout.async_nolink{ref, result}completion message is absorbed by a dedicatedhandle_infoclause instead of falling through to the catch-all.shared_stateclaims: O(1) prepend + reverse on read (matches the discoveries fix).:ets.inserttry/rescue that masked genuine table bugs.Tests
Process.sleepcalls (rely on FIFOGenServer.callserialization or an explicit flush);refute_receiveover instantrefute_received;start_supervised!for named helper Agents (noleak on crash); non-tautological glob-injection assertion; unique telemetry handler IDs + detach; membership asserts on the singleton persistence table;
async: truefor PathGuard tests.ETS ownership
:publicownership model for the KnowledgeBase and Decisions stores (cross-process tool execution requires:public; access is gated by possession of thetable reference). Safer slug-index write order.
Verification
mix compile --warnings-as-errors✅mix format --check-formatted✅mix credo --strict✅ (no issues)mix test✅ 1810 passed, 0 failed, 101 excluded (+4 new tests)Reviewed by security + Elixir specialist passes; findings addressed.
Deferred (intentional)
run_tool_with_hooks/11— pure readability; Credo already passes.UrlGuardresolver injectability.