Skip to content

fix: four defensive fixes from a second-lens engine hunt - #113

Merged
ElxMaj merged 1 commit into
mainfrom
fix/second-lens-hunt
Jul 19, 2026
Merged

fix: four defensive fixes from a second-lens engine hunt#113
ElxMaj merged 1 commit into
mainfrom
fix/second-lens-hunt

Conversation

@ElxMaj

@ElxMaj ElxMaj commented Jul 18, 2026

Copy link
Copy Markdown
Owner

A second-lens adversarial hunt over the shipped engine (main #104), across the axes the first correctness hunt did not cover: concurrency/transactions, prompt injection, connector SSRF/secrets, DoS/ReDoS, edge-input integrity, and the MCP/web boundary. Every candidate had to survive an independent refute-by-default verifier. Five distinct defects were confirmed; the scarier answer() double-decide candidate was correctly refuted by the skeptic and dropped.

This PR ships the four rule-safe, defensive fixes (each with a regression test) and defers the one security-boundary finding to you (below).

Shipped here

Sev File Defect Fix
HIGH scrub.ts The private-key detector was O(n^2). The lazy BEGIN...END regex scanned to end-of-string once per BEGIN header, so a crafted multi-MB blob of BEGIN lines stalled the single event loop for seconds on one evidence insert (measured ~4.4s at 2MB, ~1000x amplification vs benign text). A non-backtracking scanner that finds each BEGIN and jumps to the next END by index. Both cursors only move forward: linear.
HIGH sync.ts A connector item whose source timestamp does not parse became a truthy Invalid-Date watermark. No real date can exceed NaN, so it pinned the cursor, then crashed the unguarded watermark.toISOString() outside the try/catch. The cursor never advanced, so every later run refetched the same item and crashed identically: a permanent per-connector wedge on ordinary provider glitches. Treat an Invalid Date like a missing timestamp; the cursor falls back to wall-clock.
MED marrow.ts / store.ts distill TOCTOU. The dedup seen set is built from an in-memory read and each insert mints a fresh UUID, so two distills of the same fresh evidence (a scheduled distill --pending overlapping a manual distill) both read empty and each insert the full node set, duplicating every node. Retrieval and the neighbor walk then double-count. A per-evidence session advisory lock (withDistillLock, its own namespace) mirroring the connector lock, so the second pass blocks, then sees the first's nodes and skips them. Distinct evidence still distills in parallel.
LOW injection.ts The instruction-smell detector missed the canonical "ignore the above and <do X>" override, because pattern 1 required a trailing instructions/rules noun after the directional anchor. Advisory-only (a missing SMELLS badge), no sacred-rule impact. Added the anchor-as-object pattern while keeping the stricter one.

Sacred rules preserved: no fix writes a status, sets confidence.source='human', or promotes anything. The distill lock is advisory serialization only; the injection change is an advisory read-time badge; the sync and scrub fixes are pure input hardening.

Deferred to you (not shipped): HIGH Jira credential-exfil / SSRF

connectors/jira.ts attaches the decrypted Basic email:apiToken to a fully config-controlled baseUrl with no host allowlist, so a config-only rewrite of baseUrl (the GET view never returns the token, but a re-upsert coalesces the old cipher) exfiltrates a live credential, and an internal https host turns the sink into SSRF.

I did not autonomously ship this, for three reasons:

  1. Its exfil vector is the CSRF-able unauthenticated write endpoint, which the founder-gated PR web: the local brain is not CSRF-able (R11) #96 ("the local brain is not CSRF-able") already closes. Landing web: the local brain is not CSRF-able (R11) #96 removes the reachable path.
  2. The only exfil-stopping change inside the connector is a host allowlist, which is a product decision that would break self-hosted Jira Data Center (arbitrary host).
  3. It sits on the connector/security boundary that is founder territory in this repo.

Recommend landing it as a human change alongside #96. Full repro is in the hunt output.

Verification (all from repo root, explicitly)

  • pnpm typecheck clean across all packages
  • pnpm lint clean (eslint + prettier)
  • pnpm test: core 367, mcp-server 34, web 116, cli 67; skipped 0
  • pnpm db:migrate up to date (no migration needed), check-benchmark-drift clean, no-em-dash 1/0, pnpm smoke:packed ok
  • New tests: scrub linear-scan + perf guard (~2MB completes < 2s, old code multiple s); injection anchor-as-object; sync Invalid-Date fallback; withDistillLock serialization x3; concurrent-distill no-duplicate

One patch changeset (@marrowhq/core).

Opened by the autonomous loop, which cannot merge its own PRs (two-party human review). Queued for your review.

A refute-by-default adversarial hunt across concurrency, injection, SSRF,
DoS/ReDoS, edge-input, and the MCP/web boundary surfaced these. Each is a
reachable defect with a concrete repro; each fix carries a regression test.

- scrub: the private-key detector was O(n^2). The lazy BEGIN...END regex
  scanned to end-of-string once per BEGIN header, so a crafted multi-MB blob
  of BEGIN lines stalled the single event loop for seconds on one evidence
  insert. A non-backtracking scanner that jumps BEGIN to END by index replaces
  it; work is now linear.
- sync: a connector item whose source timestamp does not parse became a truthy
  Invalid-Date watermark that pinned the cursor and crashed the unguarded
  toISOString() outside the try/catch, wedging the connector on every run. It
  is now treated as a missing timestamp.
- distill: two distills of the same evidence (a scheduled drain overlapping a
  manual distill) both read an empty dedup set and each inserted the full node
  set. A per-evidence advisory lock, mirroring the connector lock, serializes
  them so the second sees the first's nodes and skips them.
- injection: the instruction-smell detector missed the anchor-as-object
  override phrasing ("ignore the above and ...") because the pattern required
  a trailing instructions/rules noun. Advisory only; now flagged.

Deferred to founder review, not shipped here: a HIGH Jira credential-exfil /
SSRF (jira.ts attaches the decrypted token to a config-controlled baseUrl with
no allowlist). Its exfil vector is the CSRF-able write endpoint that the
founder-gated PR #96 already closes, and the only exfil-stopping change in the
connector itself (a host allowlist) is a product call that would break
self-hosted Jira Data Center. Left for a human to land alongside #96.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 18bbef81f2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

DISTILL_LOCK_NS,
evidenceId,
]);
return await fn();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Avoid exhausting the pool while holding a distill lock

Each call retains a pool client for the entire callback, but the callback passed by Marrow.distill immediately calls Store methods such as getNodesForEvidence and insert*, which acquire clients from that same pool. With the default pg pool size (10), ten concurrent distills for different evidence IDs can each acquire this lock client and then all wait indefinitely for a client to execute their first Store query; the same deadlock occurs with a one-connection pool. Keep the lock on the client used for the work, or reserve a separate connection/pool for advisory locks.

Useful? React with 👍 / 👎.

@ElxMaj
ElxMaj merged commit 707549c into main Jul 19, 2026
2 checks passed
@ElxMaj
ElxMaj deleted the fix/second-lens-hunt branch July 19, 2026 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant