# Architecture Decisions This document records the significant architectural choices in Burp AI Agent and the reasons behind them. Each section follows the ADR (Architecture Decision Record) shape: context → decision → consequences. Decisions are not frozen — they can be revisited as the plugin evolves — but changing them should be intentional, not accidental. ## ADR-1: Kotlin on the JVM, not Java, not Scala **Context.** Burp Suite is a JVM application and exposes its extension API (Montoya) as Java interfaces. The plugin must run in the same JVM as Burp, which restricts the language to one that compiles to JVM bytecode. Burp itself already bundles a JDK at runtime, so there is no cost to targeting the JVM. **Decision.** Write the plugin in Kotlin, not Java, and not Scala. **Consequences.** - Null-safety, data classes, sealed classes, and coroutines remove a large class of bugs that are easy to hit with a plugin that glues AI reasoning to HTTP traffic. - New contributors familiar with Java can still read and edit Kotlin; the learning curve is much smaller than Scala's. - Tooling (Gradle Kotlin DSL, IntelliJ, ktlint) is first-class. We pay nothing for it. - Trade-off: we rely on Kotlin stdlib in the shadow JAR (~1.6 MB). Acceptable. ## ADR-2: Swing for the UI, not JavaFX or Compose **Context.** The Burp extension tab is embedded inside Burp's own Swing UI via `api.userInterface().registerSuiteTab(…)`. Any other UI toolkit would either need to be hosted inside Swing (fragile) or rendered in a separate window (breaks the user's mental model of "my plugin lives inside Burp"). **Decision.** Use Swing directly. **Consequences.** - Zero friction with Burp's theme and keyboard handling. - We inherit Swing's verbosity (GridBagLayout, BorderLayout, explicit EDT handling). We accept it and wrap repeated patterns in helpers (`ToggleSwitch`, `AccordionPanel`, `ActionCard`, `ContextPreviewDialog`). - Testing UI components is painful. We mitigate by keeping UI components thin shells over pure-Kotlin logic that is tested directly. ## ADR-3: Pluggable backends via `ServiceLoader` **Context.** The plugin supports many AI backends (Ollama, LM Studio, NVIDIA NIM, Claude CLI, Gemini CLI, Codex CLI, OpenCode CLI, Copilot CLI, Burp native AI). New backends appear regularly, and users sometimes need a private backend that cannot live in the open-source repo. **Decision.** Backends implement the `AgentBackend` interface and are discovered via Java's `ServiceLoader` mechanism. Backends bundled in-tree are registered by `META-INF/services` entries. External backends are loaded from JARs the user drops into `~/.burp-ai-agent/backends/` via a dedicated `URLClassLoader` with proper close-on-fail handling. **Consequences.** - Adding a new backend is a one-file change (implementation + SPI entry) with no modifications to core code. - Users can ship their own closed-source backend without forking the project. - Trade-off: `ServiceLoader` gives us no dependency injection. Backends receive the Montoya API and settings via constructor; we do not inject fakes in tests. Backends are therefore tested with real `AgentSupervisor`-shaped wrappers. ## ADR-4: HTTP backends and CLI backends share an interface, split their implementation **Context.** Some backends are local HTTP servers (Ollama, LM Studio, NVIDIA NIM, generic OpenAI-compatible). Others are command-line tools that manage their own authentication and state (Claude CLI, Gemini CLI, Codex CLI, OpenCode CLI, Copilot CLI). Both have to expose the same abstraction (`send prompt, stream chunks, return or error`), but the implementation diverges sharply (HTTP client with retry/backoff vs process supervisor with session resume). **Decision.** Define a single `AgentConnection` / `AgentBackend` contract, and split the implementation in two base classes: `HttpBackendSupport` (shares OkHttp client, retry policy, conversation history, circuit breaker) and `CliBackend` (shares process supervision, session ID management, file-based prompt transfer for long inputs, Windows command normalization). **Consequences.** - New HTTP backends implement a thin subclass that knows only the request/response shape. - CLI process concerns (quoting, env, cwd, kill on shutdown) live in one place. - We accept that the two hierarchies will drift over time. When they do, we move the shared concern into a third helper, not into a shared superclass. ## ADR-5: Privacy redaction runs pre-flight and is a user-visible mode, not a silent default **Context.** The plugin sends captured HTTP traffic to external AI backends. Cookies, `Authorization` headers, JWTs, and URL tokens are routinely in that traffic. If we send them raw, we make the user's API keys and session tokens visible to a third-party AI provider — possibly in their training data. **Decision.** Define three privacy modes — `STRICT`, `BALANCED`, `OFF` — and run redaction (`RedactionPolicy`, `Redaction.apply`) before any captured traffic leaves the plugin. The default for new users is `BALANCED` (cookies and tokens redacted, hosts kept). `STRICT` also anonymizes hosts via HKDF. `OFF` exists for users who know what they are doing on private local models. **Consequences.** - The regex set that drives redaction is a hand-curated list of common header names and URL parameter names; it is not exhaustive. We accept false negatives and tighten the list when new patterns appear (last tightened for `X-Auth-Token`, `X-Access-Token`, `X-CSRF-Token`, `X-Api-Secret`, Basic auth, and query-string tokens). - The UI shows the current mode in a pill on the main tab and on every context preview dialog, so users never send traffic without knowing which rules are active. - Audit logs store only hashes of prompt bodies by default, not the bodies themselves. ## ADR-6: MCP server is embedded in the plugin, not a separate process **Context.** We want to expose Burp tools (proxy history, site map, scope check, `issue_create`, `http1_request`, …) to external AI agents (Claude Desktop, Codex CLI, etc.) over the Model Context Protocol. MCP can be served over stdio or over SSE/HTTP. A separate process would let us isolate the server but would require us to replicate scope, session, and Burp API access. **Decision.** Embed an MCP server inside the plugin's JVM using Ktor (SSE + optional stdio bridge), backed by the Montoya API directly. The server binds to `127.0.0.1` by default and is protected by a bearer token generated with `SecureRandom`. External access is an explicit opt-in and unlocks TLS. **Consequences.** - Tools see live Burp state without any IPC layer. No desync, no cache invalidation problem. - The plugin's lifecycle owns the MCP server's lifecycle — shutdown is trivial. - Trade-off: a crash in the MCP server can take the plugin down. `McpSupervisor` + restart policy mitigates this; heavy request concurrency is capped by `McpRequestLimiter`. - Unsafe tools (anything that mutates Burp state — `http1_request`, `issue_create`, `repeater_tab`, `intruder`, `collaborator_register`) are gated by a separate master switch that is off by default. ## ADR-7: Audit logging is JSONL, hash-stamped, and disabled by default **Context.** Compliance and incident response teams need a tamper-evident record of what prompts the plugin sent to which backend, and what responses came back. Writing full prompts to disk on every request, however, duplicates sensitive data and costs performance. Writing nothing leaves us blind in a post-mortem. **Decision.** Use an append-only JSONL log (`~/.burp-ai-agent/logs/audit.jsonl`, rolled by size). Every entry records backend id, model, trace id, prompt hash (SHA-256), response hash, privacy mode, and timings. Full prompt bodies are written only when the operator enables verbose mode. The audit subsystem is disabled by default; the user enables it in Settings. **Consequences.** - Default behavior is zero disk I/O for compliance. - When enabled, the log is grep-able and diff-able and can be rotated by standard log tools. - Trade-off: verifying a hash from the log against a prompt the user remembers sending requires the operator to have verbose mode on at the time of capture. This is documented in the hardening runbook. ## ADR-8: AES-256-GCM encryption for secrets at rest (SEC-01) **Context.** Seven or more secrets (all backend API keys, `mcp.token`, `mcp.tls.keystore.password`) were persisted in plaintext in Burp's Preferences store. Vendoring Bouncy Castle or Google Tink would add a heavy dependency at risk of fat-JAR class conflicts with whatever Burp itself bundles. **Decision.** Encrypt all stored secrets with AES-256-GCM via `javax.crypto` (bundled in the JVM). A per-install random 256-bit master key is generated on first use. Encrypted values are prefixed with `ENC1:` for idempotent migration detection. **Consequences.** - No new runtime dependency; the JVM's built-in crypto covers the requirement. - Existing plaintext secrets are migrated automatically on first load after upgrade. - The `ENC1:` prefix allows safe forward/backward migration: a plaintext value without the prefix is used as-is (legacy pass-through) until it is next written. ## ADR-9: Real HKDF for STRICT-mode host anonymization (PRIV-01) **Context.** STRICT privacy mode documented "HKDF host anonymization" but the implementation used salted `MessageDigest.getInstance("SHA-256")` — a standard hash function, not HKDF. The privacy guarantee stated in documentation was not delivered. **Decision.** Replace `MessageDigest.getInstance("SHA-256")` with `Mac.getInstance("HmacSHA256")` extract/expand (proper HKDF per RFC 5869). `SecretShapes` becomes the single AWT-free source of truth for privacy-curated patterns and the HKDF implementation. **Consequences.** - Anonymized host values change on upgrade from any prior version (expected; anonymization is not required to be stable across versions — only stable within a session). - Existing tests that asserted the salted-SHA-256 output were updated to match the HKDF output. - `SecretShapes` is now the canonical place for any future privacy-pattern additions. ## ADR-10: Anthropic backend uses MontoyaHttpTransport, not a vendored Anthropic SDK (CAP-01) **Context.** Direct Anthropic API access was the top user-requested backend. Vendoring the official Anthropic Java/Kotlin SDK would embed its own OkHttp client, bypassing `MontoyaHttpTransport` and repeating the silent-exfiltration regression fixed in Phase 7 (#69), where HTTP backends could send AI traffic without it appearing in Burp's proxy or upstream proxy. **Decision.** Implement `AnthropicBackend` using the existing `HttpBackendSupport` base class and `MontoyaHttpTransport`, calling the Anthropic Messages API (`/v1/messages`) directly. No vendored Anthropic SDK is included. **Consequences.** - All Anthropic API traffic (requests and responses) appears in Burp's Proxy > HTTP history, where it can be inspected, replayed, and intercepted. - The API key is encrypted at rest via ADR-8 (SEC-01). - Native tool-use and prompt-caching features of the Anthropic API are deferred; the current implementation covers single-turn and streaming chat completions. ## ADR-11: External MCP client wraps untrusted server output in a trust-boundary marker (CAP-02) **Context.** External MCP servers registered by the user may be attacker-controlled, compromised, or return maliciously crafted tool results. If those results are fed directly into the AI prompt without marking, they constitute an untrusted injection surface — a classic prompt-injection attack vector. **Decision.** Wrap all external MCP server tool results in an explicit trust-boundary marker string before they are concatenated into the AI context. The kotlin-mcp-sdk 0.5.0 (already present from the built-in MCP server) provides `SseClientTransport` and `StdioClientTransport` — no SDK version bump is required (Path A confirmed). **Consequences.** - Prompt-injection from untrusted external server responses is bounded by the trust-boundary marker; the AI prompt structure makes the boundary explicit. - Auth tokens for external servers are stored encrypted (ADR-8 / SEC-01). - All external tool invocations are recorded in the audit log for traceability. - A non-loopback SSE URL triggers a runtime SSRF warning before the connection is made. ## ADR-12: Per-session token-budget guardrails via BudgetGuard (CAP-04) **Context.** Long passive-scan sessions could exhaust user API token budgets silently or unpredictably, especially with cloud backends like Anthropic or Perplexity that charge per token. **Decision.** Introduce `BudgetGuard`, a pure object with three reversible states: `OFF` (no limit), `WARN` (advisory warning shown in UI), and `CAP` (passive scanner pauses automatically). Both the warn threshold and hard cap are user-configurable; `0` means unlimited (off). State is per-session and does not persist across Burp restarts. **Consequences.** - Users can cap per-session spend; the passive scanner pauses automatically when the hard cap fires and resumes when the cap is raised or cleared. - No token-count state leaks between sessions. - The `BudgetGuard` object is pure and testable independently of the scanner lifecycle. ## ADR-13: Coalesce the MCP transport-block audit event for pre-authentication denials (amends D-06) **Context.** Phase 20 decision D-06 (recorded in `.planning/phases/20-mcp-access-control-correctness/20-CONTEXT.md`) locked that a blocked MCP transport request emits an audit event on EVERY occurrence, and decision D-09 deliberately scoped the per-reason rate-limit window to the Burp Output-tab line only. `McpBlockedRequestReporter.report` therefore calls `AuditLogger.emitGlobal` once per blocked request; `AuditLogger.logEvent` reaches a synchronous `logFile.appendText` to `~/.burp-ai-agent/audit.jsonl`, the global emitter is registered unconditionally at startup (`App.kt`), and the reporter runs on Netty event-loop threads. In external mode every route except `/__mcp/health` answers `401`, and every one of those `401`s is a `BlockReason.UNAUTHORIZED` or `BlockReason.BLANK_TOKEN` denial. A peer that can merely reach the port, with no credential at all, therefore controls the append rate to an unbounded file in the operator's home directory, and buries genuine audit records under attacker-chosen noise — which defeats the audit trail's purpose (CWE-400 resource exhaustion / CWE-779 excessive logging). Before phase 20 a blocked transport request emitted no audit event at all, so this primitive is new in phase 20 rather than pre-existing. Both preconditions — audit logging enabled and external access enabled — are supported, documented configurations (`docs/mcp-hardening.md` is a checklist for exactly that mode), so "off by default" is not a mitigation. **Decision.** Coalesce the `mcp_transport_blocked` audit event for the two pre-authentication reasons — `BlockReason.UNAUTHORIZED` and `BlockReason.BLANK_TOKEN`, the only two an unauthenticated remote peer can trigger — into one audit event per 60-second per-reason window, carrying a `suppressed` count of the occurrences collapsed away since the previous emission. Retain per-occurrence emission, with the payload key set unchanged, for the four local-mode reasons (`ORIGIN_MISMATCH`, `HOST_MISMATCH`, `REFERER_MISMATCH`, `BROWSER_NO_ORIGIN`), which require local code execution to trigger and are the diagnosable ones. Explicitly reject the alternative of adding size-capping or rotation to the shared `AuditLogger`: every phase depends on that infrastructure and the blast radius is out of proportion to this change. This amends D-06's literal "an audit event on every occurrence" for those two reasons only, and this ADR is the authorisation for that amendment. **Consequences.** - The flood ceiling for the two pre-authentication reasons becomes one audit record per reason per minute, regardless of the incoming request rate. - A burst is still visible: the `suppressed` count is carried in the emitted record, so detection is preserved and only per-request granularity is lost. - Output-tab behaviour is unchanged — D-09 still governs it — and the audit window and the Output window are independent counters that never consume each other's suppression count. - The four local-mode reasons keep full per-occurrence audit fidelity, so nothing diagnosable is lost. - Residual: `AuditLogger` still has no size cap or rotation, so a local process able to trigger local-mode denials in a loop can still grow `audit.jsonl`. That is a broader concern about shared audit infrastructure and is deliberately left to a future phase. ## ADR-14: The redaction body stage never fails open (PRIV-06) **Context.** Before Phase 21, `redact/Redaction.kt:257` wrapped the entire body-redaction stage in an `if (out.length <= Defaults.MAX_REDACTION_BODY_CHARS)` guard — a 1 MB cap. Above that cap the form-parameter stage, the JSON key/value stage **and the user's own custom patterns** were all skipped and the original string passed through untouched, so the control most likely to matter switched itself off on exactly the inputs most likely to carry a secret (CWE-200, exposure of sensitive information to an unauthorized actor — here a third-party AI provider). The size of a response body is attacker-selectable: a target can pad a response past the cap deliberately, so "bodies over 1 MB are rare in practice" is not a mitigation, it is an invitation. A related emergent behaviour sat immediately beside it — the custom-pattern loop lived inside the `policy.redactTokens` branch, so a user's hand-written "never send this" patterns were inert under `PrivacyMode.OFF`, a consequence of where the loop happened to be written rather than a decision anyone made. Both contradict ADR-5, which establishes redaction as a pre-flight, user-visible control rather than a silent default: a stage that quietly declines to run is precisely the silent default ADR-5 rejects. The finding is recorded as `.planning/notes/2026-08-05-code-review.md` section F5. **Decision.** The body stage chunk-and-scans the whole input rather than skipping any of it: the input is cut into windows at **line boundaries**, never splitting a line — with one documented exception, added by this phase's own code review, for a window that contains **no line boundary** at all, which is the normal shape of minified JSON and therefore of everything `McpToolContext.redactIfNeeded` sees, since `toolJson.encodeToString(...)` emits newline-free output. Such a window is split at a bounded safe cut (`SAFE_CUT_SEARCH_CHARS`) rather than discarded outright, because the alternative preserved nothing at all and a newline-free window has no interior line anchors for a cut to corrupt. The whole stage runs under a total wall-clock budget (`Defaults.MAX_REDACTION_BUDGET_MS`, two seconds), with every rule bounded by `min(SafeRegex.DEFAULT_TIMEOUT_MS, remaining budget)` through a new timeout-reporting API — `SafeRegex.replaceAllSafeReporting`, whose `timedOut` flag is the only thing that distinguishes "the pattern matched nothing" from "the pattern never finished", since the older façade returns its input unchanged in both cases. A window that could not be fully scanned, and everything past an exhausted budget, is **dropped behind a visible marker rather than passed through** — fail closed, so unscanned bytes never reach a backend — and the user is told through a rate-limited line in Burp's Output tab. This holds at **every** input size, with no carve-out: the at-or-below-1 MB single pass branches on the same `timedOut` flag, discards its partial result and re-scans the original input through the windowed path, so there is no size class at which a body rule can time out and its unscanned bytes still be emitted. User custom patterns now apply in **every** privacy mode including `OFF`, so `OFF` means *"no built-in redaction"* rather than *"no redaction at all"*, and that is enforced structurally by deleting the caller-side OFF short-circuits at `scanner/PassiveAiScannerAnalysis` and `mcp/McpToolContext` so the mode is expressed once, as a policy, instead of being re-decided at each call site. Rejected: truncate-and-redact (silent capability loss on large JSON — the model stops seeing the document without being told why); refuse outright (turns a redaction concern into a functional failure at five call sites, each needing its own failure story); overlap-based windowing (the value side of the built-in body rules is length-unbounded — measured single matches of 200 006 characters — so no finite overlap constant is defensible, which makes D-01's overlap clause unsatisfiable as literally written); `Matcher.region()` (its bounds semantics are correct, but `Matcher.replaceAll()` silently resets the region, so region-scoped replacement is not available at all); OFF-means-off for custom patterns (a user who added a pattern for a corporate token and later flipped to `OFF` for a debugging session would leak it with no warning); always-apply-plus-opt-out (a third state across the STRICT/BALANCED/OFF matrix, on a panel whose entire value is that privacy is simple); a fourth `RedactionPolicy` flag instead of deleting the short-circuits (it would have fixed the unit test, not the leak, because both short-circuits bypass `Redaction.apply` altogether); and renaming the constants or exposing the window and budget in Privacy settings (a user who set the budget to zero would silently disable body redaction — exactly the class of bug this ADR exists to kill). This ADR authorises the amendment of D-01's overlap clause to line-boundary cutting, on the measured grounds above. **Consequences.** - Input at or below the window width remains a single pass whose cost and behaviour match the previous implementation whenever no rule times out, so the overwhelming majority of payloads are unaffected. - The two built-in body rules previously ran with **no** deadline at all — only custom patterns went through `SafeRegex` — and now every body rule runs bounded. That is a real change on the common path and an easy one to assume was already true. - Oversized input costs more CPU than before, because it is now actually scanned instead of skipped. - The drop marker is a constant shape plus one integer and carries no attacker-controlled substring, so it is not an injection vector into model context; it is also distinct from `[REDACTED]`, keeping "removed for size" and "removed for secrecy" tellable apart. - The persistent passive-scan prompt cache is keyed on the SHA-256 of the **post**-redaction prompt, so changing redaction causes a one-time cache-miss wave with no stored secrets and no stale leak. A reviewer should not mistake it for a regression. - `SecretTripwire` hit counts under `OFF` may drop slightly, because custom patterns now run before the tripwire sees the text. That is correct and intended. - Widening the sensitive-key mechanism costs ten accepted over-redactions, every one of them in the fail-safe direction: seven under the separator rule (`token_bucket_size`, `session_timeout_seconds`, `auth_provider`, `key_size`, `code_version`, `secret_santa`, `password_hint_enabled`) and three under the camelCase rule (`codeName`, `keyName`, `tokenCount`), the latter asserted **as accepted** in `RedactionTest` with a one-line revert point recorded in source. Only `auth_provider` has real analytic value. **Superseded in part by the WR-01 bullet below**, which frees four of these ten (`key_size`, `code_version`, `codeName`, `keyName`); the list is left standing rather than edited because it is the record of what the widening cost when it shipped. - **WR-01, decided by the maintainer on 2026-08-12: the two broadest vocabulary words are narrowed to a credential-bearing context.** The widening above let `key` and `code` take part in D-11's free containment rule, and this phase's code review measured the resulting class rather than estimating it: 32 names redacted, including `status_code`, `error_code`, `response_code`, `http_code`, `statusCode`, `errorCode`, `zip_code`, `country_code`, `postal_code`, `currency_code`, `language_code`, `product_code`, `promo_code`, `coupon_code`, `area_code`, `qr_code`, `primary_key`, `foreign_key`, `sort_key`, `partition_key`, `cache_key`, `idempotency_key`, `row_key`, `public_key`, `sortKey`, `cacheKey` and `zipCode`. `{"statusCode": 401, "errorCode": "AUTH_FAILED"}` reached the analysis prompt as two `[REDACTED]` tokens while the model was being asked to find an authentication flaw — a functional regression in a passive vulnerability scanner rather than a cosmetic over-redaction, and one that had been accepted by omission rather than chosen. `key` and `code` now redact only on **whole-key equality** or behind one of seven confirmed **credential-bearing prefixes** — `api`, `access`, `secret`, `auth`, `private`, `signing`, `enc` — so `api_key`, `api-key`, `api.key`, `apiKey`, `access_code`, `secret_key`, `private_key` and `signing_key` still redact while the 32 names above survive. **Rejected:** keeping the current breadth and pinning it as accepted (the scanner keeps losing its status and error codes on every JSON body, which is its core reasoning input); and dropping `key` and `code` entirely (`api_key` and `access_code` would then depend wholly on enumeration, which is the brittleness the token-boundary rule exists to escape). This direction **loosens** a security pattern, so `.planning/codebase/CONCERNS.md`'s tightening protocol was invoked in full and all three SC3 corpora were **re-measured against the live regexes rather than argued**: 31/31 must-redact unchanged, 21/21 must-not-redact unchanged, and the camelCase set changed only by `codeName` and `keyName` moving from redacted to surviving. The outcome is pinned by `wr01BroadWordKeysSurviveUnlessCredentialBearing` across query-string, form-body and JSON, in both directions, so it cannot silently drift back. - **Accepted cost of WR-01, recorded rather than left implicit:** a bespoke vendor-shaped name whose prefix is not one of the seven — `stripe_key`, `encrypted_key`, `myapi_key` — no longer redacts on the broad-word path. Bespoke API-key names were already a documented accepted gap in `CONCERNS.md`, and the 17-entry `KNOWN_SESSION_KEYS` vendor list plus the enumerated `api_key` / `apikey` / `access_token` literals still cover the enumerated ones. `public_key` and `publicKey` also now survive, because the confirmed prefix set does not contain `public` and a public key is publishable by definition; that follows from the ruling rather than from an executor judgement, and it is pinned in the corpus so revisiting it is a visible test change. - **WR-01 did not reach the whole measured class, and the limit is asserted rather than assumed.** Five of the 32 names are driven by `auth`, `session` and `token` — words the ruling left alone — so `token_type`, `tokenType`, `session_count`, `auth_type` and `auth_url` still redact and are pinned **as accepted** by `wr01NonBroadWordOverRedactionsRemainAccepted`. `token_type: "Bearer"` is benign OAuth metadata and is the analytically painful case; freeing it needs either a suffix denylist, which D-12 rejects on principle because every entry is a place a real credential could be allowlisted, or a narrowing of `token` itself, which would put `access-token` and `XSRF-TOKEN` at risk. Both are maintainer decisions and neither was taken here. This bullet exists so a later reader can tell that limit from an oversight. - **The key vocabulary is now compiled first-letter factored, and that shape is load-bearing rather than stylistic.** Measured on a 1 MB maximum-key-density JSON body, best of five, driving the live `jsonSecretKeyRegex`: the pre-WR-01 vocabulary cost 50 ms flat; the WR-01 vocabulary 58 ms flat (+16%); factoring only the seven prefixes 53 ms (+6%); factoring by first letter 47 ms (-6% against the cost this file shipped with). The 4 MB newline-free fixture in `newlineFreeOversizeBodyIsScannedNotDestroyed` already spends ~1.9 s of the body stage's 2 s `MAX_REDACTION_BUDGET_MS`, so both flat shapes exhausted the budget and dropped the window carrying the secret behind a marker — fail-closed and never a leak, but exactly the capability regression this ADR exists to prevent, and the +6% shape was measured failing that test one run in two. Hand-factoring a security-critical alternation is the kind of change that looks right and is subtly wrong, so it is **checked rather than trusted**: `Redaction.NAIVE_KEY_EXPR_FOR_TEST` builds the same expression straight from the readable `SENSITIVE_WORDS` / `CREDENTIAL_PREFIXES` / `BROAD_WORDS` constants and `factoredKeyVocabularyMatchesItsReadableSpecification` asserts the two classify all 120 corpus names identically. Edit the readable constants first, then re-factor; do not flatten the compiled form back out. - The single-pass fail-open that plan 21-06 recorded as a residual — a rule overrunning the 50 ms deadline below 1 MB being silently skipped — was **fixed in this phase, not accepted**. That is why the claim above carries no size carve-out. - This phase's own code review found that the boundary mitigation shipped above did **not** deliver the property the design claimed, and the record was corrected rather than the claim retained. The source comment asserting that line-boundary cutting had been proven equivalent, byte for byte, to whole-document processing is gone: it was falsified by a reproduction rather than by argument, and it had been established without ever sweeping a fixture across the cut — which is precisely why it survived review. D-08 REFINED is the governing precedent, that a record claims only what ships. The claim now lives in a named test, `windowedScanRedactsJsonPairAcrossEveryBoundaryAlignment`, which sweeps a full pad-line period and additionally asserts that no drop marker was emitted, so it cannot pass by dropping the window instead of redacting the pair. A future contributor should read the retirement of the byte-identity phrasing as deliberate, not as something lost in an edit. **This happened twice.** The wording that replaced the byte-identity claim then asserted, in its turn, that `jsonSecretKeyRegex` was fully handled by the capped lookahead — and a round-2 review falsified that too, with a live leak at a shape the cap never bounded (see the `jsonSecretKeyRegex` residual bullet below). Both retirements are recorded in `Redaction.kt`'s D-01 paragraph. The recurrence is the point: correcting an overclaim by writing a *narrower* overclaim is the failure mode this ADR keeps reproducing, and the guard against it is a named test per claim rather than a better-worded sentence. - Residual: two gaps are knowingly left open, and neither is claimed fixed. The eight rules of the header stage still run unbounded on the full input, with no per-pattern deadline and outside the total budget — pre-existing, explicitly outside D-01/D-02's scope, and the reason this ADR is scoped to the body stage rather than making the unqualified claim. And a **user** custom pattern whose match spans a window boundary can be missed, because there is no principled bound on a user regex's match length, so no window scheme closes it. Plural key forms (`codes`, `tokens`, `keys`) are also still unmatched. All three are recorded in `.planning/codebase/CONCERNS.md`. This bullet is about **user** patterns only: the built-in newline-spanning case is a separate residual and is recorded in the bullet below, because the earlier phrasing named only user patterns here and thereby left the built-ins looking safe when they were not. - This phase's code review also found that the windowing it shipped **destroyed** a newline-free body above the window width in its entirety, and that consequence is recorded rather than quietly fixed. `windowEnd` gives an over-width line its own window, so a body with no `\n` is one window at any size; `splitPoint` then returned `0` for any window with no interior newline, and `dropOrRetry`'s `if (cut <= 0 …)` turned that into a total drop. The halve-and-retry ladder — justified in source as existing precisely so a 2-3× slower machine would not lose content that ships today — was therefore structurally inapplicable to the single most common oversized payload shape there is. Measured on the reference hardware: a 2 MiB newline-free body is one window and the JSON rule alone takes 62-66 ms against a 50 ms per-pattern deadline, so a default-configuration 2 MiB MCP tool response reached the model as `[REDACTION INCOMPLETE - 2097156 CHARS DROPPED AND NOT SENT]` and nothing else. The `Defaults.kt` sizing note of ~27 ms/MB was evidently taken on newline-bearing content; dense newline-free JSON costs ~31 ms/MB, putting the cliff near 1.6 MB on fast hardware and ~800 KB on a machine half as fast. This was **fail-closed rather than a leak, and a capability regression rather than a security one** — but silently emitting an empty analysis for a default input is incorrect behaviour, and it is the reason the line-boundary clause above now carries an exception instead of an unqualified claim. - Residual: a built-in match that spans the **character cut** in a newline-free window can be truncated, and a false `(?m)^` anchor created at the cut can produce a false positive. Both are bounded and both are strictly better than the behaviour they replace, which was emitting nothing at all. The cut prefers the first position just after `&`, `,`, `}`, `]` or whitespace within `SAFE_CUT_SEARCH_CHARS` (1 024) of the midpoint, and that set is derived from the rules themselves rather than asserted: `formBodyParamRegex` and `urlTokenParamRegex` have the value class `[^&\s"'<>]+`, so neither match can span an `&` or any whitespace, and `jsonSecretKeyRegex`'s value is either a `"`-delimited string or an unquoted scalar, both immediately followed by `,`, `}` or `]` in well-formed minified JSON. A user **custom** pattern can still span the cut, which is the same unbounded-match-length residual already recorded above and is not made worse here. Three points justify the exception rather than merely noting it, and all three are written into `splitPoint`'s source comment: a window with no interior newline has no interior line anchors to corrupt, so a cut creates exactly one artificial line start; that artificial anchor can only over-redact, which is fail-safe; and the branch is reachable **only** from `dropOrRetry`, i.e. only where the alternative is discarding the whole window behind a marker, so a match truncated by the cut loses nothing that would otherwise have been emitted. This is **not** overlap and does not revive the rejected alternative above: the two halves are disjoint and no region is processed twice. `WINDOW_RETRY_MAX_DEPTH` rose from 2 to 4 in the same change, because quarters are still ~500 KB and still over the deadline for a 2 MiB newline-free window while sixteenths are ~128 KB at roughly 4 ms; the real ceiling on retry work remains `MAX_REDACTION_BUDGET_MS`, since every retry runs under the same budget clock, and the depth is capped at 4 rather than higher to hold marker bloat at `2^depth` per window. Measured at the new depth, a wholly-unscannable 1 201 200-character body collapses to 1 084 characters across 19 markers, three orders of magnitude under the half-length bound its test asserts. - Residual: a built-in `jsonSecretKeyRegex` pair spread over more than `MAX_JSON_BOUNDARY_LOOKAHEAD_LINES` (eight) lines can still straddle a window cut and be missed. `jsonSecretKeyRegex` is the one built-in body rule whose match can span newlines — its `\s*` sits on both sides of the colon — and the mitigation this ADR originally shipped pulled in exactly **one** following line and never re-checked, so the pretty-printed shape its own source comment cited was unhandled. The Phase 21 code review **reproduced a pair spread over three lines leaking** on the windowed path at shift 7 while the single-pass path redacted it, which is a redaction bypass keyed only on payload size. `windowEnd` now loops, re-checking each newly included line and treating a blank line as a continuation of risk, and the initial test walks backward over blank lines so a cut landing on one inside a pair still triggers the extension. The eight-line cap is deliberate: without it, "keep extending until the pair closes" would trade the leak for an unbounded window on crafted input. Dropping the window at the cap was considered and rejected, because the trigger is a content heuristic that ordinary multi-line HTML attributes and nested YAML both satisfy, so it would destroy up to a megabyte of analytic context on benign input. The residual is therefore **narrowed to pairs spread over more than eight lines, not closed**. `formBodyParamRegex` and `urlTokenParamRegex` remain genuinely unaffected, because their value classes exclude `\s`. **Round-2 correction (2026-08-12): the sentence above was not merely narrow, it was incomplete, and its incompleteness is how a live leak survived a second review.** The eight-line cap residual stands exactly as written — it is still true — but it was recorded as if it were the *whole* class of window-boundary miss for this rule, and it was not. The round-2 review found that the risk predicate could not see a cut landing **inside an open quoted value**: `jsonSecretKeyRegex`'s value alternative is `"[^"]*"` and `[^"]*` matches newlines, so a pair whose value carries a raw newline is a **two-line** shape on which the window's last line ends with an ordinary value character. The predicate tested only for `:` or `"`, so no extension was ever started, the cap was never reached, and the lookahead never began — the recorded residual described a bound the failing shape never touched. It was reproduced against the **compiled shipped classes** at **6 of 40 alignments** of a 1 MB body with **`dropMarker=false`** — a leak, not a fail-closed drop — while the single-pass control did not leak; this repository reproduced the same class at 8 of 40 alignments once the fixture geometry was corrected. It is reachable in the **default configuration** through `McpToolContext.redactIfNeeded`, whose `maxBodyBytes` default is 2 MiB, twice `MAX_REDACTION_BODY_CHARS`. It is now **closed** by `endsInsideOpenQuotedValue`, which models the state `[^"]*` is actually in rather than the punctuation around the colon, and is guarded by `windowedScanRedactsJsonPairWhoseValueStraddlesTheCut` — named here so the claim and its evidence cannot drift apart again. The governing lesson is D-08 REFINED's: **this record described the residual its authors had in mind rather than the residual the code had**, and correcting it is what this ADR's own "claims only what ships" clause requires, not an optional tidy-up. Note the failure mode for future readers — the claim was falsified by the **absence of a fixture family**, since every line of both committed sweeps' fixtures ends on `:` or `"`, which is precisely the state the old predicate already detected; a coverage sentence is only as strong as the shapes its tests can construct. ## ADR-15: A model-emitted tool call requires a user decision before it reaches Burp (SEC-06) **Context.** The chat agent's context window contains attacker-controlled data by design: HTTP traffic the user sent with "Send to AI", passive-scan findings whose URLs and detail text come from the target, and results returned by external MCP servers (ADR-11). The model chooses which tool to call out of that context, so **tool selection is attacker-influenceable** — and until this phase nothing stood between that choice and Burp. `ChatPanel.maybeExecuteToolCall` took `ToolCallParser.extractFirst`'s output — where the tool name is whatever string the model wrote, never validated against the catalog — and called `McpToolExecutor.executeTool` directly, chaining up to `ChatPanel.MAX_AUTO_TOOL_ITERATIONS` (8) with only the per-tool toggles and the `unsafeOnly` flag in the path. The finding is recorded as `.planning/notes/2026-08-05-code-review.md` §F3. The instance is **demonstrated rather than argued**: driving the real Send button of a real `ChatPanel`, a model-emitted `proxy_http_history` reached `api.proxy().history()` — `McpToolExecutorImpl.kt:694` as the file stood at pre-gate commit `5863de8` — **four separate times**, once per chained turn, with nobody asked. The assertion that proves it, `ChatPanelToolGateTest.confirmToolDoesNotReachBurpBeforeADecision`, failed there on Mockito's `NeverWantedButInvoked` and passes now. **The `[EXTERNAL-TOOL-RESULT:...]` marker and its advisory note (ADR-11) are mitigation, not a control**, and the reason is concrete and checkable rather than philosophical: the note at `McpToolExecutorImpl.kt:128-134` is appended **only when external tools are present**, so a session using no external MCP server receives no trust-boundary instruction at all — even though "Send to AI" proxy traffic is already sitting in its context. A prompt-level instruction is in any case advisory to a model the attacker is also instructing. The marker keeps its value; it stops being counted as a control. **Decision.** Every model-originated tool call is classified by a required, non-defaulted `secTier` on `McpToolDescriptor` (`McpToolCatalog.kt:45`) and must pass `ToolApprovalGate.evaluate` (called from `ChatPanel.maybeExecuteToolCall`) before anything reaches the executor. Three tiers: `AUTO` runs silently; `CONFIRM` prompts and offers a per-tool, per-chat-session approval; `CONFIRM_EACH` prompts on every call and has no session memory in either direction. External `ext::` names derive `CONFIRM_EACH` from the namespace rather than declaring it per tool, and an unrecognised name resolves to `CONFIRM_EACH` — fail closed, never `AUTO` (`ToolApprovalGate.tierFor`). The definition that governs the classification, and the sentence future tool authors inherit, is D-05's, unaltered: > `AUTO` means read-only AND bounded output. A tool qualifies only if it neither mutates Burp state nor > pulls bulk attacker-controlled traffic into model context. Two worked examples close the gap a literal reading leaves open, **without altering that sentence**. `project_options_get` and `user_options_get` are `CONFIRM` despite being read-only, because what they return is not attacker-controlled but *is* the user's own credential material — upstream-proxy configuration, session-handling rules, platform-auth material — and the destination is the same third-party model. And `proxy_http_history`, site-map listing and issue listing are `CONFIRM`, not `AUTO`, which is the load-bearing consequence of the definition. `AUTO` is **enumerated, not derived**: the tier is a declared descriptor field, the definition guides an author rather than computing anything at runtime, and `McpToolCatalogTierParityTest` pins the exact 19-tool `AUTO` set so a promotion is a deliberate, reviewed diff. The classification is **deliberately independent of `unsafeOnly` (D-01)**, and the sentence a future contributor needs when they ask why there are two classifications is this: *`unsafeOnly` is a capability switch — may this tool ever run — not a trust model.* `McpToolContext.isUnsafeToolAllowed` (`McpToolContext.kt:53-57`) returns `true` for everything the moment `unsafeEnabled` is on, so binding the trust boundary to it would let one unrelated toggle disable the gate; concretely, `ai_analyze` and `ai_passive_scan` are `CONFIRM_EACH` while not being `unsafeOnly` at all. `CONFIRM_EACH` is the tier for tools that put attacker-chosen traffic on the wire, **or stage it for one click** — `intruder`, `intruder_prepare`, `repeater_tab` and `repeater_tab_with_payload` populate a tab rather than launching an attack, so the unqualified wire-traffic claim would have been false the day it shipped. The two repeater tools appear in that list after a correction, and the correction is the second time this ADR describes a table that did not match it: both shipped as `CONFIRM` while this very clause was already written, even though `McpToolExecutorImpl.kt:243-274` builds an `HttpRequest` from model-supplied `content` and hands it to `sendToRepeater` in exactly the shape the clause describes. One `Approve for session` click therefore covered every later call in that chat with *different* request content, which is the hole `CONFIRM_EACH` exists to close. `repeater_tab_with_payload` is the stronger case rather than the weaker one: `McpToolExecutorImpl.kt:261` runs `applyReplacements(input.content, input.replacements)` before staging, so the substituted payloads are model-supplied too, and substitution running first widens what a session grant would have covered rather than narrowing it. The phase's own verification recorded the contradiction (WARN-2, which named both tools); the maintainer moved the table to match the ADR rather than weakening the ADR to match the table, because the criterion is the thing future tool authors inherit. The decision surface is an inline card in the chat transcript, not a modal, carrying four explicit actions (`Approve once` / `Approve for session` / `Deny` / `Deny for session`) on `CONFIRM` and two on `CONFIRM_EACH`; denial returns one fixed neutral constant that is deliberately **not** `Error:`-prefixed, so a policy outcome is never reported to the model — or to the audit log — as a malfunction. Origin is **structural rather than checked**: `McpToolExecutor.executeTool` takes a required, non-defaulted `ToolCallOrigin`, and the model-approved variant is a **file-private** class (`ModelApproved`, in `ToolApprovalGate.kt`) minted only by `approvedOrigin`, which returns the interface type and never the implementing one and which is **`private` to the `ToolApprovalGate` object** — so `evaluate` and `resolve` are the only code that *can* call it, rather than merely the only code that does today, and a future parse-and-execute call site cannot obtain one without going through the decision. That last clause is stated after a correction: `approvedOrigin` shipped as `internal`, and this ADR asserted the property anyway. The phase's own code review measured that a module-wide factory for a file-private type is simply a module-wide factory — any file in the main source set could write `ToolApprovalGate.approvedOrigin(SecTier.AUTO, ToolDecision.AUTO)` and reach Burp with no card and no audit record — so the factory was narrowed to `private`, and the narrowing is what the sentence above now describes. Kotlin's `internal` is insufficient for both halves: it is module-wide, so the type and its factory alike would have been reachable from `ChatPanel.kt`. **The boundary is stated at the strength the compiler actually checks, because a claim the compiler does not check is worse than a weaker accurate one.** Minting is file-scoped and enforced; *implementing* `ToolCallOrigin` is not, because Kotlin seals a `sealed interface` to a package and module rather than to a file, so a new file under `com.six2dez.burp.aiagent.mcp` can declare its own implementation — verified by compiling one. Kotlin offers no idiomatic file-scoped seal (the unnameable-marker-member trick closes it only behind suppressed `EXPOSED_PROPERTY_TYPE` compiler errors, which trades a checked property for a suppressed diagnostic, and was rejected). What ships is therefore *an accidental bypass cannot compile*, not *a bypass cannot exist*: the deliberate one is answered by code review and by the audit record, not by the type system. `executeTool` became `internal` as a consequence, because Kotlin refuses to let a public function expose an internal parameter type; that is a narrowing, not a weakening. **No opt-out and no tier downgrade ship (D-09).** A persisted per-tool `CONFIRM`→`AUTO` downgrade was rejected because it would make a malicious settings import a gate bypass; a warned global off-switch was rejected because it is the control's own bypass shipped in the box, and Unsafe Mode being on is precisely the state in which the gate matters most. The escape hatch that does exist is `ToolSessionState.toolsMode`, which stops the model calling tools at all rather than un-gating them. **Consequences.** - `AUTO` is deliberately a small set — 19 of the 59 built-ins, against 24 `CONFIRM` and 16 `CONFIRM_EACH` — so the gate is felt. Fewer, more meaningful prompts beat many cheap ones; every rejected alternative that produced more dialogs was rejected partly because a safety control that trains the user to dismiss it is not a safety control. - **Forgetting to classify a new tool is a compile error** at the catalog site (`No value passed for parameter 'secTier'`), not a silent inheritance of somebody else's tier. That, and not the prose above, is the mechanism by which a tool added in a later phase cannot skip the trust boundary. - A denied call decrements the iteration budget, so the counter is monotone regardless of what the user clicks and `MAX_AUTO_TOOL_ITERATIONS = 8` is respected with no case analysis. Free denials were rejected because injected traffic could otherwise walk the model through 59 different tools and produce 59 cards — a denial of service delivered through the safety control itself. - Clear Chat clears the per-session approval memory alongside `toolsMode` and `toolCatalogSent` (`ChatPanel.clearCurrentChat`), because Clear Chat is the user declaring a new task, which is D-10's own justification for re-consent. An approval granted while reviewing target A cannot run silently against target B in the same chat window. - The audit record's denial test is an exhaustive `when` over `ToolDecision` (`ToolDecisionReporter.isDenial`) rather than a boolean predicate, so a future denial-shaped constant is a compile error instead of a call silently recorded as `status = "ok"` — which would be a fail-open in the audit trail rather than in the gate. - **`argsSha256` digests the whole argument string, and the reason it is spelled out here is that it did not at first.** The value used to run through `sanitizeInline`, whose cap is 120 characters, so the digest identified a *prefix* of the arguments: two `http1_request` calls differing only in the request body hashed identically, and an attacker wanting an exfiltration request to look like a benign one in the audit trail only had to keep the first 120 characters constant. A prefix digest is worse than no digest, because it looks like an answer to "which arguments ran?" — and D-07's whole premise is that the args are where exfiltration hides. Sanitization now applies only to the verbose plaintext seam, which is the only form ever rendered. - **`toolNameSha256` digests the whole tool name, and it took a second pass to get there.** The identical truncation survived on the unresolved-name branch after `argsSha256` was fixed: the digest ran through `sanitizeInline`'s 120-character cap, so two unresolvable names sharing a 120-character prefix produced one record — on precisely the branch where the plaintext name is replaced by the constant `unknown`, which makes that digest the only record of what the model asked for. Phase 22's verification recorded it as WARN-1. Sanitizing there bought nothing that was not already guaranteed elsewhere: a hex digest is 64 characters of `[0-9a-f]` by construction, so nothing model-authored survives it, and the only *rendered* form of the name is the Output line, which is sanitized unconditionally on both branches (below). The lesson worth carrying: a defect class is not closed by fixing the instance that was reported. - The tool name on the Output line is inline-sanitized on **both** branches of `knownTool`, not just the unresolved one. The `knownTool = true` branch wrote the canonical ID verbatim on the reasoning that a recognised name is a catalog ID; that reasoning does not hold, because `ChatPanel.isKnownTool` accepted any `ext:`-prefixed string without asking the configured server (fixed since, see below), and `McpToolExecutor.canonicalToolId` returns unmatched names unchanged. A tool name of `ext:x\n[SEC-06] decision=approve_once …` therefore forged a second `[SEC-06]` line in the Output tab (CWE-117) — on approve, deny and implicit deny alike, since every decision branch reports. Sanitizing unconditionally is a control; sanitizing on the branch that was believed to be model-controlled was a second place to get the belief right. - **`ChatPanel.isKnownTool` asks the configured servers, and it took a third pass to get there.** It trusted the bare `ext:` prefix, so the record filed an `ext:` name no configured server exposes as a recognised tool — `knownTool = true`, the model's own string in `toolName`, and `toolNameSha256` omitted entirely — while the executor rejected the very same call with "External MCP client not available". That bypassed the whole-name digest for precisely the class of names where the digest is the only surviving record of what was asked for, which is the third time in this phase a control was written down as holding on a branch nobody had checked. It now consults `ExternalMcpClientManager.availableTools()`, the same source `McpToolExecutor.describeTools` advertises from, so the classification is derived rather than assumed; `ChatPanelToolGateTest.anExtNameNoConfiguredServerExposesIsRecordedAsUnknown` pins it and was red before the change. The gate never depended on this — `tierFor` derives `CONFIRM_EACH` from the same prefix, so such a call always prompted and always showed the user the full sanitized name — so what changed is audit fidelity, not gate integrity. - Residual: nothing in the main source set builds an `McpToolContext` carrying an `externalClientManager`, so **no `ext:` tool is reachable from the chat path at all** and every `ext:` name is correctly recorded as unknown today. That is the accurate answer rather than a stopgap, but it means the membership check above is currently exercised only in its negative direction; the day a manager is wired in, the check moves with it because it takes the context rather than assuming its contents. - **Cross-references into this repository name SYMBOLS, not line numbers, and that is a correction.** This ADR and the SEC-06 sources carried eighteen distinct `ChatPanel.kt:NNN` citations, every one of them off by 100-500 lines within the same phase that wrote them — a codebase whose design rationale travels through cross-file citations turns into a maze the moment those citations stop resolving, and they drift on every edit by construction. Symbols survive refactoring and fail visibly when renamed. Where a citation genuinely refers to a HISTORICAL state it says so with the commit, as the `McpToolExecutorImpl.kt:694` reference above does for pre-gate `5863de8`; a line number that pins evidence is fine, a line number that points at current code is a maintenance liability. - ADR-11 is **not** superseded. The `[EXTERNAL-TOOL-RESULT:...]` marker still wraps external tool output and still carries its advisory note; what changes is its standing in the argument. It is a labelling aid, and the gate is the control. - **Deliberate omission:** the tier is **not** advertised in the tool preamble beside the existing `[unsafe]` / `[pro]` / `[external]` markers (`McpToolExecutorImpl.kt:113-123`), because that would hand an injected prompt a map of which tools run silently. - Residual: deny-for-session bounds the *prompting*, not the *token cost* — a session-denied tool still costs up to seven further backend round-trips within one chain. - Residual: approvals do not survive a Burp restart. `ChatPanel.restoreSessions` builds a fresh `ToolSessionState`, so a chat session restored from `.burp` project data comes back with no approvals at all. That is stricter than D-10 requires and it is intentional; do not "fix" it by persisting the set. - Residual: the resolved card is a **live-session record only**. Decisions are deliberately not appended to `session.messages`, because that would feed them back to the model as history — a new prompt-injection surface for no stated requirement — and because tool results are already transient. The durable record is the `mcp_tool_decision` audit event plus the enriched `MCP_TOOL_CALL` entry. - Residual: four of the five implicit-denial paths destroy the transcript or the whole panel, so they leave no observable card behind. For those four the audit event is the only record. - Residual: an unrecognised tool name resolves to `CONFIRM_EACH`, which has no session memory, so an unknown tool can never occupy a session-suppressed compact row. The compact unknown-tool string is implemented for exhaustiveness and is unreachable in this release. - Residual (updated by 26-04): `assertEdt()` remains a production no-op — the JVM disables the debug-time assertion facility without `-ea`, which no shipped Burp passes. QUAL-07 / SC4 resolved this by declaring the limit rather than removing it: the helper's KDoc and all four call sites now state that the check has no production effect and name the marshalling remedy, and `ChatPanelEdtGuardTest` fails if that wording drifts back into claiming enforcement. REL-01 is held by the callers' marshalling discipline, evidenced by `ChatPanelEdtConfinementTest`, not by this helper. The decision that made the limit explicit, the measured evidence that argued the other way, and the off-EDT `shutdown()` path it does not cover are recorded in ADR-17 clause 2. Tool execution still runs on the EDT at three call sites — Phase 23 / REL-05 owns moving it, and this ADR still makes no claim about EDT behaviour. ## ADR-16: The MCP takeover path proves possession of the token and pins the loopback certificate (SEC-07) **Context.** The bind-conflict takeover client had two independent identity defects, and both were reachable in a default installation. First, the credential: `McpSupervisor.requestRemoteShutdownWithToken` presented `Authorization: Bearer ` to whatever process held the MCP port, and the identity `probeExistingServer` established before it did so does not survive contact with an attacker — in local mode it accepted the `X-Burp-AI-Agent: mcp` response header, which any port holder can echo, and in external mode it checked no header at all (D-02 stops the server emitting one there), so liveness alone gated the takeover. A local process that binds the MCP port before Burp does therefore received the MCP bearer token by return of post. Second, the transport: `McpSupervisor.openConnection` installed an `X509TrustManager` whose `checkServerTrusted` body was `= Unit` together with a `HostnameVerifier` returning true unconditionally whenever TLS was on and the host was loopback, so even under TLS the client could not distinguish this extension's own MCP server from any local listener presenting any self-signed certificate. The defect is **demonstrated rather than argued**: `McpTakeoverSquatterTest` drives a real hostile listener that spoofs the identifying header and asserts it receives no credential, and `McpTakeoverCertificatePinTest.aForeignCertificateIsRefusedAndTheServerSurvives` drives a second certificate minted through the same `keytool` path against a real Netty TLS server on a loopback port. Both fail against the pre-phase code. Cross-references here name **symbols, not line numbers**, per ADR-15's correction; a line number appears only where it pins historical evidence at a named commit. **Decision.** Three clauses. 1. **The takeover credential is a proof of possession.** The developer's selection, taken at plan 25-01's blocking checkpoint and copied verbatim from `25-01-SUMMARY.md` §"SC1 decision": > proof-of-possession That is Option C — the bind-conflict takeover client presents `HMAC-SHA256(key = token, message = "burp-ai-agent/mcp-takeover|v1|:|<10s window>")` instead of the MCP bearer token. Concretely: the token becomes an HMAC key and never leaves the process, the value on the wire is `McpTakeoverProof.forTarget`'s output carried in `McpTakeoverProof.HEADER`, and `POST /__mcp/shutdown` accepts **either** that proof or the pre-existing bearer form, so an operator driving the endpoint by hand is unaffected. A blank configured token fails closed before a connection is opened rather than presenting a bare `Bearer `. Option A (drop automatic takeover) was rejected because every ordinary extension reload that hits a bind conflict would leave the MCP server down; Option B (server-supplied challenge/response) was rejected because any pre-authentication signal only a Burp AI Agent emits re-creates the external-mode identification oracle D-02 removed. 2. **`X-Burp-AI-Agent: mcp` is demoted from control to hint.** It stays in `probeExistingServer` as a cheap filter that avoids issuing shutdown requests at listeners that are obviously not ours. It is no longer load-bearing for credential disclosure, because after clause 1 there is no credential to disclose. Phase 20's D-02 is untouched: no new pre-authentication response signal was added anywhere, which is precisely why the challenge/response option was rejected rather than adopted. 3. **The loopback TLS handshake is pinned to the certificate this extension generated.** `McpTls.pinnedLeafSha256` reads the leaf certificate at `settings.tlsKeystorePath` and returns its SHA-256 digest; `McpSupervisor.openConnection` installs an `SSLContext` whose only trust manager accepts a chain if and only if its leaf digests to that pin, compared with `MessageDigest.isEqual`. The hostname verifier is **replaced, not disabled**: `McpTls` generates the certificate with `-dname CN=burp-mcp`, which can never match `localhost` or `127.0.0.1`, so name-based identity was never available on this path and the identity assertion moves from the name to the key. When no pin can be read, **no TLS override is installed at all** and the takeover fails closed against the JDK defaults. That is a classification and not a regression: `KtorMcpServerManager.start` throws `IllegalStateException("TLS enabled but keystore not available.")` when `McpTls.resolve` returns null, so this extension's own MCP server cannot be running under TLS unless a readable keystore exists at that path — a client that cannot read one there is not talking to our server, and weakening TLS to reach it would be exactly backwards. `pinnedLeafSha256` is deliberately **not** implemented in terms of `resolve`, which auto-generates a keystore when none exists: a client-side probe doing that would write a file the user never asked for and mint a fresh certificate that by construction cannot match the running server's, producing a pin guaranteed wrong exactly when it matters. **Consequences.** - The takeover is now bound to a specific target: the proof covers host, port and a time window, so a proof captured on one endpoint authorises nothing on another. The bearer form remains for the operator, and the two are checked independently at both the route and the access-control gate. - **The external-mode access-control gate is part of the takeover credential path, and it was not obvious.** `McpAccessControl` runs in Ktor's `Plugins` phase, before routing, and in external mode denies every non-`/__mcp/health` path lacking a valid bearer — so a proof-only shutdown request was `401`'d before the shutdown route could ever run. `McpAccessControlDecision` and `McpAccessControlPlugin` therefore recognise the proof form for the shutdown path (two defaulted `RequestFacts` fields, with the clock injected so `evaluate` stays pure, and one `when` limb below the SEC-05 5c blank-token guard). Without that change, removing the bearer from the client would have silently broken external-mode takeover in the one configuration where the operator is least able to notice. - The operator-visible behaviour of a bind conflict, and the Output lines it produces, are documented in `docs/mcp-hardening.md` under "Takeover on a bind conflict". That section is the procedure; this ADR is the reasoning. Neither restates the other. - Residual: **proof replay inside the validity window** (25-01 T-25-04). The squatting process does receive the proof and can replay it within its 10-second window — plus one fallback window, so 20 seconds worst case — to shut down the freshly-bound MCP server. That is a denial of service by a process which is already denying the service by holding the port. A server-side single-use nonce cache was rejected as disproportionate. This residual was stated to the developer before the SC1 selection was taken and accepted with it. - Residual: **host-string identity** (25-01 A-25-05). If the configured host string changes between the old server binding and the new one starting, the proof mismatches and the takeover is refused. The server stays down with an error log and no credential leaks — a safe failure, and deliberately not a silent one. - Residual: **version skew** (25-01 A-25-06). A new client against an old server gets a rejection rather than a leak, and does not retry. - Residual: **filesystem read defeats the pin.** A local attacker who can read `settings.tlsKeystorePath` can present that certificate and satisfy the pin. The same attacker also holds the MCP token, because `SecretCipher`'s master key sits beside its ciphertext in Burp Preferences (QUAL-07), so pinning was never the control standing between them and the server. What pinning defends is the boundary this phase is actually about: a local process that can bind the port but cannot read the user's files. - Residual: **fail-closed under TLS** (T-25-16). A user whose keystore has moved or been deleted loses automatic takeover under TLS rather than falling back to trusting everything, and learns about it from one Output line naming the path. The alternative — pin when available, trust everything otherwise — would leave the original weakness reachable by deleting one file, and every pre-existing test would still have passed; `McpSupervisorConnectionTest.openConnection_loopbackTlsWithoutAPin_installsNoOverrideAtAll` is the row that fails when someone reintroduces it. - Residual: **the SSRF half of SEC-07 stays advisory** (plan 25-02). `SsrfGuard` now parses IPv4 literals in decimal, octal and hexadecimal notation and classifies them from raw bytes through `InetAddress.getByAddress`, closing the notation-evasion bypass without resolving any name. Its verdict remains advisory and non-blocking per D-01, so classifying a notation correctly changes what the user is told, not what the extension permits. - Residual: **offline token guessing from a captured proof.** The bind-conflict takeover proof is `HMAC-SHA256(key = token, message = "burp-ai-agent/mcp-takeover|v1|:|")`, and every byte of that message is known to a squatting local process — the port it holds, the host string the client dialled, and a 10-second window index. A captured proof is therefore an offline verifier for the token itself: one HMAC-SHA256 per guess, with no victim interaction, no rate limit and no lockout. Infeasible against `McpSettings.generateToken()`'s 32 random bytes (43 Base64URL characters); a short operator-typed token is recoverable in seconds. Mitigated by `Defaults.MCP_MIN_TOKEN_LENGTH` (32) and `McpSettings.isTokenWeak`, surfaced as a RISK item in the MCP Server tab's advisory whenever MCP is enabled with a non-blank but weak token — in local mode as well as external, because the takeover path runs in both. The control is **advisory only**: it does not block saving, does not refuse to start the server, and never rewrites the operator's token. - **The pin's loopback scope is a limit, not an oversight, and it is recorded in ADR-17.** `openConnection`'s TLS branch is gated on `isLoopbackUrlHost`, so an external-mode deployment bound to any other host installs no override and cannot take over its own listener. Phase 26 made that case say so out loud instead of reporting "no compatible MCP server", and left the wider change — dropping the host gate — as an accepted residual with its reason under ADR-17. ## ADR-17: QUAL-07's three dispositions — the detekt baseline only shrinks, `ChatPanel`'s EDT check is declared test-only, and `SecretCipher` protects an export rather than a local attacker (QUAL-07) **Context.** QUAL-07 is the phase-26 requirement that the project's static-analysis debt, its remaining unenforced invariants and its written security claims are brought into agreement with what ships. Three of its items were decisions rather than edits, and each had already been made once implicitly — by a comment, by an omission, or by a README sentence — which is the shape of thing an ADR exists to make explicit. The detekt baseline had been carried unchanged across six phases with no written rule about which direction it is allowed to move. `ChatPanel`'s `assertEdt()` helper had been recorded in ADR-15 as a production no-op with no statement of whether that was accepted or merely observed. And `SecretCipher`'s AES-256-GCM envelope had been described in places as protecting secrets "at rest" without saying what it does not protect against. Cross-references here name **symbols, not line numbers**, per ADR-15's correction; a line number or a commit hash appears only where it pins historical evidence. **Decision.** Three clauses. 1. **`detekt-baseline.xml` shrinks and is never appended to.** A finding introduced by new work is **fixed in source, not baselined**. The baseline is a record of debt that predates this milestone; a diff to it that adds an `` entry inverts what the file is for, because it converts a finding somebody would otherwise have had to answer into a finding nobody will ever see again. The rule is stated here rather than as a number because a number goes stale and a direction does not — a future phase inherits *the baseline may lose entries and may not gain them*, whatever count it starts from. The measured fact behind the rule: the last commit to touch the file is `ab567fb` (2026-07-29), which predates Phase 20's first commit, so Phases 20 through 25 added nothing to it despite six phases of new code. That is checkable in one command rather than argued — `git log --oneline ab567fb..HEAD -- detekt-baseline.xml` returning nothing is the whole evidence, and it becomes the standing check. In the other direction, a baseline entry that no longer corresponds to a live finding is **dead weight and may be pruned freely**: detekt ignores baseline entries that match nothing, so removing a stale one cannot turn the build red and needs no ceremony. Two anti-patterns are ruled out with it, because both produce a smaller number without producing less debt: deactivating a rule, and relaxing a threshold or widening an `excludes` glob in `detekt.yml`. A finding count that falls because the detector stopped looking is not a smaller baseline, it is a smaller detector. 2. **`ChatPanel`'s `assert()`-based EDT enforcement stays an assert and is documented as a test-only mechanism.** The selection, copied verbatim from `26-04-SUMMARY.md` §"SC4 decision": > **`document-test-only`** (Option A — document it as a test-only mechanism). **How that selection was reached is part of the record, because it changes how much weight it carries.** It was **auto-selected by the harness**: the plan made it a `gate="blocking-human"` checkpoint and explicitly forbade the executor from choosing, this project runs `mode: yolo`, and yolo's mechanical rule is to take the first listed option — which Option A was. No human weighed the options *at the checkpoint*. The user was shown the probe evidence afterwards and chose to keep the result, so `document-test-only` is the user's accepted disposition; it is not a judgement taken at the gate, and this ADR does not pretend otherwise. What shipped as a consequence: `assertEdt()` is unchanged, and the helper's KDoc plus all four guarded call sites now state that the check has no effect in a shipped Burp, name `invokeAndWait` marshalling as the actual remedy, and say that a violation must be fixed in the caller. `ChatPanelEdtGuardTest` pins that framing — it turns red if the wording drifts back into claiming field enforcement — so what is enforced is the honesty of the documentation, not the confinement. **The measured evidence argues for the option that was not taken, and recording it is the point of this clause.** Option B (upgrade `assert(` to a throwing `check(`, so the mechanism fires in shipped Burp) was applied as a probe and measured before any option was selected. It broke **exactly one test** out of 880, `ChatPanelEdtConfinementTest.theEdtConfinementAssertionIsByteIdenticalAndStillHasSixMentions`, and that test failed because it pins the helper's source text — not because any thread misbehaved. **Zero tests failed from an off-EDT call.** That result does not rest on the probe alone: `build.gradle.kts` already passes `-ea` to `tasks.test` (its comment says "Enable JVM assertions so EDT assert() fires in CI"), so the assertion is live in every test run and the green baseline suite is itself evidence that nothing reaches those four call sites off the EDT. **The upgrade was therefore behaviourally free against today's suite.** Option C (upgrade to an observable but non-fatal report) was rejected on its merits and independently of the selection: a violation still proceeds into the race it was meant to prevent, which makes it strictly weaker than Option B, and it needs a per-call-site once-only latch or a hot path floods Burp's Errors tab. The one honest argument for the cheaper option is scope, not correctness: adopting Option B requires editing `src/test/kotlin/com/six2dez/burp/aiagent/ui/ChatPanelEdtConfinementTest.kt` to move a byte-identical source pin, and that file appears in **no phase-26 plan's** `files_modified` — while four executors ran concurrently in separate worktrees on the strength of a non-overlap guarantee. That is a planning gap rather than a reason Option B is wrong, and it is recorded as a residual below so a future reversal starts from a known precondition instead of rediscovering it. There is consequently **no CI gate proving the mechanism fires with assertions disabled**, because under this disposition there is no mechanism that fires: `edtGuardWithoutAssertionsTest`, the only `-da` task in this build and the only gate that can tell a `check` from an `assert`, still names `McpToolExecutorEdtGuardTest` alone. A green `-ea` suite could never have stood in for it — under `-ea` the `assert` and the `check` forms behave identically, so the whole class of evidence the `-da` task exists to produce is unavailable by construction. 3. **`SecretCipher`'s at-rest guarantee is protection of a preferences file or export, not protection against a local attacker.** The master key is a per-install random 256-bit key stored **Base64-encoded in a dedicated Burp Preferences entry** (`SecretCipher.MASTER_KEY_PREF_KEY`), beside the ciphertext it protects. The property that follows, and the only one that may be claimed: an API key or bearer token does not appear in cleartext in a Burp preferences file or in a preferences export, and a reader of that material cannot recover a secret without also reading the master-key entry. The property that does **not** follow, and must not be implied anywhere in this repository's documentation: protection against an attacker or a process that can read Burp Preferences. Such a reader holds the key and the ciphertext together and decrypts at will. The design is still the right one for this extension and this is a trade-off rather than an apology — a passphrase-derived key would mean a prompt on every Burp start, would break headless and CI use, and would push most users toward a weak passphrase or toward turning encryption off, all to defend against an attacker who by hypothesis already runs code as the user. The obfuscation is what is worth having; the overstatement is not. ADR-16's filesystem-read residual already depends on exactly this property, and states it the same way: an attacker who can read the TLS keystore also holds the MCP token, because the master key sits beside its ciphertext. **Consequences.** - Clause 2 closes ADR-15's `assertEdt()` residual as a **declared** limit rather than a removed one. ADR-15's bullet is updated to point here; the no-op itself is unchanged, and REL-01 is held by the callers' marshalling discipline, evidenced by `ChatPanelEdtConfinementTest`, not by the helper. - Clause 3 is the same statement ADR-16's filesystem-read residual already relies on, stated once positively rather than only as an aside inside a threat argument. The two must be edited together or one of them becomes the wrong one to read. - Clause 1 turns "the baseline did not grow" from an accident of six phases into a rule with a one-command check, so the next contributor who hits a new finding has a written answer to "may I just baseline this?" — no. - Residual: **external-mode TLS takeover on a non-loopback host is still impossible, and this phase only made the diagnostic honest.** `McpSupervisor.openConnection`'s certificate pin remains gated on `isLoopbackUrlHost`, so a deployment bound to any other host installs no TLS override, the JDK default trust store refuses this extension's own `CN=burp-mcp` certificate, and the takeover does not happen. External mode is the only mode that permits a non-loopback host, so the uncovered configuration is exactly the intended external deployment. What phase 26 changed is that the operator is now told the real reason and given the manual remedy, instead of being told no compatible MCP server was found when the listener was their own. The **wider remedy** 25-REVIEW WR-03 proposed — dropping `isLoopbackUrlHost` from the TLS condition, which would make external-mode TLS takeover work for the first time while keeping the fail-closed rule intact — is **accepted as out of scope and carried to the backlog**, deliberately and not by omission. The reason is proportionality, not disagreement: it changes when this extension will shut down a listener on a non-loopback host, on a path that had zero test coverage before this phase, inside a phase scoped to coverage, static-analysis debt and documentation. It should be taken in a hardening phase that can carry its own threat model. - Residual: **`ChatPanel.cancelInFlightRequest` is reachable from `shutdown()`, which Burp's unload handler calls off the EDT.** It marshals correctly today and the phase-26 tests confirm that, but this is the risk the Option-B probe could **not** retire and the reason a human should still weigh clause 2. The probe proves nothing breaks *now*; it cannot prove that a future caller added to that path will marshal. Under the selected disposition such a caller produces a silent data race rather than a thrown exception, and the `assert` will not report it in a shipped Burp. This residual is recorded here rather than only in a plan SUMMARY on purpose — a residual that lives in `.planning/` does not ship, which is the failure mode ADR-16's residual discipline exists to prevent. - Residual: **reversing clause 2 has a concrete, unmet precondition.** `src/test/kotlin/com/six2dez/burp/aiagent/ui/ChatPanelEdtConfinementTest.kt` must be added to the scope of whichever plan adopts the upgrade, because the byte-identical source pin and its mention counter both live there. That file's own KDoc already names QUAL-07 as the owner of the upgrade — the planner intended the change and simply did not put the file in any plan's `files_modified`. `26-04-SUMMARY.md` §"How to reverse this choice without re-running the probe" carries the five mechanical steps; no probe needs re-running. - **Deliberate omission:** this ADR does not restate the operator procedure for a bind conflict. That is `docs/mcp-hardening.md`'s job and ADR-16's closing pointer already carries the division; duplicating it is how the two copies start disagreeing.