fix keytar interop, security hardening, refactoring#1
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded LICENSE and Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/workflows/input-from-file.md (1)
87-98:⚠️ Potential issue | 🟠 MajorFix
npxinvocations to target the scoped package.The no-install guidance still references the old unscoped package name.
npx mainwpctlresolves the wrong package; scoped packages requirenpx --package@mainwp/controlmainwpctl. Readers following Option B will hit a not-found error or the wrong package.♻️ Suggested doc fix
-npx mainwpctl --version +npx --package `@mainwp/control` mainwpctl --version-`npx` downloads and runs the package temporarily. Every example in this guide uses `mainwpctl` directly. If you chose Option B, replace `mainwpctl` with `npx mainwpctl` in every command. +`npx` downloads and runs the package temporarily. Every example in this guide uses `mainwpctl` directly. If you chose Option B, replace `mainwpctl` with `npx --package `@mainwp/control` mainwpctl` in every command.Also update these files with the same fix:
- docs/workflows/monthly-batch-updates.md (line 68)
- docs/workflows/plugin-deployment-verification.md (lines 75, 78)
- docs/workflows/monitoring-integration.md (line 71)
- docs/workflows/daily-health-check.md (lines 70, 81)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/workflows/input-from-file.md` around lines 87 - 98, Replace all unscoped npx invocations that use "npx mainwpctl" with the scoped package invocation using the `@mainwp/control` package (i.e., use npx --package `@mainwp/control` mainwpctl or include a version if desired) so npx resolves the correct package; update the occurrences in docs/workflows/input-from-file.md (the example command), and make the same replacement in docs/workflows/monthly-batch-updates.md, docs/workflows/plugin-deployment-verification.md, docs/workflows/monitoring-integration.md, and docs/workflows/daily-health-check.md where "npx mainwpctl" is referenced.src/core/http-client.ts (1)
477-485:⚠️ Potential issue | 🟠 MajorExact-key redaction still misses common secret field variants.
The equality check catches
appPasswordandapiKey, but it still leaves composites likeprivateKey,signingKey,accessToken,refreshToken, and header-style keys such asset-cookieuntouched. On the HTTP error path, that leaves credential-leakage gaps.🔒 Suggested hardening
- const sensitiveFields = [ - 'password', 'token', 'secret', 'authorization', 'cookie', - 'apppassword', 'apikey', 'api_key', 'app_password', 'bearer', - 'credential', 'private_key', 'signing_key', - ]; + const sensitivePatterns = [ + /password/, + /token/, + /secret/, + /authorization/, + /cookie/, + /apikey/, + /bearer/, + /credential/, + /privatekey/, + /signingkey/, + ]; for (const [key, value] of Object.entries(data)) { - if (sensitiveFields.includes(key.toLowerCase())) { + const normalizedKey = key.toLowerCase().replace(/[^a-z0-9]/g, ''); + if (sensitivePatterns.some(pattern => pattern.test(normalizedKey))) { sanitized[key] = '[REDACTED]'; } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/http-client.ts` around lines 477 - 485, The current redaction loop (sensitiveFields array and the for (const [key, value] of Object.entries(data)) block that sets sanitized[key] = '[REDACTED]') only matches exact keys and misses common variants like privateKey, signingKey, accessToken, refreshToken and header-style keys like set-cookie; change the logic to match key.toLowerCase() against a set of patterns instead of exact equality: replace sensitiveFields with a list of case-insensitive regexes or substrings (e.g., /\b(token|refresh|access)\b/, /key$/, /secret/, /^set-?cookie$/, /authorization/, /api[-_]?key/, /password/) and in the loop use something like const keyLower = key.toLowerCase(); if (sensitivePatterns.some(p => (typeof p === 'string' ? keyLower.includes(p) : p.test(keyLower)))) { sanitized[key] = '[REDACTED]'; } to ensure composite and header variants are caught.
🧹 Nitpick comments (2)
src/commands/jobs/watch.ts (1)
322-327: Consider sanitizing error messages and items for consistency.Result items at lines 305-308 are sanitized with
safeString, but error messages (error.message) and error items (error.item) are rendered without sanitization. For consistency in the security hardening approach, consider applyingsafeStringto these as well.♻️ Suggested fix
for (const error of status.errors) { const prefix = error.code ? `[${error.code}] ` : ''; - lines.push(formatWarning(` ${prefix}${error.message}`)); + lines.push(formatWarning(` ${prefix}${safeString(error.message)}`)); if (error.item) { - lines.push(` Item: ${JSON.stringify(error.item)}`); + lines.push(` Item: ${safeString(error.item)}`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/jobs/watch.ts` around lines 322 - 327, In the loop over status.errors (the block that builds lines with formatWarning and Item details), sanitize error.message and error.item just like the result items: wrap the message with safeString before passing to formatWarning (e.g., formatWarning(` ${prefix}${safeString(error.message)}`)) and when error.item exists stringify and then sanitize it (e.g., safeString(JSON.stringify(error.item))) before pushing; keep using prefix and formatWarning as-is but ensure all rendered error text passes through safeString for consistency and security.src/config/keychain.ts (1)
69-75: The fallback branch is unlikely to execute in practice for the actualkeytarpackage.The web search confirms that
keytar(a CommonJS module) exports functions directly on the namespace when imported as ESM on Node 20/22/23, sotypeof mod.setPassword === 'function'is always true. The fallback to(mod as any).defaultis dead code for this package.However, the underlying point is valid from a defensive perspective: if the fallback were to execute (for a different package or export shape), caching
keytarAvailable = truewithout validating the.defaultobject would hide import errors until the first function call. If you want to ensure robustness across unexpected module shapes, add a type guard to the fallback branch before caching.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/keychain.ts` around lines 69 - 75, The current import fallback assigns keytar = (mod as any).default and sets keytarAvailable = true without validating that the .default object actually exposes the expected API; update the fallback in the import block to check that (mod as any).default has the required methods (e.g., typeof default.setPassword === 'function' and/or default.getPassword) before assigning to keytar and setting keytarAvailable = true, and if the guard fails treat it as a failed import (do not set keytarAvailable).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 1-2: Update the .env example wording or add a dotenv loader:
either modify the top lines of .env.example to explicitly state that the CLI
(bin/run.js using oclif's execute) does not auto-load .env files and that users
must export credentials to their shell or CI, or alternatively load dotenv in
the CLI bootstrap (require('dotenv').config()) before calling oclif's execute in
bin/run.js so .env is read automatically; pick one approach and ensure the
message references bin/run.js, oclif's execute, and the .env.example header so
users are not misled.
In `@README.md`:
- Around line 82-95: Update the README guidance around MAINWP_APP_PASSWORD:
remove the recommendation to persist the export line in shell profiles (e.g.,
"~/.bashrc", "~/.zshrc") and replace it with a brief secure-alternative sentence
instructing users to use an OS keychain/secret manager or a
restricted-permission env store (or a temporary session export) for long-term
storage of MAINWP_APP_PASSWORD; ensure the variable name MAINWP_APP_PASSWORD is
referenced so readers know which credential to protect.
- Around line 116-127: Update the README wording to soften the "required
two-step confirmation" claim: change the language around the example commands
using mainwpctl abilities run delete-site-v1 (the preview with --dry-run and the
execute with --confirm/--force) to present the two-step preview-then-execute as
a recommended safe workflow rather than an enforced CLI guarantee (the CLI
cannot verify a prior --dry-run); also mirror this rephrasing in the other
occurrence mentioned (around the second block at lines 237-239) and clarify that
server-side confirmation/enforcement is handled by the Abilities REST API's own
confirm requirement.
In `@src/core/http-client.ts`:
- Around line 429-439: The AbortError handler currently treats every AbortError
as a timeout; update the aborting logic to set explicit reasons on the signals
(e.g., when creating the timeout controller call controller.abort(new
DOMException('TimeoutError', 'TimeoutError'))) and ensure the request code
stores the combined/effective signal reference (e.g., effectiveSignal). Then
change the error handling in the catch path that looks at error.name ===
'AbortError' to inspect effectiveSignal?.reason (or check that it's a
DOMException with name 'TimeoutError') — if reason indicates a timeout return
the existing timeout NetworkError, otherwise return a distinct cancellation
NetworkError (with an appropriate message/hint for user cancellation). Ensure
symbol names referenced: effectiveSignal, controller.abort(new
DOMException(...)), AbortError check, and NetworkError.
---
Outside diff comments:
In `@docs/workflows/input-from-file.md`:
- Around line 87-98: Replace all unscoped npx invocations that use "npx
mainwpctl" with the scoped package invocation using the `@mainwp/control` package
(i.e., use npx --package `@mainwp/control` mainwpctl or include a version if
desired) so npx resolves the correct package; update the occurrences in
docs/workflows/input-from-file.md (the example command), and make the same
replacement in docs/workflows/monthly-batch-updates.md,
docs/workflows/plugin-deployment-verification.md,
docs/workflows/monitoring-integration.md, and
docs/workflows/daily-health-check.md where "npx mainwpctl" is referenced.
In `@src/core/http-client.ts`:
- Around line 477-485: The current redaction loop (sensitiveFields array and the
for (const [key, value] of Object.entries(data)) block that sets sanitized[key]
= '[REDACTED]') only matches exact keys and misses common variants like
privateKey, signingKey, accessToken, refreshToken and header-style keys like
set-cookie; change the logic to match key.toLowerCase() against a set of
patterns instead of exact equality: replace sensitiveFields with a list of
case-insensitive regexes or substrings (e.g., /\b(token|refresh|access)\b/,
/key$/, /secret/, /^set-?cookie$/, /authorization/, /api[-_]?key/, /password/)
and in the loop use something like const keyLower = key.toLowerCase(); if
(sensitivePatterns.some(p => (typeof p === 'string' ? keyLower.includes(p) :
p.test(keyLower)))) { sanitized[key] = '[REDACTED]'; } to ensure composite and
header variants are caught.
---
Nitpick comments:
In `@src/commands/jobs/watch.ts`:
- Around line 322-327: In the loop over status.errors (the block that builds
lines with formatWarning and Item details), sanitize error.message and
error.item just like the result items: wrap the message with safeString before
passing to formatWarning (e.g., formatWarning(`
${prefix}${safeString(error.message)}`)) and when error.item exists stringify
and then sanitize it (e.g., safeString(JSON.stringify(error.item))) before
pushing; keep using prefix and formatWarning as-is but ensure all rendered error
text passes through safeString for consistency and security.
In `@src/config/keychain.ts`:
- Around line 69-75: The current import fallback assigns keytar = (mod as
any).default and sets keytarAvailable = true without validating that the
.default object actually exposes the expected API; update the fallback in the
import block to check that (mod as any).default has the required methods (e.g.,
typeof default.setPassword === 'function' and/or default.getPassword) before
assigning to keytar and setting keytarAvailable = true, and if the guard fails
treat it as a failed import (do not set keytarAvailable).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 99e8030d-0c20-4836-8b3e-1977fbba0d99
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (35)
.env.example.github/workflows/ci.ymlLICENSEREADME.mdbin/_exit.jsbin/dev.jsbin/run.jsdocs/workflows/daily-health-check.mddocs/workflows/input-from-file.mddocs/workflows/monitoring-integration.mddocs/workflows/monthly-batch-updates.mddocs/workflows/plugin-deployment-verification.mdpackage.jsonsrc/__tests__/e2e/login-abilities-flow.test.tssrc/__tests__/process/fixtures/cli-runner.tssrc/chat/chat-engine.tssrc/chat/providers/provider-fetch.tssrc/commands/abilities/run.tssrc/commands/jobs/watch.tssrc/config/keychain.tssrc/core/batch-manager.tssrc/core/http-client.test.tssrc/core/http-client.tssrc/core/safety-controller.test.tssrc/core/safety-controller.tssrc/utils/audit-logger.test.tssrc/utils/audit-logger.tssrc/utils/colors.tssrc/utils/errors.tssrc/utils/exit-codes.tssrc/utils/prompt.tssrc/validation/input-sanitizer.test.tssrc/validation/input-sanitizer.tssrc/validation/schema-validator.test.tssrc/validation/schema-validator.ts
💤 Files with no reviewable changes (4)
- src/utils/exit-codes.ts
- src/utils/audit-logger.ts
- src/utils/errors.ts
- src/validation/input-sanitizer.ts
| # MainWP Control CLI — Environment Variables | ||
| # Copy to .env or export in your shell profile. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether any CLI/bootstrap path auto-loads `.env` files.
rg -n --glob '!docs/**' --glob '!**/dist/**' --glob '!**/node_modules/**' \
'dotenv|dotenv/config|--env-file|loadEnvFile' .Repository: mainwp/mainwp-control
Length of output: 47
🏁 Script executed:
cat -n package.json | head -50Repository: mainwp/mainwp-control
Length of output: 1854
🏁 Script executed:
find . -name "bin" -o -name "cli.js" -o -name "index.js" -type f 2>/dev/null | grep -v node_modules | grep -v dist | head -20Repository: mainwp/mainwp-control
Length of output: 70
🏁 Script executed:
rg -n "process\.env|\.env" --type js --type ts --max-count 5 | head -30Repository: mainwp/mainwp-control
Length of output: 2794
🏁 Script executed:
cat -n bin/run.jsRepository: mainwp/mainwp-control
Length of output: 280
Reword .env.example — the CLI does not auto-load .env files.
Copying to .env will not work; the entrypoint (bin/run.js) uses only oclif's execute function with no dotenv loader. Users following the instruction will populate .env and then encounter missing credentials without understanding why. Either add a dotenv loader to the CLI bootstrap, or reword the template to clarify it's only a reference—credentials must be exported manually or set in CI/shell profiles.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.example around lines 1 - 2, Update the .env example wording or add a
dotenv loader: either modify the top lines of .env.example to explicitly state
that the CLI (bin/run.js using oclif's execute) does not auto-load .env files
and that users must export credentials to their shell or CI, or alternatively
load dotenv in the CLI bootstrap (require('dotenv').config()) before calling
oclif's execute in bin/run.js so .env is read automatically; pick one approach
and ensure the message references bin/run.js, oclif's execute, and the
.env.example header so users are not misled.
There was a problem hiding this comment.
Skipping this one. The .env.example header already says "Copy to .env or export in your shell profile." That is standard convention for env example files. Adding a dotenv loader would mean pulling in a new dependency for something oclif does not use, and the current wording is accurate: the CLI does not auto-load .env files. Users who want that can add dotenv themselves.
There was a problem hiding this comment.
@dennisdornon, that's fair — the "export in your shell profile" phrasing is the standard convention for .env.example files used purely as documentation references, and avoiding a dotenv dependency for a CLI entrypoint that doesn't need it is a reasonable call. I'll note this for future reviews.
✏️ Learnings added
Learnt from: dennisdornon
Repo: mainwp/mainwp-control PR: 1
File: .env.example:1-2
Timestamp: 2026-03-24T00:59:34.367Z
Learning: In mainwp/mainwp-control, the `.env.example` file is intentionally documentation-only. The CLI bootstrap (`bin/run.js`) uses oclif's `execute()` with no dotenv loader, and does not auto-load `.env` files. Users are expected to export variables manually to their shell or CI environment. Adding a dotenv loader is explicitly out of scope. Do not flag the `.env.example` header wording ("Copy to .env or export in your shell profile") as misleading in future reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: phuochoit
Repo: mainwp/ai-bridge PR: 10
File: backends/mintlify/lib/validation.js:17-19
Timestamp: 2026-01-26T16:51:12.280Z
Learning: In the mainwp/ai-bridge repository, the maintainer (phuochoit) prefers NODE_ENV to default to "development" rather than "production" in backends/mintlify/lib/validation.js.
| if (!(error instanceof Error)) { | ||
| return new NetworkError(String(error)); | ||
| } | ||
|
|
||
| // Check for network errors (check both error and cause chain) | ||
| const errorCode = this.extractErrorCode(error); | ||
| if (errorCode) { | ||
| if (errorCode === 'ECONNREFUSED') { | ||
| return new NetworkError( | ||
| 'Connection refused. Is the Dashboard running?', | ||
| undefined, | ||
| 'Verify the Dashboard is running and the URL is correct' | ||
| ); | ||
| } | ||
| if (errorCode === 'ENOTFOUND') { | ||
| return new NetworkError( | ||
| 'Host not found. Check the Dashboard URL.', | ||
| undefined, | ||
| 'Check the Dashboard URL in your profile with `mainwpctl config show`' | ||
| ); | ||
| } | ||
| if (errorCode === 'CERT_HAS_EXPIRED' || errorCode === 'UNABLE_TO_VERIFY_LEAF_SIGNATURE') { | ||
| return new TLSError( | ||
| 'SSL certificate error. Use --skip-ssl-verify if needed.', | ||
| undefined, | ||
| 'Use --skip-ssl-verify flag if using self-signed certificates (not recommended for production)' | ||
| ); | ||
| } | ||
| } | ||
| if (error.name === 'AbortError') { | ||
| return new NetworkError( | ||
| 'Request timed out', | ||
| undefined, | ||
| 'Increase timeout with --timeout flag or check network connection' | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js fetch with AbortSignal.any, do both user-triggered aborts and timeout-triggered aborts surface as AbortError, and what is the recommended way to distinguish which signal aborted?
💡 Result:
Yes. In Node.js fetch(), if you pass a signal created by AbortSignal.any([...]), the rejection you catch is commonly a DOMException named "AbortError" regardless of whether the underlying abort was user-driven or timeout-driven (this is a known real-world behavior/interop gap vs the Fetch spec’s intent). (stackoverflow.com)
Recommended way to distinguish which one fired: don’t rely on err.name. Instead, inspect the abort reason on the combined signal (or on the original signals) via signal.reason. AbortSignal.any() guarantees the combined signal’s reason is the reason of the first signal that aborted. (developer.mozilla.org)
Practical pattern:
- Give the user controller a distinct reason:
userController.abort(new DOMException('User cancelled', 'AbortError'))(or a customError). - For timeouts, use
AbortSignal.timeout(ms), whosesignal.reasonis aDOMExceptiontypically named"TimeoutError". - In
catch, classify based oncombinedSignal.reason(or compare identities:err === combinedSignal.reasonwhen it matches in your runtime), e.g.combinedSignal.reason?.name === 'TimeoutError'vs your user-cancel reason. (developer.mozilla.org)
Citations:
- 1: https://stackoverflow.com/questions/75969669/abortsignal-timeout-in-fetch-request-always-responds-with-aborterror-but-not-t
- 2: https://developer.mozilla.org/docs/Web/API/AbortSignal/any_static
- 3: https://developer.mozilla.org/docs/Web/API/AbortSignal/any_static
🏁 Script executed:
cd /repo && wc -l src/core/http-client.tsRepository: mainwp/mainwp-control
Length of output: 120
🏁 Script executed:
cd /repo && sed -n '400,450p' src/core/http-client.tsRepository: mainwp/mainwp-control
Length of output: 120
🏁 Script executed:
cd /repo && sed -n '1,100p' src/core/http-client.ts | head -50Repository: mainwp/mainwp-control
Length of output: 120
🏁 Script executed:
git ls-files src/core/http-client.tsRepository: mainwp/mainwp-control
Length of output: 88
🏁 Script executed:
find . -name "http-client.ts" -type fRepository: mainwp/mainwp-control
Length of output: 90
🏁 Script executed:
pwdRepository: mainwp/mainwp-control
Length of output: 83
🏁 Script executed:
wc -l ./src/core/http-client.tsRepository: mainwp/mainwp-control
Length of output: 94
🏁 Script executed:
sed -n '400,460p' ./src/core/http-client.tsRepository: mainwp/mainwp-control
Length of output: 2091
🏁 Script executed:
sed -n '1,100p' ./src/core/http-client.tsRepository: mainwp/mainwp-control
Length of output: 3024
🏁 Script executed:
sed -n '140,250p' ./src/core/http-client.tsRepository: mainwp/mainwp-control
Length of output: 3592
🏁 Script executed:
rg "AbortSignal" ./src/core/http-client.ts -A 3 -B 3Repository: mainwp/mainwp-control
Length of output: 415
🏁 Script executed:
sed -n '250,350p' ./src/core/http-client.tsRepository: mainwp/mainwp-control
Length of output: 3007
🏁 Script executed:
sed -n '350,499p' ./src/core/http-client.tsRepository: mainwp/mainwp-control
Length of output: 4913
Fix AbortError classification to distinguish timeouts from user cancellations.
AbortSignal.any() combines the caller signal and internal timeout signal, so both user-initiated aborts and timeout aborts surface as AbortError. The current code at lines 430-437 checks only error.name === 'AbortError' and always reports 'Request timed out', which incorrectly treats cancellations as timeouts and gives wrong retry hints upstream.
To distinguish, set explicit reasons on each signal (e.g., controller.abort(new DOMException('TimeoutError', 'TimeoutError'))) and store the effectiveSignal reference so the error handler can check signal.reason rather than relying on error.name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/http-client.ts` around lines 429 - 439, The AbortError handler
currently treats every AbortError as a timeout; update the aborting logic to set
explicit reasons on the signals (e.g., when creating the timeout controller call
controller.abort(new DOMException('TimeoutError', 'TimeoutError'))) and ensure
the request code stores the combined/effective signal reference (e.g.,
effectiveSignal). Then change the error handling in the catch path that looks at
error.name === 'AbortError' to inspect effectiveSignal?.reason (or check that
it's a DOMException with name 'TimeoutError') — if reason indicates a timeout
return the existing timeout NetworkError, otherwise return a distinct
cancellation NetworkError (with an appropriate message/hint for user
cancellation). Ensure symbol names referenced: effectiveSignal,
controller.abort(new DOMException(...)), AbortError check, and NetworkError.
There was a problem hiding this comment.
Skipping the AbortError distinction. The only source of AbortError in this CLI is the timeout controller. There is no user-cancellation path (no UI signal, no external abort). Adding signal reason inspection for a scenario that cannot happen is unnecessary complexity. If we add user-cancellable requests later, we can revisit.
Separately: the sensitive field redaction was updated to use substring matching instead of exact key matching. This catches camelCase variants like accessToken, refreshToken, privateKey, and header-style keys like set-cookie.
There was a problem hiding this comment.
@dennisdornon, understood on both points.
The AbortError-only timeout path makes sense given there's no external abort signal wired in — the concern only applies when AbortSignal.any() is combining signals from different sources with different semantics. Happy to revisit if a user-cancellable request path is added later.
The substring-based redaction in sanitizeErrorData is a solid improvement — it covers the camelCase (accessToken, refreshToken, privateKey), snake_case (api_key, private_key, signing_key), and header-style (set-cookie, authorization) variants that exact-key matching would miss.
✏️ Learnings added
Learnt from: dennisdornon
Repo: mainwp/mainwp-control PR: 1
File: src/core/http-client.ts:429-439
Timestamp: 2026-03-24T01:00:10.885Z
Learning: In `src/core/http-client.ts` (mainwp/mainwp-control), the only source of AbortError is the internal timeout controller. There is no user-cancellation path (no external AbortSignal, no UI signal). AbortError always means a timeout in this codebase, so distinguishing between timeout and user-cancel AbortErrors is unnecessary unless a user-cancellable request path is added.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
dennisdornon
force-pushed
the
fix/keytar-esm-cjs-interop
branch
from
d687464 to
3285e64
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
README.md (2)
95-95:⚠️ Potential issue | 🟠 MajorAvoid recommending persistent plaintext password storage in shell profiles.
This line encourages long-term storage of
MAINWP_APP_PASSWORDin dotfiles, which weakens the security posture.📝 Suggested wording
-To make it permanent, add the `export` line to your shell profile file (`~/.bashrc`, `~/.zshrc`, or similar). +For local use, keep this export scoped to the current terminal session. +For persistent setups, prefer `mainwpctl login` (OS keychain) or inject `MAINWP_APP_PASSWORD` +from your platform's secret store per run (CI/cron), instead of writing it to shell profiles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 95, Replace the current instruction that tells users to add an `export MAINWP_APP_PASSWORD=...` line to their shell profile with guidance that discourages storing plaintext secrets in dotfiles; update the README to remove the explicit `export` recommendation for MAINWP_APP_PASSWORD and instead recommend secure alternatives such as using a system keyring/credential manager, a secrets manager (e.g., HashiCorp Vault, AWS Secrets Manager), a process-specific environment manager, or prompting at runtime, and if a file-based approach is absolutely necessary, advise using a restricted-permission .env file and documenting the required file permissions.
116-117:⚠️ Potential issue | 🟡 MinorSoften “required two-step confirmation” wording to recommended workflow.
Current phrasing implies strict CLI-enforced preview-before-execute gating. The docs should state this as the recommended safety flow, while clarifying server-side
confirmenforcement is handled by the Abilities API.📝 Suggested wording
-Destructive operations use a two-step pattern: preview first, then execute. +For destructive operations, the recommended safety flow is preview first (`--dry-run`), then execute (`--confirm`). -All destructive operations require a two-step confirmation: preview with `--dry-run`, then execute with `--confirm`. +Destructive operations should usually be previewed with `--dry-run` before execution with `--confirm`. -In CI/scripted workflows, you can pass `--confirm --force` directly if you've already validated the operation. +In CI/scripted workflows, you can pass `--confirm --force` directly after your own validation step.Based on learnings, the Abilities REST API enforces
confirmserver-side and the two-phase preview flow is an optional safety layer rather than a mandatory CLI replacement.Also applies to: 237-239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 116 - 117, Update the README sentence "Destructive operations use a two-step pattern: preview first, then execute." to soften it to a recommended workflow (e.g., "We recommend a two-step pattern: preview first, then execute") and add a brief clarifying note that server-side `confirm` enforcement is performed by the Abilities REST API and that the CLI preview step is an optional safety layer rather than a strict enforcement; apply the same wording change to the other occurrence of the original phrasing (the block around the second instance).
🧹 Nitpick comments (1)
src/commands/jobs/watch.ts (1)
318-329: Consider sanitizing error messages for consistency.The results section now sanitizes output via
safeString(), but the errors section still outputserror.messageanderror.itemwithout sanitization. For defense-in-depth and consistency, consider applying the same treatment here.🔧 Suggested fix
// Errors if (status.errors && status.errors.length > 0) { lines.push(''); lines.push(formatHeading('Errors:')); for (const error of status.errors) { const prefix = error.code ? `[${error.code}] ` : ''; - lines.push(formatWarning(` ${prefix}${error.message}`)); + lines.push(formatWarning(` ${prefix}${safeString(error.message)}`)); if (error.item) { - lines.push(` Item: ${JSON.stringify(error.item)}`); + lines.push(` Item: ${safeString(error.item)}`); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/jobs/watch.ts` around lines 318 - 329, The errors block currently logs raw error.message and error.item from status.errors; update it to sanitize both using the existing safeString utility for consistency with the results section: when building the prefix and message use safeString(error.message) (and safeString(error.code) if used), and when logging an item replace JSON.stringify(error.item) with safeString(JSON.stringify(error.item)) so all output lines produced in the loop go through safeString; refer to symbols status.errors, error.message, error.item, formatWarning, and safeString to locate and change the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config/keychain.ts`:
- Around line 69-75: The code currently sets keytar = mod or (mod as
any).default and then sets keytarAvailable = true without verifying the exported
object has the required API; update the logic around mod, keytar and
keytarAvailable to validate that the chosen export (mod or mod.default) actually
implements setPassword, getPassword and deletePassword (each typeof ===
'function') before assigning to keytar and setting keytarAvailable to true; if
neither shape is valid, leave keytar undefined and keytarAvailable false (and
optionally log/debug the unexpected shape) so downstream calls to keytar methods
cannot fail at runtime.
---
Duplicate comments:
In `@README.md`:
- Line 95: Replace the current instruction that tells users to add an `export
MAINWP_APP_PASSWORD=...` line to their shell profile with guidance that
discourages storing plaintext secrets in dotfiles; update the README to remove
the explicit `export` recommendation for MAINWP_APP_PASSWORD and instead
recommend secure alternatives such as using a system keyring/credential manager,
a secrets manager (e.g., HashiCorp Vault, AWS Secrets Manager), a
process-specific environment manager, or prompting at runtime, and if a
file-based approach is absolutely necessary, advise using a
restricted-permission .env file and documenting the required file permissions.
- Around line 116-117: Update the README sentence "Destructive operations use a
two-step pattern: preview first, then execute." to soften it to a recommended
workflow (e.g., "We recommend a two-step pattern: preview first, then execute")
and add a brief clarifying note that server-side `confirm` enforcement is
performed by the Abilities REST API and that the CLI preview step is an optional
safety layer rather than a strict enforcement; apply the same wording change to
the other occurrence of the original phrasing (the block around the second
instance).
---
Nitpick comments:
In `@src/commands/jobs/watch.ts`:
- Around line 318-329: The errors block currently logs raw error.message and
error.item from status.errors; update it to sanitize both using the existing
safeString utility for consistency with the results section: when building the
prefix and message use safeString(error.message) (and safeString(error.code) if
used), and when logging an item replace JSON.stringify(error.item) with
safeString(JSON.stringify(error.item)) so all output lines produced in the loop
go through safeString; refer to symbols status.errors, error.message,
error.item, formatWarning, and safeString to locate and change the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 06be4cf9-639a-400f-9151-c00d170532af
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (35)
.env.example.github/workflows/ci.ymlLICENSEREADME.mdbin/_exit.jsbin/dev.jsbin/run.jsdocs/workflows/daily-health-check.mddocs/workflows/input-from-file.mddocs/workflows/monitoring-integration.mddocs/workflows/monthly-batch-updates.mddocs/workflows/plugin-deployment-verification.mdpackage.jsonsrc/__tests__/e2e/login-abilities-flow.test.tssrc/__tests__/process/fixtures/cli-runner.tssrc/chat/chat-engine.tssrc/chat/providers/provider-fetch.tssrc/commands/abilities/run.tssrc/commands/jobs/watch.tssrc/config/keychain.tssrc/core/batch-manager.tssrc/core/http-client.test.tssrc/core/http-client.tssrc/core/safety-controller.test.tssrc/core/safety-controller.tssrc/utils/audit-logger.test.tssrc/utils/audit-logger.tssrc/utils/colors.tssrc/utils/errors.tssrc/utils/exit-codes.tssrc/utils/prompt.tssrc/validation/input-sanitizer.test.tssrc/validation/input-sanitizer.tssrc/validation/schema-validator.test.tssrc/validation/schema-validator.ts
💤 Files with no reviewable changes (4)
- src/utils/audit-logger.ts
- src/utils/exit-codes.ts
- src/validation/input-sanitizer.ts
- src/utils/errors.ts
✅ Files skipped from review due to trivial changes (12)
- bin/run.js
- src/utils/colors.ts
- LICENSE
- src/tests/process/fixtures/cli-runner.ts
- src/utils/audit-logger.test.ts
- .env.example
- docs/workflows/monthly-batch-updates.md
- src/validation/schema-validator.test.ts
- docs/workflows/input-from-file.md
- src/core/batch-manager.ts
- src/validation/input-sanitizer.test.ts
- src/utils/prompt.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- bin/dev.js
- src/chat/providers/provider-fetch.ts
- .github/workflows/ci.yml
- src/tests/e2e/login-abilities-flow.test.ts
- bin/_exit.js
- docs/workflows/plugin-deployment-verification.md
- src/core/safety-controller.test.ts
- package.json
- src/commands/abilities/run.ts
- src/core/safety-controller.ts
- src/validation/schema-validator.ts
- src/core/http-client.test.ts
- src/core/http-client.ts
dennisdornon
force-pushed
the
fix/keytar-esm-cjs-interop
branch
2 times, most recently
from
278c0e7 to
1dbf0f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/workflows/plugin-deployment-verification.md (1)
62-78:⚠️ Potential issue | 🟠 MajorUpdate
npxinvocations to use the scoped package name across all installation guides.The docs correctly show
npm install -g@mainwp/control`` but then instruct users to runnpx mainwpctl, which will attempt to resolve the unscoped package `mainwpctl` instead of the renamed `@mainwp/control`. This breaks the no-install path unless a local install already exists. The issue appears in five files: `docs/workflows/plugin-deployment-verification.md`, `docs/workflows/monitoring-integration.md`, `docs/workflows/daily-health-check.md`, `docs/workflows/input-from-file.md`, and `docs/workflows/monthly-batch-updates.md`.Change all
npx mainwpctlinvocations tonpx@mainwp/control`` in these files, and update the corresponding guidance text that tells users to substitutenpx mainwpctl.Example fix for plugin-deployment-verification.md
-```bash -npx mainwpctl --version -``` +```bash +npx `@mainwp/control` --version +``` -`npx` downloads the package temporarily each time you run it. For this guide, the examples use `mainwpctl` directly, but you can substitute `npx mainwpctl` anywhere you see `mainwpctl`. +`npx` downloads the package temporarily each time you run it. For this guide, the examples use `mainwpctl` directly, but you can substitute `npx `@mainwp/control`` anywhere you see `mainwpctl`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/workflows/plugin-deployment-verification.md` around lines 62 - 78, Replace all occurrences of the unscoped npx invocation "npx mainwpctl" with the scoped package name "npx `@mainwp/control`" and update the descriptive guidance text that tells users they can substitute the npx command (change sentences that say "you can substitute `npx mainwpctl`" to "you can substitute `npx `@mainwp/control``"); target the places showing "npm install -g `@mainwp/control`", the `npx mainwpctl --version` example, and any inline mentions of `mainwpctl` as the npx substitution to ensure the no-install path resolves the scoped package.
🧹 Nitpick comments (1)
src/commands/abilities/run.ts (1)
496-503: Null byte check timing concern — check should occur beforeresolve().The null byte check at line 498 happens after
resolve()is called. Whileresolve()in Node.js typically preserves null bytes in the string, the check should ideally occur on the originalinputFilePathbefore any processing to catch malicious input early. Additionally, the comment mentions "directory traversal" prevention, butresolve()alone doesn't prevent traversal attacks—it just normalizes the path.♻️ Suggested improvement: check null bytes before resolve
if (inputFilePath) { - // SECURITY: Validate file path to prevent directory traversal + // SECURITY: Reject null bytes before any path processing + if (inputFilePath.includes('\0')) { + throw new InputError('Invalid file path: contains null bytes'); + } const resolvedPath = resolve(inputFilePath); - if (resolvedPath.includes('\0')) { - throw new InputError('Invalid file path: contains null bytes'); - } try { return await readFile(resolvedPath, 'utf-8');Note: The current code still provides protection since
resolve()doesn't strip null bytes, but validating untrusted input before processing is a defense-in-depth best practice. Also consider whether additional path validation (e.g., ensuring the resolved path doesn't escape a designated directory) is needed for your threat model.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/abilities/run.ts` around lines 496 - 503, The null-byte validation is performed after calling resolve(), so move the check to validate the raw inputFilePath for '\0' before any processing (i.e., before calling resolve(inputFilePath)) and throw InputError if found; keep the existing resolvedPath = resolve(inputFilePath) and subsequent readFile(resolvedPath, 'utf-8') but add an additional post-resolve containment check (e.g., ensure resolvedPath does not escape the allowed base directory) to mitigate directory traversal risks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config/keychain.ts`:
- Around line 69-82: The code currently assigns a possibly undefined candidate
into the module-level keytar cache which later is only checked against null,
causing loadKeytar()/Keychain.isAvailable() to misreport availability; change
loadKeytar() to keep the imported value in a local candidate (e.g.,
modCandidate), validate the full API (check setPassword, getPassword,
deletePassword) on that local candidate, and only assign to the module-level
keytar and set keytarAvailable = true when the candidate is fully valid; ensure
that when the import shape is rejected you leave module-level keytar as null and
set keytarAvailable = false so subsequent calls re-run the import logic
correctly.
In `@src/core/safety-controller.ts`:
- Around line 64-69: The execution-time fallback must fail-closed: update the
default annotations used when meta.annotations is missing/malformed so that
destructive defaults to true (change DEFAULT_ANNOTATIONS destructiveness to true
or ensure the execution-path that reads meta.annotations overrides
missing/invalid shapes to set destructive: true); keep any
permissive/display-only defaults separate from the runtime safety check, and
make the same adjustment wherever meta.annotations is normalized (e.g., the code
paths around DEFAULT_ANNOTATIONS and the normalization/validation logic
referenced near the handling of meta.annotations so that destructive is true on
missing/invalid annotation shapes).
---
Outside diff comments:
In `@docs/workflows/plugin-deployment-verification.md`:
- Around line 62-78: Replace all occurrences of the unscoped npx invocation "npx
mainwpctl" with the scoped package name "npx `@mainwp/control`" and update the
descriptive guidance text that tells users they can substitute the npx command
(change sentences that say "you can substitute `npx mainwpctl`" to "you can
substitute `npx `@mainwp/control``"); target the places showing "npm install -g
`@mainwp/control`", the `npx mainwpctl --version` example, and any inline mentions
of `mainwpctl` as the npx substitution to ensure the no-install path resolves
the scoped package.
---
Nitpick comments:
In `@src/commands/abilities/run.ts`:
- Around line 496-503: The null-byte validation is performed after calling
resolve(), so move the check to validate the raw inputFilePath for '\0' before
any processing (i.e., before calling resolve(inputFilePath)) and throw
InputError if found; keep the existing resolvedPath = resolve(inputFilePath) and
subsequent readFile(resolvedPath, 'utf-8') but add an additional post-resolve
containment check (e.g., ensure resolvedPath does not escape the allowed base
directory) to mitigate directory traversal risks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f856904d-6caf-4573-8346-3923a5e40523
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (36)
.env.example.github/workflows/ci.ymlLICENSEREADME.mdbin/_exit.jsbin/dev.jsbin/run.jsdocs/workflows/daily-health-check.mddocs/workflows/input-from-file.mddocs/workflows/monitoring-integration.mddocs/workflows/monthly-batch-updates.mddocs/workflows/plugin-deployment-verification.mdpackage.jsonsrc/__tests__/e2e/login-abilities-flow.test.tssrc/__tests__/process/fixtures/cli-runner.tssrc/chat/chat-engine.tssrc/chat/providers/provider-fetch.tssrc/commands/abilities/run.tssrc/commands/jobs/watch.tssrc/config/keychain.tssrc/core/batch-manager.tssrc/core/http-client.test.tssrc/core/http-client.tssrc/core/safety-controller.test.tssrc/core/safety-controller.tssrc/utils/audit-logger.test.tssrc/utils/audit-logger.tssrc/utils/colors.tssrc/utils/errors.tssrc/utils/exit-codes.tssrc/utils/prompt.tssrc/validation/input-sanitizer.test.tssrc/validation/input-sanitizer.tssrc/validation/schema-validator.test.tssrc/validation/schema-validator.tsvitest.config.ts
💤 Files with no reviewable changes (4)
- src/utils/exit-codes.ts
- src/utils/audit-logger.ts
- src/utils/errors.ts
- src/validation/input-sanitizer.ts
✅ Files skipped from review due to trivial changes (9)
- bin/run.js
- LICENSE
- vitest.config.ts
- src/tests/process/fixtures/cli-runner.ts
- src/validation/input-sanitizer.test.ts
- src/core/batch-manager.ts
- .env.example
- docs/workflows/input-from-file.md
- src/validation/schema-validator.test.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- bin/dev.js
- .github/workflows/ci.yml
- src/chat/providers/provider-fetch.ts
- src/commands/jobs/watch.ts
- src/chat/chat-engine.ts
- bin/_exit.js
- src/core/safety-controller.test.ts
- src/utils/prompt.ts
- src/utils/audit-logger.test.ts
- src/core/http-client.ts
- docs/workflows/monthly-batch-updates.md
- package.json
| /** Default annotations for abilities without explicit metadata */ | ||
| const DEFAULT_ANNOTATIONS: AbilityAnnotations = { | ||
| readonly: false, | ||
| destructive: false, | ||
| idempotent: false, | ||
| }; |
There was a problem hiding this comment.
Keep the execution-time fallback fail-closed.
Missing or malformed meta.annotations now resolves to { destructive: false }, so any destructive ability that is not caught by the name regexes will skip the safety flow and can reach direct execution. That turns an annotation omission/shape drift into a bypass of the preview/confirm gate.
🔒 Suggested fix
const DEFAULT_ANNOTATIONS: AbilityAnnotations = {
readonly: false,
- destructive: false,
+ destructive: true,
idempotent: false,
};If you still want a permissive default for schema/display code, keep that separate from this execution path.
Based on learnings: execution paths intentionally default missing destructive annotations to true as a fail-closed safety guardrail.
Also applies to: 79-81, 122-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/safety-controller.ts` around lines 64 - 69, The execution-time
fallback must fail-closed: update the default annotations used when
meta.annotations is missing/malformed so that destructive defaults to true
(change DEFAULT_ANNOTATIONS destructiveness to true or ensure the execution-path
that reads meta.annotations overrides missing/invalid shapes to set destructive:
true); keep any permissive/display-only defaults separate from the runtime
safety check, and make the same adjustment wherever meta.annotations is
normalized (e.g., the code paths around DEFAULT_ANNOTATIONS and the
normalization/validation logic referenced near the handling of meta.annotations
so that destructive is true on missing/invalid annotation shapes).
There was a problem hiding this comment.
Disagree. Defaulting destructive: true when annotations are missing would break every read-only ability in the CLI (list-sites, get-site, list-updates, check-site, etc.). Users would need --dry-run or --confirm for basic queries, which makes the tool unusable.
The codebase already has a fail-closed mechanism for this: isKnownDestructivePattern() (line 87) force-classifies abilities matching destructive name patterns (delete, disconnect, suspend, deactivate, remove, run-updates, update-all) regardless of what annotations say. This is defense-in-depth against a compromised server downgrading destructive abilities.
Missing annotations means a new or unknown ability, not a security threat. The pattern matching layer handles that case.
There was a problem hiding this comment.
@dennisdornon, that's a fair and well-reasoned explanation. The two-layer design makes sense:
isKnownDestructivePattern()(lines 87–101) acts as a fail-closed override for all known mutation verb patterns, regardless of what the server reports in annotations.DEFAULT_ANNOTATIONSwithdestructive: falseis deliberately permissive so that read-only/query abilities (list-sites,get-site, etc.) that lack annotations continue to work without requiring--dry-runor--confirm.
The distinction from the mainwp-mcp execution path is important: there, the MCP tool executor defaults missing annotations to destructive: true because the surface is AI-driven and confirmation flows are cheap. Here, the CLI's primary surface is direct human invocation, where requiring --dry-run or --confirm on every read query would be a usability regression. The regex pattern list is the right place to enforce the fail-closed contract for mutation abilities.
I'll withdraw the suggestion and record this design rationale.
✏️ Learnings added
Learnt from: dennisdornon
Repo: mainwp/mainwp-control PR: 1
File: src/core/safety-controller.ts:64-69
Timestamp: 2026-03-24T01:00:37.162Z
Learning: In `src/core/safety-controller.ts` (mainwp/mainwp-control), `DEFAULT_ANNOTATIONS` intentionally uses `destructive: false` as the fallback when `meta.annotations` is missing. This is by design: defaulting to `destructive: true` would require `--dry-run` or `--confirm` for every read-only ability (list-sites, get-site, list-updates, etc.), breaking normal CLI usage. The fail-closed safety guarantee for mutation abilities is instead enforced by `isKnownDestructivePattern()` (a static regex list of verb prefixes: delete, disconnect, suspend, deactivate, remove, run-updates, update-all), which force-classifies abilities as destructive by name regardless of API-reported annotations. This is a defense-in-depth layer against a compromised server downgrading destructive annotations.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: dennisdornon
Repo: mainwp/mainwp-mcp PR: 21
File: src/tools.ts:463-481
Timestamp: 2026-03-02T21:34:09.733Z
Learning: In src/tools.ts, the default-deny behavior intentionally differs between schema generation and execution:
- Schema generation (abilityToTool, line ~450): `meta?.destructive ?? false` — defaults to non-destructive to avoid cluttering safe tool schemas with unnecessary user_confirmed parameters
- Execution (executeTool, line ~662): `annotations?.destructive ?? true` — defaults to destructive to require confirmation as a safety guardrail when annotations are missing
This asymmetry is a defense-in-depth design: schema layer optimizes for UX (clean interfaces), execution layer optimizes for safety (fail-closed).
Learnt from: dennisdornon
Repo: mainwp/mainwp-mcp PR: 21
File: src/tools.ts:915-923
Timestamp: 2026-03-03T01:16:04.784Z
Learning: In src/tools.ts executeTool, when requireUserConfirmation is enabled but a destructive tool with a confirm parameter is called without confirm/user_confirmed/dry_run, the default branch intentionally acts as a passthrough to the Abilities API rather than blocking execution. This is by design: the Abilities REST API enforces its own server-side confirm requirement (returning 400/422 errors), and the MCP server is a passthrough that doesn't replace API validation. The requireUserConfirmation feature adds an optional two-phase confirmation flow on top of API enforcement, not a mandatory replacement for it. Blocking at the MCP layer would create redundant double-gating and break backward compatibility with callers that don't use the two-phase flow.
- resolve keytar dynamic import failure on Node v23+ - drain stdout/stderr before forced exit, add keytar timeout - security: harden input validation, error sanitization, schema coercion - security: fix prototype pollution, credential leakage, xml-parser CVE - security: truncate LLM provider errors, validate --input-file paths - refactor: table-driven error mapping, inline action verbs, shared helpers - refactor: remove dead code (unused exports, redundant factories) - npm publishing prep: rename to @mainwp/control, add LICENSE - fix null data in batch manager success envelope - ci/docs cleanup
dennisdornon
force-pushed
the
fix/keytar-esm-cjs-interop
branch
from
1dbf0f8 to
2bccc21
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/core/safety-controller.ts (1)
64-69:⚠️ Potential issue | 🔴 CriticalKeep execution-time defaults fail-closed.
destructive: falseturns missing or malformed annotations into a safety bypass for any destructive ability that doesn't match the regex fallback. This runtime path should defaultdestructivetotrue, andvalidateAnnotations()should keep using that same fail-closed default.🔒 Minimal fix
const DEFAULT_ANNOTATIONS: AbilityAnnotations = { readonly: false, - destructive: false, + destructive: true, idempotent: false, };Based on learnings: execution paths intentionally default missing destructive annotations to
trueas a fail-closed safety guardrail.Also applies to: 79-81, 123-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/safety-controller.ts` around lines 64 - 69, The DEFAULT_ANNOTATIONS constant currently sets destructive: false which makes missing/malformed annotations unsafe; change DEFAULT_ANNOTATIONS to set destructive: true and update validateAnnotations() so its runtime fallback/merge logic uses the same fail-closed default (treat absent or invalid destructive values as true) when constructing/returning AbilityAnnotations; ensure any other defaults or merge code paths that reference DEFAULT_ANNOTATIONS (or perform manual property checks around destructive in validateAnnotations) are aligned to the new fail-closed behavior.src/config/keychain.ts (1)
69-80:⚠️ Potential issue | 🟠 MajorDon't cache an invalid keytar candidate.
Line 72 can still assign
undefinedintokeytar, and Lines 54-56 then treat that as a populated cache because the guard only checks!== null. After one rejected import shape,loadKeytar()can returnundefined,isAvailable()flips totrue, and the import path is never retried. This branch also only verifiessetPassword, even though Lines 152 and 178 later rely ongetPasswordanddeletePasswordtoo.🔧 Proposed fix
+function hasKeytarApi(value: unknown): value is typeof import('keytar') { + return !!value + && typeof (value as { setPassword?: unknown }).setPassword === 'function' + && typeof (value as { getPassword?: unknown }).getPassword === 'function' + && typeof (value as { deletePassword?: unknown }).deletePassword === 'function'; +} + async function loadKeytar(): Promise<typeof import('keytar') | null> { if (keytar !== null) { return keytar; } @@ try { const mod = await import('keytar'); - // CJS/ESM interop: on newer Node versions, CJS exports are nested under .default. - // Check for the expected API on mod first; only unwrap .default if needed. - keytar = typeof mod.setPassword === 'function' - ? mod - : typeof (mod as any).default?.setPassword === 'function' - ? (mod as any).default - : undefined; - - if (!keytar) { + const candidate = hasKeytarApi(mod) + ? mod + : hasKeytarApi((mod as { default?: unknown }).default) + ? (mod as { default: typeof import('keytar') }).default + : null; + + if (!candidate) { keytarAvailable = false; return null; } + keytar = candidate; keytarAvailable = true; return keytar; } catch {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/keychain.ts` around lines 69 - 80, The current loadKeytar() implementation can cache undefined as keytar and only checks setPassword; update it so you only assign to the cached keytar variable when the imported module (or its .default) implements all required methods (setPassword, getPassword, deletePassword) and otherwise leave the cache as null; ensure loadKeytar() returns null on failure (not undefined) and that isAvailable() checks the cache against null (or uses a dedicated boolean) so failed imports are retried instead of treated as available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/workflows/monitoring-integration.md`:
- Around line 63-64: Update the npx example to use the scoped package name used
in the install command: replace the stray reference to `npx mainwpctl` with `npx
`@mainwp/control`` so the example matches the earlier `npm install -g
`@mainwp/control`` instruction and avoids confusion about the package name.
In `@docs/workflows/plugin-deployment-verification.md`:
- Line 65: Update the no-install fallback commands that currently call "npx
mainwpctl" to use the scoped package name by replacing them with "npx
`@mainwp/control`"; specifically search for occurrences of the unscoped invocation
("npx mainwpctl") and change them to the scoped form so they match the global
install command "npm install -g `@mainwp/control`" (affects the instances noted in
the review).
In `@src/core/http-client.test.ts`:
- Around line 688-692: Add an object-level prototype pollution assertion to the
test: besides checking that the parsed result doesn't have a "__proto__" key,
assert that the returned data object itself wasn't polluted (e.g. add
expect((data as any).isAdmin).toBeUndefined() or an equivalent check) to catch
regressions if the JSON.parse replacer (the code that strips "__proto__") is
removed or weakened.
---
Duplicate comments:
In `@src/config/keychain.ts`:
- Around line 69-80: The current loadKeytar() implementation can cache undefined
as keytar and only checks setPassword; update it so you only assign to the
cached keytar variable when the imported module (or its .default) implements all
required methods (setPassword, getPassword, deletePassword) and otherwise leave
the cache as null; ensure loadKeytar() returns null on failure (not undefined)
and that isAvailable() checks the cache against null (or uses a dedicated
boolean) so failed imports are retried instead of treated as available.
In `@src/core/safety-controller.ts`:
- Around line 64-69: The DEFAULT_ANNOTATIONS constant currently sets
destructive: false which makes missing/malformed annotations unsafe; change
DEFAULT_ANNOTATIONS to set destructive: true and update validateAnnotations() so
its runtime fallback/merge logic uses the same fail-closed default (treat absent
or invalid destructive values as true) when constructing/returning
AbilityAnnotations; ensure any other defaults or merge code paths that reference
DEFAULT_ANNOTATIONS (or perform manual property checks around destructive in
validateAnnotations) are aligned to the new fail-closed behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6431ff9c-8746-49a3-a998-24f6814567d6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (36)
.env.example.github/workflows/ci.ymlLICENSEREADME.mdbin/_exit.jsbin/dev.jsbin/run.jsdocs/workflows/daily-health-check.mddocs/workflows/input-from-file.mddocs/workflows/monitoring-integration.mddocs/workflows/monthly-batch-updates.mddocs/workflows/plugin-deployment-verification.mdpackage.jsonsrc/__tests__/e2e/login-abilities-flow.test.tssrc/__tests__/process/fixtures/cli-runner.tssrc/chat/chat-engine.tssrc/chat/providers/provider-fetch.tssrc/commands/abilities/run.tssrc/commands/jobs/watch.tssrc/config/keychain.tssrc/core/batch-manager.tssrc/core/http-client.test.tssrc/core/http-client.tssrc/core/safety-controller.test.tssrc/core/safety-controller.tssrc/utils/audit-logger.test.tssrc/utils/audit-logger.tssrc/utils/colors.tssrc/utils/errors.tssrc/utils/exit-codes.tssrc/utils/prompt.tssrc/validation/input-sanitizer.test.tssrc/validation/input-sanitizer.tssrc/validation/schema-validator.test.tssrc/validation/schema-validator.tsvitest.config.ts
💤 Files with no reviewable changes (4)
- src/utils/audit-logger.ts
- src/utils/exit-codes.ts
- src/utils/errors.ts
- src/validation/input-sanitizer.ts
✅ Files skipped from review due to trivial changes (10)
- bin/dev.js
- src/chat/chat-engine.ts
- vitest.config.ts
- src/validation/input-sanitizer.test.ts
- docs/workflows/input-from-file.md
- docs/workflows/monthly-batch-updates.md
- LICENSE
- src/validation/schema-validator.test.ts
- src/core/http-client.ts
- src/core/batch-manager.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- bin/run.js
- src/chat/providers/provider-fetch.ts
- src/tests/process/fixtures/cli-runner.ts
- .github/workflows/ci.yml
- src/core/safety-controller.test.ts
- bin/_exit.js
- src/tests/e2e/login-abilities-flow.test.ts
- src/utils/prompt.ts
- package.json
- .env.example
| npm install -g @mainwp/control | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n docs/workflows/monitoring-integration.md | sed -n '50,80p'Repository: mainwp/mainwp-control
Length of output: 1221
🏁 Script executed:
cat package.json | head -40Repository: mainwp/mainwp-control
Length of output: 1293
Update the npx command to use the scoped package name.
Line 71 shows npx mainwpctl, but the global install at line 63 uses npm install -g @mainwp/control. The npx example should use the scoped package form `npx `@mainwp/control for consistency and to prevent readers from being confused about the correct package name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/workflows/monitoring-integration.md` around lines 63 - 64, Update the
npx example to use the scoped package name used in the install command: replace
the stray reference to `npx mainwpctl` with `npx `@mainwp/control`` so the example
matches the earlier `npm install -g `@mainwp/control`` instruction and avoids
confusion about the package name.
There was a problem hiding this comment.
Skipping for now. The package @mainwp/control is not published to npm yet. Running npx @mainwp/control would fail with a 404. We will update all npx references in the docs when we publish.
There was a problem hiding this comment.
@dennisdornon, understood — keeping npx mainwpctl in place until @mainwp/control is actually published makes sense. Flagging the change pre-publish would only confuse readers who try to run it. I'll make a note to revisit all npx references in the docs once the package is live on npm.
✏️ Learnings added
Learnt from: dennisdornon
Repo: mainwp/mainwp-control PR: 1
File: docs/workflows/monitoring-integration.md:63-64
Timestamp: 2026-03-24T01:00:27.139Z
Learning: In mainwp/mainwp-control, the npm package `mainwp/control` is not yet published to npm. All `npx mainwpctl` references in the docs are intentionally kept as-is until the package is published; do not suggest changing them to `npx mainwp/control` until then.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: dennisdornon
Repo: mainwp/mainwp-control PR: 1
File: .env.example:1-2
Timestamp: 2026-03-24T00:59:34.367Z
Learning: In mainwp/mainwp-control, the `.env.example` file is intentionally documentation-only. The CLI bootstrap (`bin/run.js`) uses oclif's `execute()` with no dotenv loader, and does not auto-load `.env` files. Users are expected to export variables manually to their shell or CI environment. Adding a dotenv loader is explicitly out of scope. Do not flag the `.env.example` header wording ("Copy to .env or export in your shell profile") as misleading in future reviews.
Learnt from: phuochoit
Repo: mainwp/ai-bridge PR: 10
File: backends/mintlify/lib/validation.js:17-19
Timestamp: 2026-01-26T16:51:12.280Z
Learning: In the mainwp/ai-bridge repository, the maintainer (phuochoit) prefers NODE_ENV to default to "development" rather than "production" in backends/mintlify/lib/validation.js.
1 participant
Fixes keytar ESM/CJS dynamic import on Node v23+, plus a batch of security and cleanup work.
Changes:
All 663 tests pass. CodeRabbit review clean.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Improvements
Chores