Security fixes for 1.1.1 - #9
Merged
Merged
Conversation
Remediates the blocker set from the 2026-07-24 scan (.mwpdev/reviews/security-scan-pre-1.1.0-publish_2026-07-24.md). Tag v1.1.0 stays unpublished; these fixes ship as 1.1.1. F13, async-schema validation bypass: sanitize-schema now strips $async, so a hostile inputSchema can no longer make ajv compile a Promise-returning validator whose truthy result reads as "valid" and whose rejection crashes the process. schema-validator additionally fails closed on any non-boolean result, covering future async keywords the stripper does not know about. Terminal-escape cluster (F2/F5/F7/F17/F20/F22): sanitize in the output layer rather than per call site. formatHeading, formatSuccess and formatInfo collapse to a single sanitized row; formatKeyValue now collapses its value too, which live-verify caught still leaking a lone CR through safeString. Ability descriptions and instructions go through a new sanitizeMultiLine that strips escapes but keeps real newlines, so multi-paragraph text still renders. Doctor's verbose details path collapses CR per line, matching its message path. Userinfo cluster (F1/F3/F4/F23): profile use and profile list mask embedded credentials in both the JSON envelope and the human table. Debug context is masked centrally in redactDebugValue, before truncation, so no debug value can carry a credentialed URL to stderr. maskUrlUserinfoInText now delegates each URL to maskUrlUserinfo and inherits its fail-closed behavior, closing the case where a tab or newline in the userinfo defeated the old single regex. ReDoS trio (F6/F11/F19): the OSC/DCS/APC/PM/SOS bodies use negated classes that fail linearly instead of lazy bodies that rescan to end-of-input from every start. Error messages are length-capped before the credential scan, and the host class no longer overlaps the optional password group. The tool-envelope brace scan is bounded by both length and a step budget, and streamed LLM content is capped at the point of accumulation. F8/F9/F15/F21, env-var identity binding: BREAKING. MAINWP_APP_PASSWORD is now released only when MAINWP_DASHBOARD_URL names the same Dashboard the profile points at. This closes the case where a profiles.json an attacker can write redirects the credential to their host, which mattered most in CI, where that env var is the documented credential path and the profile file may be shared. login is unaffected (it takes --url), and display-only paths such as doctor and config show pass no expected URL and still read the credential. Docs and the test harnesses declare the URL the way an operator now must. F14, password echo: promptForPassword no longer creates a terminal-mode readline interface. It only ever closed it, never read from it, and the interface made the terminal echo the typed password. Verified on a real pty: the old code printed "Password: hunter2SECRET", the new code prints only asterisks. Reviewers should look hardest at the env-var binding, since it is the one behavior change users will notice, and at the maskUrlUserinfoInText rewrite, where the candidate-matching regex has to admit the control characters the WHATWG parser strips without swallowing surrounding text. Verified: typecheck, lint, npm test (980), npm run test:process (106), build, git diff --check. Live-verified each cluster against a mock Dashboard serving hostile ability metadata and a profile carrying embedded credentials.
Two defects found by exercising the new code directly rather than through the unit suite, which passed with both present. maskUrlUserinfo now returns an already-masked URL unchanged. Delegating each candidate to it made maskUrlUserinfoInText non-idempotent: re-masking `https://***:***@host` produces a byte-identical string, which the fail-closed check reads as "credentials the regex could not isolate" and replaces with [URL_WITH_CREDENTIALS_REDACTED]. That path is now reachable because the debug redactor masks centrally, so a value can arrive here twice. It failed safe but destroyed the diagnostic. Nothing leaks either way: the userinfo is literally `***`. The streamed-content cap no longer splits a surrogate pair. Slicing at 1MB can cut between the halves of an astral character and leave a lone surrogate in the accumulated response.
Four of six findings accepted. Three were regressions this branch introduced, and the suite passed with all of them present. format.ts, credential leak (High): tokenizing URL candidates on whitespace swallowed closing delimiters and following URLs, so `<https://u:p@host>`, `[https://u:p@host]`, and comma-adjacent URLs passed through unmasked. All were masked before this branch. Restored the original whitespace-and-/?#-bounded pattern as the primary pass, which stops at a delimiter or the next scheme, and narrowed the fail-closed sweep to candidates that actually carry tab/CR/LF, which is the only case the WHATWG parser sees and that pattern cannot. error-sanitizer.ts, credential leak (High): the new 16KB cap truncated before redaction, so a URL whose `@` fell past the boundary no longer matched the credential pattern and its userinfo was emitted verbatim. Probed at four offsets, all leaked. Truncation now ends on a whitespace boundary, so every token that survives is whole; a single token longer than the cap is dropped rather than half-emitted. jobs/watch.ts (Medium): Dashboard-controlled batch result labels render through safeString(), which preserves CR/LF/tab, so a result named "site\n✓ Job completed" forges a status line. Same class as the formatKeyValue leak found during live-verify, and the same fix. login.ts (High, partially accepted): login reads MAINWP_APP_PASSWORD directly, outside the new binding. It takes its destination from --url and never from profiles.json, so the tampered-file attack the binding exists to stop does not reach it, and requiring the operator to repeat the URL would break the documented non-interactive flow. But a declared destination should still be honoured, so login now refuses when MAINWP_DASHBOARD_URL disagrees with --url while allowing it to be absent. The binding check moved to a shared exported function so both callers use one implementation. Findings 4 and 5 (unbounded provider tool-argument accumulation, unbounded non-streaming response body) are pushed back with reasoning in .mwpdev/reviews/REVIEW_DECISIONS.md: both are pre-existing provider-boundary surfaces, F18 is named out of scope by the 1.1.1 handoff, and both need the same budget-threading change that does not belong in this PR. Verified: typecheck, lint, 986 unit tests, 106 process tests, build, and each of Codex's probes re-run against the fixes. Live-verified the binding still refuses on the authenticated path and that login accepts an absent declaration but refuses a mismatched one.
Both reviewers landed on the same conclusion about maskUrlUserinfoInText, from different angles, so it is now a linear scanner instead of a fourth regex. The masking hole: `https:\n//user:pass@host` and `https:user:pass@host` are both credentialed URLs to the WHATWG parser, which discards tab/CR/LF anywhere (including inside `://`) and gives special schemes an authority without `//`. Neither pattern could see either form, so the credential printed in cleartext. Codex also measured the surrounding pattern as quadratic, 947ms at 32k characters and no completion at 2M, which matters because this now runs on every debug value. The scanner walks a copy with those characters removed, maps offsets back so only the matching span is rewritten, and resumes past the last `@` in each authority so adjacent URLs are still found. 2M characters complete in ~66ms. Brackets, adjacency, `@` in passwords, idempotency and mailto are pinned by tests. Truncation could still expose a credential: tab, CR and LF are not safe token boundaries, because the parser ignores them. Cutting on the newline inside `https://user:secret\n...@host` kept `https://user:secret`, which no longer matched the credential pattern. Boundaries now exclude those three characters. login now requires MAINWP_DASHBOARD_URL, reversing the exemption from round 1. The argument that `--url` already names the destination does not survive the CI case Codex put: the password is a protected secret, command arguments usually are not, so anyone who can edit the pipeline can redirect it without touching the secret. Binding is worth having only if nothing skips it, so the requireDeclaration escape hatch is gone. Docs and the process harness updated; the harness now resolves login's URL from --url rather than the active profile. Also fixed: an async validator's promise is adopted before the schema guard throws, so its rejection cannot become the unhandled crash that guard exists to prevent; the stream cap uses >= so a surrogate pair landing exactly on the boundary is trimmed, and truncated content reports finishReason 'length' rather than passing as a complete answer; sanitizeMultiLine collapses tabs, which a hostile description could use to fake columns; and a malformed profile URL fails closed as an AuthError instead of a raw TypeError. Two CodeRabbit findings pushed back in REVIEW_DECISIONS.md: the adjacent-URL finding describes the superseded implementation it saw in the branch diff, and the request for hostile-URL cases in the profile command tests duplicates coverage that belongs to the masker. One matched the existing deferral for provider tool-argument accumulation. Verified: typecheck, lint, 987 unit tests, 106 process tests, build, git diff --check, every probe from both reviewers re-run against the fixes, and live-verified that login refuses an undeclared or mismatched destination and succeeds on a matching one.
…URL var) Making MAINWP_DASHBOARD_URL mandatory broke every documented GitHub Actions workflow. The guides already store the Dashboard URL as secrets.DASHBOARD_URL, but nothing exposed it under the name the CLI now requires, so those pipelines would have failed at their first authenticated command. CodeRabbit caught the mapping; auditing the rest of the docs for the same omission found four more places. Both env: blocks in monthly-batch-updates.md and both in plugin-deployment-verification.md now set MAINWP_DASHBOARD_URL from the existing secret, so the "three secrets" setup steps stay correct, and the env: explanation in each says why it is needed. getting-started.md sets both variables in its shell and PowerShell examples, and acceptance-testing.md records that the runner supplies it. One pushback in REVIEW_DECISIONS.md: the request to drop `export MAINWP_APP_PASSWORD='xxxx ...'` from the examples. The value is a placeholder, the reader needs to see how the variable is set, and interactive prompting, keychain storage, and the --password process-list warning already cover the concern where it belongs. Verified: typecheck, lint, 987 tests, git diff --check.
The scanner from round 2 had two credential leaks and had reintroduced the
quadratic behaviour it was written to remove. Replaced its per-colon loop with a
single forward pass that carries authority state.
Leaks: `https:/u:p@h/x` passed through unchanged, because the scanner accepted
only exactly `//` or no slashes at all, while the parser tolerates any run of
slashes after a special scheme. And in `https://safe,https://u:p@h/x` the second
URL escaped entirely: the first authority ran through `,https:` to the slash,
found no `@`, and the scan resumed past the second scheme's colon. A safe URL
sitting next to a credentialed one hid it.
Complexity: `scan.lastIndexOf('@', end - 1)` searched backward from every
authority to a distant `@`, which is quadratic when many short authorities
follow one. Measured 130ms, 512ms and 2034ms over 8k/16k/32k authorities. The
forward pass tracks the last `@` as it goes, so 200k now takes 55ms.
The same pass fixes two false positives: a backslash ends the authority for
special schemes, so the `@` in `https://h\path@x` belongs to the path, and
`file:` is no longer treated as credential-bearing, so `file:u:p@h/x` is left
as the local path it is.
Elsewhere: the stream cap tracks whether it has been reached rather than
inferring it from length, because trimming an orphaned high surrogate drops
back under the cap and the next chunk was then appending its unpaired low half;
the schema guard now contains a hostile thenable whose `then` throws on access
or hands back a rejected promise, neither of which may replace the schema
error; and error truncation scans backward for its boundary instead of using a
trailing-anchored pattern, which discarded an entire message when the tail was a
long run of tabs.
Round 3 traced the credential binding end to end and found no bug there.
One finding pushed back in REVIEW_DECISIONS.md: protocol-relative references.
There is no base to resolve against in error text, nothing here produces
scheme-less URLs, and matching bare `//` would rewrite commented-out code.
Verified: typecheck, lint, 993 unit tests, 106 process tests, build,
git diff --check, and every probe from the round re-run against the fixes,
including the exact chunk sequence for the surrogate case.
Found while probing the forward-pass scanner: an authority ended only at
`/ ? # \` or whitespace, so in `{"url":"https://h.test","user":"a@b"}` it ran
from the URL through the rest of the object to the address's `@` and rewrote
everything between them as userinfo. Dashboard error bodies are JSON, so this
corrupted exactly the diagnostics the masker is supposed to leave readable.
Authorities now also end at the characters a URI cannot contain unencoded:
`" < > ` { } | ^`. Sub-delimiters are deliberately not in that set. They are
legal in userinfo, so ending an authority on one would cut `pa,ss@host` short
of its `@` and turn a display bug into a credential leak. Both directions are
pinned by tests.
Verified: typecheck, lint, 995 tests, plus the full probe set (15 credential
forms masked or failed closed, 11 non-credential inputs unchanged, idempotent,
linear at 1M-character scale).
Three more defects in the forward-pass scanner, all in how an authority opens and how much text a match replaces. `http:` appearing inside a password was read as a new URL starting, which closed the authority it belonged to: `https://u:http:p@h/x` masked only the tail and left `u:http:` in the output. A special scheme with no slashes now opens an authority only when none is already open, so a scheme-like substring inside userinfo no longer splits the URL it is part of. Any scheme followed by a single slash was treated as having an authority, but the parser gives that behaviour only to special schemes. `custom:/u:p@h/x` and `file:/u:p@h/x` are paths and were being rewritten. Non-special schemes now need a real `//`. Replacement started at the scheme, so a span whose offsets had shifted took the text in front of it too: `PRE\nhttps://u:p@h/x` lost `PRE` along with the credential. Only the userinfo is replaced now, and "obscured" is judged on that region alone. Obscured cases still fail closed but keep their surroundings and host, which is what the function always claimed to do: `x https://u:se\ncret@h/x y` now yields `x https://[URL_WITH_CREDENTIALS_REDACTED]h/x y`. The fourth finding, an authority running through JSON to a later `@`, was already fixed in 94ad3f5 while this round was in flight. Round 4 found no bug in truncateAtTokenBoundary, the stream cap, or the schema guard, and measured the scanner linear across four worst-case shapes up to two million characters. Verified: typecheck, lint, 999 unit tests, 106 process tests, build, git diff --check, and the full probe set re-run (15 credential forms masked or failed closed, 11 non-credential inputs unchanged, idempotent, linear).
Found while probing span edges: `https://@host` and forms the parser empties by dropping control characters were rewritten to `***:***@`, which claims a credential had been there when the parser reports none. Emitting a span now requires the userinfo to be non-empty. A bare username is still a credential and is still masked.
Round 5 found three parser mismatches, one of them a leak introduced by my own
previous fix. The function now asks the URL parser what is credentialed instead
of deciding from a character table.
The leak: `" < > ` { } | ^` were added as authority terminators to stop a URL
running through surrounding JSON. RFC 3986 forbids those characters, which is
true and irrelevant — the parser that actually runs follows WHATWG, which
percent-encodes them inside userinfo. So `https://user:pa"ss@host` really does
carry a password, and ending the authority at the quote walked past its `@` and
printed the credential in full. Eight characters, eight leaks.
Second, a scheme longer than the 32-character backward walk went unrecognised
whenever the character at the boundary was a digit, so `a1111…://u:p@h` was
never masked. The scan now tracks the start of the current scheme-legal run as
it moves forward, which recognises a scheme of any length in constant time and
removes the limit rather than raising it.
Third, backslashes counted as slashes for every scheme, so `custom:\\u:p@h`
was rewritten although it is a path. Only special schemes treat them that way.
Deciding by parser needed two guards to stay useful: the candidate ends where
the host ends, so a trailing `>`, `]` or `,` is not handed to the parser as part
of the host (it rejects those outright, which would have lost the credential),
and the parsed host must look like a host, which keeps prose, email addresses,
git remotes and markdown links untouched.
One case is deliberately over-masked: a URL inside a JSON error body parses as
userinfo plus host `b`, textually identical to a password containing a quote.
Both readings cannot hold, so it errs toward masking — reasoning in
REVIEW_DECISIONS.md under url-masking-errs-toward-over-redaction-in-structured-text.
Verified: typecheck, lint, 1003 unit tests, 106 process tests, build,
git diff --check, and the probe sets re-run — 12 previously leaking or
false-positive cases now correct, every earlier regression case still holding,
idempotent, and linear at million-character scale.
Found while probing the two guards added last round: the host scan accepted only ASCII, so `https://u:p@пример.example.com/x` truncated its candidate to `https://u:p@`, which does not parse, and the credential was left visible. The parser punycodes internationalized hosts rather than rejecting them, so they are ordinary credentialed URLs and were the one host shape the new parser-delegating design still got wrong. The host test is now a predicate rather than a character class: anything above ASCII counts, and expressing the ASCII half numerically also drops a no-control-regex lint error that the range form introduced. Ports, IPv6 literals with and without ports, percent-encoded hosts, trailing-dot hosts, underscore hosts and long multi-label hosts were all checked in the same pass and were already correct; tests pin the IDN, port and IPv6 cases. Verified: typecheck, lint (0 errors), 1005 unit tests, 106 process tests, build, git diff --check, and all four probe sets.
Twenty-eight inputs where the parser reported a credential and the output still
showed it. All of them traced to the same root cause: the host extent was being
decided by a character table narrower than what the parser accepts. That table
is gone.
Hosts can begin with characters the table rejected — `!example`, `,example`,
`{example` — and can be percent-encoded forms that decode to them, `%21example`
and `%2Cexample`. Rather than widening the table again, the candidate is now
offered to the parser at two extents: the conservative host, which stops at the
first character that certainly is not one and keeps `one.t,https://…` as two
URLs, and the structural end, which runs to the next `/?#` or space and accepts
everything else. Either verdict is safe, because only the userinfo is rewritten,
so the extent affects the decision and never the output. The plausibility guard
that used to sit on the parsed host is gone with the table.
An authority closed by the start of the next URL ended before its own host, so
`https://a:b@onehttps://safe/x` handed the parser a candidate with nothing to
validate. The host extent is now measured from the `@`, independent of where the
authority closed.
A space inside userinfo is the one case the scan cannot see, because in free text
a space almost always ends a URL and treating it otherwise would swallow prose
into every authority. WordPress Application Passwords contain spaces, and a
stored dashboardUrl reaching the debug redactor is exactly that shape, so a
whole-value fallback now runs when the scan finds nothing: if the entire value
parses as one credentialed URL it is masked as one. It runs only as a fallback,
so text holding several URLs still goes through the scan and every one is
masked, not just the first.
The fifth finding is pushed back in REVIEW_DECISIONS.md: a dropped character
immediately before the userinfo does not shift anything inside it, so masking in
place is precise and the placeholder would lose the scheme and host for no gain.
Verified: typecheck, lint (0 errors), 1008 unit tests, 106 process tests, build,
git diff --check, and all five probe sets — no leaks, free-text collateral
unchanged, idempotent, linear at million-character scale.
Round 11 review: the OpenAI-compatible stream keyed partial calls by index and yielded nothing until the finish event, so the chat engine's cap could not engage while a hostile endpoint opened fresh indices; the overflow guard also sat after the finish block, so a final chunk carrying the overflowing delta emitted the truncated call instead of throwing. maskUrlCredentials classified the raw parameter key, so api%5Fkey passed through unmasked. Two further findings (truncation splitting a spaced Application Password, soft wrap escaping the untrusted-block prefix) are deferred to the structured-field masking branch, documented in .mwpdev/reviews/REVIEW_DECISIONS.md.
CodeRabbit caught the encoded-key gap in the sibling path: maskUrlCredentials decodes a parameter key before classifying it, sanitizeErrorMessage did not, so an error echoing https://host/wp?api%5Fkey=SECRET printed the secret in full while the same URL masked correctly on the display path. Both now share isSensitiveParameterKey; the character classes stay separate because one scans free prose and the other a whole URL. Also asserts the F19 bounded-scan test returns the payload whole, so a scan bound cannot be met by dropping content.
Sprint 1.1.1-R items 1-2 and Part C: the breaking MAINWP_APP_PASSWORD/MAINWP_DASHBOARD_URL binding note and the security fixes land in the Unreleased changelog section, and provider.ts now states why the three streamed-tool-call bounds (8/2/1) are intentionally distinct rather than derived from each other.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (9)
WalkthroughThe pull request binds environment passwords to dashboard identities, rejects and masks credential-bearing URLs, bounds streamed chat and JSON processing, sanitizes terminal output, hardens schema validation, and updates related tests and documentation. ChangesSecurity hardening
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
GHSA-mh99-v99m-4gvg (high, DoS via unbounded expansion) covers every brace-expansion release below 5.0.8 and has no 1.x/2.x backport, so the CI audit step fails with no fix path through normal resolution. An override moves filelist's minimatch from 5.x to 10.x, whose brace-expansion range reaches 5.0.8; the only API filelist uses, minimatch.match, behaves the same in 10.x (probed before committing). A global brace-expansion override would not work: 5.x exports a named expand instead of a callable module, which breaks minimatch 3/5/9. That is also why eslint and typescript-eslint keep vulnerable 1.x/2.x copies in the dev tree; they parse local config globs only, and CI audits the production tree.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/chat/chat-engine.ts (1)
830-868: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDisplay callback stays unbounded even after the content cap is hit.
onStreamChunk(chunk.content)fires for every chunk regardless ofcapReached/contentTruncated, so a hostile provider that keeps streaming content forever can still flood the terminal/consumer indefinitely even though the accumulatedcontentused for parsing is capped at 1 MB. The comment documents this as intentional, but given the PR's explicit threat model (a hostile SSE endpoint), consider also bounding what's forwarded to the display callback (e.g., stop emitting oncecapReachedis true, or truncate the forwarded chunk to the remaining budget).💡 Possible approach
// Call callback for progressive display - if (this.onStreamChunk) { + if (this.onStreamChunk && !capReached) { this.onStreamChunk(chunk.content); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/chat/chat-engine.ts` around lines 830 - 868, Bound the display output in the content-chunk handling of the stream loop so onStreamChunk no longer receives unbounded data after MAX_STREAM_CONTENT_LENGTH is reached. Update the onStreamChunk invocation to stop forwarding chunks once capReached is true, or forward only the portion within the remaining content budget while preserving the existing capped accumulation and truncation state.src/utils/format.test.ts (1)
659-682: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWall-clock budgets will flake on loaded CI runners.
These assert real linearity guarantees, so keep them — but 500 ms/1 s absolutes are tight for a shared runner. Consider deriving the budget from a baseline measurement (e.g. time a 1/10-size input and assert the full run is within a small multiple), which keeps the quadratic-regression signal without the machine-speed dependency.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/format.test.ts` around lines 659 - 682, Replace the fixed wall-clock thresholds in the two performance tests around maskUrlUserinfoInText with baseline-relative assertions: measure a smaller representative input, then require the full input to complete within a modest multiple of that baseline. Preserve both hostile-input cases and their linearity/regression coverage while avoiding machine-speed-dependent absolute budgets.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/configuration.md`:
- Line 31: Update the documentation statement about interactive login to match
the behavior in login.ts: login uses MAINWP_APP_PASSWORD when no --password flag
is provided, including in a TTY, and therefore applies the MAINWP_DASHBOARD_URL
binding requirement. Keep the unaffected descriptions of doctor and config show
accurate.
In `@src/lib/base-command.ts`:
- Around line 339-344: The debug-context redaction in maskUrlUserinfoInText
currently handles only URL userinfo; extend it to apply the same
sensitive-parameter classification used by maskUrlCredentials to query and
fragment parameters before truncation. Update src/lib/base-command.ts lines
339-344 accordingly, and add regressions in src/lib/base-command.test.ts lines
70-106 covering both regular and encoded sensitive parameter keys; both sites
require changes.
In `@src/utils/format.ts`:
- Line 164: Update the URL parameter regex in format.ts and the corresponding
regex in error-sanitizer.ts to remove or sufficiently increase the 64-character
key limit while retaining the existing delimiter, whitespace, and equals
exclusions. Ensure long parameter names still reach isSensitiveKey
classification and are not emitted verbatim.
---
Nitpick comments:
In `@src/chat/chat-engine.ts`:
- Around line 830-868: Bound the display output in the content-chunk handling of
the stream loop so onStreamChunk no longer receives unbounded data after
MAX_STREAM_CONTENT_LENGTH is reached. Update the onStreamChunk invocation to
stop forwarding chunks once capReached is true, or forward only the portion
within the remaining content budget while preserving the existing capped
accumulation and truncation state.
In `@src/utils/format.test.ts`:
- Around line 659-682: Replace the fixed wall-clock thresholds in the two
performance tests around maskUrlUserinfoInText with baseline-relative
assertions: measure a smaller representative input, then require the full input
to complete within a modest multiple of that baseline. Preserve both
hostile-input cases and their linearity/regression coverage while avoiding
machine-speed-dependent absolute budgets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b506413-f978-4472-9a77-55723b29ec4d
📒 Files selected for processing (55)
CHANGELOG.mdREADME.mddocs/acceptance-testing.mddocs/cli-reference.mddocs/configuration.mddocs/getting-started.mddocs/troubleshooting.mddocs/workflows/daily-health-check.mddocs/workflows/input-from-file.mddocs/workflows/monitoring-integration.mddocs/workflows/monthly-batch-updates.mddocs/workflows/plugin-deployment-verification.mdsrc/__tests__/process/abilities-info.test.tssrc/__tests__/process/fixtures/cli-runner.tssrc/__tests__/process/profile.test.tssrc/chat/chat-engine.test.tssrc/chat/chat-engine.tssrc/chat/providers/anthropic.tssrc/chat/providers/openai-compatible.tssrc/chat/providers/provider.tssrc/chat/providers/streamed-tool-arguments.test.tssrc/chat/tool-envelope.test.tssrc/chat/tool-envelope.tssrc/commands/abilities/info.tssrc/commands/config/show.tssrc/commands/doctor.tssrc/commands/jobs/watch.test.tssrc/commands/jobs/watch.tssrc/commands/login.tssrc/commands/profile/list.tssrc/commands/profile/profile-mask.test.tssrc/commands/profile/use.tssrc/config/keychain.test.tssrc/config/keychain.tssrc/config/profile-store.test.tssrc/config/profile-store.tssrc/lib/base-command.test.tssrc/lib/base-command.tssrc/output/formatter.test.tssrc/output/formatter.tssrc/output/json-envelope.test.tssrc/utils/error-sanitizer.test.tssrc/utils/error-sanitizer.tssrc/utils/format.test.tssrc/utils/format.tssrc/utils/prompt.tssrc/utils/terminal-sanitizer.test.tssrc/utils/terminal-sanitizer.tssrc/validation/input-sanitizer.test.tssrc/validation/sanitize-schema.test.tssrc/validation/sanitize-schema.tssrc/validation/schema-validator.test.tssrc/validation/schema-validator.tstests/acceptance/agent-run.tstests/acceptance/lib/cli.ts
CodeRabbit's PR review caught the debug-context redactor and doctor's fetch-error path masking only user:pass@ userinfo, so a legacy profile URL carrying ?access_token= reached stderr in full under --debug and appeared in doctor's connection details. Both now go through maskUrlCredentialsInText, the free-text counterpart of maskUrlCredentials: userinfo first, then sensitive parameter values. The parameter regexes in format.ts and error-sanitizer.ts also drop their 64-character key bound, which failed open: a longer key could not match, so its value printed verbatim. The negated character class is linear with or without the bound. Also corrects the configuration doc and README claim that interactive login ignores MAINWP_APP_PASSWORD. It uses the env var when set, with the same MAINWP_DASHBOARD_URL binding as every other command.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Fixes the gating findings from the 2026-07-24 pre-publish security scan, plus everything the review rounds surfaced on top of them.
src/utils/format.ts):maskUrlUserinfoInTextis now a linear scanner that delegates the credential decision to the WHATWG parser instead of character heuristics. Covers control-character glue, digit-led schemes, spaced Application Passwords, IDN hosts, and unparseable authorities (fails closed). Verified by a seeded differential fuzzer with four properties (no-leak, idempotency, reconstruction, no-crash); fuzzer findings are pinned as named regression tests.src/chat/chat-engine.ts): streamed text content is capped at 1MB; overflow surfaces as an error event instead of unbounded accumulation.any.Review history
error-sanitizer.tsand the soft-wrap frame escape informatUntrustedBlock. Both are design-change work for a follow-up branch, not ordering patches.Verification
npm run typecheck,npm run lint,npm test,npm run test:process,npm run buildall green.What to look at closely
src/utils/format.tsis the highest-risk file: nine case-verified implementations of the masker each still leaked before the fuzzer-verified rewrite. The accepted trade-off throughout is over-redaction in ambiguous text rather than a credential in a log.No version bump in this PR; 1.1.1 changelog entries are staged under Unreleased.
Summary by CodeRabbit
Security
Bug Fixes
Documentation
MAINWP_APP_PASSWORDandMAINWP_DASHBOARD_URLfor env-based auth.