diff --git a/CHANGELOG.md b/CHANGELOG.md index 330051b..1de1965 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- **Breaking:** `MAINWP_APP_PASSWORD` is now identity-bound the same way keychain credentials are: every authenticated command, `login` included, requires `MAINWP_DASHBOARD_URL` to be set and to match the profile's canonical Dashboard identity before the password is sent. Without it the command refuses to send the credential, with a hint naming the fix. This closes a redirect where an edited or committed `profiles.json` could silently point the environment password at a different host (CI, where the env var is the documented credential path, is exactly where `profiles.json` is easiest to tamper with). Interactive login with a prompted password and display-only commands (`doctor`, `config show`) are unaffected + +### Security + +- Dashboard URLs carrying credentials in the query string or fragment (`?access_token=...`, `#api_key=...`, including percent-encoded key variants) are rejected when a profile is created; profiles already on disk with such URLs have the sensitive parameter values masked on every display path, including error messages +- Streamed chat tool calls are bounded at every layer (provider stream buffer, engine collection, tool-call envelope), so a hostile or malfunctioning provider stream cannot grow memory or dispatch work without limit +- `abilities info` renders Dashboard-supplied ability descriptions and annotation instructions inside a visibly quoted block, so remote metadata cannot pose as CLI output or smuggle formatting into the terminal + ## [1.1.0] - 2026-07-22 ### Fixed diff --git a/README.md b/README.md index d9251a5..fbf24bb 100644 --- a/README.md +++ b/README.md @@ -167,14 +167,29 @@ Interactive use needs no configuration beyond `mainwpcontrol login`. For CI, Doc | Variable | Description | |----------|-------------| | `MAINWP_APP_PASSWORD` | Application Password for non-interactive login, and for commands when no OS keychain is available | +| `MAINWP_DASHBOARD_URL` | The Dashboard `MAINWP_APP_PASSWORD` belongs to. Required whenever a command authenticates using that fallback | | `MAINWPCONTROL_NO_KEYTAR` | Set to `1` to skip keychain loading entirely | | `MAINWP_ALLOW_HTTP` | Set to `1` to allow insecure `http://` Dashboard URLs | ```bash export MAINWP_APP_PASSWORD='xxxx xxxx xxxx xxxx xxxx xxxx' +export MAINWP_DASHBOARD_URL='https://dashboard.example.com' mainwpcontrol login --url https://dashboard.example.com --username admin +mainwpcontrol abilities list ``` +Whenever the password comes from `MAINWP_APP_PASSWORD`, the CLI sends it only to the +Dashboard named in `MAINWP_DASHBOARD_URL`, and fails instead of sending it anywhere +else. That covers `login` as well as later commands: in CI the password usually lives +in a protected secret store while command arguments do not, so pinning the destination +next to the secret is what stops an edited pipeline from redirecting it. It also means +a `profiles.json` someone else can write cannot point your credential at their server. + +Credentials in the OS keychain are bound to their Dashboard the same way and need no +extra variable. `login` only prompts for the password when `MAINWP_APP_PASSWORD` is +unset; when it is set, the same binding applies. Commands that only display +configuration, such as `doctor` and `config show`, are unaffected. + Optional defaults (JSON output, timeouts, chat provider) live in `~/.config/mainwpcontrol/settings.json`. The full list of settings, chat provider keys, and the credential storage model are in the [Configuration guide](docs/configuration.md). ## Abilities @@ -212,6 +227,7 @@ CI runs lint, type check, tests, and build on every pull request. export MAINWP_API_URL=https://your-dashboard.example.com export MAINWP_USER=your-admin-username export MAINWP_APP_PASSWORD='your-application-password' +export MAINWP_DASHBOARD_URL="$MAINWP_API_URL" npm run test:live ``` diff --git a/docs/acceptance-testing.md b/docs/acceptance-testing.md index 432672d..3208e63 100644 --- a/docs/acceptance-testing.md +++ b/docs/acceptance-testing.md @@ -39,7 +39,7 @@ Live credentials are resolved in this order: The environment file maps `LLM_DASH_URL` to the Dashboard URL and reads `MAINWP_USER` and `MAINWP_APP_PASSWORD`. -The agent runner creates one temporary XDG configuration directory per scenario. Its profile contains the Dashboard URL and username. The Application Password exists only in the Claude child environment as `MAINWP_APP_PASSWORD`; it is not written to the profile, consumer, transcript, command record, or result files. `MAINWPCONTROL_NO_KEYTAR=1` keeps the run independent of the OS keychain. +The agent runner creates one temporary XDG configuration directory per scenario. Its profile contains the Dashboard URL and username. The Application Password exists only in the Claude child environment as `MAINWP_APP_PASSWORD`; it is not written to the profile, consumer, transcript, command record, or result files. The runner also sets `MAINWP_DASHBOARD_URL` to the scenario's Dashboard, which the CLI requires before it will send an environment-supplied password. `MAINWPCONTROL_NO_KEYTAR=1` keeps the run independent of the OS keychain. `MAINWP_CONTROL_ACCEPTANCE_TOGGLE_PLUGIN` can select the plugin slug preferred by the `agent-plugin-active` scenario and the reversible deterministic plugin scenario. diff --git a/docs/cli-reference.md b/docs/cli-reference.md index e757436..ee788c5 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -22,8 +22,10 @@ Connects to a Dashboard and creates a profile named after its hostname. Interact # Interactive mainwpcontrol login -# Non-interactive (CI, headless): password from the environment +# Non-interactive (CI, headless): password from the environment. +# MAINWP_DASHBOARD_URL is required with it and must match --url. export MAINWP_APP_PASSWORD='xxxx xxxx xxxx xxxx xxxx xxxx' +export MAINWP_DASHBOARD_URL='https://dashboard.example.com' mainwpcontrol login --url https://dashboard.example.com --username admin ``` @@ -35,7 +37,7 @@ mainwpcontrol login --url https://dashboard.example.com --username admin | `--password ` | Application Password; prefer `MAINWP_APP_PASSWORD` or the prompt, since flags are visible in the process list | | `--skip-ssl-verify` | Accept a self-signed certificate for this profile (not for production) | -When no OS keychain is available, credentials are not stored on disk; keep `MAINWP_APP_PASSWORD` set for each run. +When no OS keychain is available, credentials are not stored on disk; keep `MAINWP_APP_PASSWORD` set for each run, along with `MAINWP_DASHBOARD_URL` naming the Dashboard it belongs to. Commands that authenticate release the password only when the two match the profile they are about to contact. ## `abilities list` diff --git a/docs/configuration.md b/docs/configuration.md index 88d910c..b499dc5 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -16,10 +16,21 @@ For CI, Docker, and machines without a keychain, put the password in the environ ```bash export MAINWP_APP_PASSWORD='xxxx xxxx xxxx xxxx xxxx xxxx' +export MAINWP_DASHBOARD_URL='https://dashboard.example.com' mainwpcontrol login --url https://dashboard.example.com --username admin ``` -When no keychain is available, the password is never written to disk; keep `MAINWP_APP_PASSWORD` set for each run. The profile file is still written and records the Dashboard URL and username, as it does in every mode. If keytar is installed but broken, set `MAINWPCONTROL_NO_KEYTAR=1` to skip loading it. +When no keychain is available, the password is never written to disk; keep both variables set for each run: + +```bash +export MAINWP_APP_PASSWORD='xxxx xxxx xxxx xxxx xxxx xxxx' +export MAINWP_DASHBOARD_URL='https://dashboard.example.com' +mainwpcontrol abilities list +``` + +Any command that authenticates with `MAINWP_APP_PASSWORD`, `login` included, sends it only to the Dashboard named in `MAINWP_DASHBOARD_URL` and fails rather than sending it anywhere else. Two things follow: a `profiles.json` that someone else can write cannot point your credential at their server, and in CI, where the password usually comes from a protected secret store and command arguments do not, an edited pipeline cannot redirect it either. Keychain-stored credentials carry the same binding internally and need no extra variable. `login` only prompts for the password when `MAINWP_APP_PASSWORD` is unset; when it is set, the same binding applies. `doctor` and `config show` only display configuration, so they are unaffected. + +The profile file is still written and records the Dashboard URL and username, as it does in every mode. If keytar is installed but broken, set `MAINWPCONTROL_NO_KEYTAR=1` to skip loading it. ## Profiles @@ -68,6 +79,7 @@ Inspect the active values with `mainwpcontrol config show`. | Variable | Description | |----------|-------------| | `MAINWP_APP_PASSWORD` | Application Password for non-interactive login, and for commands when no keychain is available | +| `MAINWP_DASHBOARD_URL` | The Dashboard `MAINWP_APP_PASSWORD` belongs to. Required whenever a command authenticates using that fallback | | `MAINWPCONTROL_NO_KEYTAR` | Set to `1` to skip keytar (keychain) loading entirely | | `MAINWP_ALLOW_HTTP` | Set to `1` to allow insecure HTTP Dashboard URLs | diff --git a/docs/getting-started.md b/docs/getting-started.md index 5a836fb..e9f86bd 100644 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -37,11 +37,16 @@ An environment variable is a named value that programs can read. They're commonl ```bash # macOS / Linux / Git Bash (lasts until you close the terminal) export MAINWP_APP_PASSWORD='xxxx xxxx xxxx xxxx xxxx xxxx' +export MAINWP_DASHBOARD_URL='https://dashboard.example.com' # Windows PowerShell (lasts until you close the window) $env:MAINWP_APP_PASSWORD = 'xxxx xxxx xxxx xxxx xxxx xxxx' +$env:MAINWP_DASHBOARD_URL = 'https://dashboard.example.com' ``` +Set both. The second names the Dashboard the password belongs to, and the CLI +refuses to send it anywhere else. + For long-term storage, use the OS keychain (the default when you run `mainwpcontrol login`) or a restricted-permission `.env` file rather than pasting credentials into shell profile files. Note that `mainwpcontrol` does not read `.env` files itself: source the file (or export the variable another way) before running the CLI. ## Reading command output diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index b922ace..2555e3c 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -16,8 +16,11 @@ Keytar (the keychain module) requires native C++ compilation on some platforms. ```bash export MAINWPCONTROL_NO_KEYTAR=1 export MAINWP_APP_PASSWORD='your-application-password' + export MAINWP_DASHBOARD_URL='https://dashboard.example.com' mainwpcontrol login --url https://dashboard.example.com --username admin + mainwpcontrol abilities list ``` + `MAINWP_DASHBOARD_URL` is required alongside the password: the CLI releases the environment credential only to the Dashboard it names, and refuses when it is missing or points elsewhere. 2. **Or install C++ build tools** (`gcc`, `g++`, `make`) and reinstall. ## "command not found" after install diff --git a/docs/workflows/daily-health-check.md b/docs/workflows/daily-health-check.md index 0e8390f..93829e3 100644 --- a/docs/workflows/daily-health-check.md +++ b/docs/workflows/daily-health-check.md @@ -123,7 +123,7 @@ You will be prompted for three pieces of information: 2. **Username:** Your WordPress admin username on the Dashboard site. 3. **Application Password:** The password you created in Step 1. Paste it in when prompted. The spaces in the password are fine; include them or omit them, both work. -After entering these, MainWP Control stores your credentials in your system's keychain when one is available (macOS Keychain, Linux secret service, or Windows Credential Manager). If the machine cannot use a keychain, keep `MAINWP_APP_PASSWORD` available in the environment for future runs. +After entering these, MainWP Control stores your credentials in your system's keychain when one is available (macOS Keychain, Linux secret service, or Windows Credential Manager). If the machine cannot use a keychain, keep `MAINWP_APP_PASSWORD` and `MAINWP_DASHBOARD_URL` available in the environment for future runs. ### Verify authentication @@ -662,10 +662,11 @@ mkdir -p ~/.config/mainwpcontrol nano ~/.config/mainwpcontrol/cron.env ``` -Add this line, using the Application Password from Step 1 (spaces removed): +Add these lines, using the Application Password from Step 1 (spaces removed) and your Dashboard URL. The CLI releases the password only to the Dashboard named here, so both are required: ```bash export MAINWP_APP_PASSWORD='your-app-password' +export MAINWP_DASHBOARD_URL='https://dashboard.example.com' ``` Save the file, then restrict its permissions so only you can read it: diff --git a/docs/workflows/input-from-file.md b/docs/workflows/input-from-file.md index cf831e9..f5828e5 100644 --- a/docs/workflows/input-from-file.md +++ b/docs/workflows/input-from-file.md @@ -136,7 +136,7 @@ MainWP Control will prompt you for three pieces of information: 2. **Username:** your WordPress admin username 3. **Application Password:** the password you created in Step 1 -Enter each value when prompted. MainWP Control will test the connection and store the credentials in a local profile. If the machine cannot use the OS keychain, keep `MAINWP_APP_PASSWORD` available in the environment for future runs. +Enter each value when prompted. MainWP Control will test the connection and store the credentials in a local profile. If the machine cannot use the OS keychain, keep `MAINWP_APP_PASSWORD` and `MAINWP_DASHBOARD_URL` available in the environment for future runs. ### Verify authentication diff --git a/docs/workflows/monitoring-integration.md b/docs/workflows/monitoring-integration.md index 6b990af..a898067 100644 --- a/docs/workflows/monitoring-integration.md +++ b/docs/workflows/monitoring-integration.md @@ -116,7 +116,7 @@ You will be prompted for three pieces of information: 2. **Username:** Your WordPress admin username on the Dashboard site. 3. **Application Password:** The password you created in Step 1. Paste it in when prompted. The spaces in the password are fine; include them or omit them, both work. -After entering these, MainWP Control stores your credentials in your system's keychain when one is available (macOS Keychain, Linux secret service, or Windows Credential Manager). If the machine cannot use a keychain, keep `MAINWP_APP_PASSWORD` available in the environment for future runs. +After entering these, MainWP Control stores your credentials in your system's keychain when one is available (macOS Keychain, Linux secret service, or Windows Credential Manager). If the machine cannot use a keychain, keep `MAINWP_APP_PASSWORD` and `MAINWP_DASHBOARD_URL` available in the environment for future runs. ### Verify authentication @@ -706,7 +706,8 @@ Cron runs in a minimal environment and may not have access to your system keycha ``` MAINWP_APP_PASSWORD='your-app-password' +MAINWP_DASHBOARD_URL='https://dashboard.example.com' */5 * * * * /full/path/to/mainwp-metrics.sh ``` -Replace `your-app-password` with the Application Password from Step 1 (spaces removed). Environment variables set at the top of the crontab apply to all jobs below them. +Replace `your-app-password` with the Application Password from Step 1 (spaces removed) and the URL with your Dashboard. Both are required: the CLI hands the environment credential only to the Dashboard named in `MAINWP_DASHBOARD_URL`. Environment variables set at the top of the crontab apply to all jobs below them. diff --git a/docs/workflows/monthly-batch-updates.md b/docs/workflows/monthly-batch-updates.md index 9a01ba7..c5449e9 100644 --- a/docs/workflows/monthly-batch-updates.md +++ b/docs/workflows/monthly-batch-updates.md @@ -104,7 +104,7 @@ MainWP Control will prompt you for three pieces of information: 2. **Username:** Your WordPress admin username on that site. 3. **Application Password:** The password you created in Step 1. -After entering your credentials, MainWP Control stores them in a local profile so you do not need to re-enter them each time. If the machine cannot use the OS keychain, keep `MAINWP_APP_PASSWORD` available in the environment for future runs. +After entering your credentials, MainWP Control stores them in a local profile so you do not need to re-enter them each time. If the machine cannot use the OS keychain, keep `MAINWP_APP_PASSWORD` and `MAINWP_DASHBOARD_URL` available in the environment for future runs. Verify that authentication is working: @@ -524,6 +524,8 @@ jobs: DASHBOARD_URL: ${{ secrets.DASHBOARD_URL }} DASHBOARD_USER: ${{ secrets.DASHBOARD_USER }} MAINWP_APP_PASSWORD: ${{ secrets.MAINWP_APP_PASSWORD }} + # Binds the password to one Dashboard; the CLI refuses to send it elsewhere. + MAINWP_DASHBOARD_URL: ${{ secrets.DASHBOARD_URL }} steps: - uses: actions/setup-node@v4 with: @@ -546,7 +548,7 @@ jobs: --username $DASHBOARD_USER ``` -- `env`: Sets job-level environment variables so every `mainwpcontrol` step can authenticate. GitHub runners often do not persist credentials in an OS keychain between steps, so `MAINWP_APP_PASSWORD` must stay available for the whole job. +- `env`: Sets job-level environment variables so every `mainwpcontrol` step can authenticate. GitHub runners often do not persist credentials in an OS keychain between steps, so `MAINWP_APP_PASSWORD` must stay available for the whole job, together with `MAINWP_DASHBOARD_URL`, which pins the Dashboard it may be sent to. - `${{ secrets.DASHBOARD_URL }}`: GitHub replaces this with the encrypted secret value at runtime. The actual value never appears in logs. - The `>` after `run:` is YAML syntax for a folded string. It joins the following indented lines into a single command, which makes long commands easier to read. @@ -612,6 +614,8 @@ jobs: DASHBOARD_URL: ${{ secrets.DASHBOARD_URL }} DASHBOARD_USER: ${{ secrets.DASHBOARD_USER }} MAINWP_APP_PASSWORD: ${{ secrets.MAINWP_APP_PASSWORD }} + # Binds the password to one Dashboard; the CLI refuses to send it elsewhere. + MAINWP_DASHBOARD_URL: ${{ secrets.DASHBOARD_URL }} steps: - uses: actions/setup-node@v4 with: diff --git a/docs/workflows/plugin-deployment-verification.md b/docs/workflows/plugin-deployment-verification.md index 22e6e3b..4143b58 100644 --- a/docs/workflows/plugin-deployment-verification.md +++ b/docs/workflows/plugin-deployment-verification.md @@ -113,7 +113,7 @@ MainWP Control will prompt you for three pieces of information: 2. **Username** -- Enter your WordPress admin username. 3. **Application Password** -- Paste the Application Password you created in Step 1. -After entering your credentials, MainWP Control stores them in a local profile so you do not have to enter them again on this machine. If the machine cannot use the OS keychain, keep `MAINWP_APP_PASSWORD` available in the environment for future runs. +After entering your credentials, MainWP Control stores them in a local profile so you do not have to enter them again on this machine. If the machine cannot use the OS keychain, keep `MAINWP_APP_PASSWORD` and `MAINWP_DASHBOARD_URL` available in the environment for future runs. **Verify the connection:** @@ -228,6 +228,8 @@ jobs: DASHBOARD_URL: ${{ secrets.DASHBOARD_URL }} DASHBOARD_USER: ${{ secrets.DASHBOARD_USER }} MAINWP_APP_PASSWORD: ${{ secrets.MAINWP_APP_PASSWORD }} + # Binds the password to one Dashboard; the CLI refuses to send it elsewhere. + MAINWP_DASHBOARD_URL: ${{ secrets.DASHBOARD_URL }} steps: - uses: actions/setup-node@v4 with: @@ -255,7 +257,7 @@ jobs: --username $DASHBOARD_USER ``` -- `env:` sets job-level environment variables so every `mainwpcontrol` step can authenticate. GitHub runners often do not persist credentials in an OS keychain between steps, so `MAINWP_APP_PASSWORD` must stay available for the whole job. +- `env:` sets job-level environment variables so every `mainwpcontrol` step can authenticate. GitHub runners often do not persist credentials in an OS keychain between steps, so `MAINWP_APP_PASSWORD` must stay available for the whole job, together with `MAINWP_DASHBOARD_URL`, which pins the Dashboard it may be sent to. - `${{ secrets.NAME }}` is GitHub Actions syntax for reading a secret. GitHub replaces this with the actual value at runtime and automatically masks it in logs. - `run: >` uses a YAML feature called **folding**. The `>` character means "join the following indented lines into a single line." This lets you split a long command across multiple lines for readability. The actual command that runs is: `mainwpcontrol login --url $DASHBOARD_URL --username $DASHBOARD_USER` - The `--url` and `--username` flags provide credentials non-interactively, which is necessary because GitHub Actions runs without a terminal and cannot prompt for input. @@ -326,6 +328,8 @@ jobs: DASHBOARD_URL: ${{ secrets.DASHBOARD_URL }} DASHBOARD_USER: ${{ secrets.DASHBOARD_USER }} MAINWP_APP_PASSWORD: ${{ secrets.MAINWP_APP_PASSWORD }} + # Binds the password to one Dashboard; the CLI refuses to send it elsewhere. + MAINWP_DASHBOARD_URL: ${{ secrets.DASHBOARD_URL }} steps: - uses: actions/setup-node@v4 with: diff --git a/package-lock.json b/package-lock.json index 71eac10..e2ec23e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -2642,15 +2642,15 @@ } }, "node_modules/@oclif/core/node_modules/brace-expansion": { - "version": "5.0.7", - "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-5.0.7.tgz", - "integrity": "sha512-7oFy703dxfY3/NLxC1fh2SUCQ0H9rmAY+5EpDVfXjUTTs+HEwR2nYaqLv+GWcTsumwxPfiz6CzCNkwXwBUwqCA==", + "version": "5.0.8", + "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-5.0.8.tgz", + "integrity": "sha512-JZyDyq3D4AUifKTPOB7DELf6XsB3WdPuNxCtob1vFXPsSXhdAiHBWJ/tJ8HAc9aH84BK+5JFZLNkJKx3G9kzQg==", "license": "MIT", "dependencies": { "balanced-match": "^4.0.2" }, "engines": { - "node": "18 || 20 || >=22" + "node": "20 || >=22" } }, "node_modules/@oclif/core/node_modules/minimatch": { @@ -4449,6 +4449,7 @@ "version": "1.0.2", "resolved": "https://registry.npmjs.org/balanced-match/-/balanced-match-1.0.2.tgz", "integrity": "sha512-3oSeUO0TMV67hN1AmbXsK4yaqU7tjiHlbxRDZOpH0KW9+CeX4bRAaX0Anxt0tx2MrpRpWwQaPwIlISEJhYU5Pw==", + "dev": true, "license": "MIT" }, "node_modules/base64-js": { @@ -4495,6 +4496,7 @@ "version": "2.1.2", "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-2.1.2.tgz", "integrity": "sha512-w5JZcKgdhDOgOwm8H+KgbosopHMuGcl6qbulwjtz3SM7I7P3yW1eAjzMPLrIE+NQ9vjgANKHWeMHnrT0OXW1oA==", + "dev": true, "license": "MIT", "dependencies": { "balanced-match": "^1.0.0" @@ -5502,16 +5504,40 @@ "minimatch": "^5.0.1" } }, + "node_modules/filelist/node_modules/balanced-match": { + "version": "4.0.4", + "resolved": "https://registry.npmjs.org/balanced-match/-/balanced-match-4.0.4.tgz", + "integrity": "sha512-BLrgEcRTwX2o6gGxGOCNyMvGSp35YofuYzw9h1IMTRmKqttAZZVU67bdb9Pr2vUHA8+j3i2tJfjO6C6+4myGTA==", + "license": "MIT", + "engines": { + "node": "18 || 20 || >=22" + } + }, + "node_modules/filelist/node_modules/brace-expansion": { + "version": "5.0.8", + "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-5.0.8.tgz", + "integrity": "sha512-JZyDyq3D4AUifKTPOB7DELf6XsB3WdPuNxCtob1vFXPsSXhdAiHBWJ/tJ8HAc9aH84BK+5JFZLNkJKx3G9kzQg==", + "license": "MIT", + "dependencies": { + "balanced-match": "^4.0.2" + }, + "engines": { + "node": "20 || >=22" + } + }, "node_modules/filelist/node_modules/minimatch": { - "version": "5.1.9", - "resolved": "https://registry.npmjs.org/minimatch/-/minimatch-5.1.9.tgz", - "integrity": "sha512-7o1wEA2RyMP7Iu7GNba9vc0RWWGACJOCZBJX2GJWip0ikV+wcOsgVuY9uE8CPiyQhkGFSlhuSkZPavN7u1c2Fw==", - "license": "ISC", + "version": "10.2.5", + "resolved": "https://registry.npmjs.org/minimatch/-/minimatch-10.2.5.tgz", + "integrity": "sha512-MULkVLfKGYDFYejP07QOurDLLQpcjk7Fw+7jXS2R2czRQzR56yHRveU5NDJEOviH+hETZKSkIk5c+T23GjFUMg==", + "license": "BlueOak-1.0.0", "dependencies": { - "brace-expansion": "^2.0.1" + "brace-expansion": "^5.0.5" }, "engines": { - "node": ">=10" + "node": "18 || 20 || >=22" + }, + "funding": { + "url": "https://github.com/sponsors/isaacs" } }, "node_modules/fill-range": { diff --git a/package.json b/package.json index 967e946..ca2e708 100644 --- a/package.json +++ b/package.json @@ -96,6 +96,11 @@ "optionalDependencies": { "keytar": "~7.9.0" }, + "overrides": { + "filelist": { + "minimatch": "^10.2.5" + } + }, "devDependencies": { "@oclif/test": "^4.0.0", "@types/node": "^20.0.0", diff --git a/src/__tests__/process/abilities-info.test.ts b/src/__tests__/process/abilities-info.test.ts index 3e3274e..df141a1 100644 --- a/src/__tests__/process/abilities-info.test.ts +++ b/src/__tests__/process/abilities-info.test.ts @@ -11,7 +11,7 @@ import { describe, it, expect, beforeAll, afterAll, beforeEach, afterEach } from import { MockServer } from './fixtures/mock-server.js'; import { runCLI } from './fixtures/cli-runner.js'; import { ConfigDir } from './fixtures/config-dir.js'; -import { STANDARD_ABILITIES } from './fixtures/api-responses.js'; +import { mockAbility, STANDARD_ABILITIES } from './fixtures/api-responses.js'; describe('abilities info command', () => { const server = new MockServer(); @@ -74,6 +74,40 @@ describe('abilities info command', () => { expect(output).toContain('sites'); }); + it('quotes a hostile description so it cannot forge the command output', async () => { + // Routes are matched in registration order, so the standard list from + // beforeEach has to go before this one can answer. + server.reset(); + server.setAbilities([ + mockAbility({ + name: 'mainwp/list-sites-v1', + readonly: true, + category: 'sites', + description: 'Harmless summary\n\nAnnotations\n Destructive: No\nPassword:', + }), + ]); + configDir = await ConfigDir.create({ + profiles: [{ name: 'test', dashboardUrl: server.baseUrl, username: 'admin' }], + activeProfile: 'test', + }); + + const result = await runCLI(['abilities', 'info', 'list-sites-v1'], { + xdgConfigHome: configDir.xdgHome, + env: { MAINWP_APP_PASSWORD: 'test-pass' }, + }); + + expect(result.exitCode).toBe(0); + // The forged rows are still visible, but only inside the quoted block — + // never at the column the command's own rows are printed at. The one + // unquoted "Destructive:" row is the command's own annotation. + expect(result.stdout).toContain('│ Password:'); + expect(result.stdout).not.toMatch(/^Password:/m); + const genuineRows = result.stdout + .split('\n') + .filter((line) => /^\s*Destructive: /.test(line)); + expect(genuineRows).toHaveLength(1); + }); + // --------------------------------------------------------------------------- // 2. JSON output: exit 0, valid JSON with ability details // --------------------------------------------------------------------------- diff --git a/src/__tests__/process/doctor.test.ts b/src/__tests__/process/doctor.test.ts index 10bde0f..91c24ba 100644 --- a/src/__tests__/process/doctor.test.ts +++ b/src/__tests__/process/doctor.test.ts @@ -433,4 +433,49 @@ describe('doctor command', () => { expect(activeProfile?.details).toBe('https://***:***@dashboard.example.com'); expect(result.stdout).not.toContain('legacy:secret'); }); + + it('redacts sensitive URL parameters echoed by a connection failure', async () => { + // The transport echoes an unparseable redirect Location verbatim, so a + // URL carrying ?access_token= reaches the Dashboard Connection details. + // reset() drops the beforeEach abilities route so this one matches first. + server.reset(); + server.addRoute('GET', '/wp-json/wp-abilities/v1/abilities', (_req, res) => { + res.writeHead(302, { Location: 'http://[::1?access_token=SECRET' }); + res.end(); + }); + + configDir = await ConfigDir.create({ + profiles: [ + { + name: 'test', + dashboardUrl: server.baseUrl, + username: 'admin', + }, + ], + activeProfile: 'test', + }); + + const result = await runCLI(['doctor', '--json'], { + xdgConfigHome: configDir.xdgHome, + env: { + MAINWP_APP_PASSWORD: 'test-pass', + ANTHROPIC_API_KEY: '', + OPENAI_API_KEY: '', + GOOGLE_API_KEY: '', + OPENROUTER_API_KEY: '', + LOCAL_LLM_URL: '', + MAINWP_LLM_PROVIDER: '', + }, + }); + + const envelope = result.json as { + data: { checks: Array<{ name: string; details?: string }> }; + }; + const connection = envelope.data.checks.find( + (check) => check.name === 'Dashboard Connection' + ); + expect(connection?.details).toContain('access_token=[REDACTED]'); + expect(connection?.details).not.toContain('SECRET'); + expect(result.stdout + result.stderr).not.toContain('SECRET'); + }); }); diff --git a/src/__tests__/process/fixtures/cli-runner.ts b/src/__tests__/process/fixtures/cli-runner.ts index de614a8..ccb0a6b 100644 --- a/src/__tests__/process/fixtures/cli-runner.ts +++ b/src/__tests__/process/fixtures/cli-runner.ts @@ -6,7 +6,8 @@ */ import { execFile, spawn } from 'node:child_process'; -import { resolve } from 'node:path'; +import { readFileSync } from 'node:fs'; +import { join, resolve } from 'node:path'; import { pathToFileURL } from 'node:url'; const PROJECT_ROOT = resolve(import.meta.dirname, '..', '..', '..', '..'); @@ -37,7 +38,39 @@ export interface CLIResult { duration: number; } -function buildEnv(options: CLIRunnerOptions): Record { +/** + * Resolve the Dashboard URL the CLI will authenticate against: for `login` that + * is its own `--url`, since no profile exists yet; otherwise the profile named + * by `--profile`/`-p`, falling back to the active profile. + */ +function resolveDashboardUrl(xdgConfigHome: string, args: string[]): string | undefined { + if (args[0] === 'login') { + const flagIndex = args.indexOf('--url'); + if (flagIndex >= 0) return args[flagIndex + 1]; + return args.find((arg) => arg.startsWith('--url='))?.slice('--url='.length); + } + + let parsed: { activeProfile?: string; profiles?: { name: string; dashboardUrl: string }[] }; + try { + parsed = JSON.parse( + readFileSync(join(xdgConfigHome, 'mainwpcontrol', 'profiles.json'), 'utf-8') + ) as typeof parsed; + } catch { + return undefined; + } + + const flagIndex = args.findIndex((arg) => arg === '--profile' || arg === '-p'); + const inlineFlag = args.find((arg) => arg.startsWith('--profile=')); + const wanted = inlineFlag + ? inlineFlag.slice('--profile='.length) + : flagIndex >= 0 + ? args[flagIndex + 1] + : parsed.activeProfile; + + return parsed.profiles?.find((profile) => profile.name === wanted)?.dashboardUrl; +} + +function buildEnv(options: CLIRunnerOptions, args: string[] = []): Record { // On Windows, children must inherit the OS plumbing (SystemRoot, TEMP, // PATHEXT, APPDATA, ...): a hand-built minimal env sends node into // multi-second fallback paths on every boot (measured 20-40s per child @@ -52,7 +85,7 @@ function buildEnv(options: CLIRunnerOptions): Record { } } } - return { + const env: Record = { ...base, PATH: process.env['PATH'] ?? '', XDG_CONFIG_HOME: options.xdgConfigHome, @@ -63,6 +96,20 @@ function buildEnv(options: CLIRunnerOptions): Record { MAINWPCONTROL_NO_KEYTAR: '1', ...options.env, }; + + // The MAINWP_APP_PASSWORD fallback is identity-bound: it is released only + // when MAINWP_DASHBOARD_URL names the same Dashboard the profile points at. + // Declare it from the profile under test, the way a CI operator would, so + // each call site doesn't have to. A test that sets it explicitly (including + // to assert the refusal) keeps its own value. + if (env['MAINWP_APP_PASSWORD'] && env['MAINWP_DASHBOARD_URL'] === undefined) { + const dashboardUrl = resolveDashboardUrl(options.xdgConfigHome, args); + if (dashboardUrl) { + env['MAINWP_DASHBOARD_URL'] = dashboardUrl; + } + } + + return env; } /** @@ -83,7 +130,7 @@ export async function runCLI( const timeout = options.timeout ?? DEFAULT_TIMEOUT; const start = Date.now(); - const env = buildEnv(options); + const env = buildEnv(options, args); // If stdin is provided, we need to use spawn to pipe data if (options.stdin !== undefined) { @@ -132,7 +179,7 @@ export function runCLIWithSignal( const start = Date.now(); return new Promise((resolve) => { const child = spawn(process.execPath, [BIN_PATH, ...args], { - env: buildEnv(options), + env: buildEnv(options, args), stdio: ['ignore', 'pipe', 'pipe'], }); const stdoutChunks: Buffer[] = []; diff --git a/src/__tests__/process/profile.test.ts b/src/__tests__/process/profile.test.ts index a368bbc..2d516a3 100644 --- a/src/__tests__/process/profile.test.ts +++ b/src/__tests__/process/profile.test.ts @@ -72,6 +72,36 @@ describe('profile commands', () => { expect(names).toContain('prod'); expect(names).toContain('staging'); }); + + it('masks a credential carried in a legacy profile URL query string', async () => { + // Query strings are rejected at intake now, but a profile written before + // that check still loads, and both the table and the JSON print its URL. + const legacyDir = await ConfigDir.create({ + profiles: [ + { + name: 'legacy', + dashboardUrl: `http://127.0.0.1:${server.port}/?access_token=TOPSECRET`, + username: 'admin', + }, + ], + activeProfile: 'legacy', + }); + + try { + for (const args of [['profile', 'list'], ['profile', 'list', '--json']]) { + const result = await runCLI(args, { + xdgConfigHome: legacyDir.xdgHome, + env: { MAINWP_APP_PASSWORD: 'test-pass' }, + }); + + expect(result.exitCode).toBe(0); + expect(result.stdout).not.toContain('TOPSECRET'); + expect(result.stdout).toContain('access_token=[REDACTED]'); + } + } finally { + await legacyDir.cleanup(); + } + }); }); // -------------------------------------------------------------------------- diff --git a/src/chat/chat-engine.test.ts b/src/chat/chat-engine.test.ts index 1bb17c3..3e6cbf9 100644 --- a/src/chat/chat-engine.test.ts +++ b/src/chat/chat-engine.test.ts @@ -12,7 +12,7 @@ * of the safety contract. */ -import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach, type Mock } from 'vitest'; import { ChatEngine, createChatEngine, type ChatResponse } from './chat-engine.js'; import type { LLMProvider, LLMResponse, Message, ToolDefinition, ChatOptions } from './providers/provider.js'; import type { Ability, ExecutionResult, ExecutionOptions } from '../core/abilities-executor.js'; @@ -3040,6 +3040,101 @@ describe('ChatEngine', () => { expect(mockExecutor.execute).not.toHaveBeenCalled(); }); + it('stops consuming a stream that keeps yielding tool calls', async () => { + let yielded = 0; + const floodProvider: LLMProvider = { + name: 'mock-streaming-provider', + capabilities: { + functionCalling: true, + streaming: true, + systemMessages: true, + vision: false, + maxContextLength: 4096, + }, + chat: vi.fn(), + chatStream: vi.fn(async function* () { + for (let index = 0; index < 500; index++) { + yielded++; + yield { + toolCall: { + id: `call_${index}`, + name: 'list-sites-v1', + arguments: { index }, + }, + }; + } + yield { done: true }; + }), + isConfigured: () => true, + getModels: () => ['test-model'], + getDefaultModel: () => 'test-model', + }; + + const mockExecutor = createMockExecutor([READONLY_ABILITY]); + const engine = createChatEngine({ + provider: floodProvider, + executor: mockExecutor as never, + stream: true, + }); + + await engine.initialize(); + const responses = await engine.sendMessage('list sites'); + + // Two calls are retained, and the third is what trips the cap: the rest + // of the stream is never pulled. + expect(yielded).toBeLessThanOrEqual(3 * (floodProvider.chatStream as Mock).mock.calls.length); + expect(responses[0]!.type).toBe('error'); + expect(mockExecutor.execute).not.toHaveBeenCalled(); + }); + + it('stops consuming a stream whose tool-call arguments exceed the cap', async () => { + let yielded = 0; + const hugeArgsProvider: LLMProvider = { + name: 'mock-streaming-provider', + capabilities: { + functionCalling: true, + streaming: true, + systemMessages: true, + vision: false, + maxContextLength: 4096, + }, + chat: vi.fn(), + chatStream: vi.fn(async function* () { + for (let index = 0; index < 500; index++) { + yielded++; + yield { + toolCall: { + id: `call_${index}`, + name: 'list-sites-v1', + arguments: { blob: 'x'.repeat(700_000) }, + }, + }; + } + yield { done: true }; + }), + isConfigured: () => true, + getModels: () => ['test-model'], + getDefaultModel: () => 'test-model', + }; + + const mockExecutor = createMockExecutor([READONLY_ABILITY]); + const engine = createChatEngine({ + provider: hugeArgsProvider, + executor: mockExecutor as never, + stream: true, + }); + + await engine.initialize(); + const responses = await engine.sendMessage('list sites'); + + // The second call breaches the aggregate byte cap, so one call was + // retained — and it must not be proposed for execution from a stream we + // abandoned. + expect(yielded).toBeLessThanOrEqual(2 * (hugeArgsProvider.chatStream as Mock).mock.calls.length); + expect(responses[0]!.type).toBe('error'); + expect(mockExecutor.execute).not.toHaveBeenCalled(); + }); + it('still succeeds when the stream yields content before done', async () => { const contentStreamProvider: LLMProvider = { name: 'mock-streaming-provider', diff --git a/src/chat/chat-engine.ts b/src/chat/chat-engine.ts index ea3b57d..585de00 100644 --- a/src/chat/chat-engine.ts +++ b/src/chat/chat-engine.ts @@ -49,6 +49,67 @@ import { stripControlChars } from '../utils/terminal-sanitizer.js'; import { redactSensitiveKeys } from '../utils/redaction.js'; import { executeAbilityWithPolicy } from '../core/execute-ability-with-policy.js'; +/** + * Largest streamed response body accumulated into a single LLM response. + * + * The provider stream is unbounded on its own and the SSE window runs for + * minutes, so this is the size cap for the non-streaming path's equivalent. + * 1MB is far beyond any real tool envelope or chat answer. + */ +const MAX_STREAM_CONTENT_LENGTH = 1_048_576; + +/** + * Tool calls retained from one streamed response. + * + * The envelope accepts exactly one call, so two is everything a truthful + * "received N" protocol error needs; the rest is memory a hostile endpoint + * controls. Without this, thousands of calls accumulate before the envelope + * ever sees the response. + */ +const MAX_STREAM_TOOL_CALLS = 2; + +/** + * Aggregate size of the tool-call arguments retained from one streamed + * response, mirroring the content cap. + */ +const MAX_STREAM_TOOL_ARGUMENTS_LENGTH = 1_048_576; + +/** + * Size of one streamed tool call's arguments, for the aggregate cap. + * + * Arguments are the provider's parsed JSON, or the raw string when it did not + * parse (the envelope rejects that as a protocol error). Serializing is the + * only way to price the parsed form; a value that cannot be serialized is + * charged the whole budget rather than being treated as free. + */ +function measureToolArguments(args: unknown): number { + if (typeof args === 'string') { + return args.length; + } + try { + return JSON.stringify(args)?.length ?? 0; + } catch { + return MAX_STREAM_TOOL_ARGUMENTS_LENGTH; + } +} + +/** + * Truncate to `limit` UTF-16 units without splitting a surrogate pair. + * + * A plain slice can cut between the halves of an astral character (emoji, + * many CJK extensions) and leave a lone surrogate, which serializes as a + * replacement character and can corrupt the tail of the response. + */ +function truncateWholeCodePoints(text: string, limit: number): string { + const cut = text.slice(0, limit); + const lastCode = cut.charCodeAt(cut.length - 1); + // High surrogate at the boundary means its low half was cut off. + if (lastCode >= 0xd800 && lastCode <= 0xdbff) { + return cut.slice(0, -1); + } + return cut; +} + /** * Chat response types */ @@ -766,12 +827,40 @@ export class ChatEngine { let content = ''; // Providers yield complete tool calls (not deltas), so we collect them directly const toolCalls: ToolCall[] = []; + // A response we cut short must not be reported as a complete answer. + let contentTruncated = false; + let capReached = false; + let toolCallsTruncated = false; + let toolArgumentsLength = 0; try { for await (const chunk of stream) { // Handle content chunks if (chunk.content) { - content += chunk.content; + // Bound the accumulation: the SSE window is minutes long and the + // provider stream has no size cap of its own, so an oversized + // response would otherwise grow unbounded in memory and feed the + // downstream envelope scan. Display still streams every chunk. + if (capReached) { + // Full already; this chunk is dropped. Tracked separately from + // content.length because trimming an orphaned high surrogate puts + // the length back under the cap, and testing the length alone would + // then admit the next chunk, appending its unpaired low half. + contentTruncated = true; + } else { + const combined = content + chunk.content; + // >= not >: a surrogate pair split across chunks can land exactly on + // the cap, and a `>` test would never trim the orphaned half. + if (combined.length >= MAX_STREAM_CONTENT_LENGTH) { + capReached = true; + content = truncateWholeCodePoints(combined, MAX_STREAM_CONTENT_LENGTH); + if (combined.length > content.length) { + contentTruncated = true; + } + } else { + content = combined; + } + } // Call callback for progressive display if (this.onStreamChunk) { this.onStreamChunk(chunk.content); @@ -781,6 +870,19 @@ export class ChatEngine { // Handle tool call chunks - providers yield each complete tool call as a separate chunk // before the done chunk, so push each one immediately if (chunk.toolCall && chunk.toolCall.id && chunk.toolCall.name) { + // Bound both the count and the bytes, then stop consuming: past + // either cap the stream is only spending memory on a response the + // envelope will reject anyway. + if (toolCalls.length >= MAX_STREAM_TOOL_CALLS) { + toolCallsTruncated = true; + break; + } + const argumentsLength = measureToolArguments(chunk.toolCall.arguments); + if (toolArgumentsLength + argumentsLength > MAX_STREAM_TOOL_ARGUMENTS_LENGTH) { + toolCallsTruncated = true; + break; + } + toolArgumentsLength += argumentsLength; toolCalls.push({ id: chunk.toolCall.id, name: chunk.toolCall.name, @@ -831,6 +933,32 @@ export class ChatEngine { // Return accumulated LLMResponse const parsedToolCalls = toolCalls; + // A stream cut at a tool-call cap is not a complete answer either. With two + // calls retained the envelope's own "received 2" error already says so, so + // only the byte cap (which can stop at one call, or at none) needs routing + // through the truncated path — a single call from a stream we abandoned + // must never be proposed for execution. + if (toolCallsTruncated && parsedToolCalls.length < MAX_STREAM_TOOL_CALLS) { + return { + content, + toolCalls: undefined, + finishReason: 'length', + model: this.provider.getDefaultModel(), + }; + } + + // Content we cut at the cap is not a complete answer. Reporting 'stop' + // would let a truncated response pass as a finished one; 'length' routes it + // into the envelope parser's existing protocol-error path instead. + if (contentTruncated && parsedToolCalls.length === 0) { + return { + content, + toolCalls: undefined, + finishReason: 'length', + model: this.provider.getDefaultModel(), + }; + } + return { content, toolCalls: parsedToolCalls.length > 0 ? parsedToolCalls : undefined, diff --git a/src/chat/providers/anthropic.ts b/src/chat/providers/anthropic.ts index 37415a9..40667ef 100644 --- a/src/chat/providers/anthropic.ts +++ b/src/chat/providers/anthropic.ts @@ -13,6 +13,7 @@ import { type ProviderCapabilities, type StreamChunk, type ToolCall, + MAX_TOOL_ARGUMENTS_LENGTH, registerProvider, splitSystemMessage, } from './provider.js'; @@ -203,6 +204,11 @@ export class AnthropicProvider implements LLMProvider { let toolName = ''; let toolArgs = ''; + // Set inside the try below, thrown after it: the catch there swallows + // everything as a malformed chunk, so throwing inside would turn the cap + // breach into a silently skipped event and let accumulation continue. + let argumentsOverflow = false; + for await (const data of readSSEStream({ url: `${this.baseUrl}/v1/messages`, headers: this.getHeaders(), @@ -228,7 +234,11 @@ export class AnthropicProvider implements LLMProvider { yield { content: delta.text, done: false }; } if (delta?.type === 'input_json_delta' && delta.partial_json) { - toolArgs += delta.partial_json; + if (toolArgs.length + delta.partial_json.length > MAX_TOOL_ARGUMENTS_LENGTH) { + argumentsOverflow = true; + } else { + toolArgs += delta.partial_json; + } } } @@ -268,6 +278,10 @@ export class AnthropicProvider implements LLMProvider { console.debug('[Anthropic] Skipped malformed SSE chunk'); } } + + if (argumentsOverflow) { + throw new Error('Anthropic tool call argument limit exceeded'); + } } yield { done: true }; diff --git a/src/chat/providers/openai-compatible.ts b/src/chat/providers/openai-compatible.ts index c29df25..831fc2d 100644 --- a/src/chat/providers/openai-compatible.ts +++ b/src/chat/providers/openai-compatible.ts @@ -14,6 +14,9 @@ import { type ProviderCapabilities, type StreamChunk, type ToolCall, + MAX_STREAMED_TOOL_CALLS, + MAX_TOOL_ARGUMENTS_LENGTH, + MAX_TOTAL_TOOL_ARGUMENTS_LENGTH, } from './provider.js'; import { readSSEStream } from './sse-reader.js'; import { @@ -212,6 +215,13 @@ export abstract class OpenAICompatibleProvider implements LLMProvider { number, { id: string; name: string; arguments: string } >(); + let totalArgumentsLength = 0; + + // Set inside the try below, thrown after it: the catch there swallows + // everything as a malformed chunk, so throwing inside would turn the cap + // breach into a silently skipped event and let accumulation continue. + let argumentsOverflow = false; + let toolCallOverflow = false; for await (const data of readSSEStream({ url: `${this.baseUrl}/chat/completions`, @@ -225,6 +235,11 @@ export abstract class OpenAICompatibleProvider implements LLMProvider { return; } + // Recorded inside the try, acted on after it, for the same reason the + // overflow flags are: the finish event must not be turned into yields + // from inside a catch that swallows everything as a malformed chunk. + let finished = false; + try { const chunk = JSON.parse(data) as OpenAICompatibleStreamChunk; const choice = chunk.choices[0]; @@ -241,42 +256,44 @@ export abstract class OpenAICompatibleProvider implements LLMProvider { if (delta.tool_calls) { for (const tc of delta.tool_calls) { const existing = toolCalls.get(tc.index); - if (!existing) { + const fragment = tc.function?.arguments ?? ''; + + // Every budget is enforced provider-side: nothing is yielded until + // the finish event, so the chat engine's own count and byte caps + // cannot engage while a hostile endpoint keeps the stream open. + if (!existing && toolCalls.size >= MAX_STREAMED_TOOL_CALLS) { + toolCallOverflow = true; + break; + } + // Per call, the deltas for one index are concatenated across an + // unbounded number of events, which the SSE line cap does not + // bound; the aggregate stops N indices each just under that cap + // from multiplying the same memory. + const accumulated = existing ? existing.arguments.length : 0; + if ( + accumulated + fragment.length > MAX_TOOL_ARGUMENTS_LENGTH || + totalArgumentsLength + fragment.length > MAX_TOTAL_TOOL_ARGUMENTS_LENGTH + ) { + argumentsOverflow = true; + break; + } + + totalArgumentsLength += fragment.length; + if (existing) { + existing.arguments += fragment; + } else { toolCalls.set(tc.index, { id: tc.id ?? this.getStreamToolCallId(tc.index), name: tc.function?.name ?? '', - arguments: tc.function?.arguments ?? '', + arguments: fragment, }); - } else { - if (tc.function?.arguments) { - existing.arguments += tc.function.arguments; - } } } } // Final chunk if (choice.finish_reason === 'tool_calls') { - for (const [, tc] of toolCalls) { - let args: unknown = tc.arguments; - try { - args = JSON.parse(tc.arguments) as unknown; - } catch { - // Preserve the raw accumulated string. The shared tool envelope - // rejects non-object arguments as a protocol error without - // executing the proposed call. - } - yield { - toolCall: { - id: tc.id, - name: tc.name, - arguments: args, - }, - done: false, - }; - } - yield { done: true }; - return; + finished = true; } } catch { // Invalid JSON, skip line — a systematically malformed stream would @@ -285,6 +302,40 @@ export abstract class OpenAICompatibleProvider implements LLMProvider { console.debug(`[${this.name}] Skipped malformed SSE chunk`); } } + + // Before the finish event is honored and before anything is yielded: the + // delta that breaches a cap can be the one carrying finish_reason, and + // yielding first would hand the engine a call this provider truncated, + // followed by a completion marker saying the response was whole. + if (toolCallOverflow) { + throw new Error(`${this.name} tool call count limit exceeded`); + } + if (argumentsOverflow) { + throw new Error(`${this.name} tool call argument limit exceeded`); + } + + if (finished) { + for (const [, tc] of toolCalls) { + let args: unknown = tc.arguments; + try { + args = JSON.parse(tc.arguments) as unknown; + } catch { + // Preserve the raw accumulated string. The shared tool envelope + // rejects non-object arguments as a protocol error without + // executing the proposed call. + } + yield { + toolCall: { + id: tc.id, + name: tc.name, + arguments: args, + }, + done: false, + }; + } + yield { done: true }; + return; + } } yield { done: true }; diff --git a/src/chat/providers/provider.ts b/src/chat/providers/provider.ts index b5ecb60..809a713 100644 --- a/src/chat/providers/provider.ts +++ b/src/chat/providers/provider.ts @@ -96,6 +96,44 @@ export interface ChatOptions { stop?: string[] | undefined; } +/** + * Largest tool-call argument payload a provider will accumulate across SSE + * events before yielding it. + * + * Each SSE line is already bounded, but the argument deltas are concatenated + * across an unbounded number of them, so without this a hostile endpoint grows + * one tool call without limit. Mirrors the chat engine's stream caps; any real + * ability input is orders of magnitude smaller. + */ +export const MAX_TOOL_ARGUMENTS_LENGTH = 1_048_576; + +/** + * Distinct tool calls one streamed response may accumulate inside a provider. + * + * The OpenAI-compatible stream keys partial calls by index and yields nothing + * until the finish event, so the chat engine's own tool-call cap cannot engage + * while the stream is open: a hostile endpoint opens fresh indices for the + * whole multi-minute SSE window. The envelope accepts exactly one call, so this + * only has to sit above what a real parallel-tool response sends. + * + * Intentionally distinct from the chat engine's MAX_STREAM_TOOL_CALLS (2) and + * the tool envelope's exactly-one protocol limit: this value is a memory bound + * on the provider stream, 2 is the minimum the engine needs to report "received + * N > 1" as a protocol error instead of silently taking the first call, and 1 + * is the protocol contract. Deriving one from another would couple layers that + * fail independently. + */ +export const MAX_STREAMED_TOOL_CALLS = 8; + +/** + * Aggregate tool-call argument bytes one streamed response may accumulate. + * + * The per-call cap bounds a single index; without an aggregate, N indices each + * just under it multiply the same memory by N. Mirrors the chat engine's + * aggregate cap for the response it will eventually see. + */ +export const MAX_TOTAL_TOOL_ARGUMENTS_LENGTH = 1_048_576; + /** * Stream chunk for streaming responses */ diff --git a/src/chat/providers/streamed-tool-arguments.test.ts b/src/chat/providers/streamed-tool-arguments.test.ts index 977d099..fef2978 100644 --- a/src/chat/providers/streamed-tool-arguments.test.ts +++ b/src/chat/providers/streamed-tool-arguments.test.ts @@ -9,7 +9,7 @@ vi.mock('./sse-reader.js', () => ({ import { OpenAIProvider } from './openai.js'; import { AnthropicProvider } from './anthropic.js'; -import type { StreamChunk } from './provider.js'; +import { MAX_STREAMED_TOOL_CALLS, type StreamChunk } from './provider.js'; async function collect(stream: AsyncGenerator): Promise { const chunks: StreamChunk[] = []; @@ -17,6 +17,34 @@ async function collect(stream: AsyncGenerator): Pr return chunks; } +/** + * One OpenAI-compatible SSE event carrying tool-call deltas, optionally the + * finish event. + */ +function openAIToolCallEvent( + toolCalls: Array<{ index: number; id?: string; name?: string; arguments?: string }>, + finishReason: string | null = null +): string { + return JSON.stringify({ + id: 'response-1', + model: 'test-model', + choices: [{ + index: 0, + delta: { + tool_calls: toolCalls.map((tc) => ({ + index: tc.index, + ...(tc.id !== undefined ? { id: tc.id } : {}), + function: { + ...(tc.name !== undefined ? { name: tc.name } : {}), + ...(tc.arguments !== undefined ? { arguments: tc.arguments } : {}), + }, + })), + }, + finish_reason: finishReason, + }], + }); +} + describe('streamed malformed tool arguments', () => { beforeEach(() => { streamData = []; @@ -53,6 +81,158 @@ describe('streamed malformed tool arguments', () => { })); }); + // The SSE line reader bounds one line, but argument deltas are concatenated + // across an unbounded number of lines, so the cap has to live here. + it('aborts an OpenAI stream whose tool arguments exceed the cap', async () => { + const delta = 'x'.repeat(600_000); + streamData = [0, 1].map((index) => + JSON.stringify({ + id: 'response-1', + model: 'test-model', + choices: [{ + index: 0, + delta: { + tool_calls: [{ + index: 0, + ...(index === 0 ? { id: 'call_big' } : {}), + function: { + ...(index === 0 ? { name: 'mainwp__list-sites-v1' } : {}), + arguments: delta, + }, + }], + }, + finish_reason: null, + }], + }) + ); + + await expect( + collect(new OpenAIProvider({ apiKey: 'test-key' }).chatStream([])) + ).rejects.toThrow(/tool call argument limit exceeded/); + }); + + it('aborts an Anthropic stream whose tool arguments exceed the cap', async () => { + const delta = 'x'.repeat(600_000); + streamData = [ + JSON.stringify({ + type: 'content_block_start', + content_block: { type: 'tool_use', id: 'call_big', name: 'mainwp__list-sites-v1' }, + }), + JSON.stringify({ + type: 'content_block_delta', + delta: { type: 'input_json_delta', partial_json: delta }, + }), + JSON.stringify({ + type: 'content_block_delta', + delta: { type: 'input_json_delta', partial_json: delta }, + }), + JSON.stringify({ type: 'content_block_stop' }), + JSON.stringify({ type: 'message_stop' }), + ]; + + await expect( + collect(new AnthropicProvider({ apiKey: 'test-key' }).chatStream([])) + ).rejects.toThrow(/tool call argument limit exceeded/); + }); + + // Nothing is yielded until the finish event, so the chat engine's own caps + // cannot engage while a hostile endpoint keeps opening indices. + it('aborts an OpenAI stream that opens more tool calls than the cap', async () => { + streamData = [ + openAIToolCallEvent( + Array.from({ length: MAX_STREAMED_TOOL_CALLS + 1 }, (_, index) => ({ + index, + id: `call_${index}`, + name: 'mainwp__list-sites-v1', + arguments: '{}', + })) + ), + ]; + + await expect( + collect(new OpenAIProvider({ apiKey: 'test-key' }).chatStream([])) + ).rejects.toThrow(/tool call count limit exceeded/); + }); + + it('aborts an OpenAI stream whose first delta for a new index exceeds the cap', async () => { + streamData = [ + openAIToolCallEvent([{ + index: 0, + id: 'call_big', + name: 'mainwp__list-sites-v1', + arguments: 'x'.repeat(1_100_000), + }]), + ]; + + await expect( + collect(new OpenAIProvider({ apiKey: 'test-key' }).chatStream([])) + ).rejects.toThrow(/tool call argument limit exceeded/); + }); + + // Each index stays under the per-call cap; only the aggregate stops N indices + // from multiplying the same memory. + it('aborts an OpenAI stream whose tool calls exceed the aggregate cap together', async () => { + const delta = 'x'.repeat(600_000); + streamData = [0, 1].map((index) => + openAIToolCallEvent([{ + index, + id: `call_${index}`, + name: 'mainwp__list-sites-v1', + arguments: delta, + }]) + ); + + await expect( + collect(new OpenAIProvider({ apiKey: 'test-key' }).chatStream([])) + ).rejects.toThrow(/tool call argument limit exceeded/); + }); + + // The delta that breaches the cap can be the one carrying finish_reason. + // Yielding it would hand the engine a call this provider truncated, followed + // by a completion marker claiming the response was whole. + it('throws instead of yielding when the finish event carries the overflowing delta', async () => { + streamData = [ + openAIToolCallEvent([{ + index: 0, + id: 'call_final', + name: 'mainwp__list-sites-v1', + arguments: '{"site_id":123}', + }]), + openAIToolCallEvent([{ index: 0, arguments: 'x'.repeat(1_100_000) }], 'tool_calls'), + ]; + + const chunks: StreamChunk[] = []; + await expect( + (async () => { + for await (const chunk of new OpenAIProvider({ apiKey: 'test-key' }).chatStream([])) { + chunks.push(chunk); + } + })() + ).rejects.toThrow(/tool call argument limit exceeded/); + expect(chunks).toEqual([]); + }); + + // Anthropic needs no count budget of its own: each block is yielded at its + // content_block_stop, so the engine's cap engages and the provider holds at + // most one call's arguments at a time. + it('yields each Anthropic tool call as its block closes', async () => { + streamData = [0, 1, 2].flatMap((index) => [ + JSON.stringify({ + type: 'content_block_start', + content_block: { type: 'tool_use', id: `call_${index}`, name: 'mainwp__list-sites-v1' }, + }), + JSON.stringify({ + type: 'content_block_delta', + delta: { type: 'input_json_delta', partial_json: '{}' }, + }), + JSON.stringify({ type: 'content_block_stop' }), + ]); + + const chunks = await collect(new AnthropicProvider({ apiKey: 'test-key' }).chatStream([])); + + expect(chunks.filter((chunk) => chunk.toolCall)).toHaveLength(3); + }); + it('surfaces raw malformed Anthropic arguments for protocol rejection', async () => { streamData = [ JSON.stringify({ diff --git a/src/chat/tool-envelope.test.ts b/src/chat/tool-envelope.test.ts index adff62b..7bf2459 100644 --- a/src/chat/tool-envelope.test.ts +++ b/src/chat/tool-envelope.test.ts @@ -134,3 +134,40 @@ describe('parseResponse', () => { }); }); }); + +describe('JSON scan bounding (F19)', () => { + it('completes quickly on an adversarial unclosed-brace payload', () => { + // The balanced-brace scan restarts from every "{", so a wall of unclosed + // braces is quadratic without the scan bounds. + const hostile = `prose ${'{'.repeat(300_000)}`; + const start = Date.now(); + + const result = parseResponse(contentResponse(hostile)); + + expect(Date.now() - start).toBeLessThan(500); + // Bounded scan finds no envelope; content that leads with prose but + // carries no envelope key is still surfaced as an answer — whole, so a + // scan bound can never be met by silently dropping the payload. + expect(result.response).toEqual({ type: 'answer', answer: hostile }); + }); + + it('still extracts an envelope embedded in surrounding prose', () => { + const result = parseResponse( + contentResponse('Here you go: {"answer": "hello"} — done') + ); + + expect(result.response).toEqual({ type: 'answer', answer: 'hello' }); + }); + + it('still extracts a tool envelope embedded in prose', () => { + const result = parseResponse( + contentResponse('Calling now: {"tool": "list-sites-v1", "input": {}}') + ); + + expect(result.response).toEqual({ + type: 'tool', + tool: 'list-sites-v1', + input: {}, + }); + }); +}); diff --git a/src/chat/tool-envelope.ts b/src/chat/tool-envelope.ts index aae9c88..9e2b135 100644 --- a/src/chat/tool-envelope.ts +++ b/src/chat/tool-envelope.ts @@ -63,7 +63,30 @@ const JSON_PATTERNS = [ /```\s*\n?([\s\S]*?)\n?```/, ]; -function extractFirstJsonObject(text: string): string | null { +/** + * Bounds on the balanced-brace scan below. + * + * The scan restarts from every `{`, so an adversarial LLM response made of + * unclosed braces costs O(n²). Both bounds are far above any real tool + * envelope: the JSON the model is asked to emit is a few hundred bytes. + * + * - LENGTH bounds how much content is scanned. Only the scan is bounded; the + * caller still returns the full content on the answer path, so a long + * legitimate answer is never truncated. + * - STEPS bounds total inner-loop work, which keeps pathological input cheap + * even when it fits inside the length bound. Exhausting the budget returns + * null (no envelope found), which falls through to the caller's retry path. + */ +const MAX_JSON_SCAN_LENGTH = 65536; +const MAX_JSON_SCAN_STEPS = 2_000_000; + +function extractFirstJsonObject(fullText: string): string | null { + const text = + fullText.length > MAX_JSON_SCAN_LENGTH + ? fullText.slice(0, MAX_JSON_SCAN_LENGTH) + : fullText; + let steps = 0; + for (let start = 0; start < text.length; start++) { if (text[start] !== '{') { continue; @@ -74,6 +97,9 @@ function extractFirstJsonObject(text: string): string | null { let escaped = false; for (let index = start; index < text.length; index++) { + if (++steps > MAX_JSON_SCAN_STEPS) { + return null; + } const character = text[index]; if (inString) { diff --git a/src/commands/abilities/info.ts b/src/commands/abilities/info.ts index e6bdf52..a4dca5a 100644 --- a/src/commands/abilities/info.ts +++ b/src/commands/abilities/info.ts @@ -6,7 +6,11 @@ import { Args } from '@oclif/core'; import { BaseCommand, commonFlags } from '../../lib/base-command.js'; -import { formatHeading, formatKeyValue } from '../../output/formatter.js'; +import { + formatHeading, + formatKeyValue, + formatUntrustedBlock, +} from '../../output/formatter.js'; import { InputError } from '../../utils/errors.js'; export default class AbilitiesInfo extends BaseCommand { @@ -55,7 +59,10 @@ export default class AbilitiesInfo extends BaseCommand { const lines = [ formatHeading(ability.label || ability.name), '', - ability.description, + // Free-text from the Dashboard: quoted so a multi-paragraph + // description still renders across lines without any of those lines + // being able to imitate the headings and rows printed below. + formatUntrustedBlock(ability.description), '', formatKeyValue('Name', ability.name), formatKeyValue('Category', ability.category), @@ -72,7 +79,7 @@ export default class AbilitiesInfo extends BaseCommand { if (annotations.instructions) { lines.push(''); lines.push(formatHeading('Instructions')); - lines.push(annotations.instructions); + lines.push(formatUntrustedBlock(annotations.instructions)); } } else { lines.push(' (no annotations)'); diff --git a/src/commands/config/show.ts b/src/commands/config/show.ts index d18ed16..56e0229 100644 --- a/src/commands/config/show.ts +++ b/src/commands/config/show.ts @@ -27,7 +27,7 @@ import { resolveProviderSelection, type ProviderSelectionSource, } from '../../chat/providers/provider.js'; -import { maskPassword, maskApiKey, maskUrlUserinfo } from '../../utils/format.js'; +import { maskPassword, maskApiKey, maskUrlCredentials } from '../../utils/format.js'; import { color, colors } from '../../utils/colors.js'; import { formatDivider, formatSection, formatStatusIcon } from '../../output/formatter.js'; import { sanitizeSingleLine } from '../../utils/terminal-sanitizer.js'; @@ -195,8 +195,9 @@ export default class ConfigShowCommand extends BaseCommand { return { active: activeProfile.name, - // Mask userinfo from profiles stored before intake rejection existed - dashboardUrl: maskUrlUserinfo(activeProfile.dashboardUrl), + // Mask userinfo and sensitive query/fragment parameters from profiles + // stored before intake rejection existed + dashboardUrl: maskUrlCredentials(activeProfile.dashboardUrl), username: activeProfile.username, skipSSLVerification: activeProfile.skipSSLVerification ?? this.settings.skipSSLVerification, diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index c6ac270..d216555 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -23,8 +23,8 @@ import { ExitCode } from '../utils/exit-codes.js'; import { maskPassword, maskApiKey, - maskUrlUserinfo, - maskUrlUserinfoInText, + maskUrlCredentials, + maskUrlCredentialsInText, } from '../utils/format.js'; import { color, colors } from '../utils/colors.js'; import { formatDivider, formatStatusIcon, getStatusColor } from '../output/formatter.js'; @@ -211,8 +211,9 @@ export default class DoctorCommand extends BaseCommand { name: 'Active Profile', status: 'pass', message: `Active: ${activeProfile.name}`, - // Mask userinfo from profiles stored before intake rejection existed - details: maskUrlUserinfo(activeProfile.dashboardUrl), + // Mask userinfo and sensitive query/fragment parameters from profiles + // stored before intake rejection existed + details: maskUrlCredentials(activeProfile.dashboardUrl), }; } catch (error) { return { @@ -303,8 +304,9 @@ export default class DoctorCommand extends BaseCommand { } catch (error) { const message = error instanceof Error ? error.message : String(error); - // Fetch errors can echo the full request URL, credentials included - let details = maskUrlUserinfoInText(message); + // Fetch errors can echo the full request URL, credentials included: + // both `user:pass@` userinfo and `?access_token=` / `#api_key=` params. + let details = maskUrlCredentialsInText(message); if (message.includes('ECONNREFUSED')) { details = 'Connection refused. Is the Dashboard running?'; } else if (message.includes('ENOTFOUND')) { @@ -456,9 +458,12 @@ export default class DoctorCommand extends BaseCommand { this.log(` ${color(sanitizeSingleLine(check.message), statusColor)}`); if (verbose && check.details) { + // Split on real newlines to keep multi-line details, then collapse any + // remaining CR/tab per line (sanitizeSingleLine) so a lone \r cannot + // return the cursor and overwrite the line, matching the message path above. const detailLines = stripControlChars(check.details).split('\n'); for (const line of detailLines) { - this.log(` ${color(line, colors.gray)}`); + this.log(` ${color(sanitizeSingleLine(line), colors.gray)}`); } } } diff --git a/src/commands/jobs/watch.test.ts b/src/commands/jobs/watch.test.ts index 03eddb5..a62e998 100644 --- a/src/commands/jobs/watch.test.ts +++ b/src/commands/jobs/watch.test.ts @@ -211,6 +211,30 @@ describe('jobs watch command', () => { expect(output).not.toContain(`- ${excludedItem.name}`); }); + it('collapses Dashboard-controlled result labels to one row', () => { + // safeString() strips escape sequences but preserves CR/LF/tab, so a + // hostile result name could forge a status line of its own. + const { command, log } = createWatchCommand(); + + const result: WatchResult = { + status: { + id: 'job_123', + status: 'completed', + results: [{ name: 'site-1\n ✓ Job completed' }, 'plain\rOVERWRITTEN'], + }, + timedOut: false, + elapsed: 5000, + }; + + (command as any).outputResult('job_123', result); + const output = log.mock.calls[0]![0] as string; + + expect(output).toContain('- site-1 ✓ Job completed'); + expect(output).toContain('- plain OVERWRITTEN'); + expect(output).not.toMatch(/- site-1\n/); + expect(output).not.toContain('plain\rOVERWRITTEN'); + }); + it('shows all results when under the limit', () => { const { command, log } = createWatchCommand(); diff --git a/src/commands/jobs/watch.ts b/src/commands/jobs/watch.ts index 1d2c6be..6e686c5 100644 --- a/src/commands/jobs/watch.ts +++ b/src/commands/jobs/watch.ts @@ -16,7 +16,7 @@ import { formatProgressBar, formatElapsed, } from '../../output/formatter.js'; -import { safeString } from '../../utils/terminal-sanitizer.js'; +import { safeString, sanitizeSingleLine } from '../../utils/terminal-sanitizer.js'; import { APIError } from '../../utils/errors.js'; import { errorOutput } from '../../output/json-envelope.js'; import { @@ -355,12 +355,15 @@ export default class JobsWatch extends BaseCommand { // Show first few results const preview = status.results.slice(0, RESULTS_PREVIEW_LIMIT); for (const item of preview) { + // These labels are Dashboard-controlled. safeString() strips escape + // sequences but preserves CR/LF/tab, so a result named + // "site\n✓ Job completed" would forge a status line; collapse to one row. if (typeof item === 'object' && item !== null) { const obj = item as Record; const label = safeString(obj['name'] ?? obj['url'] ?? obj['id'] ?? JSON.stringify(obj)); - lines.push(` - ${label}`); + lines.push(` - ${sanitizeSingleLine(label)}`); } else { - lines.push(` - ${safeString(item)}`); + lines.push(` - ${sanitizeSingleLine(safeString(item))}`); } } diff --git a/src/commands/login.ts b/src/commands/login.ts index 7c17512..99ddfa1 100644 --- a/src/commands/login.ts +++ b/src/commands/login.ts @@ -7,7 +7,7 @@ import { Flags } from '@oclif/core'; import { BaseCommand, commonFlags } from '../lib/base-command.js'; import { getProfileStore, validateDashboardUrl, type Profile } from '../config/profile-store.js'; -import { getKeychain } from '../config/keychain.js'; +import { getKeychain, assertEnvCredentialDeclaredFor } from '../config/keychain.js'; import { createHttpClient } from '../core/http-client.js'; import { formatSuccess, formatWarning, formatInfo } from '../output/formatter.js'; import { AuthError, InputError } from '../utils/errors.js'; @@ -106,7 +106,17 @@ export default class Login extends BaseCommand { // Reject malformed URLs (embedded credentials included) before the // connection test — undici otherwise fails first with an opaque // NetworkError and the user never sees the real reason. - validateDashboardUrl(normalizedUrl, { rejectUserinfo: true }); + validateDashboardUrl(normalizedUrl, { strictIntake: true }); + + // The env credential is identity-bound here too. --url names the + // destination, but in CI the password comes from a protected secret store + // while command arguments generally do not, so requiring the operator to + // declare the Dashboard separately is what stops an edited workflow from + // redirecting it. Checked before the client is built, so a mismatch never + // reaches the network. + if (!flags.password && envPassword) { + assertEnvCredentialDeclaredFor(normalizedUrl, '--url'); + } // Generate profile name from URL if not provided const profileName = flags.name ?? new URL(normalizedUrl).hostname; diff --git a/src/commands/profile/list.ts b/src/commands/profile/list.ts index 12c1c4e..1aa86d6 100644 --- a/src/commands/profile/list.ts +++ b/src/commands/profile/list.ts @@ -7,6 +7,7 @@ import { BaseCommand, commonFlags } from '../../lib/base-command.js'; import { getProfileStore } from '../../config/profile-store.js'; import { formatTable, formatHeading } from '../../output/formatter.js'; +import { maskUrlCredentials } from '../../utils/format.js'; export default class ProfileList extends BaseCommand { static description = 'List saved Dashboard profiles'; @@ -37,7 +38,10 @@ export default class ProfileList extends BaseCommand { { profiles: profiles.map((p) => ({ name: p.name, - url: p.dashboardUrl, + // Legacy profiles may carry user:pass@ or ?access_token= in the + // stored URL; the table below prints even without --json, so both + // paths must mask it. + url: maskUrlCredentials(p.dashboardUrl), username: p.username, active: p.name === activeName, })), @@ -53,7 +57,7 @@ export default class ProfileList extends BaseCommand { const headers = ['Name', 'URL', 'Username', 'Active']; const rows = profiles.map((p) => [ p.name, - p.dashboardUrl, + maskUrlCredentials(p.dashboardUrl), p.username, p.name === activeName ? '*' : '', ]); diff --git a/src/commands/profile/profile-mask.test.ts b/src/commands/profile/profile-mask.test.ts new file mode 100644 index 0000000..87490bc --- /dev/null +++ b/src/commands/profile/profile-mask.test.ts @@ -0,0 +1,128 @@ +/** + * Tests for URL-credential masking in the profile commands. + * + * Profiles saved before userinfo rejection was added may still carry + * `user:pass@` in the stored dashboard URL, and the load path deliberately + * accepts them. Every display path must mask it — these pin `profile use` + * (JSON envelope) and `profile list` (JSON envelope + human table). + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +vi.mock('../../config/profile-store.js', async (importOriginal) => ({ + ...(await importOriginal()), + getProfileStore: vi.fn(), +})); + +import { getProfileStore } from '../../config/profile-store.js'; +import ProfileUse from './use.js'; +import ProfileList from './list.js'; + +const CREDENTIALED_URL = 'https://legacy:s3cr3t@dashboard.example.com'; + +const mockConfig = { + root: '/mock/root', + bin: 'mainwpcontrol', + name: 'mainwpcontrol', + version: '1.0.0', + pjson: { name: 'mainwpcontrol', version: '1.0.0' }, + dataDir: '/mock/data', + cacheDir: '/mock/cache', + configDir: '/mock/config', + findCommand: vi.fn(), + runCommand: vi.fn(), + runHook: vi.fn(), +}; + +/** + * Drive a command's real output() path with a captured logger, skipping the + * full oclif lifecycle (same approach as jobs/watch.test.ts). + */ +function emit( + command: T, + json: boolean, + data: unknown, + humanFormatter?: () => string +): string { + const log = vi.fn(); + command.log = log; + (command as unknown as { jsonOutput: boolean }).jsonOutput = json; + ( + command as unknown as { output(d: unknown, h?: () => string): void } + ).output(data, humanFormatter); + + return log.mock.calls.map((call) => String(call[0])).join('\n'); +} + +describe('profile use masks embedded URL credentials', () => { + beforeEach(() => { + vi.mocked(getProfileStore).mockReturnValue({ + get: vi.fn().mockResolvedValue({ + name: 'legacy', + dashboardUrl: CREDENTIALED_URL, + username: 'admin', + }), + setActive: vi.fn().mockResolvedValue(undefined), + } as never); + }); + + it('masks the url in the --json envelope', async () => { + const command = new ProfileUse([], mockConfig as never); + vi.spyOn(command, 'parse' as never).mockResolvedValue({ + args: { name: 'legacy' }, + flags: {}, + } as never); + vi.spyOn( + command as unknown as { initCommon(f: unknown): Promise }, + 'initCommon' + ).mockResolvedValue(undefined); + + const log = vi.fn(); + command.log = log; + (command as unknown as { jsonOutput: boolean }).jsonOutput = true; + + await command.run(); + + const output = log.mock.calls.map((call) => String(call[0])).join('\n'); + expect(output).not.toContain('s3cr3t'); + expect(output).toContain('***:***@dashboard.example.com'); + }); +}); + +describe('profile list masks embedded URL credentials', () => { + it('masks the url in both the JSON envelope and the human table', async () => { + const profiles = [ + { name: 'legacy', dashboardUrl: CREDENTIALED_URL, username: 'admin' }, + ]; + vi.mocked(getProfileStore).mockReturnValue({ + list: vi.fn().mockResolvedValue(profiles), + getActiveName: vi.fn().mockResolvedValue('legacy'), + } as never); + + for (const json of [true, false]) { + const command = new ProfileList([], mockConfig as never); + vi.spyOn(command, 'parse' as never).mockResolvedValue({ flags: {} } as never); + vi.spyOn( + command as unknown as { initCommon(f: unknown): Promise }, + 'initCommon' + ).mockResolvedValue(undefined); + + const log = vi.fn(); + command.log = log; + (command as unknown as { jsonOutput: boolean }).jsonOutput = json; + + await command.run(); + + const output = log.mock.calls.map((call) => String(call[0])).join('\n'); + expect(output, `json=${json}`).not.toContain('s3cr3t'); + expect(output, `json=${json}`).toContain('***:***@dashboard.example.com'); + } + }); +}); + +describe('emit helper sanity', () => { + it('captures human output when json is off', () => { + const command = new ProfileList([], mockConfig as never); + expect(emit(command, false, { a: 1 }, () => 'human line')).toBe('human line'); + }); +}); diff --git a/src/commands/profile/use.ts b/src/commands/profile/use.ts index e3c8be3..4e2c9b0 100644 --- a/src/commands/profile/use.ts +++ b/src/commands/profile/use.ts @@ -8,6 +8,7 @@ import { Args } from '@oclif/core'; import { BaseCommand, commonFlags } from '../../lib/base-command.js'; import { getProfileStore } from '../../config/profile-store.js'; import { formatSuccess } from '../../output/formatter.js'; +import { maskUrlCredentials } from '../../utils/format.js'; import { ConfigError } from '../../utils/errors.js'; export default class ProfileUse extends BaseCommand { @@ -52,7 +53,9 @@ export default class ProfileUse extends BaseCommand { this.output( { profile: args.name, - url: profile.dashboardUrl, + // Legacy profiles may carry user:pass@ or ?access_token= in the stored + // URL; every display path must mask it. + url: maskUrlCredentials(profile.dashboardUrl), username: profile.username, }, () => formatSuccess(`Switched to profile: ${args.name}`) diff --git a/src/config/keychain.test.ts b/src/config/keychain.test.ts index 9dfcb0a..e381088 100644 --- a/src/config/keychain.test.ts +++ b/src/config/keychain.test.ts @@ -16,7 +16,11 @@ vi.mock('keytar', () => ({ })); import * as keytar from 'keytar'; -import { Keychain, canonicalDashboardIdentity } from './keychain.js'; +import { + Keychain, + canonicalDashboardIdentity, + assertEnvCredentialDeclaredFor, +} from './keychain.js'; import { AuthError } from '../utils/errors.js'; describe('Keychain error normalization', () => { @@ -272,15 +276,101 @@ describe('Keychain identity binding', () => { expect(vi.mocked(keytar.setPassword)).not.toHaveBeenCalled(); }); - it('get() falls back to MAINWP_APP_PASSWORD without identity-checking the env var', async () => { - vi.mocked(keytar.getPassword).mockResolvedValue(null); - vi.stubEnv('MAINWP_APP_PASSWORD', 'env-secret'); + describe('env-var credential identity binding', () => { + beforeEach(() => { + vi.mocked(keytar.getPassword).mockResolvedValue(null); + }); - await expect( - new Keychain().get('default', 'https://dash.example.com') - ).resolves.toBe('env-secret'); + afterEach(() => { + vi.unstubAllEnvs(); + }); - vi.unstubAllEnvs(); + it('releases the env password when MAINWP_DASHBOARD_URL matches the profile', async () => { + vi.stubEnv('MAINWP_APP_PASSWORD', 'env-secret'); + vi.stubEnv('MAINWP_DASHBOARD_URL', 'https://dash.example.com'); + + await expect( + new Keychain().get('default', 'https://dash.example.com') + ).resolves.toBe('env-secret'); + }); + + it('matches on canonical identity, not raw string', async () => { + vi.stubEnv('MAINWP_APP_PASSWORD', 'env-secret'); + vi.stubEnv('MAINWP_DASHBOARD_URL', 'https://dash.example.com/'); + + await expect( + new Keychain().get('default', 'https://dash.example.com') + ).resolves.toBe('env-secret'); + }); + + it('refuses when MAINWP_DASHBOARD_URL is absent', async () => { + // A tampered profiles.json could otherwise redirect the env credential + // to an attacker host without the operator ever naming a destination. + vi.stubEnv('MAINWP_APP_PASSWORD', 'env-secret'); + + await expect( + new Keychain().get('default', 'https://dash.example.com') + ).rejects.toBeInstanceOf(AuthError); + }); + + it('refuses when MAINWP_DASHBOARD_URL points somewhere else', async () => { + vi.stubEnv('MAINWP_APP_PASSWORD', 'env-secret'); + vi.stubEnv('MAINWP_DASHBOARD_URL', 'https://attacker.example.com'); + + const keychain = new Keychain(); + await expect( + keychain.get('default', 'https://dash.example.com') + ).rejects.toBeInstanceOf(AuthError); + await expect( + keychain.get('default', 'https://dash.example.com') + ).rejects.toMatchObject({ + message: expect.stringContaining('https://attacker.example.com'), + }); + }); + + it('refuses when MAINWP_DASHBOARD_URL is not a parseable URL', async () => { + vi.stubEnv('MAINWP_APP_PASSWORD', 'env-secret'); + vi.stubEnv('MAINWP_DASHBOARD_URL', 'not-a-url'); + + await expect( + new Keychain().get('default', 'https://dash.example.com') + ).rejects.toBeInstanceOf(AuthError); + }); + + it('requires a declaration on every authenticated path, login included', () => { + // No skip-the-check mode: in CI the password is a protected secret while + // command arguments are not, so an unbound login would reopen the hole. + vi.stubEnv('MAINWP_APP_PASSWORD', 'env-secret'); + + expect(() => assertEnvCredentialDeclaredFor('https://dash.example.com', '--url')).toThrow( + AuthError + ); + + vi.stubEnv('MAINWP_DASHBOARD_URL', 'https://attacker.example.com'); + expect(() => assertEnvCredentialDeclaredFor('https://dash.example.com', '--url')).toThrow( + AuthError + ); + + vi.stubEnv('MAINWP_DASHBOARD_URL', 'https://dash.example.com/'); + expect(() => + assertEnvCredentialDeclaredFor('https://dash.example.com', '--url') + ).not.toThrow(); + }); + + it('fails closed as AuthError when the destination URL is malformed', () => { + vi.stubEnv('MAINWP_APP_PASSWORD', 'env-secret'); + vi.stubEnv('MAINWP_DASHBOARD_URL', 'https://dash.example.com'); + + expect(() => assertEnvCredentialDeclaredFor('not-a-url', 'the profile')).toThrow(AuthError); + }); + + it('still reads the env password for display paths with no expected URL', async () => { + // Display paths (config show, doctor) pass no expected URL and send + // nothing to a Dashboard, so the binding does not apply. + vi.stubEnv('MAINWP_APP_PASSWORD', 'env-secret'); + + await expect(new Keychain().get('default')).resolves.toBe('env-secret'); + }); }); it('get() treats a "{"-prefixed non-envelope payload as a legacy raw password', async () => { diff --git a/src/config/keychain.ts b/src/config/keychain.ts index 7751bc7..4a4244f 100644 --- a/src/config/keychain.ts +++ b/src/config/keychain.ts @@ -23,6 +23,15 @@ const SERVICE_NAME = 'mainwpcontrol'; */ const ENV_VAR = 'MAINWP_APP_PASSWORD'; +/** + * Environment variable naming the Dashboard the env credential is for. + * + * Required alongside ENV_VAR for authenticated use: the credential is released + * only when this matches the profile's canonical identity, so a tampered + * profiles.json cannot redirect the password to another host. + */ +const ENV_URL_VAR = 'MAINWP_DASHBOARD_URL'; + /** * Timeout for keytar operations (ms). If macOS shows a blocking keychain * dialog, this prevents the CLI from hanging indefinitely. @@ -83,14 +92,17 @@ async function loadKeytar(): Promise { } try { - const mod = await import('keytar'); + const mod: unknown = 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; + // Check for the expected API on mod first; only touch .default if needed + // (mocked modules can throw on access of an export they don't define). + const direct = mod as typeof import('keytar'); + if (typeof direct.setPassword === 'function') { + keytar = direct; + } else { + const unwrapped = (mod as { default?: typeof import('keytar') }).default; + keytar = typeof unwrapped?.setPassword === 'function' ? unwrapped : null; + } if (!keytar) { keytarAvailable = false; @@ -110,6 +122,68 @@ async function loadKeytar(): Promise { * Profile names are user-facing selectors, not an authorization boundary * (AGENTS.md) — this is the boundary. */ +/** + * Refuse the env credential unless the operator declared the same Dashboard it + * is about to be sent to. Fails closed on a missing or unparseable declaration. + * + * Every authenticated use of the env credential goes through here, `login` + * included. An earlier revision let login proceed without a declaration on the + * grounds that `--url` already names the destination, but that reopened the + * hole: in CI the password lives in a protected secret store while command + * arguments usually do not, so anyone who can edit the workflow can redirect it + * without touching the secret. Binding is only worth having if nothing skips it. + * + * @param expectedDashboardUrl - Where the credential would be sent + * @param destinationLabel - How to name that destination in the error + */ +export function assertEnvCredentialDeclaredFor( + expectedDashboardUrl: string, + destinationLabel: string +): void { + const declaredUrl = process.env[ENV_URL_VAR]; + + if (!declaredUrl) { + throw new AuthError( + `${ENV_VAR} is set but ${ENV_URL_VAR} is not, so the destination cannot be verified. Refusing to send the credential.`, + undefined, + `Set ${ENV_URL_VAR} to the Dashboard URL the credential belongs to, or run \`mainwpcontrol login\` to store it in the keychain.` + ); + } + + // The expected URL comes from profiles.json, which is untrusted input, so a + // malformed one must fail closed as an AuthError rather than surface a raw + // TypeError from the parser. + let expected: string; + try { + expected = canonicalDashboardIdentity(expectedDashboardUrl); + } catch { + throw new AuthError( + `The destination URL is not valid, so it cannot be verified against ${ENV_URL_VAR}. Refusing to send the credential.`, + undefined, + 'Check the Dashboard URL on the profile, or run `mainwpcontrol login` to recreate it.' + ); + } + + let declared: string; + try { + declared = canonicalDashboardIdentity(declaredUrl); + } catch { + throw new AuthError( + `${ENV_URL_VAR} is not a valid URL, so the destination cannot be verified. Refusing to send the credential.`, + undefined, + `Set ${ENV_URL_VAR} to the full Dashboard URL, for example https://dashboard.example.com.` + ); + } + + if (declared !== expected) { + throw new AuthError( + `${ENV_VAR} is declared for ${declared}, but ${destinationLabel} points to ${expected}. Refusing to send it.`, + undefined, + `Point ${ENV_URL_VAR} at ${expected}, or target a Dashboard at ${declared}.` + ); + } +} + export function canonicalDashboardIdentity(dashboardUrl: string): string { const parsed = new URL(dashboardUrl); const path = parsed.pathname.replace(/\/+$/, ''); @@ -273,8 +347,14 @@ export class Keychain { * a hand-edited profiles.json must not redirect a stored credential to a * different host. Legacy (unbound) entries are refused for authenticated * use when an expected URL is provided; a one-time `login` re-binds them. - * Without an expected URL they still read, for display paths. The env var - * is per-invocation operator input and is not identity-checked. + * Without an expected URL they still read, for display paths. + * + * The MAINWP_APP_PASSWORD fallback is identity-bound the same way: for + * authenticated use the operator must also set MAINWP_DASHBOARD_URL, and it + * must canonically match the profile. Without that, a profiles.json an + * attacker can write (shared or committed in CI, where this env var is the + * documented credential path) would silently redirect the password to a host + * of their choosing. Display paths pass no expected URL and still read it. */ async get( profileName: string, @@ -311,6 +391,9 @@ export class Keychain { // Fallback to environment variable const envPassword = process.env[ENV_VAR]; if (envPassword) { + if (expectedDashboardUrl) { + assertEnvCredentialDeclaredFor(expectedDashboardUrl, 'the profile'); + } return envPassword; } diff --git a/src/config/profile-store.test.ts b/src/config/profile-store.test.ts index ec3278c..98e601f 100644 --- a/src/config/profile-store.test.ts +++ b/src/config/profile-store.test.ts @@ -41,6 +41,37 @@ describe('ProfileStore URL validation', () => { }); }); + it.each([ + 'https://dashboard.example.com/?access_token=abc123', + 'https://dashboard.example.com/#api_key=abc123', + ])('rejects dashboard URLs carrying a query or fragment: %s', async (dashboardUrl) => { + const store = new ProfileStore(); + + await expect(store.save({ ...baseProfile, dashboardUrl })).rejects.toMatchObject({ + message: 'The dashboard URL must not carry a query string or fragment', + hint: expect.stringMatching(/base URL only/i), + }); + }); + + it('still loads a legacy profile whose stored URL carries a query string', async () => { + // Strict validation is intake-only: a profile already on disk must keep + // loading so its URL can be masked at display instead of bricking the config. + const configDir = join(tempRoot, 'mainwpcontrol'); + await fs.mkdir(configDir, { recursive: true }); + const dashboardUrl = 'https://dashboard.example.com/?access_token=abc123'; + await fs.writeFile( + join(configDir, 'profiles.json'), + JSON.stringify({ + activeProfile: baseProfile.name, + profiles: [{ ...baseProfile, dashboardUrl }], + }) + ); + + const profile = await new ProfileStore().get(baseProfile.name); + + expect(profile?.dashboardUrl).toBe(dashboardUrl); + }); + async function writeProfilesFile(skipSSLVerification: unknown): Promise { const configDir = join(tempRoot, 'mainwpcontrol'); await fs.mkdir(configDir, { recursive: true }); diff --git a/src/config/profile-store.ts b/src/config/profile-store.ts index fc97eb3..e8a7bc9 100644 --- a/src/config/profile-store.ts +++ b/src/config/profile-store.ts @@ -83,13 +83,14 @@ async function saveProfilesFile(data: ProfilesFile): Promise { /** * Validate a Dashboard URL's format and protocol * - * `rejectUserinfo` is set only on intake paths (login, save): legacy profiles - * already on disk with embedded credentials must keep loading so their - * URLs can be masked at display instead of bricking the config. + * `strictIntake` is set only on intake paths (login, save): legacy profiles + * already on disk with embedded credentials or a query string must keep + * loading so their URLs can be masked at display instead of bricking the + * config. */ export function validateDashboardUrl( url: string, - options: { rejectUserinfo?: boolean } = {} + options: { strictIntake?: boolean } = {} ): void { let parsed: URL; try { @@ -114,7 +115,7 @@ export function validateDashboardUrl( // SECURITY: Reject rather than silently strip — the user should know // their pasted URL carried credentials. - if (options.rejectUserinfo && (parsed.username || parsed.password)) { + if (options.strictIntake && (parsed.username || parsed.password)) { throw new ConfigError( 'Embedded credentials in the dashboard URL are not supported', undefined, @@ -122,6 +123,17 @@ export function validateDashboardUrl( ); } + // SECURITY: a query string or fragment is not part of a Dashboard base URL, + // and `?access_token=`/`#api_key=` are credential carriers that would be + // stored in profiles.json and reprinted by every URL display path. + if (options.strictIntake && (parsed.search || parsed.hash)) { + throw new ConfigError( + 'The dashboard URL must not carry a query string or fragment', + undefined, + 'Use the Dashboard base URL only, for example https://dashboard.example.com/' + ); + } + // HTTP warning is emitted at login time via formatWarning, not here } @@ -131,7 +143,7 @@ export function validateDashboardUrl( export class ProfileStore { private data: ProfilesFile | null = null; - private validateUrl(url: string, options: { rejectUserinfo?: boolean } = {}): void { + private validateUrl(url: string, options: { strictIntake?: boolean } = {}): void { validateDashboardUrl(url, options); } @@ -140,7 +152,7 @@ export class ProfileStore { */ private validateProfile( profile: Profile, - options: { rejectUserinfo?: boolean } = {} + options: { strictIntake?: boolean } = {} ): void { const validationHint = 'Run `mainwpcontrol login` to create a valid profile'; @@ -302,7 +314,7 @@ export class ProfileStore { async save(profile: Profile): Promise { // Validate profile before saving; intake is the only place userinfo // URLs are rejected outright (legacy stored profiles are masked instead) - this.validateProfile(profile, { rejectUserinfo: true }); + this.validateProfile(profile, { strictIntake: true }); const data = await this.ensureLoaded(); diff --git a/src/lib/base-command.test.ts b/src/lib/base-command.test.ts index 3cd80ce..8d6ef2a 100644 --- a/src/lib/base-command.test.ts +++ b/src/lib/base-command.test.ts @@ -67,3 +67,64 @@ describe('BaseCommand debug-context redaction', () => { expect(result).toEqual({ count: 3, ok: true, note: 'short', missing: null }); }); }); + +describe('BaseCommand debug-context URL credential masking', () => { + it('masks embedded userinfo in a dashboard URL', () => { + // loadProfile() debug-logs the profile URL; a legacy profile may carry + // user:pass@, which would otherwise land in stderr and CI logs. + const result = redact({ dashboardUrl: 'https://admin:s3cr3t@dashboard.example.com' }); + + expect(result['dashboardUrl']).toBe('https://***:***@dashboard.example.com'); + }); + + it('masks credentialed URLs nested in objects and arrays', () => { + const result = redact({ + config: { baseUrl: 'https://admin:s3cr3t@dashboard.example.com' }, + urls: ['https://u:p@one.example.com'], + }); + + expect(JSON.stringify(result)).not.toContain('s3cr3t'); + expect(JSON.stringify(result)).not.toContain('u:p@'); + }); + + it('masks credentials before truncating, so a long value cannot leak them', () => { + // Truncation keeps the first 297 chars; masking must happen first or a + // credential sitting inside that prefix survives. + const long = `https://admin:s3cr3t@dashboard.example.com/${'a'.repeat(400)}`; + const result = redact({ body: long }); + + expect(result['body']).not.toContain('s3cr3t'); + expect(String(result['body'])).toContain('***:***@'); + expect(String(result['body'])).toMatch(/\.\.\.$/); + }); + + it('redacts a sensitive query parameter in a debug-logged URL', () => { + // A profile saved before query strings were rejected can still carry + // ?access_token=; the debug path masked userinfo only and shipped this + // credential to stderr in full. + const result = redact({ + dashboardUrl: 'https://dashboard.example.com/wp-json?access_token=SECRET', + }); + + expect(result['dashboardUrl']).toBe( + 'https://dashboard.example.com/wp-json?access_token=[REDACTED]' + ); + }); + + it('redacts a percent-encoded sensitive key in a debug-logged URL', () => { + const result = redact({ + dashboardUrl: 'https://dashboard.example.com/wp-json?api%5Fkey=SECRET', + }); + + expect(result['dashboardUrl']).toBe( + 'https://dashboard.example.com/wp-json?api%5Fkey=[REDACTED]' + ); + expect(JSON.stringify(result)).not.toContain('SECRET'); + }); + + it('leaves URLs without credentials unchanged', () => { + const result = redact({ dashboardUrl: 'https://dashboard.example.com/wp-json' }); + + expect(result['dashboardUrl']).toBe('https://dashboard.example.com/wp-json'); + }); +}); diff --git a/src/lib/base-command.ts b/src/lib/base-command.ts index 85d3a79..341719c 100644 --- a/src/lib/base-command.ts +++ b/src/lib/base-command.ts @@ -24,6 +24,7 @@ import { successOutput, errorOutput } from '../output/json-envelope.js'; import { ExitCode } from '../utils/exit-codes.js'; import { formatError, formatWarning } from '../output/formatter.js'; import { isSensitiveKey } from '../utils/redaction.js'; +import { maskUrlCredentialsInText } from '../utils/format.js'; /** * Common flags available to all commands @@ -335,8 +336,14 @@ export abstract class BaseCommand extends Command { * cycles truncate, legitimately shared references survive. */ private redactDebugValue(value: unknown, depth = 0, ancestors = new WeakSet()): unknown { - if (typeof value === 'string' && value.length > 300) { - return `${value.slice(0, 297)}...`; + if (typeof value === 'string') { + // Mask credentialed URLs before truncating, so a credential sitting + // inside the kept prefix cannot survive into stderr, CI logs, or bug + // reports. Legacy profiles predate both intake checks, so the stored + // dashboard URL can carry `user:pass@` or `?access_token=` / `#api_key=`; + // masking has to cover both forms. + const masked = maskUrlCredentialsInText(value); + return masked.length > 300 ? `${masked.slice(0, 297)}...` : masked; } if (value && typeof value === 'object') { diff --git a/src/output/formatter.test.ts b/src/output/formatter.test.ts index f5329ce..43e75ec 100644 --- a/src/output/formatter.test.ts +++ b/src/output/formatter.test.ts @@ -14,6 +14,10 @@ import { formatSection, formatStatusIcon, getStatusColor, + formatHeading, + formatUntrustedBlock, + formatSuccess, + formatInfo, } from './formatter.js'; import { colors } from '../utils/colors.js'; import { InputError } from '../utils/errors.js'; @@ -21,7 +25,7 @@ import { InputError } from '../utils/errors.js'; describe('formatError credential redaction', () => { it.each([ ['Bearer token', 'Request failed with Bearer abc123secret', 'abc123secret', 'Bearer [REDACTED]'], - ['credential URL', 'Request failed at https://user:pass@host/x', 'user:pass', '[URL_WITH_CREDENTIALS]'], + ['credential URL', 'Request failed at https://user:pass@host/x', 'user:pass', 'https://***:***@host/x'], ])('redacts %s credentials from Error messages', (_label, message, secret, marker) => { const output = formatError(new Error(message)); @@ -84,6 +88,83 @@ describe('single-row formatter sanitization', () => { expect(formatList(['list\nitem'])).toContain('list item'); expect(formatPreview('delete\nsite', [])).toContain('delete site'); }); + + it('collapses a lone carriage return in a key-value value', () => { + // stripControlChars preserves \r by design, so a value carrying one would + // return the cursor to column 0 and overwrite the row already printed. + const result = formatKeyValue('Category', 'EvilCategory\rOVERWRITTEN'); + + expect(result).not.toContain('\r'); + expect(result).toContain('EvilCategory OVERWRITTEN'); + }); +}); + +describe('heading/success/info sanitization (F2/F5/F7)', () => { + it('strips escape sequences and collapses newlines in headings', () => { + // Untrusted ability category/label reaches formatHeading on the human path. + const malicious = '\x1b[2JCategory\r\nInjected line\x1b]0;title\x07'; + const result = formatHeading(malicious); + + expect(result).not.toContain('\x1b'); + expect(result).not.toContain('\r'); + expect(result).not.toContain('\n'); + expect(result).toContain('Category Injected line'); + }); + + it('strips escape sequences from success messages', () => { + const result = formatSuccess('\x1b[2JDone\r\nfaked'); + expect(result).not.toContain('\x1b'); + expect(result).toContain('Done faked'); + }); + + it('strips escape sequences from info messages', () => { + const result = formatInfo('\x1b]0;pwn\x07Heads up\nsecond'); + expect(result).not.toContain('\x1b'); + expect(result).toContain('Heads up second'); + }); +}); + +describe('formatUntrustedBlock', () => { + it('keeps multi-paragraph text readable across lines', () => { + expect(formatUntrustedBlock('First line\n\nSecond line')).toBe( + ' │ First line\n │ \n │ Second line' + ); + }); + + it('prefixes every line so remote text cannot reach column 0', () => { + // A hostile ability description imitating this command's own output. + const spoof = 'Harmless summary\n\nAnnotations\nDestructive: No\nPassword:'; + const result = formatUntrustedBlock(spoof); + + for (const line of result.split('\n')) { + expect(line.startsWith(' │ ')).toBe(true); + } + expect(result).not.toMatch(/^Destructive: No$/m); + expect(result).not.toMatch(/^Annotations$/m); + }); + + it('strips escape sequences and normalizes carriage returns', () => { + const result = formatUntrustedBlock('\x1b[2JOverwrite\rfaked\ttab'); + + expect(result).not.toContain('\x1b'); + expect(result).not.toContain('\r'); + expect(result).toBe(' │ Overwrite\n │ faked tab'); + }); + + it('bounds the block with a visible truncation marker', () => { + const result = formatUntrustedBlock('x'.repeat(5000)); + + expect(result).toContain('... [truncated]'); + expect(result.length).toBeLessThan(5000); + }); + + it('does not split a surrogate pair at the truncation boundary', () => { + // 4095 filler characters puts the cut inside the emoji that follows. + const result = formatUntrustedBlock('x'.repeat(4095) + '😀'.repeat(10)); + + expect(result).not.toContain('�'); + expect(JSON.stringify(result)).not.toMatch(/\\ud83d(?!\\ude)/); + }); }); describe('formatDivider', () => { diff --git a/src/output/formatter.ts b/src/output/formatter.ts index 8385fbf..b07a788 100644 --- a/src/output/formatter.ts +++ b/src/output/formatter.ts @@ -5,6 +5,7 @@ import { isMainWPCTLError } from '../utils/errors.js'; import { sanitizeForTerminal, + sanitizeMultiLine, sanitizeSingleLine, safeString, } from '../utils/terminal-sanitizer.js'; @@ -15,7 +16,9 @@ import { colors, color } from '../utils/colors.js'; * Format a success message */ export function formatSuccess(message: string): string { - return color('✓ ', colors.green) + message; + // Single-line: the message may interpolate untrusted values (ability names, + // job status), so collapse escapes and line breaks like the other terminal fields. + return color('✓ ', colors.green) + sanitizeSingleLine(message); } /** @@ -57,14 +60,63 @@ export function formatWarning(message: string): string { * Format an info message */ export function formatInfo(message: string): string { - return color('ℹ ', colors.blue) + message; + return color('ℹ ', colors.blue) + sanitizeSingleLine(message); } /** * Format a heading + * + * Headings render untrusted Dashboard metadata (ability category, label) on the + * human output path, which the JSON envelope sanitizes but the human path does + * not. Sanitize here so every call site is safe rather than relying on each to + * opt in. */ export function formatHeading(text: string): string { - return color(text, colors.bold, colors.cyan); + return color(sanitizeSingleLine(text), colors.bold, colors.cyan); +} + +/** + * Longest untrusted free-text block rendered on the human path. Real ability + * descriptions and instruction blocks are a few hundred characters; a remote + * field long enough to scroll the surrounding output off the screen is an + * output-forging tool, not documentation. + */ +const MAX_UNTRUSTED_BLOCK_LENGTH = 4096; + +/** + * Prefix stamped on every line of an untrusted block, including the first and + * any empty one. Remote text cannot reach column 0 through it, which is what + * stops a description from printing its own `Annotations` heading or a + * `Destructive: No` row that reads as this CLI's own output. + */ +const UNTRUSTED_LINE_PREFIX = ' │ '; + +/** + * Format remote multi-line free text (ability descriptions, instruction + * blocks) as a quoted block. + * + * Escape stripping alone does not stop line-oriented spoofing: `sanitizeMultiLine` + * keeps newlines on purpose, so a hostile field can still emit lines that + * imitate trusted output or a password prompt. Quoting every line is the + * structural fix — no filtering of what the text says, just a frame it cannot + * escape. + */ +export function formatUntrustedBlock(text: string): string { + const sanitized = sanitizeMultiLine(safeString(text)); + const overLimit = sanitized.length > MAX_UNTRUSTED_BLOCK_LENGTH; + let bounded = sanitized.slice(0, MAX_UNTRUSTED_BLOCK_LENGTH); + + // A lone high surrogate at the cut serializes as a replacement character. + const lastCode = bounded.charCodeAt(bounded.length - 1); + if (lastCode >= 0xd800 && lastCode <= 0xdbff) { + bounded = bounded.slice(0, -1); + } + + const lines = bounded.split('\n').map((line) => UNTRUSTED_LINE_PREFIX + line); + if (overLimit) { + lines.push(`${UNTRUSTED_LINE_PREFIX}... [truncated]`); + } + return lines.join('\n'); } /** @@ -118,9 +170,12 @@ export function formatSection(title: string, rows: string[]): string { * Format a key-value pair */ export function formatKeyValue(key: string, value: unknown): string { - // Sanitize both key and value (may contain untrusted API data) + // Sanitize both key and value (may contain untrusted API data). + // The value is collapsed to one row as well: safeString() strips escape + // sequences but deliberately preserves \r, which on its own returns the + // cursor to column 0 and overwrites the row that was just printed. const safeKey = sanitizeSingleLine(key); - const valueStr = safeString(value); + const valueStr = sanitizeSingleLine(safeString(value)); return color(safeKey + ': ', colors.dim) + valueStr; } diff --git a/src/output/json-envelope.test.ts b/src/output/json-envelope.test.ts index c12254a..044ec6e 100644 --- a/src/output/json-envelope.test.ts +++ b/src/output/json-envelope.test.ts @@ -134,7 +134,7 @@ describe('Golden Test: JSON Output Parses Cleanly', () => { describe('Golden Test: Error Code Propagation', () => { it.each([ ['Bearer token', 'Request failed with Bearer abc123secret', 'abc123secret', 'Bearer [REDACTED]'], - ['credential URL', 'Request failed at https://user:pass@host/x', 'user:pass', '[URL_WITH_CREDENTIALS]'], + ['credential URL', 'Request failed at https://user:pass@host/x', 'user:pass', 'https://***:***@host/x'], ])('redacts %s credentials from Error messages', (_label, message, secret, marker) => { const output = errorOutput(new Error(message)); diff --git a/src/utils/error-sanitizer.test.ts b/src/utils/error-sanitizer.test.ts index de2040d..b219958 100644 --- a/src/utils/error-sanitizer.test.ts +++ b/src/utils/error-sanitizer.test.ts @@ -10,15 +10,18 @@ import { describe, it, expect } from 'vitest'; import { sanitizeErrorMessage, sanitizeErrorValue } from './error-sanitizer.js'; describe('sanitizeErrorMessage', () => { + // Userinfo masking is delegated to maskUrlUserinfoInText, which keeps the + // scheme and host and replaces only the credential — a more useful + // diagnostic than the whole-URL placeholder this used to emit. it('redacts user-and-password credentialed URLs', () => { expect(sanitizeErrorMessage('failed: https://admin:secret@dashboard.example.com/wp-json')).toBe( - 'failed: [URL_WITH_CREDENTIALS]' + 'failed: https://***:***@dashboard.example.com/wp-json' ); }); it('redacts username-only credentialed URLs', () => { expect(sanitizeErrorMessage('failed: https://alice@dashboard.example.com')).toBe( - 'failed: [URL_WITH_CREDENTIALS]' + 'failed: https://***:***@dashboard.example.com' ); }); @@ -32,6 +35,85 @@ describe('sanitizeErrorMessage', () => { sanitizeErrorMessage('failed: https://dashboard.example.com/cb?access_token=abc123&page=2') ).toBe('failed: https://dashboard.example.com/cb?access_token=[REDACTED]&page=2'); }); + + // Every case below leaked past the local credential pattern this function + // used before the shared scanner replaced it (adversarial review round 10). + it('redacts an uppercase scheme', () => { + const result = sanitizeErrorMessage('HTTPS://admin:secret@host/x'); + + expect(result).not.toContain('secret'); + expect(result).toBe('HTTPS://***:***@host/x'); + }); + + it('redacts a scheme split by a newline the URL parser discards', () => { + const result = sanitizeErrorMessage('https:\n//admin:secret@host/x'); + + expect(result).not.toContain('secret'); + expect(result).toBe('https:\n//***:***@host/x'); + }); + + it('redacts a password containing spaces', () => { + const result = sanitizeErrorMessage('https://admin:my secret pass@host/x'); + + expect(result).not.toContain('my secret pass'); + expect(result).toBe('https://***:***@host/x'); + }); + + it('redacts a whole b64token Bearer value', () => { + expect(sanitizeErrorMessage('Bearer abc+def/ghi~=')).toBe('Bearer [REDACTED]'); + }); + + it('redacts a base64url Basic value', () => { + expect(sanitizeErrorMessage('Basic YWRtaW4-c2Vj_cmV0==')).toBe('Basic [REDACTED]'); + }); + + it('redacts sensitive parameters carried in a fragment', () => { + expect(sanitizeErrorMessage('https://dash.example/wp#api_key=TOPSECRET')).toBe( + 'https://dash.example/wp#api_key=[REDACTED]' + ); + }); + + it('redacts a fragment key that follows a harmless query parameter', () => { + expect(sanitizeErrorMessage('https://dash.example/wp?page=2#api_key=TOPSECRET')).toBe( + 'https://dash.example/wp?page=2#api_key=[REDACTED]' + ); + }); + + // The display masker decodes before classifying; an error message carrying + // the same URL has to reach the same verdict, or the encoding picks which + // output path leaks. + it('redacts a percent-encoded sensitive key in a query', () => { + expect(sanitizeErrorMessage('failed: https://dash.example/wp?api%5Fkey=TOPSECRET')).toBe( + 'failed: https://dash.example/wp?api%5Fkey=[REDACTED]' + ); + }); + + it('redacts a percent-encoded sensitive key in a fragment', () => { + expect(sanitizeErrorMessage('https://dash.example/wp#%61ccess_token=TOPSECRET')).toBe( + 'https://dash.example/wp#%61ccess_token=[REDACTED]' + ); + }); + + it('redacts an undecodable key rather than failing open', () => { + expect(sanitizeErrorMessage('https://dash.example/wp?api%ZZkey=TOPSECRET')).toBe( + 'https://dash.example/wp?api%ZZkey=[REDACTED]' + ); + }); + + // A length bound on the key class fails open: the key cannot match, so the + // pattern skips the parameter and its value goes out verbatim. + it('redacts a sensitive key longer than 64 characters', () => { + const key = `${'p'.repeat(70)}api_key`; + expect(sanitizeErrorMessage(`failed: https://dash.example/wp?${key}=TOPSECRET`)).toBe( + `failed: https://dash.example/wp?${key}=[REDACTED]` + ); + }); + + it('leaves a harmless parameter untouched', () => { + expect(sanitizeErrorMessage('https://dash.example/wp?page=2')).toBe( + 'https://dash.example/wp?page=2' + ); + }); }); describe('sanitizeErrorValue', () => { @@ -40,7 +122,7 @@ describe('sanitizeErrorValue', () => { sanitizeErrorValue({ urls: ['https://admin:secret@dashboard.example.com'], }) - ).toEqual({ urls: ['[URL_WITH_CREDENTIALS]'] }); + ).toEqual({ urls: ['https://***:***@dashboard.example.com'] }); }); it('redacts values under sensitive keys outright', () => { @@ -84,3 +166,57 @@ describe('sanitizeErrorValue', () => { expect(sanitizeErrorValue(true)).toBe(true); }); }); + +describe('sanitizeErrorMessage input bounding (F11)', () => { + it('completes quickly on an adversarial credential-URL payload', () => { + // Repeated "http://a" gives the credentials pattern many valid start + // prefixes that each fail only at end-of-input: quadratic before the cap. + const hostile = 'http://a'.repeat(200_000); // 1.6MB + const start = Date.now(); + + sanitizeErrorMessage(hostile); + + expect(Date.now() - start).toBeLessThan(500); + }); + + it('does not emit a credential that straddles the truncation boundary', () => { + // Cutting mid-URL removes the "@" the credential pattern needs, so the + // retained prefix stopped matching and the userinfo was emitted verbatim. + const url = 'https://leakeduser:leakedpassword@dash.example.com/path'; + for (const offset of [30, 20, 10, 5]) { + const message = `${'x'.repeat(16384 - offset)}${url}`; + const result = sanitizeErrorMessage(message); + + expect(result, `offset ${offset}`).not.toContain('leakeduser'); + expect(result, `offset ${offset}`).not.toContain('leakedpass'); + } + }); + + it('keeps a usable prefix when the tail is a long run of tabs', () => { + // A trailing-anchored search cannot cross the tabs that follow the space, + // so it found no boundary and discarded the whole message. + const result = sanitizeErrorMessage(`useful context ${'\t'.repeat(17_000)}tail`); + + expect(result).toContain('useful context'); + expect(result).toContain('[truncated]'); + }); + + it('truncates over-long messages with a visible marker', () => { + const result = sanitizeErrorMessage('x'.repeat(20_000)); + + expect(result).toContain('[truncated]'); + expect(result.length).toBeLessThan(20_000); + }); + + it('still redacts credentials in a normal-length message', () => { + expect(sanitizeErrorMessage('failed at https://alice:pw@host/x')).toBe( + 'failed at https://***:***@host/x' + ); + }); + + it('still redacts a username-only credential URL', () => { + expect(sanitizeErrorMessage('failed at https://alice@host/x')).toBe( + 'failed at https://***:***@host/x' + ); + }); +}); diff --git a/src/utils/error-sanitizer.ts b/src/utils/error-sanitizer.ts index 308bac5..50b1118 100644 --- a/src/utils/error-sanitizer.ts +++ b/src/utils/error-sanitizer.ts @@ -3,6 +3,7 @@ */ import { isSensitiveKey } from './redaction.js'; +import { isSensitiveParameterKey, maskUrlUserinfoInText } from './format.js'; const PATH_PATTERNS = [ /\/Users\/[^/\s]+/g, @@ -11,34 +12,100 @@ const PATH_PATTERNS = [ /\.config\/mainwpcontrol/g, ]; +/** + * Longest error text scanned by the patterns below. + * + * Error strings reach here from hostile Dashboard response bodies, where the + * transport's byte cap (10MB) is far too coarse to keep the credential scan + * cheap. Any genuine error message is orders of magnitude shorter than this, so + * truncating first bounds the work without losing real diagnostics. + */ +const MAX_ERROR_MESSAGE_LENGTH = 16384; + +/** + * Whitespace that may end a retained token, excluding tab/CR/LF. + * + * Those three are not boundaries here: the URL parser discards them, so + * `https://user:secret\n...@host` is a single credential to it even though it + * looks like two tokens. Cutting on the newline would keep + * `https://user:secret`, which the credential pattern can no longer recognize. + */ +const SAFE_BOUNDARY = /[^\S\t\n\r]/; + +/** + * Truncate without cutting through the middle of a token. + * + * Cutting mid-token hides credentials instead of redacting them: the patterns + * below need the whole `user:pass@host` construct to match, so a URL sliced + * before its `@` stops matching and the userinfo is emitted as plain text. + * Ending on a whitespace boundary guarantees every token that survives is + * complete. A single token longer than the limit carries no diagnostic value + * and is dropped entirely rather than half-emitted. + */ +function truncateAtTokenBoundary(text: string, limit: number): string { + const cut = text.slice(0, limit); + // Tab, CR and LF are NOT safe boundaries: the URL parser discards them, so + // `https://user:secret\n...@host` is one credential to the parser even though + // it looks like two tokens here. Cutting on the newline would keep + // `https://user:secret`, which the pattern below can no longer recognize. + // Scan back for the last usable boundary directly. A trailing-anchored + // pattern cannot cross tabs or newlines that appear after it, so a message + // ending in a long run of them discarded an otherwise fine prefix. + for (let index = cut.length - 1; index >= 0; index--) { + if (SAFE_BOUNDARY.test(cut[index]!)) { + return cut.slice(0, index); + } + } + return ''; +} + export function sanitizeErrorMessage(message: string): string { - let sanitized = message; + let sanitized = + message.length > MAX_ERROR_MESSAGE_LENGTH + ? `${truncateAtTokenBoundary(message, MAX_ERROR_MESSAGE_LENGTH)}... [truncated]` + : message; for (const pattern of PATH_PATTERNS) { sanitized = sanitized.replace(pattern, '[PATH]'); } - // Password is optional: `https://alice@host` still leaks a username. - sanitized = sanitized.replace( - /https?:\/\/[^\s@/]+(?::[^\s@]*)?@[^\s]+/g, - '[URL_WITH_CREDENTIALS]' - ); + // Embedded userinfo, delegated to the shared linear scanner. A local pattern + // lived here through three rewrites and still missed an uppercase scheme, a + // scheme split by a newline the URL parser discards, and a password + // containing spaces (the Application Password format). The scanner masks the + // userinfo in place and keeps scheme/host, which is the more useful + // diagnostic than the old whole-URL placeholder. + sanitized = maskUrlUserinfoInText(sanitized); + + // RFC 6750 b64token: `Basic`/`Bearer` values may use base64url (`-` `_`) and + // the token68 extras (`.` `~` `+` `/`). Stopping at the first character + // outside a narrower class left the credential's tail in the message. sanitized = sanitized.replace( - /Basic\s+[A-Za-z0-9+/]+=*/gi, + /Basic\s+[A-Za-z0-9+/_-]+=*/gi, 'Basic [REDACTED]' ); sanitized = sanitized.replace( - /Bearer\s+[A-Za-z0-9._-]+/gi, + /Bearer\s+[A-Za-z0-9._~+/-]+=*/gi, 'Bearer [REDACTED]' ); - // Query-string parameters whose key is on the shared sensitive list - // (access_token, api_key, ...) — a URL like ?access_token=... carries the - // credential outside the userinfo form handled above. + // Parameters whose key is on the shared sensitive list (access_token, + // api_key, ...) — a URL like ?access_token=... carries the credential + // outside the userinfo form handled above. `#` is a separator too: a + // fragment-carried key never reaches a server but does reach the terminal. + // `#` also has to leave the key/value classes, or a preceding harmless + // parameter's value swallows `#api_key=...` and the scan never sees it. + // + // Classification is shared with the URL masker rather than calling + // isSensitiveKey directly: the raw key is not the parameter's name, so + // `api%5Fkey` would otherwise pass through with its value intact here even + // though the same URL masks correctly on the display path. The character + // classes stay local — this scans free prose, where quotes terminate a + // value, not a whole URL. sanitized = sanitized.replace( - /([?&])([^=&\s"']{1,64})=([^&\s"']+)/g, + /([?&#])([^=&#\s"']+)=([^&#\s"']+)/g, (match, sep: string, key: string) => - isSensitiveKey(key) ? `${sep}${key}=[REDACTED]` : match + isSensitiveParameterKey(key) ? `${sep}${key}=[REDACTED]` : match ); return sanitized; diff --git a/src/utils/format.test.ts b/src/utils/format.test.ts index d3287ee..141daa5 100644 --- a/src/utils/format.test.ts +++ b/src/utils/format.test.ts @@ -7,6 +7,8 @@ import { maskSecret, maskPassword, maskApiKey, + maskUrlCredentials, + maskUrlCredentialsInText, maskUrlUserinfo, maskUrlUserinfoInText, type MaskOptions, @@ -178,6 +180,140 @@ describe('maskUrlUserinfo', () => { }); }); +describe('maskUrlCredentials', () => { + it('masks userinfo like maskUrlUserinfo', () => { + expect(maskUrlCredentials('https://admin:secret@dashboard.example.com/path')).toBe( + 'https://***:***@dashboard.example.com/path' + ); + }); + + it('redacts a sensitive query parameter', () => { + expect(maskUrlCredentials('https://dashboard.example.com/?access_token=abc123')).toBe( + 'https://dashboard.example.com/?access_token=[REDACTED]' + ); + }); + + it('redacts a sensitive fragment parameter', () => { + expect(maskUrlCredentials('https://dashboard.example.com/wp#api_key=TOPSECRET')).toBe( + 'https://dashboard.example.com/wp#api_key=[REDACTED]' + ); + }); + + it('redacts a fragment key that follows a harmless query parameter', () => { + expect(maskUrlCredentials('https://dashboard.example.com/wp?page=2#api_key=TOPSECRET')).toBe( + 'https://dashboard.example.com/wp?page=2#api_key=[REDACTED]' + ); + }); + + it('redacts every sensitive parameter and keeps the rest byte-for-byte', () => { + expect( + maskUrlCredentials('https://dashboard.example.com/wp?site=1&api_key=a&password=b&page=2') + ).toBe('https://dashboard.example.com/wp?site=1&api_key=[REDACTED]&password=[REDACTED]&page=2'); + }); + + it('leaves non-sensitive parameters untouched', () => { + const url = 'https://dashboard.example.com/wp-json?page=1&per_page=50#section'; + expect(maskUrlCredentials(url)).toBe(url); + }); + + it('masks userinfo and parameters together', () => { + expect(maskUrlCredentials('https://admin:secret@dashboard.example.com/?api_key=abc')).toBe( + 'https://***:***@dashboard.example.com/?api_key=[REDACTED]' + ); + }); + + it('keeps the fail-closed sentinel when userinfo cannot be isolated', () => { + const result = maskUrlCredentials('https://admin:sec\nret@dashboard.example.com/?api_key=abc'); + expect(result).toBe('[URL_WITH_CREDENTIALS_REDACTED]'); + expect(result).not.toContain('abc'); + }); + + it('returns invalid URL input unchanged', () => { + const url = 'not a valid URL'; + expect(maskUrlCredentials(url)).toBe(url); + }); + + // Key classification strips `-`/`_`, so encoding just the separator is enough + // to walk a known sensitive name past a raw-key test. + it('redacts a query key whose separator is percent-encoded', () => { + expect(maskUrlCredentials('https://dashboard.example.com/?api%5Fkey=TOPSECRET')).toBe( + 'https://dashboard.example.com/?api%5Fkey=[REDACTED]' + ); + }); + + it('redacts a fragment key whose separator is percent-encoded', () => { + expect(maskUrlCredentials('https://dashboard.example.com/#api%5Fkey=TOPSECRET')).toBe( + 'https://dashboard.example.com/#api%5Fkey=[REDACTED]' + ); + }); + + it('redacts a percent-encoded hyphen separator', () => { + expect(maskUrlCredentials('https://dashboard.example.com/?api%2Dkey=TOPSECRET')).toBe( + 'https://dashboard.example.com/?api%2Dkey=[REDACTED]' + ); + }); + + it('redacts a key whose sensitive term itself is percent-encoded', () => { + expect(maskUrlCredentials('https://dashboard.example.com/?%61ccess_token=TOPSECRET')).toBe( + 'https://dashboard.example.com/?%61ccess_token=[REDACTED]' + ); + expect(maskUrlCredentials('https://dashboard.example.com/wp#p%61ssword=TOPSECRET')).toBe( + 'https://dashboard.example.com/wp#p%61ssword=[REDACTED]' + ); + }); + + it('redacts encoded and plain sensitive keys in one URL and keeps the rest', () => { + expect( + maskUrlCredentials('https://dashboard.example.com/wp?page=2&api%5Fkey=a#p%61ssword=b') + ).toBe('https://dashboard.example.com/wp?page=2&api%5Fkey=[REDACTED]#p%61ssword=[REDACTED]'); + }); + + // A key that cannot be decoded is not a key that can be cleared: fail closed + // rather than echo whatever it carries. + it('redacts an undecodable key instead of throwing', () => { + expect(maskUrlCredentials('https://dashboard.example.com/?api%ZZkey=TOPSECRET')).toBe( + 'https://dashboard.example.com/?api%ZZkey=[REDACTED]' + ); + // Over-redaction of a harmless-looking key is the accepted cost. + expect(maskUrlCredentials('https://dashboard.example.com/#page%2=2')).toBe( + 'https://dashboard.example.com/#page%2=[REDACTED]' + ); + }); + + // A length bound on the key class fails open: the key cannot match, so the + // pattern skips the parameter and its value goes out verbatim. + it('redacts a sensitive key longer than 64 characters', () => { + const key = `${'p'.repeat(70)}api_key`; + expect(maskUrlCredentials(`https://dashboard.example.com/wp?${key}=TOPSECRET`)).toBe( + `https://dashboard.example.com/wp?${key}=[REDACTED]` + ); + }); +}); + +describe('maskUrlCredentialsInText', () => { + it('masks userinfo and sensitive parameters in embedded URLs', () => { + expect( + maskUrlCredentialsInText( + 'Loaded profile https://admin:secret@dashboard.example.com/wp-json?access_token=abc123&page=2' + ) + ).toBe( + 'Loaded profile https://***:***@dashboard.example.com/wp-json?access_token=[REDACTED]&page=2' + ); + }); + + it('redacts a sensitive key longer than 64 characters', () => { + const key = `${'p'.repeat(70)}api_key`; + expect(maskUrlCredentialsInText(`failed: https://dashboard.example.com/wp?${key}=TOPSECRET`)).toBe( + `failed: https://dashboard.example.com/wp?${key}=[REDACTED]` + ); + }); + + it('leaves text without credentials unchanged', () => { + const text = 'Connection refused for https://dashboard.example.com/wp-json?page=1'; + expect(maskUrlCredentialsInText(text)).toBe(text); + }); +}); + describe('maskUrlUserinfoInText', () => { it('masks credentialed URLs embedded in error messages', () => { expect( @@ -199,4 +335,421 @@ describe('maskUrlUserinfoInText', () => { maskUrlUserinfoInText('fetch failed: https://legacy:p@ss@dashboard.example.com/wp-json timed out') ).toBe('fetch failed: https://***:***@dashboard.example.com/wp-json timed out'); }); + + it('masks a credentialed URL only the WHATWG parser can detect', () => { + // new URL() strips \n before detecting credentials, so a raw string + // carrying one slips past a whitespace-excluding replace. Previously this + // returned the text untouched (fail open) and leaked the password. The + // span from the first userinfo character through the @ contains every + // credential byte, dropped controls included, so masking in place is + // complete. + const result = maskUrlUserinfoInText( + 'fetch failed: https://legacy:sec\nret@dashboard.example.com/wp-json' + ); + + expect(result).not.toContain('sec\nret'); + expect(result).not.toContain('ret@dashboard'); + expect(result).toBe('fetch failed: https://***:***@dashboard.example.com/wp-json'); + }); + + it('masks a tab-obscured credentialed URL in place', () => { + const result = maskUrlUserinfoInText('at https://legacy:sec\tret@dashboard.example.com'); + + expect(result).not.toContain('sec\tret'); + expect(result).toBe('at https://***:***@dashboard.example.com'); + }); + + it('masks a credentialed URL wrapped in brackets or angle brackets', () => { + // A tokenizer that runs to the next whitespace swallows the closing + // delimiter, and the resulting string no longer parses as a URL, so the + // credential passed through untouched. + expect(maskUrlUserinfoInText('')).toBe( + '' + ); + expect(maskUrlUserinfoInText('see [https://u:p@h.example.com] here')).toBe( + 'see [https://***:***@h.example.com] here' + ); + expect(maskUrlUserinfoInText('(https://u:p@host.example.com)')).toBe( + '(https://***:***@host.example.com)' + ); + }); + + it('masks special-scheme URLs with an irregular slash run', () => { + // The parser tolerates any number of slashes after a special scheme, so + // these all carry real userinfo. + expect(maskUrlUserinfoInText('https:/u:p@h.example.com/x')).toBe( + 'https:/***:***@h.example.com/x' + ); + expect(maskUrlUserinfoInText('https:///u:p@h.example.com/x')).toBe( + 'https:///***:***@h.example.com/x' + ); + }); + + it('does not let a credential-free URL hide the next one', () => { + // The first authority runs through ",https:" to the slash. Resuming the + // scan past it skipped the second URL's scheme entirely. + expect(maskUrlUserinfoInText('https://safe,https://u:p@h.example.com/x')).toBe( + 'https://safe,https://***:***@h.example.com/x' + ); + }); + + it('leaves an @ that belongs to a path rather than an authority', () => { + // A backslash ends the authority for special schemes, so the @ here is in + // the path and there is no userinfo to mask. + expect(maskUrlUserinfoInText(String.raw`https://h\path@x`)).toBe( + String.raw`https://h\path@x` + ); + // file: takes no credentials; this is a local path. + expect(maskUrlUserinfoInText('file:u:p@h/x')).toBe('file:u:p@h/x'); + }); + + it('leaves an empty userinfo alone', () => { + // The parser reports no credentials for these, so masking would claim one + // had been there. + expect(maskUrlUserinfoInText('https://@h.example.com/x')).toBe( + 'https://@h.example.com/x' + ); + expect(maskUrlUserinfoInText('https://\t\t@h.example.com/x')).toBe( + 'https://\t\t@h.example.com/x' + ); + // A username on its own is still a credential. + expect(maskUrlUserinfoInText('https://alice@h.example.com/x')).toBe( + 'https://***:***@h.example.com/x' + ); + }); + + it('does not let scheme-like text inside userinfo split the URL', () => { + // `http:` sitting in a password looked like a new URL starting, which closed + // the authority it actually belonged to and left part of it in the output. + expect(maskUrlUserinfoInText('https://u:http:p@h.example.com/x')).toBe( + 'https://***:***@h.example.com/x' + ); + }); + + it('requires a real // for schemes the parser does not treat as special', () => { + // `custom:/…` and `file:/…` are paths, so their `@` is not userinfo. + expect(maskUrlUserinfoInText('custom:/u:p@h.example.com/x')).toBe( + 'custom:/u:p@h.example.com/x' + ); + expect(maskUrlUserinfoInText('file:/u:p@h.example.com/x')).toBe( + 'file:/u:p@h.example.com/x' + ); + expect(maskUrlUserinfoInText('custom://u:p@h.example.com/x')).toBe( + 'custom://***:***@h.example.com/x' + ); + }); + + it('keeps text in front of a URL whose offsets shifted', () => { + // Removing the newline joined "PRE" to the scheme, and replacing from the + // scheme then deleted the preceding line along with the credential. + expect(maskUrlUserinfoInText('PRE\nhttps://u:p@h.example.com/x POST')).toBe( + 'PRE\nhttps://***:***@h.example.com/x POST' + ); + }); + + it('masks obscured userinfo in place without discarding its surroundings', () => { + const result = maskUrlUserinfoInText('before https://u:se\ncret@h.example.com/x after'); + + expect(result).not.toContain('cret@'); + expect(result).toBe('before https://***:***@h.example.com/x after'); + }); + + it('over-masks rather than under-masks when a URL sits in JSON', () => { + // The parser reads `h.test","user":"a` as userinfo and `b` as the host + // here, and that is textually identical to a password containing a quote + // (`https://user:pa"ss@host`), which is a real credential. There is no way + // to tell them apart, so this errs toward masking: a mangled error body + // costs diagnostics, the other direction costs a credential. + const result = maskUrlUserinfoInText('{"url":"https://h.test","user":"a@b"}'); + + expect(result).toContain('***:***@'); + expect(result).not.toContain('"user":"a@'); + }); + + it('masks a password containing characters the parser percent-encodes', () => { + // These are legal in userinfo (the parser encodes them), so treating them + // as authority terminators walked straight past the `@` and leaked. + for (const char of ['"', '<', '>', '`', '{', '}', '|', '^']) { + expect(maskUrlUserinfoInText(`https://user:pa${char}ss@host.example.com/x`)).toBe( + 'https://***:***@host.example.com/x' + ); + } + }); + + it('does not leak a password containing spaces, as WordPress passwords do', () => { + // The scan has to treat a space as the end of a URL because in free text it + // almost always is, but the parser percent-encodes spaces inside userinfo, + // and an Application Password is exactly this shape. The space look-ahead + // catches it: `https://admin:AbCD` alone does not parse (its "port" is not + // a number), so the text after the space is offered to the parser as + // userinfo continuation and masked in place. + const result = maskUrlUserinfoInText('https://admin:AbCD 1234 efGH@host.example.com/x'); + + expect(result).not.toContain('AbCD'); + expect(result).toBe('https://***:***@host.example.com/x'); + }); + + it('masks a spaced password even when another URL already matched', () => { + // The whole-value fallback only ran when the scan found nothing, so a + // spaced credential embedded alongside any other URL survived untouched. + expect( + maskUrlUserinfoInText( + 'first https://a:b@one.example/x then ' + ) + ).toBe('first https://***:***@one.example/x then '); + }); + + it('does not let the space look-ahead swallow prose after a real URL', () => { + // `https://host.test` and `https://host.test:8443` parse on their own, so + // the space genuinely ends them and the email stays untouched. + for (const text of [ + 'Connection to https://host.test failed for admin@example.com', + 'Connection to https://host.test:8443 failed for admin@example.com', + ]) { + expect(maskUrlUserinfoInText(text)).toBe(text); + } + }); + + it('stops a spaced credential at its first credentialed extent', () => { + // The shortest extent that parses with credentials wins, so a bare-host + // spaced credential followed by prose and an email masks only itself. + expect( + maskUrlUserinfoInText('Request to https://admin:AbCD 1234@host failed for admin@e.com') + ).toBe('Request to https://***:***@host failed for admin@e.com'); + }); + + it('masks a wrapped URL whose host starts with a sub-delimiter', () => { + // The conservative host extent is empty when the host starts with `!`, and + // the structural extent swallowed the closing `>` and failed to parse, so + // neither candidate matched the visible URL and the credential leaked. + expect(maskUrlUserinfoInText('')).toBe(''); + expect(maskUrlUserinfoInText('(https://u:p@,host.example)')).toBe( + '(https://***:***@,host.example)' + ); + }); + + it('masks a wrapped, control-obscured URL with a sub-delimiter host', () => { + const result = maskUrlUserinfoInText(''); + + expect(result).not.toContain('cret@'); + expect(result).toBe(''); + }); + + it('masks a wrapped URL whose whole host is outside the character table', () => { + // The conservative and trimmed extents collapse to nothing when every + // host character is outside the table, and the structural extent + // swallowed the closing `>`; the bounded backward walk offers the extent + // just inside the wrapper and the parser confirms the credential. + expect(maskUrlUserinfoInText('')).toBe(''); + }); + + it('does not trust its own sentinel when hostile text embeds it', () => { + // The look-ahead and fallback briefly keyed on the sentinel string to stay + // idempotent, and hostile text containing that literal suppressed masking + // entirely. Idempotency now comes from the uniform in-place replacement, + // which re-parses as ordinary userinfo, so no marker is trusted. + expect( + maskUrlUserinfoInText('https://admin:[URL_WITH_CREDENTIALS_REDACTED] secret@host/x') + ).toBe('https://***:***@host/x'); + }); + + it('extends a spaced credential through its whitespace-free run', () => { + // Stopping at the first credentialed @ made the second pass mask further + // than the first: `***:***@chunk@host` re-parses with userinfo up to the + // LAST @. The look-ahead mirrors that greedy rule within the run. + const once = maskUrlUserinfoInText('https://admin:AbCD 1234@chunk@host/x'); + expect(once).toBe('https://***:***@host/x'); + expect(maskUrlUserinfoInText(once)).toBe(once); + }); + + it('masks userinfo the parser cannot rule on, at any authority length', () => { + // An unterminated IPv6 host rejects every candidate extent, so the parser + // never gets to rule on the credential. "No verdict" is not "no + // credential", so this fails closed — at the window edge, past it, and at + // end of input, all of which previously left the userinfo in place. + for (const input of [ + 'https://u:p@[', + `https://u:p@[${'a'.repeat(1023)} tail`, + `https://u:p@[${'a'.repeat(1023)}`, + `https://u:p@[${'a'.repeat(4000)} tail`, + ]) { + const result = maskUrlUserinfoInText(input); + expect(result).not.toContain('u:p@'); + expect(result.startsWith('https://***:***@')).toBe(true); + } + }); + + it('keeps a spaced credential whole when its run crosses the look-ahead cap', () => { + // The span has to cover every byte the parser read as the credential. A + // run-extension bounded by the look-ahead window ended the span mid-run, + // which both left password bytes in the output and made a second pass + // mask further than the first. + const run = 'a'.repeat(1021); + const once = maskUrlUserinfoInText(`https://admin:AbCD x@${run}@host/x`); + + expect(once).toBe('https://***:***@host/x'); + expect(maskUrlUserinfoInText(once)).toBe(once); + }); + + it('ends the authority at a backslash for special schemes', () => { + // The parser treats `\` as `/` for special schemes, so an `@` after it is + // in the path. Letting the scan run past it replaced the real host and + // part of the path along with the userinfo. + expect(maskUrlUserinfoInText('https://u:p@h\\path@x')).toBe('https://***:***@h\\path@x'); + }); + + it('finds a scheme hidden by glue on either side', () => { + // Stripping a control character joins the preceding word to the scheme, + // and ordinary text can run straight into one. Both readings are offered + // to the parser; only the scanned one used to be. + expect(maskUrlUserinfoInText('1\nhttps://u:p@h/x')).toBe('1\nhttps://***:***@h/x'); + expect(maskUrlUserinfoInText('PRE\nftp:/u:p@h.test/x')).toBe('PRE\nftp:/***:***@h.test/x'); + expect(maskUrlUserinfoInText('9a1111://u:p@h.test/x')).toBe('9a1111://***:***@h.test/x'); + expect(maskUrlUserinfoInText('/xhttps:/u:p@h.test/x')).toBe('/xhttps:/***:***@h.test/x'); + }); + + it('masks a digit-first spaced password a wrapper hides from the fallback', () => { + // `https://u:1234` parses alone as host and port, so the look-ahead + // declined and the whole-value fallback could not fire through the + // brackets. The dotless "host" is the signal that it is really a username. + expect(maskUrlUserinfoInText('[https://u:1234 5678@h.test/x]')).toBe( + '[https://***:***@h.test/x]' + ); + }); + + it('over-masks an unparseable-port URL followed by prose and an email', () => { + // Documented direction (REVIEW_DECISIONS.md): `https://host.test:bad` is + // byte-shape-identical to `https://admin:AbCD`, so refusing to extend it + // would reopen the spaced Application Password leak. A typo'd port plus a + // later email over-masks; a valid port (test above) never does. + expect( + maskUrlUserinfoInText('Connection to https://host.test:bad failed for admin@example.com') + ).toBe('Connection to https://***:***@example.com'); + }); + + it('masks hosts the parser accepts but a character set would not', () => { + // Sub-delimiters and their percent-encoded forms are legal in a host, so + // deciding the host extent from a character table left these unmasked. + for (const host of ['!example', ',example', '%21example', '%2Cexample']) { + expect(maskUrlUserinfoInText(`https://u:p@${host}/x`)).toBe( + `https://***:***@${host}/x` + ); + } + }); + + it('masks when a scheme-like suffix follows the host', () => { + // Opening the next URL closed this authority before its host, so the + // candidate handed to the parser had no host to validate. + expect(maskUrlUserinfoInText('https://a:b@onehttps://safe/x')).toBe( + 'https://***:***@onehttps://safe/x' + ); + }); + + it('masks credentials on an internationalized host', () => { + // The parser punycodes these rather than rejecting them, so stopping the + // host scan at the first non-ASCII character truncated the candidate to + // `https://u:p@`, which does not parse, and the credential survived. + expect(maskUrlUserinfoInText('https://u:p@пример.example.com/x')).toBe( + 'https://***:***@пример.example.com/x' + ); + expect(maskUrlUserinfoInText('https://u:p@xn--e1afmkfd.example.com/x')).toBe( + 'https://***:***@xn--e1afmkfd.example.com/x' + ); + }); + + it('keeps ports and IPv6 literals intact while masking', () => { + expect(maskUrlUserinfoInText('https://u:p@host.example.com:8443/x')).toBe( + 'https://***:***@host.example.com:8443/x' + ); + expect(maskUrlUserinfoInText('https://u:p@[2001:db8::1]:8443/x')).toBe( + 'https://***:***@[2001:db8::1]:8443/x' + ); + }); + + it('recognises a scheme of any length', () => { + // A bounded backward walk missed schemes longer than its limit whenever the + // character at the boundary was a digit. + expect(maskUrlUserinfoInText(`a${'1'.repeat(40)}://u:p@h.example.com/x`)).toBe( + `a${'1'.repeat(40)}://***:***@h.example.com/x` + ); + }); + + it('treats backslashes as slashes only for special schemes', () => { + expect(maskUrlUserinfoInText('custom:\\\\u:p@h.example.com/x')).toBe( + 'custom:\\\\u:p@h.example.com/x' + ); + }); + + it('still masks a password containing sub-delimiters', () => { + // `,` and `;` are legal in userinfo, so they must not end the authority. + expect(maskUrlUserinfoInText('https://user:pa,ss@host.example.com/x')).toBe( + 'https://***:***@host.example.com/x' + ); + expect(maskUrlUserinfoInText("https://user:pa;s's@host.example.com/x")).toBe( + 'https://***:***@host.example.com/x' + ); + }); + + it('stays linear when many short authorities follow a distant @', () => { + // A backward lastIndexOf for the authority's @ made this quadratic. + const hostile = `@${' http:x/'.repeat(50_000)}`; + const start = Date.now(); + + maskUrlUserinfoInText(hostile); + + expect(Date.now() - start).toBeLessThan(500); + }); + + it('stays linear on terminator-free scheme repeats and unclosed brackets', () => { + // Slicing and parsing candidates out to the next structural character was + // quadratic when none exists: repeated scheme opens each re-scanned the + // rest of the input, and an unclosed IPv6 bracket searched to the end for + // its `]`. Past MAX_AUTHORITY_SPAN the adjudicator now fails closed + // instead of parsing unbounded candidates. + for (const unit of ['https:\\\\u:p@', 'https:\\\\u:p@!', 'https://u:p@[/']) { + const start = Date.now(); + + maskUrlUserinfoInText(unit.repeat(20_000)); + + expect(Date.now() - start).toBeLessThan(1_000); + } + }); + + it('masks both URLs when they are adjacent with no whitespace between', () => { + expect( + maskUrlUserinfoInText('https://a:b@one.example.com,https://c:d@two.example.com') + ).toBe('https://***:***@one.example.com,https://***:***@two.example.com'); + }); + + it('is idempotent: masking already-masked text does not collapse it', () => { + // The debug redactor masks centrally, so text can reach this twice. The + // masked form re-parses as credentialed, and re-masking it produced an + // identical string, which the fail-closed check read as "could not + // isolate" and replaced with the sentinel. + const once = maskUrlUserinfoInText('failed at https://u:p@host.example.com/x'); + expect(maskUrlUserinfoInText(once)).toBe(once); + }); + + it('is idempotent for control-obscured and spaced credentials', () => { + // Every span is replaced with the same `***:***@`, which re-parses as + // ordinary userinfo, so a second pass reproduces the first byte for byte + // without any marker being trusted. + for (const text of [ + 'https://u:se\ncret@h contact admin@e.com', + 'https://admin:AbCD 1234@host/x https://u:se\ncret@h/x', + 'https://admin:AbCD 1234@chunk@host/x', + '', + ]) { + const once = maskUrlUserinfoInText(text); + expect(maskUrlUserinfoInText(once)).toBe(once); + expect(once).not.toContain('cret@'); + expect(once).not.toContain('AbCD'); + } + }); + + it('masks several credentialed URLs in one string', () => { + expect( + maskUrlUserinfoInText('first https://a:b@one.example.com then https://c:d@two.example.com') + ).toBe('first https://***:***@one.example.com then https://***:***@two.example.com'); + }); }); diff --git a/src/utils/format.ts b/src/utils/format.ts index 0796fdb..f2a3f79 100644 --- a/src/utils/format.ts +++ b/src/utils/format.ts @@ -4,6 +4,8 @@ * Provides consistent secret masking across all commands. */ +import { isSensitiveKey } from './redaction.js'; + /** * Options for customizing secret masking behavior */ @@ -112,6 +114,10 @@ export function maskApiKey(apiKey: string): string { * maskUrlUserinfo('https://example.com/path') // unchanged * ``` */ +/** The placeholder both userinfo components are replaced with. */ +const MASKED_USERINFO = '***'; +const REDACTED_SENTINEL = '[URL_WITH_CREDENTIALS_REDACTED]'; + export function maskUrlUserinfo(url: string): string { let parsed: URL; try { @@ -124,6 +130,15 @@ export function maskUrlUserinfo(url: string): string { return url; } + // Already masked. Re-masking would produce a byte-identical string, which + // the fail-closed check below reads as "credentials the regex could not + // isolate" and replaces with the sentinel. Masking must be idempotent: the + // debug redactor applies it centrally, so a value can arrive here twice. + // There is nothing to leak either way, since the userinfo is literally `***`. + if (parsed.username === MASKED_USERINFO && parsed.password === MASKED_USERINFO) { + return url; + } + // Greedy through the LAST @ in the authority: a password containing "@" // must not leak its tail. `?`/`#`/`/` bound the authority section. const masked = url.replace(/^([a-z][a-z0-9+.-]*:\/\/)[^/?#\s]*@/i, '$1***:***@'); @@ -133,23 +148,627 @@ export function maskUrlUserinfo(url: string): string { // so a raw string containing them slips past the whitespace-excluding // regex. Fail closed rather than echo the credentials. if (masked === url) { - return '[URL_WITH_CREDENTIALS_REDACTED]'; + return REDACTED_SENTINEL; } return masked; } +/** + * Query and fragment parameters, for sensitive-key redaction. + * + * `#` is a separator alongside `?`/`&` and is excluded from the key and value + * classes: without that, a harmless leading parameter's value swallows + * `#api_key=...` and the fragment is never examined. + */ +const URL_PARAMETER = /([?&#])([^=&#\s]+)=([^&#\s]*)/g; + +/** + * Classify a URL parameter key by its decoded spelling. + * + * The raw key is what a reader sees, but not what the parameter is named: + * `api%5Fkey` normalizes to `api%5fkey`, matches nothing on the shared + * sensitive list, and the secret goes out in full. Decoding cannot hide a term + * the raw key already showed — it only collapses `%XX` triplets, never inserts + * characters between literals — so the decoded form is the stricter test on its + * own. + * + * An undecodable key (lone `%`, truncated escape, invalid UTF-8 sequence) + * counts as sensitive. Redacting a value that was not a credential costs + * display fidelity; failing open costs the credential. + */ +export function isSensitiveParameterKey(key: string): boolean { + let decoded: string; + try { + decoded = decodeURIComponent(key); + } catch { + return true; + } + return isSensitiveKey(decoded); +} + +/** + * Mask everything credential-shaped in a URL for display: userinfo, plus the + * value of any query or fragment parameter whose key — percent-decoded first — + * is on the shared sensitive list. + * + * SECURITY: userinfo is rejected at intake, but profiles saved before that + * check — and before query strings were rejected — can still carry + * `?access_token=` or `#api_key=` in the stored dashboard URL. Every display + * path must mask both forms. + * + * @param url - The URL to mask + * @returns The URL with userinfo masked as `***:***@` and sensitive parameter + * values replaced by `[REDACTED]`, or the userinfo sentinel when credentials + * were detected but could not be isolated + */ +export function maskUrlCredentials(url: string): string { + const masked = maskUrlUserinfo(url); + if (masked === REDACTED_SENTINEL) { + return masked; + } + // The original key spelling is preserved in the output; only classification + // sees the decoded form. + return masked.replace(URL_PARAMETER, (match, separator: string, key: string) => + isSensitiveParameterKey(key) ? `${separator}${key}=[REDACTED]` : match + ); +} + +/** + * Schemes the WHATWG parser gives an authority even without `//`, so + * `https:user:pass@host` carries real userinfo. `file:` is excluded on purpose: + * it takes no credentials, and treating it as special rewrote `file:u:p@h/x`, + * which is a local path. + */ +const SPECIAL_SCHEMES = new Set(['http', 'https', 'ws', 'wss', 'ftp']); + +/** + * Characters that end an authority: only those the parser itself treats as + * structural. + * + * Nothing else belongs here. `" < > ` { } | ^` were briefly included on the + * grounds that RFC 3986 forbids them, which is true but irrelevant: the WHATWG + * parser percent-encodes them inside userinfo rather than rejecting, so + * `https://user:pa"ss@host` really does carry a password and ending the + * authority at the quote walked straight past its `@`. Sub-delimiters are out + * for the same reason. Deciding what is credentialed is left to the parser + * below; this set only finds candidates. + */ +const AUTHORITY_TERMINATORS = new Set(['/', '?', '#', ' ', '\t', '\n', '\r']); + +function isSchemeChar(code: number): boolean { + const isAlpha = (code >= 97 && code <= 122) || (code >= 65 && code <= 90); + const isDigit = code >= 48 && code <= 57; + return isAlpha || isDigit || code === 43 || code === 46 || code === 45; // + . - +} + +function isAlphaCode(code: number): boolean { + return (code >= 97 && code <= 122) || (code >= 65 && code <= 90); +} + +/** + * True for a character that can appear in a host. + * + * The bracket characters are excluded: they belong to a host only around an + * IPv6 literal, which `hostEnd` handles separately, and treating a stray `]` as + * a host character made `[https://u:p@h.t]` ask the parser about the host + * `h.t]`, which it rejects outright. + * + * Anything above ASCII counts, because the parser punycodes internationalized + * hosts rather than rejecting them. Stopping at the first such character + * truncated `https://u:p@\u043f\u0440\u0438\u043c\u0435\u0440.example.com` + * to `https://u:p@`, which does not parse, and the credential stayed visible. + */ +function isHostChar(char: string): boolean { + const code = char.charCodeAt(0); + if (code > 127) return true; + const isAlpha = (code >= 97 && code <= 122) || (code >= 65 && code <= 90); + const isDigit = code >= 48 && code <= 57; + return ( + isAlpha || + isDigit || + code === 46 || // . + code === 95 || // _ + code === 126 || // ~ + code === 37 || // % + code === 58 || // : + code === 45 // - + ); +} + +/** + * True when `candidate` parses as a URL carrying a username or password, and + * what it parsed as the host could be one. + * + * The parser decides whether userinfo is present, because no character table + * gets that right: it percent-encodes `"` inside userinfo, so + * `https://user:pa"ss@host` really does carry a password. But it is equally + * happy to read `h.test","user":"a` as userinfo and `b"}` as the host when a URL + * sits inside a JSON error body, so the host it produced has to be believable + * before the match counts. + */ +/** + * How far past the last `@` the adjudicator will look for the end of an + * authority. No credible URL carries a kilobyte of host and port, and without a + * bound every candidate is sliced and parsed out to the next structural + * character, which is where two separate quadratic blowups lived (repeated + * scheme opens in terminator-free text, and an unclosed IPv6 bracket scanning + * to end of input). Past the bound the scan fails closed and masks: for a real + * oversized URL the verdict would have been "mask" anyway, and for oversized + * junk over-masking is the documented safe direction. + */ +const MAX_AUTHORITY_SPAN = 1024; + +/** Where the authority starting at `from` ends, bounded by `limit`. */ +function authorityEnd(text: string, from: number, limit: number): number { + let end = from; + while (end < limit && !AUTHORITY_TERMINATORS.has(text[end]!)) end++; + return end; +} + +/** Where the host starting at `from` stops, bounded by `limit`. */ +function hostEnd(text: string, from: number, limit: number): number { + let end = from; + // An IPv6 literal is the one place brackets belong; take the whole `[...]`. + if (text[end] === '[') { + while (end < limit && text[end] !== ']') end++; + if (end < limit) end++; + } + while (end < limit && isHostChar(text[end]!)) end++; + return end; +} + +/** + * How many single-character steps are offered as extra candidate extents, in + * each direction, when the table-derived ones fail. + * + * Backward from the structural end, for a host made entirely of characters + * outside the host table with a wrapper after it (``): the + * conservative and trimmed extents collapse to nothing and the structural + * extent swallows the `>`. + * + * Forward from the host start, for a host whose opening characters make every + * longer extent unparseable: `https:/u:p@!>>>https://…` (a one-character host + * glued to the next URL, where the structural end runs into that URL) and + * `https:/u:p@xn--e1.ex/x` (a label the parser rejects as invalid punycode, so + * only a prefix of the host parses). Both leaked until the short extents were + * offered. The parser still decides; these only propose where to cut. + */ +const MAX_TRIM_STEPS = 8; + +function hasCredentials(scan: string, schemeStart: number, hostStart: number): boolean { + const limit = Math.min(scan.length, hostStart + MAX_AUTHORITY_SPAN); + const structEnd = authorityEnd(scan, hostStart, limit); + // No structural end within the window: fail closed (see MAX_AUTHORITY_SPAN). + // The window has to be the thing that stopped the scan — running out of + // input is a genuine end, not an oversized authority — and a terminator + // sitting exactly at the window's edge is genuine too; both adjudicate. + const stoppedByWindow = structEnd === limit && limit === hostStart + MAX_AUTHORITY_SPAN; + const terminatorAtEdge = limit < scan.length && AUTHORITY_TERMINATORS.has(scan[limit]!); + if (stoppedByWindow && !terminatorAtEdge) { + return true; + } + // Three candidate extents, cheapest first. The conservative host stops at the + // first character that is definitely not one, which separates + // `one.t,https://…` into two URLs. The structural end accepts the many host + // characters the parser allows and a character set keeps getting wrong + // (`!example`, `%21example`, `,example`). But when both a weird host AND a + // trailing wrapper are present — `` — the conservative + // extent is empty and the structural one swallows the `>` and fails to + // parse, so a third extent trims trailing non-host characters off the + // structural end. The table still only finds candidates; the parser decides. + let trimmedEnd = structEnd; + while (trimmedEnd > hostStart && !isHostChar(scan[trimmedEnd - 1]!)) trimmedEnd--; + const ends = [hostEnd(scan, hostStart, structEnd), trimmedEnd, structEnd]; + for (let step = 1; step <= MAX_TRIM_STEPS; step++) { + ends.push(structEnd - step, Math.min(hostStart + step, structEnd)); + } + let anyParsed = false; + for (let i = 0; i < ends.length; i++) { + const end = ends[i]!; + if (end <= hostStart || ends.indexOf(end) !== i) continue; + try { + const parsed = new URL(scan.slice(schemeStart, end)); + anyParsed = true; + if (parsed.username || parsed.password) return true; + } catch { + // This extent is not a URL; another may be. + } + } + // Nothing parsed at any extent, so the parser never got to rule on the + // credential — an unterminated IPv6 host (`https://u:p@[`) rejects every + // candidate. The text still shows userinfo before an `@` inside an + // authority this scan opened, and "no verdict" is not "no credential", so + // this fails closed exactly as an oversized authority does. + return !anyParsed; +} + +function parsesWithCredentials(candidate: string): boolean { + try { + const parsed = new URL(candidate); + return Boolean(parsed.username || parsed.password); + } catch { + return false; + } +} + +/** + * How many `@` positions the spaced-userinfo look-ahead will offer the parser. + * A WordPress Application Password contains spaces but no `@`, so one is the + * realistic count; the bound keeps hostile text from turning each look-ahead + * into an unbounded run of candidate parses. + */ +const MAX_LOOKAHEAD_ATS = 8; + +/** + * A WordPress Application Password is printed as six groups of four + * alphanumeric characters separated by spaces. Used only to decide whether to + * ask the parser about extending an *ambiguous* token — one that already + * parses as a URL on its own — never to decide whether something is a + * credential. See spacedUserinfoEnd. + */ +const APP_PASSWORD_GROUP = /^[A-Za-z0-9]{4}$/; + +/** + * The scan treats a space as the end of a URL, because in free text it almost + * always is — but the parser percent-encodes spaces inside userinfo, and a + * WordPress Application Password contains them, so `https://admin:AbCD 1234 + * efGH@host/x` is a credential the plain scan cannot see. When an authority + * closes at a space with no `@` seen, this decides whether the token continues + * through the space as userinfo. + * + * The discriminator is the parser, not a character rule: the look-ahead runs + * unconditionally when the closed token alone does NOT parse as a URL. + * `https://admin:AbCD` does not parse (its "port" is not a number), so the + * text after the space is offered to the parser as userinfo continuation. + * + * When the token DOES parse alone the reading is ambiguous, because + * `https://admin:1234` (a username and the first chunk of a spaced password) + * and `https://host.test:8443` (a real host and port) are the same shape to + * the parser. Declining outright leaked every digit-first spaced password + * whenever a wrapper kept the whole-value fallback from firing + * (`[https://u:1234 5678@h/x]`), so the extension is still offered when + * either signal says the token is not a whole URL: + * + * - the parsed "host" carries no dot and is no IP literal, so it is far more + * likely a username than a public host (`https://admin:1234` → host + * `admin`), or + * - every chunk after the space has the Application Password group shape, + * which covers a dotted username paired with the credential format this CLI + * actually stores (`https://user.name:1234 5678 abcd@h`). + * + * Ordinary prose matches neither: `https://host.test:8443 failed for + * admin@e.com` has a dotted host and the chunks `failed`, `for`, `admin`. The + * gate only decides whether to ask; the parser still decides credentials. + * Residual, documented in REVIEW_DECISIONS.md: a dotted username whose spaced + * password is neither group-shaped nor digit-free stays unmasked when it is + * not the whole value. Candidates stop at `/?#` + * (raw slashes cannot sit in userinfo) and at MAX_AUTHORITY_SPAN. The first + * credentialed `@` decides — so a bare-host credential followed by prose and + * an email never swallows the email — and the span then extends through the + * rest of that whitespace-free run exactly as the main scan's greedy-to-last-@ + * rule would on a second pass, so the output is a fixed point. Residuals + * accepted and documented in REVIEW_DECISIONS.md: a password whose first + * chunk is all digits parses as a valid port and is indistinguishable from + * `host:port`, a spaced password containing `@ ` (at plus space) masks only + * its first credentialed extent, and a URL with an unparseable port followed + * by prose and an email over-masks. + * + * @returns The scan index of the `@` ending the spaced userinfo, or -1. + */ +function spacedUserinfoEnd( + scan: string, + schemeStart: number, + authorityStart: number, + closePos: number +): number { + // An empty authority (`https:// admin@e.com`) offers nothing to continue. + if (closePos <= authorityStart) return -1; + // A token that parses alone is ambiguous rather than settled; see above. + let requireGroupShape = false; + try { + const { hostname } = new URL(scan.slice(schemeStart, closePos)); + // A dot or an IP literal means the token's host is believable as a real + // host, so only a credential-shaped continuation justifies extending it. + requireGroupShape = hostname.includes('.') || hostname.startsWith('['); + } catch { + // Not a URL alone — the space may sit inside its userinfo. + } + // Inclusive of the position exactly MAX_AUTHORITY_SPAN past the space: an + // exclusive bound skipped a credentialed `@` sitting precisely there. + const limit = Math.min(scan.length, closePos + 1 + MAX_AUTHORITY_SPAN); + let tried = 0; + for (let i = closePos + 1; i < limit; i++) { + const char = scan[i]!; + if (char === '/' || char === '?' || char === '#') break; + if (char !== '@') continue; + if (++tried > MAX_LOOKAHEAD_ATS) break; + if (requireGroupShape) { + const chunks = scan.slice(closePos + 1, i).split(' '); + // Every later `@` spans this text too, so a failure here ends the search. + if (!chunks.every((chunk) => APP_PASSWORD_GROUP.test(chunk))) break; + } + if (hasCredentials(scan, schemeStart, i + 1)) { + // Greedy through the rest of this whitespace-free run: `1234@chunk@host` + // re-parses as one authority whose userinfo ends at the LAST @, so + // stopping here would make the second pass mask further than the first. + // + // Deliberately bounded by the run, not by the candidate window: a span + // that stops early is not merely a shorter mask, it is a mask whose + // replaced range excludes part of the credential the parser read, which + // both leaks those bytes and breaks idempotency. The run is walked once + // and the caller resumes past it, so this stays linear. + let end = i; + for (let j = i + 1; j < scan.length && !AUTHORITY_TERMINATORS.has(scan[j]!); j++) { + if (scan[j] === '@') end = j; + } + return end; + } + } + return -1; +} + /** * Mask userinfo in any URLs embedded within arbitrary text. * * SECURITY: Error messages (e.g. fetch failures) can echo a full request URL * including embedded credentials from a legacy profile. * + * Implemented as a linear scan rather than a pattern, after three regex + * attempts each missed a case. The scan runs over a copy with tab/CR/LF + * removed, because the URL parser discards those characters anywhere — + * including inside `://` — so `https:\n//user:pass@host` is credentialed even + * though no pattern anchored on a literal `://` can see it. Offsets are mapped + * back so only the matching span is rewritten and surrounding lines survive. + * * @param text - Text that may contain credentialed URLs - * @returns The text with each `scheme://user:pass@` replaced by `scheme://***:***@` + * @returns The text with each URL's userinfo replaced by `***:***@`. The + * replacement is uniform on purpose: it re-parses as ordinary userinfo, so + * masking is idempotent without trusting any marker string that hostile text + * could also contain. */ export function maskUrlUserinfoInText(text: string): string { - // Greedy through the LAST @ before a path/query/fragment or whitespace, so - // passwords containing "@" mask fully instead of leaking after the first @. - return text.replace(/([a-z][a-z0-9+.-]*:\/\/)[^\s/?#]+@/gi, '$1***:***@'); + if (!text.includes('@')) { + return text; + } + + // Strip what the parser ignores, keeping a map back to the original offsets. + // `boundary` marks scan positions that had a stripped character immediately + // before them: those are where a second reading of a token can start. + let scan = ''; + const sourceIndex: number[] = []; + const boundary: boolean[] = []; + let stripped = false; + for (let index = 0; index < text.length; index++) { + const char = text[index]!; + if (char === '\t' || char === '\n' || char === '\r') { + stripped = true; + continue; + } + scan += char; + sourceIndex.push(index); + boundary.push(stripped); + stripped = false; + } + + const spans: { start: number; end: number }[] = []; + + // One forward pass. Authority state is carried in these, so each character is + // visited once: a per-colon loop with a backward lastIndexOf for the `@` is + // quadratic when many short authorities sit after a distant `@`. + // + // An authority can be open under more than one reading of its scheme. + // Stripping a newline glues the preceding word to it, and the glued scheme + // is a different URL to the parser: `PRE\nftp:/u:p@h` reads as scheme + // `preftp`, which is not special, takes no authority after a single slash, + // and reports no credentials — while the text plainly shows one. Both + // readings are kept and the parser adjudicates each; the span comes from + // whichever reading it confirms. + let openings: { schemeStart: number; authorityStart: number; special: boolean }[] = []; + let authorityStart = -1; + let lastAt = -1; + // Most recent scan position preceded by a stripped character. + let lastBoundary = -1; + // First alphabetic character of the current token run, tracked forward so a + // digit-led run still offers the scheme inside it without a backward walk. + let firstAlpha = -1; + // Start of the current run of scheme-legal characters, maintained forward so + // a scheme of any length is recognised in O(1); a bounded backward walk + // missed schemes longer than its limit, and an unbounded one is quadratic. + let tokenStart = 0; + + const closeAuthority = (): void => { + // `lastAt > authorityStart`, not `>= 0`: an empty userinfo (`https://@host`, + // or one the parser emptied by dropping control characters) carries no + // credentials, so masking it would claim one had been there. + for (const opening of openings) { + if (lastAt <= opening.authorityStart) continue; + // Ask the parser, not the scan, whether this is a credential. Several + // candidate extents are offered (see hasCredentials); any verdict is + // safe, because only the userinfo is rewritten, so the host extent + // affects the decision, never the output. + if (!hasCredentials(scan, opening.schemeStart, lastAt + 1)) continue; + // Only the userinfo is rewritten. Replacing from the scheme instead let a + // span whose offsets had shifted swallow the prose in front of it, so + // `PRE\nhttps://u:p@h` lost `PRE` as well as the credential. + const start = sourceIndex[opening.authorityStart]!; + const stop = sourceIndex[lastAt]! + 1; + // Rewriting in place is complete even when the parser dropped characters + // inside this userinfo: the original span runs from the first userinfo + // character through the `@`, so every credential byte — dropped controls + // included — sits inside [start, stop). A distinct sentinel marker here + // needed guards to stay idempotent, and those guards keyed on a string + // hostile text can also contain, which suppressed masking outright. + // `***:***@` re-parses as ordinary userinfo, so a second pass reproduces + // it byte for byte with no marker trusted anywhere. + spans.push({ start, end: stop }); + break; + } + openings = []; + authorityStart = -1; + lastAt = -1; + }; + + let index = 0; + while (index < scan.length) { + const char = scan[index]!; + if (boundary[index]) lastBoundary = index; + if (firstAlpha < tokenStart && isAlphaCode(scan.charCodeAt(index))) firstAlpha = index; + + if (char === ':') { + // Every reading of this token that could start a URL: as scanned, from a + // stripped character's boundary inside it (see `openings`), and from a + // special scheme name it ends with. The last one matters because a + // special scheme opens an authority after one slash or none, so text + // running straight into it hides the URL completely: `…/xhttps:/u:p@h` + // reads as scheme `xhttps`, which takes no authority after one slash. + // Generic schemes need `//`, which parses as an authority under any + // prefix, so they need no equivalent. + const starts = [tokenStart]; + if (lastBoundary > tokenStart && lastBoundary < index) starts.push(lastBoundary); + // A scheme must start with a letter, so a run beginning with a digit or + // `+.-` is not one under its own start — but the letter inside it can + // begin a real scheme: `9a1111://u:p@h` hid the URL completely. + if (firstAlpha > tokenStart && firstAlpha < index) starts.push(firstAlpha); + for (const scheme of SPECIAL_SCHEMES) { + const start = index - scheme.length; + if (start <= tokenStart) continue; + if (scan.slice(start, index).toLowerCase() === scheme) starts.push(start); + } + const opened: { schemeStart: number; authorityStart: number; special: boolean }[] = []; + let firstAuthority = -1; + for (const start of starts) { + if (start >= index || !isAlphaCode(scan.charCodeAt(start))) continue; + const scheme = scan.slice(start, index).toLowerCase(); + const special = SPECIAL_SCHEMES.has(scheme); + // A special scheme treats backslashes as slashes; others do not, so + // `custom:\\u:p@h` is a path and carries no userinfo. + let after = index + 1; + while ( + after < scan.length && + (scan[after] === '/' || (special && scan[after] === '\\')) + ) { + after++; + } + const slashes = after - (index + 1); + // Special schemes get an authority after any slash run, and after none + // at all — but only when no authority is already open, or `http:` sitting + // inside a password would close the URL it belongs to. Other schemes + // need a real `//`; `custom:/u:p@h` is a path, not an authority. + const opensAuthority = special ? slashes > 0 || authorityStart < 0 : slashes >= 2; + if (!opensAuthority) continue; + opened.push({ schemeStart: start, authorityStart: after, special }); + if (firstAuthority < 0 || after < firstAuthority) firstAuthority = after; + } + if (opened.length > 0) { + // A new URL begins, so whatever authority was open ends here. This is + // what keeps `https://safe,https://u:p@h` from swallowing the second. + closeAuthority(); + openings = opened; + // Scan state follows the earliest reading, so no `@` inside any + // reading's authority is missed; each reading keeps its own start. + authorityStart = firstAuthority; + index = firstAuthority; + tokenStart = firstAuthority; + continue; + } + index++; + tokenStart = index; + continue; + } + + // A special scheme's parser treats `\` as `/`, so it ends the authority + // there. Leaving it out let `lastAt` advance to an `@` in the path, and + // the span then replaced the real host and part of the path along with + // the userinfo: `https://u:p@h\path@x` collapsed to `https://***:***@x`. + // Only for a reading whose authority has actually begun: a backslash still + // inside another reading's slash run (`PRE\nhttps://\u:p@h`, where the + // glued scheme takes `//` and the real one takes `//\`) is not a + // terminator, and closing there dropped the credential entirely. + const endsAuthority = + AUTHORITY_TERMINATORS.has(char) || + (char === '\\' && + openings.some((opening) => opening.special && opening.authorityStart <= index)); + if (authorityStart >= 0 && endsAuthority) { + const sawAt = lastAt > authorityStart; + const openReadings = openings; + closeAuthority(); + // A space-closed authority with no @ may be a URL whose userinfo + // contains spaces (a WordPress Application Password). Tab/CR/LF never + // reach here — they are stripped from the scan — so the space is the + // only whitespace close that can sit inside userinfo. + if (!sawAt && char === ' ') { + let matched = false; + for (const opening of openReadings) { + const at = spacedUserinfoEnd(scan, opening.schemeStart, opening.authorityStart, index); + if (at < 0) continue; + spans.push({ start: sourceIndex[opening.authorityStart]!, end: sourceIndex[at]! + 1 }); + index = at + 1; + tokenStart = index; + matched = true; + break; + } + if (matched) continue; + } + index++; + tokenStart = index; + continue; + } + if (authorityStart >= 0 && char === '@') lastAt = index; + if (!isSchemeChar(scan.charCodeAt(index))) tokenStart = index + 1; + index++; + } + closeAuthority(); + + if (spans.length === 0) { + // Nothing found by token. Before giving up, check whether the whole value + // is itself one URL: the scan has to treat a space as the end of a URL, + // because in free text it almost always is, but the parser percent-encodes + // spaces inside userinfo and a WordPress Application Password contains + // them. `https://admin:AbCD 1234@host` is a credential the scan cannot see, + // and a stored dashboardUrl reaching the debug redactor is that shape. + // Running this only as a fallback keeps multi-URL text with the scan, which + // masks every URL rather than just the first. + const trimmed = text.trim(); + if (trimmed && parsesWithCredentials(trimmed)) { + const at = text.indexOf(trimmed); + return text.slice(0, at) + maskUrlUserinfo(trimmed) + text.slice(at + trimmed.length); + } + return text; + } + + let output = ''; + let cursor = 0; + for (const span of spans) { + // Defensive: never let a span reach back over text already emitted. + if (span.start < cursor) continue; + output += text.slice(cursor, span.start); + output += '***:***@'; + cursor = span.end; + } + return output + text.slice(cursor); +} + +/** + * Mask everything credential-shaped in arbitrary text: embedded userinfo, plus + * the value of any query or fragment parameter whose key — percent-decoded + * first — is on the shared sensitive list. + * + * SECURITY: this is the free-text counterpart of maskUrlCredentials. A URL + * reaching a display path inside a longer string carries the same legacy + * credential forms as a bare one, so both `user:pass@` and `?access_token=` + * have to be masked wherever the string is emitted. + * + * @param text - Text that may contain credentialed URLs + * @returns The text with userinfo replaced by `***:***@` and sensitive + * parameter values replaced by `[REDACTED]`, with the original key spelling + * preserved + */ +export function maskUrlCredentialsInText(text: string): string { + const masked = maskUrlUserinfoInText(text); + // The original key spelling is preserved in the output; only classification + // sees the decoded form. + return masked.replace(URL_PARAMETER, (match, separator: string, key: string) => + isSensitiveParameterKey(key) ? `${separator}${key}=[REDACTED]` : match + ); } diff --git a/src/utils/prompt.ts b/src/utils/prompt.ts index d8306eb..f0e2c6d 100644 --- a/src/utils/prompt.ts +++ b/src/utils/prompt.ts @@ -91,25 +91,35 @@ export async function promptForPassword(question: string): Promise { return ''; } - const rl = readline.createInterface({ - input: process.stdin, - output: process.stdout, - }); - - // Hide input + // No readline interface is created here. A terminal-mode interface + // (`output: process.stdout`) makes the terminal echo keystrokes itself, so + // the password could appear on screen alongside the asterisks written below. + // This function reads raw stdin directly and never needed one: the old + // interface was only ever closed, never read from. const stdin = process.stdin; + // Captured before any mode change, so the original state is what gets restored. const originalRawMode = stdin.isRaw; return new Promise((resolve) => { + // Raw mode first, then the prompt: raw mode is what suppresses the + // terminal's own echo, so it must be on before any keystroke can arrive. + if (stdin.isTTY && stdin.setRawMode) { + stdin.setRawMode(true); + } + const prompt = color('? ', colors.yellow) + question + ' '; process.stdout.write(prompt); let input = ''; - // Enable raw mode to capture individual keystrokes - if (stdin.isTTY && stdin.setRawMode) { - stdin.setRawMode(true); - } + const restoreTerminal = (): void => { + if (stdin.isTTY && stdin.setRawMode) { + stdin.setRawMode(originalRawMode ?? false); + } + stdin.removeListener('data', onData); + stdin.pause(); + process.stdout.write('\n'); + }; const onData = (char: Buffer): void => { const c = char.toString('utf8'); @@ -118,24 +128,12 @@ export async function promptForPassword(question: string): Promise { case '\n': case '\r': case '\u0004': // Ctrl-D - // Restore raw mode and cleanup - if (stdin.isTTY && stdin.setRawMode) { - stdin.setRawMode(originalRawMode ?? false); - } - stdin.removeListener('data', onData); - rl.close(); - process.stdout.write('\n'); + restoreTerminal(); resolve(input); break; case '\u0003': // Ctrl-C - // Restore raw mode and exit - if (stdin.isTTY && stdin.setRawMode) { - stdin.setRawMode(originalRawMode ?? false); - } - stdin.removeListener('data', onData); - rl.close(); - process.stdout.write('\n'); + restoreTerminal(); // 130 = 128 + SIGINT(2), the standard Unix convention for Ctrl-C. // Intentionally outside the documented 0-5 exit code contract — // see README's Exit Codes table for the carve-out. diff --git a/src/utils/terminal-sanitizer.test.ts b/src/utils/terminal-sanitizer.test.ts index 5af07b8..d0769eb 100644 --- a/src/utils/terminal-sanitizer.test.ts +++ b/src/utils/terminal-sanitizer.test.ts @@ -12,11 +12,69 @@ import { describe, it, expect } from 'vitest'; import { stripControlChars, sanitizeSingleLine, + sanitizeMultiLine, sanitizeForTerminal, safeString, containsEscapeSequences, } from './terminal-sanitizer.js'; +describe('stripControlChars input bounding (F6)', () => { + it('completes quickly on unterminated OSC sequences', () => { + // Each "ESC ]" is a valid start with no terminator; the old lazy body + // rescanned to end-of-input from every one of them (quadratic). + const hostile = '\x1b]'.repeat(200_000); + const start = Date.now(); + + const result = stripControlChars(hostile); + + expect(Date.now() - start).toBeLessThan(500); + expect(result).not.toContain('\x1b'); + }); + + it('completes quickly on unterminated DCS sequences', () => { + const hostile = '\x1bP'.repeat(200_000); + const start = Date.now(); + + stripControlChars(hostile); + + expect(Date.now() - start).toBeLessThan(500); + }); + + it('still strips a well-formed OSC sequence', () => { + expect(stripControlChars('before\x1b]0;window title\x07after')).toBe('beforeafter'); + }); + + it('still strips a well-formed DCS sequence', () => { + expect(stripControlChars('a\x1bPq body\x1b\\b')).toBe('ab'); + }); +}); + +describe('sanitizeMultiLine', () => { + it('preserves legitimate newlines so multi-line text still renders across lines', () => { + expect(sanitizeMultiLine('line one\nline two\nline three')).toBe( + 'line one\nline two\nline three' + ); + }); + + it('strips escape and control sequences', () => { + expect(sanitizeMultiLine('\x1b[2Jclean\x1b]0;title\x07 text')).toBe('clean text'); + }); + + it('collapses a lone carriage return to a newline so it cannot overwrite the line', () => { + // A hostile \r with no following \n would otherwise return the cursor to + // column 0 and overwrite what was already printed on that line. + expect(sanitizeMultiLine('real\rfake')).toBe('real\nfake'); + }); + + it('normalizes CRLF to a single newline', () => { + expect(sanitizeMultiLine('a\r\nb')).toBe('a\nb'); + }); + + it('returns empty string for non-string input', () => { + expect(sanitizeMultiLine(undefined as unknown as string)).toBe(''); + }); +}); + describe('sanitizeSingleLine', () => { it('strips terminal escapes and collapses CR, LF, and tabs to one space', () => { const unsafe = '\x1b[31mprovider\x1b[0m\r\n\tinjected'; diff --git a/src/utils/terminal-sanitizer.ts b/src/utils/terminal-sanitizer.ts index 8cd5bb4..58def6a 100644 --- a/src/utils/terminal-sanitizer.ts +++ b/src/utils/terminal-sanitizer.ts @@ -21,7 +21,14 @@ const ESCAPE_PATTERNS = { // Operating System Command sequences: ESC ] ... ST // Used for setting window titles, clipboard, etc. - osc: /\x1b\][\s\S]*?(?:\x07|\x1b\\)/g, + // + // The body is a negated class rather than a lazy `[\s\S]*?`: a lazy body with + // an alternation terminator rescans to end-of-input from every `ESC ]` when no + // terminator exists, which is quadratic on hostile input. A body that cannot + // contain its own terminator fails linearly instead. An unterminated sequence + // is left for the bare-ESC sweep at the end of stripControlChars, so nothing + // escapes; it just no longer swallows an arbitrary span of legitimate text. + osc: /\x1b\][^\x07\x1b]*(?:\x07|\x1b\\)/g, // Single-character escape sequences: ESC followed by single char singleEsc: /\x1b[^[\]]/g, @@ -30,16 +37,17 @@ const ESCAPE_PATTERNS = { c1: /[\x80-\x9f]/g, // Device Control Strings: ESC P ... ST - dcs: /\x1bP[\s\S]*?(?:\x1b\\)/g, + // Negated bodies, for the same linear-failure reason as `osc` above. + dcs: /\x1bP[^\x1b]*(?:\x1b\\)/g, // Application Program Command: ESC _ ... ST - apc: /\x1b_[\s\S]*?(?:\x1b\\)/g, + apc: /\x1b_[^\x1b]*(?:\x1b\\)/g, // Privacy Message: ESC ^ ... ST - pm: /\x1b\^[\s\S]*?(?:\x1b\\)/g, + pm: /\x1b\^[^\x1b]*(?:\x1b\\)/g, // Start of String: ESC X ... ST - sos: /\x1bX[\s\S]*?(?:\x1b\\)/g, + sos: /\x1bX[^\x1b]*(?:\x1b\\)/g, }; /** @@ -105,6 +113,24 @@ export function sanitizeSingleLine(str: string): string { return stripControlChars(str).replace(/[\r\n\t]+/g, ' '); } +/** + * Sanitize untrusted multi-line free text (ability descriptions, instruction + * blocks) for terminal output. + * + * Strips terminal control sequences like the single-line variant but keeps + * newlines, so a legitimate multi-paragraph description still renders across + * lines instead of being collapsed onto one. Carriage returns (lone or as part + * of CRLF) are normalized to a newline so a hostile field cannot return the + * cursor to column 0 and overwrite what was already printed. + */ +export function sanitizeMultiLine(str: string): string { + return stripControlChars(str) + .replace(/\r\n?/g, '\n') + // Tabs jump to the next tab stop, which lets hostile text align itself into + // fake columns; the single-line variant collapses them for the same reason. + .replace(/\t/g, ' '); +} + /** * Sanitized values can originate from hostile API responses: the traversal * is depth-bounded so deep nesting cannot overflow the stack, and the diff --git a/src/validation/input-sanitizer.test.ts b/src/validation/input-sanitizer.test.ts index 919b336..e08777e 100644 --- a/src/validation/input-sanitizer.test.ts +++ b/src/validation/input-sanitizer.test.ts @@ -154,7 +154,7 @@ describe('InputSanitizer — sanitizeErrorMessage', () => { const sanitized = sanitizer.sanitizeErrorMessage(message); expect(sanitized).not.toContain('admin:s3cr3t'); - expect(sanitized).toContain('[URL_WITH_CREDENTIALS]'); + expect(sanitized).toContain('https://***:***@dashboard.example.com/api'); }); it('redacts Bearer tokens', () => { diff --git a/src/validation/sanitize-schema.test.ts b/src/validation/sanitize-schema.test.ts index b8005bd..648b4c7 100644 --- a/src/validation/sanitize-schema.test.ts +++ b/src/validation/sanitize-schema.test.ts @@ -84,6 +84,22 @@ describe('sanitizeInputSchema', () => { expect('pattern' in props['notString']!).toBe(false); }); + it('strips $async so a hostile schema cannot compile to an async validator', () => { + const input = { + $async: true, + type: 'object', + properties: { + nested: { $async: true, type: 'string' }, + }, + }; + + const result = sanitizeInputSchema(input); + const props = result['properties'] as Record>; + + expect('$async' in result).toBe(false); + expect('$async' in props['nested']!).toBe(false); + }); + it('drops patternProperties wholesale', () => { const input = { type: 'object', diff --git a/src/validation/sanitize-schema.ts b/src/validation/sanitize-schema.ts index fb7291e..78b8c59 100644 --- a/src/validation/sanitize-schema.ts +++ b/src/validation/sanitize-schema.ts @@ -71,6 +71,11 @@ function sanitizeSchemaNode(node: Record, depth = 0): Record = { ...node }; delete out['pattern']; delete out['patternProperties']; + // A hostile schema declaring `$async: true` makes AJV compile a + // Promise-returning validator; the always-truthy Promise would read as + // "valid" and its later rejection would crash the process. Neutralize it + // structurally, same as the regex keywords above. + delete out['$async']; for (const [key, value] of Object.entries(out)) { if (DATA_KEYS.has(key)) { diff --git a/src/validation/schema-validator.test.ts b/src/validation/schema-validator.test.ts index c72c5c3..2fd314a 100644 --- a/src/validation/schema-validator.test.ts +++ b/src/validation/schema-validator.test.ts @@ -2,7 +2,7 @@ * Tests for Schema Validator */ -import { describe, it, expect, beforeEach } from 'vitest'; +import { describe, it, expect, beforeEach, vi, afterEach } from 'vitest'; import { APIError } from '../utils/errors.js'; import { ExitCode } from '../utils/exit-codes.js'; import { SchemaValidator } from './schema-validator.js'; @@ -150,4 +150,106 @@ describe('SchemaValidator', () => { }); expect((thrown as Error).message).toContain('mainwp/broken-schema-v1'); }); + + describe('async-schema hardening (F13)', () => { + let unhandled: unknown[]; + let onUnhandled: (reason: unknown) => void; + + beforeEach(() => { + unhandled = []; + onUnhandled = (reason) => unhandled.push(reason); + process.on('unhandledRejection', onUnhandled); + }); + + afterEach(() => { + process.off('unhandledRejection', onUnhandled); + vi.restoreAllMocks(); + }); + + it('strips $async so an invalid input is reported invalid, not passed as a truthy Promise', async () => { + // A hostile Dashboard schema declaring $async: true. Before the fix, AJV + // compiled a Promise-returning validator, the truthy Promise read as + // valid, and the rejection crashed the process. After the strip it is a + // plain sync validator that correctly rejects the missing required field. + const schema = { + $async: true, + type: 'object', + properties: { site_id: { type: 'integer' } }, + required: ['site_id'], + }; + + const result = validator.validate({}, schema, 'mainwp/async-schema-v1'); + + expect(result.valid).toBe(false); + expect(typeof result.valid).toBe('boolean'); + // Let any stray rejection surface before we assert none happened. + await new Promise((resolve) => setTimeout(resolve, 10)); + expect(unhandled).toEqual([]); + }); + + it('fails closed when a compiled validator returns a non-boolean (defense in depth)', () => { + // Simulate a future async path the sanitizer does not neutralize: the + // compiled validator returns a Promise. The guard must reject it as an + // invalid schema rather than reading the truthy Promise as valid. + const asyncValidator = Object.assign(() => Promise.resolve(true), { errors: null }); + vi.spyOn( + (validator as unknown as { ajv: { compile: unknown } }).ajv, + 'compile' + ).mockReturnValue(asyncValidator); + + let thrown: unknown; + try { + validator.validate({}, { type: 'object' }, 'mainwp/would-be-async-v1'); + } catch (error) { + thrown = error; + } + + expect(thrown).toBeInstanceOf(APIError); + expect(thrown).toMatchObject({ code: 'ABILITY_SCHEMA_INVALID' }); + }); + + it('contains hostile thenables without letting them replace the schema error', async () => { + // A compiled remote schema can return anything. A throwing `then` getter + // must not escape in place of APIError, and a `then` that hands back a + // rejected promise must not become an unhandled rejection. + const throwingGetter = Object.defineProperty({}, 'then', { + get() { + throw new Error('hostile getter'); + }, + }); + const rejectingThen = { + then() { + return Promise.reject(new Error('hostile rejection')); + }, + }; + + for (const hostile of [throwingGetter, rejectingThen]) { + const validator2 = new SchemaValidator(); + vi.spyOn( + (validator2 as unknown as { ajv: { compile: unknown } }).ajv, + 'compile' + ).mockReturnValue(Object.assign(() => hostile, { errors: null })); + + expect(() => validator2.validate({}, { type: 'object' }, 'mainwp/hostile-v1')).toThrow( + APIError + ); + vi.restoreAllMocks(); + } + + await new Promise((resolve) => setTimeout(resolve, 10)); + expect(unhandled).toEqual([]); + }); + + it('isValid also fails closed on a non-boolean validation result', () => { + const asyncValidator = Object.assign(() => Promise.resolve(true), { errors: null }); + vi.spyOn( + (validator as unknown as { ajv: { compile: unknown } }).ajv, + 'compile' + ).mockReturnValue(asyncValidator); + + expect(() => validator.isValid({}, { type: 'object' }, 'mainwp/would-be-async-v1')).toThrow( + APIError + ); + }); + }); }); diff --git a/src/validation/schema-validator.ts b/src/validation/schema-validator.ts index 4a0927b..f99df31 100644 --- a/src/validation/schema-validator.ts +++ b/src/validation/schema-validator.ts @@ -68,7 +68,7 @@ export class SchemaValidator { const validate = this.getCompiledSchema(schema, schemaId); // Clone input so AJV coerceTypes/useDefaults mutates the clone, not the caller's object const coerced = structuredClone(input); - const valid = validate(coerced); + const valid = this.assertSyncResult(validate(coerced), schemaId); if (valid) { return { valid: true, coerced }; @@ -114,7 +114,52 @@ export class SchemaValidator { schemaId?: string ): boolean { const validate = this.getCompiledSchema(schema, schemaId); - return validate(structuredClone(input)) as boolean; + return this.assertSyncResult(validate(structuredClone(input)), schemaId); + } + + /** + * Fail closed if a compiled validator returns anything other than a boolean. + * + * `sanitize-schema` strips `$async`, so a compiled validator is always + * synchronous in normal operation. This guard is defense in depth: should any + * future async keyword slip past the sanitizer, AJV would return a + * Promise, whose truthiness would otherwise be read as "valid" and whose + * rejection would crash the process. Reject it as an unusable schema instead. + */ + private assertSyncResult(result: unknown, schemaId?: string): boolean { + if (typeof result !== 'boolean') { + // Adopt a thenable before throwing. An async validator's promise rejects + // on invalid input, and with nothing attached that rejection is unhandled + // and kills the process — the exact crash this guard exists to prevent. + // The result comes from a compiled remote schema, so `then` may be a + // throwing getter, may throw when called, or may itself return a rejected + // promise; none of those may replace the schema error below. + try { + const thenable = result as { then?: unknown } | null; + const then = thenable?.then; + if (typeof then === 'function') { + const chained: unknown = then.call( + thenable, + () => undefined, + () => undefined + ); + if (typeof (chained as PromiseLike | null)?.then === 'function') { + void Promise.resolve(chained as PromiseLike).catch(() => undefined); + } + } + } catch { + // Containing the rejection is best effort; the schema error is what matters. + } + const schemaName = schemaId ? `"${schemaId}"` : '(unnamed)'; + throw new APIError( + 'ABILITY_SCHEMA_INVALID', + `Input schema for ability ${schemaName} produced a non-boolean validation result`, + undefined, + undefined, + 'The Dashboard served an input schema that validates asynchronously, which is not supported' + ); + } + return result; } /** diff --git a/tests/acceptance/agent-run.ts b/tests/acceptance/agent-run.ts index c267a00..6eef0a6 100644 --- a/tests/acceptance/agent-run.ts +++ b/tests/acceptance/agent-run.ts @@ -1312,6 +1312,8 @@ async function runAgentAcceptance(options: AgentRunnerOptions): Promise XDG_CONFIG_HOME: configDir.xdgHome, MAINWPCONTROL_NO_KEYTAR: '1', MAINWP_APP_PASSWORD: scenarioCredentials.appPassword, + // The env credential is identity-bound to the profile's Dashboard. + MAINWP_DASHBOARD_URL: scenarioCredentials.dashboardUrl, ...(insecureHttp ? { MAINWP_ALLOW_HTTP: '1' } : {}), }, (line, elapsedMs) => { diff --git a/tests/acceptance/lib/cli.ts b/tests/acceptance/lib/cli.ts index a627fee..f017e50 100644 --- a/tests/acceptance/lib/cli.ts +++ b/tests/acceptance/lib/cli.ts @@ -111,6 +111,9 @@ export class CLIInvoker { HOME: this.configDir.xdgHome, MAINWPCONTROL_NO_KEYTAR: '1', MAINWP_APP_PASSWORD: this.credentials.appPassword, + // The env credential is identity-bound: it is released only when this + // names the same Dashboard the profile points at. + MAINWP_DASHBOARD_URL: this.credentials.dashboardUrl, }; if (new URL(this.credentials.dashboardUrl).protocol === 'http:') {