release: v2.0.0 — upstream v11.4.0, GPL-3.0-or-later, zero open security alerts

[doublegate]

The v2.0.0 release merge. 62 commits, 323 files, and master is a strict ancestor of this branch — no conflicts are possible.

Tagging follows from master, not from here. That is not a preference: mcp-release.yml only moves the Docker Hub latest tag when is_default_branch is true, so tagging the release branch would publish 2.0.0 while leaving the most-pulled tag on 1.9.0.

---

What v2.0.0 is

	v1.9.0	v2.0.0
Upstream base	10.19.4	11.4.0
Operations	463	504
Tools in tools/list	483	524
Licence	Apache-2.0	GPL-3.0-or-later
Node	22	>=24 <27
MCP tests	689	756
Open Dependabot alerts	194	0
Open code-scanning alerts	78	0

Closing a six-release upstream gap — and the reason it opened

The base sat at 10.19.4 while GCHQ shipped through 11.4.0. The cause was mechanical: the sync copied only src/core/operations/*.mjs, comparing flat basenames in one directory. That cannot express a major-version jump, which also changes src/core/lib/, the six upstream-owned src/node/*.mjs files and the config generators — and deletes src/core/lib/ImageManipulation.mjs. Syncing operations without lib/ orphans the library; syncing lib/ without operations breaks the build.

Rebuilt as an rsync -a --delete mirror so additions, modifications and deletions apply atomically, with fork changes as patches/fork/*.patch re-applied afterwards. A patch that stops applying fails the sync — the alarm that was missing when a ReDoS mitigation was silently reverted and stayed gone for four releases. Scope is checked against an allowlist, so anything out of scope fails the run rather than being caught only if someone thought to forbid it.

Relicensed to GPL-3.0-or-later

Uniquely determined by the reference tools v2.x admits: katana is GPL-3.0-or-later (ruling out GPLv2), John the Ripper is GPL-2.0-or-later, and upstream CyberChef is Apache-2.0 (compatible with GPLv3, not GPLv2).

Not a relicensing of GCHQ's code — upstream files keep their Apache-2.0 headers; the combined work is GPLv3, which Apache-2.0's one-way compatibility permits. v1.9.x stays Apache-2.0 through its LTS window. ADR 0001.

Security: 272 open findings → 0

Every one fixed, suppressed with a written justification, or dismissed with a reason — full disposition. Including CVE-2026-42615 (XSS, HIGH), minimatch (3 × HIGH), and an unauthenticated CWE-400 in the HTTP transport that the review of this release surfaced.

One .trivyignore entry, and it says why: elliptic has no fixed version anywhere and reaches the tree only through a webpack browser polyfill whose alias never applies on Node.

Issue #36 — Streamable HTTP serves multiple clients

One process-wide transport meant the first client worked and every one after it got Invalid Request: Server already initialized. Each session now gets its own Server and transport — the shape GHSA-345p-7cg4-v4c7 requires. Reproduced on the published _v1 image and verified fixed on the new build.

Three announced breaking changes withdrawn

DEP001/DEP007/DEP008 — the cyberchef_ prefix stays. Measured: removing it saves 2.6% of the tools/list payload while colliding 19 tool names in MCP's flat namespace and breaking every integration. Stated plainly in the migration guide rather than quietly dropped, because three planning documents had described the rename as happening.

---

Verification

release/v2.0.0 @ 931b96f0, all four workflows green:

Security Scan             success
CodeQL Analysis           success
MCP Server CI             success
Build and Test MCP Server success

Local, on the merge head:

npm run lint              0 errors
npm run test:mcp          756 passed (22 files)
npm test                  241 Node-API + 2,289 operation tests
npm run testnodeconsumer  green
actionlint                0 findings across 11 workflows

docker build              ok
tools/list                524 tools
image version             2.0.0
docker run --entrypoint id  uid=65532(node) gid=65532(node)

open Dependabot alerts     0
open code-scanning alerts  0

After this merges

1. git tag -a v2.0.0 -F docs/releases/v2.0.0.md && git push origin v2.0.0 — from master.
2. mcp-release.yml publishes ghcr.io/doublegate/cyberchef-mcp_v2 and Docker Hub, and moves latest.
3. latest moving to 2.0.0 hands every unpinned user a GPL-licensed, breaking-change build. Correct for a major, and called out prominently in the release notes rather than discovered.

Still open, deliberately

#73 — five upstream defects for gchq/CyberChef, including a pooled-ArrayBuffer leak in two image operations and the @xmldom/xmldom 0.9 blocker holding back nine DoS advisories. Filing on a third-party repo is a maintainer call.

[github-actions]

Antigravity review (Gemini via Ultra)

This PR cuts the v2.0.0 release by syncing with upstream v11.4.0, relicensing to GPL-3.0-or-later, fixing the cross-client data leak by making the HTTP transport per-session, and introducing a pre-execution ReDoS screen.

Blocking issues

None found.

Suggestions

None found.

Nitpicks

None.

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}

Earlier review rounds (newest first)

Round reviewed at 2026-08-31 14:06 UTC

Antigravity review (Gemini via Ultra)

Updates CyberChef-MCP to upstream v11.4.0, relicenses to GPL-3.0-or-later, adopts Node >=24, and resolves multiple security alerts including an HTTP transport leak and ReDoS vulnerability.

Blocking issues

None found.

Suggestions

• src/node/lib/safe-regex.mjs in branchesOverlap: The regex /[+*]|\{\d+,\d*\}/ checks for variable-length branches but omits the ? quantifier. While it matches at most one character, it still makes a branch variable-length and can contribute to backtracking when nested inside an outer quantifier (e.g., (a?|b)+).

Nitpicks

• src/node/lib/safe-regex.mjs in hasOverlappingQuantifiedAlternation: The regex (?:[+*]|\{\d+,\d*\}) technically matches {1}, which is fixed-length, though it's extremely unlikely to cause false positives in real-world patterns.

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}

Earlier review rounds (newest first)

Round reviewed at 2026-08-31 13:59 UTC

Antigravity review (Gemini via Ultra)

This PR tracks upstream CyberChef v11.4.0, updates the project license to GPL-3.0-or-later, refactors the HTTP transport to isolate sessions natively, and implements a ReDoS screening mechanism for user regexes.

Blocking issues

• Correctness: In src/node/transports.mjs, if the configured mcpPath is exactly /, mcpPath.replace(/\/+$/, "") will evaluate to "", while the requested path evaluates to "/" (due to the || "/" fallback). This mismatch causes every valid request to / to be incorrectly rejected with a 404. Consider changing the check to if (path !== (mcpPath.replace(/\/+$/, "") || "/")).

Suggestions

None found.

Nitpicks

• In CHANGELOG.md, the "Security" section covering DNS rebinding and recipe storage changes was placed under the [Unreleased] header rather than the new [2.0.0] release block.

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}

Earlier review rounds (newest first)

Round reviewed at 2026-08-31 13:50 UTC

Antigravity review (Gemini via Ultra)

This PR synchronises the core engine with upstream CyberChef v11.4.0, relicenses the project to GPL-3.0-or-later, resolves all open security alerts (including HTTP DNS rebinding and insecure temp files), and withdraws the cyberchef_ prefix deprecation.

Blocking issues

None found.

Suggestions

• src/node/mcp-server.mjs (around line 25629): In csv(value, envName), if value is provided in options as a non-string and non-array primitive (e.g., a number), raw.split(",") will throw a TypeError. Consider using String(raw).split(",") as a defensive guard.
• src/node/recipe-storage.mjs (around line 25215): The fs.stat(candidate) and subsequent fs.unlink(candidate) introduces a minor TOCTOU (Time-of-Check to Time-of-Use) race condition where the file could be modified or removed between the check and the unlink. Since the cleanup is best-effort and unlink failures are caught, this is not a blocking security issue, but standardising on unlinking based on the file descriptor if possible or just ignoring the race is standard. Given Node's fs limitations, the current implementation is acceptable but could be noted.

Nitpicks

• src/node/recipe-storage.mjs (line 25168): STALE_TEMP_AFTER_MS = 60 * 60 * 1000; could have an inline comment // 1 hour appended for quicker readability.

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}

Earlier review rounds (newest first)

Round reviewed at 2026-08-31 13:42 UTC

Antigravity review (Gemini via Ultra)

This PR updates the upstream CyberChef core to v11.4.0, correctly handles bake() and execute() as async, implements ReDoS screening for user-provided regexes, secures temp file handling in recipe storage, and makes the HTTP transport strictly per-session.

Blocking issues

• DNS rebinding vulnerability (src/node/transports.mjs): The comment "DNS-rebinding protection is opt-in because the default bind is loopback, where it adds nothing" fundamentally misunderstands the attack. DNS rebinding specifically targets loopback/private interfaces by using a victim's browser as a proxy to bypass the firewall. Because a rebound request is considered same-origin by the browser, it sends no CORS preflight and the POST executes directly. By making Host validation opt-in, the default 127.0.0.1 bind is completely vulnerable to takeover by any malicious website the user visits. allowedHosts must default to a safe value (e.g., ["localhost", "127.0.0.1"]) rather than being disabled.

Suggestions

• src/node/lib/safe-regex.mjs (line 122 / diff line 23681): The nested-quantifier regex /\([^)]*[+*]\)\s*[+*]/ checks for inner quantifiers + or *, but misses ?. A pattern like (a?)+ causes catastrophic exponential backtracking on non-matching strings, but will bypass this screen. Consider including ? in the inner character class: /\([^)]*[+*?]\)\s*[+*]/.
• src/node/lib/safe-regex.mjs (line 148 / diff line 23708): Similarly, the nested-bounded-repeat regex /\([^)]*[+*][^)]*\)\s*\{\d+,?\d*\}/ misses ? for the inner quantifier. Consider updating it to: /\([^)]*[+*?][^)]*\)\s*\{\d+,?\d*\}/.

Nitpicks

• src/node/recipe-storage.mjs (line 168 / diff line 25168): In cleanupStaleTempFiles(), a vanished file (ENOENT) is caught and logged at debug level. Checking if (error.code !== "ENOENT") { ... } and ignoring it entirely would be slightly cleaner than relying on the logger level to filter normal filesystem races.

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}

Earlier review rounds (newest first)

Round reviewed at 2026-08-31 13:28 UTC

Antigravity review (Gemini via Ultra)

This PR upgrades the upstream CyberChef base to v11.4.0, relicenses the project to GPL-3.0-or-later, refactors the HTTP transport to be per-session to prevent cross-client data leaks, withdraws the removal of the cyberchef_ tool prefix, and adds robust ReDoS screening for user-supplied regular expressions in the MCP layer.

Blocking issues

None found.

Suggestions

• src/node/recipe-storage.mjs: The STALE_TEMP_AFTER_MS constant is set to 1 hour. Since recipe saves complete in milliseconds, this cutoff could safely be lowered to 5 or 10 minutes to clean up orphan staging files sooner without risking interference with concurrent writes.

Nitpicks

• .dockerignore line 38: Uses the British spelling "artefacts". While src/core/ is exempt from the en-US convention, this file is project-owned where American spelling is expected.

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}

Earlier review rounds (newest first)

Round reviewed at 2026-08-31 13:21 UTC

Antigravity review (Gemini via Ultra)

This PR synchronises the upstream base to v11.4.0, relicenses the project to GPL-3.0-or-later, sets the Node.js floor to v24, secures HTTP sessions and recipe saving, adds ReDoS screening, and withdraws the cyberchef_ prefix deprecation.

Blocking issues

None found.

Suggestions

• src/node/lib/batch.mjs: Inside executeOperation (around line 102), raw Error objects are thrown for invalid tool names and arguments. While these are caught by executeBatch and don't cross the MCP boundary as bare exceptions, using createInputError (which is already imported and used elsewhere in this file) would be more consistent with the rest of the codebase.

Nitpicks

None.

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}

Earlier review rounds (newest first)

Round reviewed at 2026-08-31 13:16 UTC

Antigravity review (Gemini via Ultra)

Updates Node to 24, syncs with upstream v11.4.0, makes HTTP transport per-session to fix state leaks, adds Dependabot, and relicenses the fork to GPL-3.0-or-later while adding ReDoS protections for regex arguments.

Blocking issues

• Unawaited promise / Silent failure: In src/node/streaming.mjs (around line 271 inside streamOperationWithProgress), the now-async bakeFunction is called with .then() instead of await. The .catch block executes throw err; inside a new Promise((resolve) => { ... }) executor that lacks a reject parameter. This results in an unhandled promise rejection and causes the progressPromise to hang indefinitely on error.

Suggestions

• src/node/recipe-storage.mjs: In cleanupStaleTempFiles(), you could use await fs.rm(candidate, { force: true }) rather than fs.unlink() to gracefully handle vanished files without throwing/catching ENOENT when a concurrent sweep beats you to it.
• src/node/transports.mjs: The closeSessionDetached function fires off closeSession without await, but since it explicitly handles errors in a .catch block and logs them, it is safe; however, wrapping it in an explicit void (e.g., void closeSession(...)) can signal intent more cleanly to linters.

Nitpicks

• src/node/transports.mjs: httpServer.closeAllConnections?.() uses optional chaining, but since the required Node version is bumped to 24, this method is guaranteed to exist and the ?. is no longer necessary.

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}

Earlier review rounds (newest first)

Round reviewed at 2026-08-31 13:08 UTC

Antigravity review (Gemini via Ultra)

This PR updates the project to upstream v11.4.0 (relicensing the combined work to GPL-3.0-or-later), fixes the HTTP transport shared-instance bug by correctly isolating per-session transports, and implements ReDoS screening natively in the MCP layer.

Blocking issues

None found.

Suggestions

• src/node/transports.mjs (around line 122): Consider renaming normaliseSessionId to normalizeSessionId. As noted in .coderabbit.yaml, the src/node/ directory is fork-owned and should follow the project's en-US spelling convention.

Nitpicks

• .dockerignore: test-results is added without an explanatory comment, unlike the carefully annotated tests exclusion directly above it.

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}

Earlier review rounds (newest first)

Round reviewed at 2026-08-31 12:54 UTC

Antigravity review (Gemini via Ultra)

1. This PR updates CyberChef-MCP to upstream v11.4.0, adds ReDoS screening for user-supplied regular expressions, relicenses the project to GPL-3.0-or-later, withdraws the deprecation of the cyberchef_ tool prefix, and fixes a cross-client data leak by making HTTP transports per-session.

Blocking issues

• Unawaited promise / Swallowed errors: In tests/node/consumers/cjs-consumer.js, require("cyberchef").then(async chef => { ... }) creates a promise chain without a .catch(). If the assert.equal fails, it will cause an unhandled promise rejection rather than a graceful test failure.

Suggestions

• tests/node/consumers/cjs-consumer.js: Add a .catch(console.error) to the end of the require("cyberchef").then(...) chain, or refactor the top level into an async IIFE (e.g. (async () => { const chef = await import("cyberchef"); ... })()) to properly await the execution and handle errors.

Nitpicks

None found.

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}

[doublegate]

Adjudication of the agy review

Blocking: tests/node/consumers/cjs-consumer.js — promise chain with no .catch()

The finding is correct. The remedy is upstream's, not ours.

require("cyberchef").then(async chef => {
    ...
    assert.equal(...);
});

A failing assertion becomes an unhandled rejection rather than a clean failure. Node exits non-zero with a rejection warning, so the consumer test does fail — but for a reason its output does not explain, and via a path that depends on the runtime's unhandled-rejection mode rather than on the test.

The file is byte-identical to upstream v11.4.0:

$ diff -q tests/node/consumers/cjs-consumer.js ref-proj/CyberChef/tests/node/consumers/cjs-consumer.js
(no output)

tests/ was adopted once with the v11.4.0 landing and is deliberately not on the sync allowlist. So a local edit here would diverge from upstream's suite without the sync ever flagging it — strictly worse than the src/core/** case, where a fork patch that stops applying fails the run. Carrying a silent, permanent delta against upstream to fix upstream's test is the wrong trade.

Tracked as item 11 on #73 with your suggested remedy recorded verbatim, so it goes upstream rather than being lost:

#73 (comment)

Not treated as blocking for this merge. It is a pre-existing property of upstream's consumer test, unchanged by this PR, and npm run testnodeconsumer is green. Blocking a release on a defect the release did not introduce and cannot durably fix would leave the repository stuck on a six-release-old upstream base — which is the position this release exists to get out of.

---

Three Copilot findings on the same PR are answered in their threads, and all three are byte-identical to upstream v11.4.0 as well. One generalised usefully: MOD.mjs throwing a bare Error turns out to be three operations doing so against 246 that use OperationError, all three on user-input validation paths. That is now item 9 on #73 with the Recipe.mjs mechanism spelled out, since a report naming one of three gets half-fixed.

Release status

CodeQL                     pass   (was failing; the one fork-owned alert is fixed in #84, merged)
Trivy / Trivy Container    pass
Dependency Vulnerability   pass
Generate SBOM              pass
core-test / test           pass
build-and-test             pass
Analyze (javascript)       pass
CodeRabbit                 pass
codecov/patch              pass

Tagging follows from master once this merges — mcp-release.yml only moves the Docker Hub latest tag when is_default_branch is true, so tagging the release branch would publish 2.0.0 while leaving the most-pulled tag on 1.9.0.

[doublegate]

/agy-review

Re-running now that #85 is on master. The previous failure here was not a finding — it was a deterministic 5m timeout on this PR's 1.6 MB diff (six attempts across two runs, every one at 5m01s), which is indistinguishable from a backend outage in the log. #85 scales --print-timeout with the diff size, so this diff now gets ~670s instead of 300s. The reviewer checks its scripts out from the default branch, which is why that fix had to land on master first.

[doublegate]

Adjudication of the agy review

First, the thing this run demonstrates: the previous review failure on this PR was never a finding. It was a deterministic 5-minute timeout on a 1.6 MB diff — six attempts across two runs, every one at 5m01s — reported as Error: timeout waiting for response, which is indistinguishable in the log from a backend outage. #85 scales --print-timeout with diff size (this diff now gets ~670s), and the review completed on the first attempt. The guard was working correctly the whole time; it just could not say why it was refusing to post.

Two rounds ran. Both adjudicated below.

---

Round 2 (current) — no blocking issues

Suggestion: rename normaliseSessionId → normalizeSessionId — adopted.

Right, and the evidence is stronger than the style rule alone. Surveying every .mjs under src/node on this branch:

spelling	where
initialize ×9, initializes ×1	transports.mjs — the same file
sanitizeToolName, sanitized	mcp-server.mjs, lib/tool-schema.mjs, lib/batch.mjs
sanitise, sanitisedOp	api.mjs, apiUtils.mjs, NodeRecipe.mjs — all upstream-owned, mirrored verbatim, must not be touched

So every en-GB spelling remaining in src/node is in a file we cannot edit, and normaliseSessionId was the sole en-GB identifier in fork-owned code — sitting nine lines from initialize. Renamed in the function, its call site, its JSDoc prose, and 13 references in the transport suite. 232c1021.

Nitpick: test-results added without a comment — adopted.

Fair, and the reason it read oddly is that the group sat directly beneath a long comment about tests, so coverage / .nyc_output / test-results looked like a continuation of that entry rather than a separate one. Given its own header saying what it is: test output, not test sources.

npm run lint                                    0 errors
npx vitest run http-transport-sessions           32 passed

---

Round 1 (archived) — the cjs-consumer.js blocking finding: rejected, twice over

require("cyberchef").then(async chef => { ... }) creates a promise chain without a .catch(). If the assert.equal fails, it will cause an unhandled promise rejection rather than a graceful test failure.

1. The premise about the consequence is wrong. An unhandled rejection is fatal by default from Node 15 onward, and this repo requires >=24. Measured:

$ node -e 'Promise.resolve().then(()=>{ require("assert").equal(1,2); })'; echo $?
1

So npm run testnodeconsumer does fail the job when the assertion fails. The difference a .catch() would make is the tidiness of the message, not whether the failure is caught. The finding describes a swallowed error; nothing is swallowed.

2. The file is byte-identical to upstream v11.4.0. Verified against the reference checkout:

$ diff <(git show origin/release/v2.0.0:tests/node/consumers/cjs-consumer.js) \
       ref-proj/CyberChef/tests/node/consumers/cjs-consumer.js
$                                    # identical

The change in this PR is exactly upstream's — adding async/await, which is required, because bake() became async in v11.4.0 and the old synchronous form would assert against a Promise. Adopting upstream's text verbatim is the deliberate choice: it makes this file a no-op for the next sync. Adding a .catch() would create fork drift in a file that has no fork reason to differ, to fix a problem that does not exist.

If it is worth fixing at all, it is worth fixing upstream, where every CyberChef consumer benefits — which is where it will go, alongside the other defects already collected in #73.

[doublegate]

core-test went red — and it was not this PR

CipherSaber2 Encrypt
  Expected  /.{10}/s
  Received  ,áiɉwA$5

A flaky assertion, not a regression. CipherSaber2 prepends a 10-byte random IV, so the only stable property of its output is the byte count — but the assertion counted characters of the string the harness builds from those random bytes. When two bytes happen to form a decodable multi-byte sequence, the string is shorter than the byte count and the test fails on nothing. In the value above, C9 89 decoded to a single U+0249, so 10 bytes rendered as 8 characters.

Measured through the real harness path before touching anything, rather than assuming:

empty-input case, /.{10}/s	1 failure in 500 runs (~0.2%)
"Hello World" case, /.{21}/s	0 in 500
raw UTF-8 decode of 10 random bytes	37% collapse

The gap between 37% and 0.2% is the harness's own conversion absorbing most cases — but not all, which is exactly why this survived dozens of green runs and then failed once. "Usually passes" is not a property a suite should rest on, and at 0.2% it would have gone on failing roughly one release-CI run in five hundred with no explanation attached.

Fixed by asserting on bytes, which is what it always meant: hex-encode before matching.

expectedMatch: /^[0-9a-f]{20}$/,     // was /.{10}/s
recipeConfig: [
    { op: "CipherSaber2 Encrypt", args: [{ option: "Latin1", string: "" }, 20] },
    { op: "To Hex", args: ["None"] },
],

Strictly stronger than what it replaces — /.{10}/s is unanchored and passes on anything at least 10 characters long, whereas /^[0-9a-f]{20}$/ pins the length exactly.

On fork hygiene, since this file was byte-identical to upstream v11.4.0. This is now a deliberate fork edit. tests/ is not on the upstream-sync allowlist, so it cannot be silently reverted the way the SafeRegex mitigation was — but the defect is upstream's and belongs upstream, so it is added to the #73 collection rather than fixed only here.

operations suite                          2289 / 2289
5,000 consecutive runs of the fixed case  0 failures   (≈10 expected at the old rate)
npm run lint                              0 errors

914a5178.

[doublegate]

Adjudication of the agy review (round 3)

Blocking: none. One suggestion, adopted — and it was half of a pair.

src/node/lib/batch.mjs: raw Error for input validation instead of createInputError

Correct. Worth saying where this came from rather than treating it as newly found: Copilot raised it on #48, and it was deferred there deliberately — #48's gate was "changes nothing observable", and fixing it inside a pure-move diff would have destroyed the property that made that PR reviewable and bisectable. It was recorded against DEP003, and v2.0.0 enacts DEP003, so shipping this release with it still open would leave a hole in a headline change of the release itself.

The reviewer raising it again on the release PR is a fair signal that "tracked in the plan" is not the same as "fixed".

Fixed, and so was the second defect from the same pair, which nobody re-raised and which is the worse of the two:

// Before: the feature-flag guard ran FIRST, and interpolates operations.length into its context.
if (!BATCH_ENABLED) {
    throw createInputError("Batch processing is disabled", { batchSize: operations.length });
}
if (!Array.isArray(operations) || operations.length === 0) { ... }

A disabled-batch call with a missing or non-array operations therefore threw a bare TypeError instead of the structured INVALID_INPUT the caller is promised — the feature flag turning a validation error into a crash. The shape check now runs first.

And all three raw throws in executeOperation are now createInputError, matching the four call sites already in that file, each carrying the offending value:

throw createInputError(`Invalid tool name: ${op.tool}`, { tool: op.tool });
throw createInputError("Operation arguments must be an object", { tool: op.tool, received: typeof op.arguments });
throw createInputError(`Operation not found: ${toolName}`, { tool: toolName });

batch.mjs now has zero bare Error throws.

npx vitest run tests/mcp/     763 passed, 22 files
npm run lint                  0 errors

3680474b.

[doublegate]

Adjudication of the agy review (round 4)

Blocking: none. One suggestion declined with reasoning, one nitpick adopted.

Suggestion: lower STALE_TEMP_AFTER_MS from 1 hour to 5–10 minutes — declined

The reasoning offered is sound and the premise is one I wrote myself: a save does complete in milliseconds. But the constant is not sized against a typical save, it is sized against the worst case in which deleting the wrong file loses data.

What the sweep does is delete recipes.json.<random>.tmp siblings — files that may be a live staging write by another process. The cost of the two errors is asymmetric:

• Too long: an orphan staging file survives an extra 50 minutes on disk. It is a few KB, it is invisible, and the next sweep gets it.
• Too short: a concurrent save whose process was descheduled, suspended, or blocked on slow I/O has its staging file deleted out from under it, and the fs.rename that follows fails or — worse — renames a file another writer has since recreated.

A laptop lid closing mid-save, a container throttled to a fraction of a CPU, or an NFS-backed storage path all make "milliseconds" stop being true, and none of them are exotic. An hour buys immunity to all of them for a cost measured in kilobytes.

There is also no user-visible benefit on the other side of the trade: nothing polls for these files, no quota counts them, and the sweep runs on every save anyway. So the change would purchase faster deletion of something nobody is waiting on, at the price of a data-loss window. The comment on the constant already states the intent — "well beyond any live write" — and an hour is what makes that sentence true rather than aspirational.

Declined deliberately, not overlooked.

Nitpick: "artefacts" in .dockerignore — adopted

Right, and the distinction drawn is exactly the correct one: src/core/ is exempt because it is mirrored from upstream, but .dockerignore is project-owned and the en-US convention applies. Fixed; artefact no longer appears in any project-owned file.

This is the second en-GB spelling this review caught (normaliseSessionId was the first), which is a better hit rate than I would have guessed for a convention that had no automated check behind it.

[doublegate]

Adjudication of the agy review (round 5)

The blocking finding is right, and it is the most valuable thing any bot has found on this PR. Adopted in full. The two regex suggestions are rejected on measurement, and the nitpick is declined with reasoning.

---

BLOCKING — DNS rebinding: adopted, and my comment was the bug

The comment being criticised was mine:

DNS-rebinding protection is opt-in because the default bind is loopback, where it adds nothing.

That is backwards, and the review's correction is exactly right: DNS rebinding exists specifically to reach loopback, using the victim's own browser as the proxy a firewall cannot see.

1. Victim loads evil.example; its DNS answer carries a 1-second TTL.
2. The page fetches http://evil.example:3000/mcp. The browser re-resolves — the attacker now answers 127.0.0.1.
3. The request lands here with Host: evil.example:3000.
4. The browser considers it same-origin with the page — origin and target are both http://evil.example:3000 — so no preflight is sent, whatever the Content-Type, and the script can read the response.

Two consequences my reasoning missed, both worse than the premise error:

• CYBERCHEF_ALLOWED_ORIGINS is not a mitigation for this. I had treated the CORS default-deny as covering the browser case. It is never consulted, because the browser never classifies the request as cross-origin.
• initialize requires no session id, so a hostile page opens its own session and drives every tool — on a server whose recipe storage reaches the filesystem via caller-supplied CYBERCHEF_RECIPE_STORAGE.

Fixed: protection is now on by default.

function effectiveAllowedHosts() {
    if (hostCheckDisabled) return undefined;          // explicit CYBERCHEF_ALLOWED_HOSTS=*
    if (configuredHosts) return configuredHosts;
    const bound = httpServer.address();
    const actualPort = (bound && typeof bound === "object" ? bound.port : null) ?? port;
    const names = new Set();
    for (const name of ["localhost", "127.0.0.1", "[::1]"]) {
        names.add(name);
        names.add(`${name}:${actualPort}`);
    }
    return [...names];
}

Two details worth stating, since both are easy to get wrong:

• Resolved after listen(), not at construction. port: 0 asks the OS for an ephemeral port, so the real one is not known until the socket is bound. Computing the list eagerly would have produced an allowlist naming port 0, and the entire existing test suite runs on ephemeral ports — it would have failed closed on every test rather than protecting anything.
• Every name is listed twice. The SDK compares the whole Host header by exact string (this._allowedHosts.includes(hostHeader)), and a browser includes the port whenever it is not the scheme default.

Four tests, speaking raw http rather than fetch — fetch refuses to let a caller set Host, and a forged Host is the entire attack, so a fetch-based test could not have exercised this at all:

✓ is ON BY DEFAULT and rejects a rebound Host                          403 + "Invalid Host header"
✓ accepts the loopback names it is actually reached by, with and without the port
✓ honours an explicit allowlist instead of the default                 (defaults replaced, not merged)
✓ can be disabled outright with the explicit `*` opt-out

Breaking for one configuration, called out rather than buried: a server bound to 0.0.0.0 and reached by a LAN name or IP now needs CYBERCHEF_ALLOWED_HOSTS set. That is the secure-by-default trade, and it is what the MCP specification asks for.

The wrong reasoning was also written into prose in docs/guides/http-transport.md and README.md, so both are corrected in the same commit with the attack walkthrough — a comment fixed in code while the docs keep asserting the error is how the next reader gets it wrong again.

d267c5e6.

---

Suggestion: add ? to the nested-quantifier inner class — rejected, measured

The stated premise is that (a?)+ "causes catastrophic exponential backtracking on non-matching strings". It does not. V8 breaks a loop whose body matched empty, which defuses exactly this shape:

n     (a?)+$    (a?){10,}$   (a|a?)+$    (a+)+$
18       0.0          6.9      137.3       22.9
20       0.1          1.1       75.5       12.9
24       0.0          1.6     1203.5      209.6
28       0.0          2.2    19869.4       skip

(a?)+ is flat across the range; (a+)+ and (a|a?)+ are exponential. So the finding's own example is safe, and the screen is right to allow it.

The genuinely dangerous variant, (a|a?)+, is already rejected — by the correct rule:

(a?)+          ALLOWED
(a|a?)+        REJECTED (quantified-alternation)
(a+)+          REJECTED (nested-quantifier)
(a|a)+         REJECTED (quantified-alternation)
(a|ab)+        REJECTED (quantified-alternation)
(foo|bar)+     ALLOWED          <- disjoint branches, linear
(\w+\s?)+      ALLOWED

Its danger is overlapping alternation, which hasOverlappingQuantifiedAlternation decides per pattern, not the ?.

And the change would cost real false positives. [+*?] newly rejects (\w+\s?)+ and (a?b)+, both measured at 0.0 ms — and (\w+\s?)+ is about as ordinary a regex as exists. That is precisely the failure mode this file's own header warns against:

Each entry is deliberately narrow. A broad "contains nested quantifiers" test rejects legitimate patterns, and a screen that fires on ordinary input gets disabled by whoever trips over it — which is a worse outcome than not having it.

Suggestion: same ? change for nested-bounded-repeat — rejected, same measurement

(a?){10,} is linear (1–2 ms, flat as n grows). Rejecting it would buy nothing and cost the same class of false positives.

Nitpick: ignore ENOENT outright instead of logging it at debug — declined

cleanupStaleTempFiles sweeps files it did not stat atomically, so ENOENT is the expected race — another process finished its save between our readdir and our unlink — and the existing branch already treats it as benign by logging at debug while everything else goes to warn.

Silence and a debug line differ in exactly one situation, and it is the one that matters: an ENOENT storm means something is deleting staging files underneath us, and a level filter can be turned up to see it. An if (error.code !== "ENOENT") guard makes that unobservable at any log level, to save one line. The asymmetry favours keeping it.

---

npx vitest run tests/mcp/     767 passed, 22 files   (was 763; +4 rebinding tests)
npm run lint                  0 errors

[doublegate]

Adjudication of the agy review (round 6)

Blocking: none. Two suggestions and a nitpick; two adopted, one acknowledged as already-correct.

Suggestion: csv() throws on a non-string, non-array value — adopted

Right, and reproducible:

$ (3000).split(",")
TypeError: raw.split is not a function

An env var is always a string, so this is unreachable from the environment — but value comes from a programmatic caller, where createTransport({ allowedHosts: 3000 }) is an easy typo, and the consequence is a TypeError thrown during construction. A config mistake should be ignored or reported, not take the server down. String(raw).split(",").

One correction to the location: csv() is in src/node/transports.mjs, not mcp-server.mjs. The finding is right regardless — noting it only because the line reference would send someone to the wrong file.

Suggestion: TOCTOU between fs.stat and fs.unlink in cleanupStaleTempFiles — acknowledged, no change

Agreed on both the analysis and the conclusion the review itself reaches. Adding the reasoning for why "acceptable" is the right verdict rather than a shrug:

The race is expected, not incidental. The sweep exists precisely because other processes are writing and renaming staging files concurrently, so a file vanishing between stat and unlink means a concurrent save finished — the good outcome. The ENOENT branch below already treats it that way.

And there is no fd-based fix available: funlink/unlinkat are not exposed by Node's fs, and opening the file first would only move the race, not close it. Nor is the failure mode dangerous — the worst case is unlinking a staging file a concurrent save is about to rename, which fails that save's rename with ENOENT and is exactly what the one-hour cutoff makes unreachable in practice.

Nitpick: // 1 hour beside STALE_TEMP_AFTER_MS — adopted

Fair. The JSDoc above explains why the value is what it is, but the value itself still needed arithmetic to read.

---

npx vitest run tests/mcp/     767 passed, 22 files
npm run lint                  0 errors

[doublegate]

Adjudication of the agy review (round 7)

Both adopted. The blocking finding is real and I confirmed it before fixing.

BLOCKING: CYBERCHEF_HTTP_PATH=/ 404s every request — adopted

Verified by running both normalizations side by side:

req        mcpPath    path     target   match?
/mcp       /mcp       /mcp     "/mcp"   OK
/mcp/      /mcp       /mcp     "/mcp"   OK
/          /          /        ""       404      <-- the finding
/other     /mcp       /other   "/mcp"   404

The cause is precisely as described: the two sides were normalized separately, and only the request side carried the || "/" fallback.

request  "/"  ->  split("?")[0].replace(/\/+$/, "") || "/"   ->  "/"
config   "/"  ->               .replace(/\/+$/, "")          ->  ""

So the one path that could never work was the root — and the mismatch was invisible on the page, because the two expressions read as equivalent.

Fixed by extracting normalizeEndpointPath() and running both sides through it, rather than by adding a second || "/". Two copies of a normalization rule that must agree is what produced this; one function cannot drift.

Two regression tests: the normalization table (/, //, "", undefined, trailing slashes, a query string), and a real server bound with path: "/" completing an initialize over a socket — the end-to-end case, since the unit table alone would not have caught a routing change.

Nitpick: the Security entries were under [Unreleased] — adopted, and it was more than cosmetic

Right, and worth more than "nitpick" on this particular PR: this branch is the 2.0.0 release and is about to be tagged, so shipping with those two entries under [Unreleased] would have published a changelog claiming that the DNS-rebinding hardening and the temp-file fix were not in the release that contains them.

Both moved into the 2.0.0 Security section, [Unreleased] reduced to "Nothing yet.", and the routing fix above added under Fixed.

---

npx vitest run tests/mcp/     769 passed, 22 files   (was 767)
npm run lint                  0 errors

[doublegate]

Adjudication of the agy review (round 8)

Blocking: none. Both remaining items rejected on measurement — but chasing the first one turned up a genuine release blocker, recorded at the bottom.

Suggestion: branchesOverlap omits ? from its variable-length test — rejected

The review's own example does not hold up:

n     (a?|b)+$    (a|a?)+$    (a?|a)+$
20         0.0       544.5       629.0
24         0.1      1184.2      1146.3
28         0.0     18866.9     18272.2

(a?|b)+ is flat, and the reason is the one the rule is built on: a? and b are disjoint — no non-empty string matches both — so there is no ambiguity for the engine to explode on, whatever quantifier decorates a branch. What blows up is overlap, and both overlapping forms are already caught:

(a?|b)+      ALLOWED            <- linear, correctly allowed
(a?|a)+      REJECTED (quantified-alternation)
(a|a?)+      REJECTED (quantified-alternation)

Adding ? to the variable-length test would newly reject (a?|b)+, measured at 0.0 ms — a false positive, against a file whose header says in so many words that a screen firing on ordinary input is worse than no screen.

This is the third round the ? quantifier has come up, in three different rules. The measurements keep landing the same way, and I think the underlying reason is worth stating once: ? is what makes a branch nullable, and V8 breaks a loop whose body matched empty. Nullability is the thing that shape-based screens expect to be dangerous and that this engine specifically defuses.

Nitpick: (?:[+*]|\{\d+,\d*\}) matches {1} — rejected, it does not

The comma is mandatory in that alternative:

{1}    matches: false
{1,}   matches: true
{1,3}  matches: true

{1} has no comma, so it never reaches the \d*} tail. No change needed.

---

Found while verifying the release wiring: mcp-release.yml would have published v2.0.0 into _v1

Not from this review, but it belongs on this PR because it would have shipped:

GHCR_IMAGE_NAME: ${{ github.repository_owner }}/cyberchef-mcp_v1

Tagging v2.0.0 would have pushed the relicensed, breaking-change image into the package this very release declares frozen — while README.md, docs/releases/v2.0.0.md, the user guide and the HTTP transport guide all already tell users to pull cyberchef-mcp_v2. The docs and the workflow disagreed, and the workflow would have won silently, because nothing in the pipeline compares them.

The suffix is now derived from the tag's major version:

v2.0.0  -> cyberchef-mcp_v2      v1.9.4   -> cyberchef-mcp_v1   (maintenance line, unchanged)
v2.1.3  -> cyberchef-mcp_v2      v10.0.0  -> cyberchef-mcp_v10
workflow_dispatch (no tag) -> falls back to package.json mcpVersion's major, with a ::notice::

So the v1.9.x security-patch line keeps publishing to _v1 under Apache-2.0 without a second workflow, and no future major needs an edit here. DOCKER_HUB_SETUP.md was the last doc still naming _v1 as the current pull; corrected in the same commit.

c07960aa.