From 509ddfa4cdabf1bab60b4dd1f9f341de021bc688 Mon Sep 17 00:00:00 2001 From: Dominic Couture Date: Wed, 9 Sep 2026 22:04:30 +0100 Subject: [PATCH 1/5] feat(security): add clerk security audit, fix, and checks Grade a Clerk instance against 19 security recommendations read from its Platform API config document, and apply the fixes as one config patch. - `clerk security [audit]` prints the grade, findings grouped by severity with their ids, and exits 1 on unmet gaps at or above --fail-on. JSON in agent mode carries the exact config patch per finding, the suggested decision for MFA and passwordless, and a pinned fix command. - `clerk security fix` opens a checklist; ids, --all (critical and recommended), and --good-to-have select explicitly. mfa and passwordless-auth ask which factors or strategy, or take --factors and --strategy from agents. Patches apply through applyConfigPatch (diff, confirmation, server-side --dry-run) and the result is re-scored from the server's response. - `clerk security checks` lists the catalog offline. Checks match the backend: MFA enrollment is required_for_sign_up, backup codes cannot be the only factor, sign-in breach enforcement needs HIBP on. zxcvbn strength scores and the inactivity timeout are deliberately absent. Co-Authored-By: Claude Fable 5.1 --- .changeset/security-recommendations.md | 5 + README.md | 1 + packages/cli-core/src/cli-program.ts | 2 + .../src/commands/completion/__complete.ts | 8 + .../src/commands/config/apply-patch.ts | 3 + .../cli-core/src/commands/security/README.md | 314 +++++++++++ .../src/commands/security/audit.test.ts | 238 +++++++++ .../cli-core/src/commands/security/audit.ts | 56 ++ .../src/commands/security/catalog.test.ts | 349 ++++++++++++ .../cli-core/src/commands/security/catalog.ts | 500 ++++++++++++++++++ .../src/commands/security/evaluate.ts | 124 +++++ .../src/commands/security/fix.test.ts | 486 +++++++++++++++++ .../cli-core/src/commands/security/fix.ts | 331 ++++++++++++ .../src/commands/security/fixtures.ts | 131 +++++ .../cli-core/src/commands/security/format.ts | 135 +++++ .../cli-core/src/commands/security/index.ts | 127 +++++ .../src/commands/security/list-checks.ts | 13 + .../cli-core/src/commands/security/load.ts | 50 ++ .../src/commands/security/merge.test.ts | 69 +++ .../cli-core/src/commands/security/merge.ts | 36 ++ .../src/commands/security/score.test.ts | 83 +++ .../cli-core/src/commands/security/score.ts | 43 ++ .../cli-core/src/commands/security/types.ts | 143 +++++ packages/cli-core/src/lib/copy.ts | 4 + packages/cli-core/src/lib/errors.ts | 23 +- packages/cli-core/src/lib/next-steps.ts | 5 + .../src/test/integration/completion.test.ts | 25 + .../src/test/integration/lib/harness.ts | 5 +- .../src/test/integration/security.test.ts | 204 +++++++ packages/cli-core/src/test/lib/stubs.ts | 3 + test/e2e/security-audit.test.ts | 85 +++ 31 files changed, 3587 insertions(+), 14 deletions(-) create mode 100644 .changeset/security-recommendations.md create mode 100644 packages/cli-core/src/commands/security/README.md create mode 100644 packages/cli-core/src/commands/security/audit.test.ts create mode 100644 packages/cli-core/src/commands/security/audit.ts create mode 100644 packages/cli-core/src/commands/security/catalog.test.ts create mode 100644 packages/cli-core/src/commands/security/catalog.ts create mode 100644 packages/cli-core/src/commands/security/evaluate.ts create mode 100644 packages/cli-core/src/commands/security/fix.test.ts create mode 100644 packages/cli-core/src/commands/security/fix.ts create mode 100644 packages/cli-core/src/commands/security/fixtures.ts create mode 100644 packages/cli-core/src/commands/security/format.ts create mode 100644 packages/cli-core/src/commands/security/index.ts create mode 100644 packages/cli-core/src/commands/security/list-checks.ts create mode 100644 packages/cli-core/src/commands/security/load.ts create mode 100644 packages/cli-core/src/commands/security/merge.test.ts create mode 100644 packages/cli-core/src/commands/security/merge.ts create mode 100644 packages/cli-core/src/commands/security/score.test.ts create mode 100644 packages/cli-core/src/commands/security/score.ts create mode 100644 packages/cli-core/src/commands/security/types.ts create mode 100644 packages/cli-core/src/test/integration/security.test.ts create mode 100644 test/e2e/security-audit.test.ts diff --git a/.changeset/security-recommendations.md b/.changeset/security-recommendations.md new file mode 100644 index 000000000..9b092908b --- /dev/null +++ b/.changeset/security-recommendations.md @@ -0,0 +1,5 @@ +--- +"clerk": minor +--- + +Add `clerk security`, a security audit for a Clerk instance. `clerk security audit` grades the instance against 19 recommendations (bot protection, breached-password detection, brute-force lockout, device trust, MFA, session limits, sign-up restrictions, and more), prints a report grouped by severity, and exits 1 when a critical recommendation is unmet (tunable with `--fail-on`). In agent mode or with `--json` every finding carries the exact `clerk config patch` payload that closes it. `clerk security fix ` applies the patches as one config patch with a diff, a confirmation, and server-side `--dry-run`, then reports the new grade and the remaining gaps. Bare `clerk security fix` opens a checklist; `--all` applies every critical and recommended gap, with `--good-to-have` opting into the rest. Checks that need a product decision, such as which second factors to offer, ask interactively or take `--factors` / `--strategy` from agents. `clerk security checks` lists the catalog offline. diff --git a/README.md b/README.md index 817fcdaab..4da9a9792 100644 --- a/README.md +++ b/README.md @@ -87,6 +87,7 @@ Commands: link [options] Link this project to a Clerk application mcp Manage the Clerk remote MCP server connection for AI editors and CLIs open Open Clerk resources in your browser + security Audit an instance against Clerk's security recommendations telemetry Control CLI usage telemetry (status, disable, enable) unlink [options] Unlink this project from its Clerk application update [options] Update the Clerk CLI to the latest version diff --git a/packages/cli-core/src/cli-program.ts b/packages/cli-core/src/cli-program.ts index ad53816fc..c4890fcb2 100644 --- a/packages/cli-core/src/cli-program.ts +++ b/packages/cli-core/src/cli-program.ts @@ -18,6 +18,7 @@ import { registerTelemetry } from "./commands/telemetry/index.ts"; import { registerToggles } from "./commands/toggles/index.ts"; import { registerApi } from "./commands/api/index.ts"; import { registerDoctor } from "./commands/doctor/index.ts"; +import { registerSecurity } from "./commands/security/index.ts"; import { registerMcp } from "./commands/mcp/index.ts"; import { registerSwitchEnv } from "./commands/switch-env/index.ts"; import { registerCompletion } from "./commands/completion/index.ts"; @@ -77,6 +78,7 @@ const registrants: CommandRegistrant[] = [ registerToggles, registerApi, registerDoctor, + registerSecurity, registerMcp, registerSwitchEnv, registerCompletion, diff --git a/packages/cli-core/src/commands/completion/__complete.ts b/packages/cli-core/src/commands/completion/__complete.ts index 7921e307e..68d1f49d8 100644 --- a/packages/cli-core/src/commands/completion/__complete.ts +++ b/packages/cli-core/src/commands/completion/__complete.ts @@ -1,4 +1,5 @@ import type { CommandUnknownOpts, Option } from "@commander-js/extra-typings"; +import { CHECKS } from "../security/catalog.ts"; import { KNOWN_DASHBOARD_PATHS } from "../open/dashboard-paths.ts"; const DIRECTIVE = { @@ -52,6 +53,12 @@ const KNOWN_OPTION_VALUES: Record = { { name: "latest", description: "Latest stable release" }, { name: "canary", description: "Latest canary (pre-release) build" }, ], + "--factors": [ + { name: "authenticator", description: "Authenticator app (TOTP)" }, + { name: "backup-code", description: "Backup codes" }, + { name: "sms", description: "SMS code" }, + { name: "authenticator,backup-code", description: "Authenticator app and backup codes" }, + ], "--for": [ { name: "orgs", description: "Organizations only" }, { name: "users", description: "Users only" }, @@ -70,6 +77,7 @@ const KNOWN_POSITIONAL_COMPLETIONS: Record = { name: path, description: "Dashboard subpath", })), + "security fix": CHECKS.map((check) => ({ name: check.id, description: check.title })), }; /** diff --git a/packages/cli-core/src/commands/config/apply-patch.ts b/packages/cli-core/src/commands/config/apply-patch.ts index f18b3f570..787c25734 100644 --- a/packages/cli-core/src/commands/config/apply-patch.ts +++ b/packages/cli-core/src/commands/config/apply-patch.ts @@ -24,6 +24,8 @@ export interface ApplyPatchOptions { warning?: string; /** Pre-fetched current config; skips the extra GET when caller already has it. */ currentConfig?: Record; + /** Receives the response body: the written document, or the projection under `--dry-run`. */ + onWritten?: (body: Record) => void; } /** Fetch + diff + confirm + PATCH, matching `clerk config patch` semantics. */ @@ -67,6 +69,7 @@ export async function applyConfigPatch(opts: ApplyPatchOptions): Promise` | Application ID to target (works from any directory) | +| `--instance ` | Instance to target (`dev`, `prod`, or a full instance ID). Defaults to development. | +| `--json` | Output the report as JSON (automatic in agent mode) | +| `--spotlight` | Only show unmet and blocked recommendations | +| `--fail-on ` | Lowest severity of an unmet recommendation that exits 1: `critical` (default), `recommended`, `any`, or `none` | + +### `clerk security fix [ids...]` + +Re-runs the audit, builds one config patch from the selected recommendations, +and applies it through the same path as `clerk config patch`: printed diff, +confirmation prompt, server-side `--dry-run`, and result reporting. With no +ids and no `--all`, human mode opens a checklist of the fixable gaps; +deselecting everything cancels without writing. + +| Flag | Description | +| ------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `[ids...]` | Recommendation ids to fix, as shown in the audit. Omit them in human mode to pick from a checklist of the fixable gaps (all preselected). Agent mode requires ids or `--all`. | +| `--check ` | Same as a positional id; repeatable, so `--input-json` can pass `{"check":[...]}` | +| `--all` | Fix every unmet critical and recommended check that has an inline patch. Add `--factors` or `--strategy` to include the decision checks too. | +| `--good-to-have` | With `--all`, also apply the good-to-have tier. | +| `--factors ` | Second factors for `mfa`: `authenticator`, `backup-code`, `sms` (comma-separated or repeated). Asked interactively when omitted in human mode; required in agent mode. | +| `--strategy ` | Sign-in method for `passwordless-auth`: `email-code`, `email-link`, `phone-code`, `passkey`. Asked interactively when omitted in human mode; required in agent mode. | +| `--app ` | Application ID to target | +| `--instance ` | Instance to target | +| `--dry-run` | Validate server-side and preview the diff without applying it | +| `--yes` | Skip the confirmation prompt. Required in agent mode unless `--dry-run` is passed. | +| `--json` | Print the result summary as JSON (automatic in agent mode) | + +Ids that are already met or not applicable are skipped with a note. Two +recommendations are product decisions rather than pure config changes: `mfa` +(which second factors) and `passwordless-auth` (which sign-in method). `fix` +asks in human mode and takes `--factors` / `--strategy` in agent mode, then +patches like any other check. `oauth-custom-credentials` needs credentials +from the provider and stays manual. A blocked id (`mfa-required`) is accepted +when its prerequisite is in the same call, e.g. +`clerk security fix mfa mfa-required --factors authenticator`, and `--all` +pulls it in automatically once `--factors` is given. Anything else that cannot +be applied exits 2 with a usage error naming the remedy, before anything is +written. Unknown ids exit 2 with the list of valid ids. Passing both ids and +`--all` is an error. + +Backup codes require another second factor: choose `authenticator` or `sms` alongside `backup-code`. Mandatory MFA enables enrollment for both sign-ups and sign-ins. Fixing breached-password sign-in protection also enables breach detection. + +Patches are applied sequentially onto a projected copy of the document, so a +later check sees the earlier ones' changes. This is what lets `--all` set the +two lockout checks without one clobbering the other, and lets checks +that emit whole arrays (`verification_strategies`) compose. + +After the write, `fix` re-evaluates every check against the document the +server returned (the projection, under `--dry-run`) and prints the grade +change, e.g. `Grade F → C · 16 of 20 recommendations met`. With `--json` or in +agent mode it prints a summary: + +```json +{ + "changed": true, + "dryRun": false, + "applied": ["user-lockout", "client-trust"], + "decisions": {}, + "skipped": [{ "id": "bot-protection", "reason": "met" }], + "score": { "before": { "grade": "F", "…": "…" }, "after": { "grade": "C", "…": "…" } }, + "remaining": ["mfa", "mfa-required", "passwordless-auth"] +} +``` + +`changed` is false when nothing was sent: every id was skipped, or the patch +matched the current document. `reason` is `met` or `not_applicable`. +`remaining` lists the ids still unmet or blocked afterwards. `decisions` +records the values each decision check was applied with, e.g. +`{ "mfa": ["authenticator", "backup-code"] }`. + +### `clerk security checks` + +Prints the catalog: id, title, severity, description, the config path each +check reads, whether it has an inline fix, and a docs link. Makes no network +requests and needs no credentials. + +## Requirements + +- A linked project, or `--app `. +- Authenticated via `CLERK_PLATFORM_API_KEY` or `clerk auth login`. +- Account mode only. An unclaimed accountless application exits with + `auth_required`: the checks read the account-level config document, and + Clerk's Backend API only exposes bot protection and organization settings to + an instance secret key. + +## Checks + +Severity is anchored to threat impact. `critical` is the credential-stuffing +and account-takeover kill chain; one unmet critical control caps the grade at +C regardless of the percentage. Password composition rules +(`require_uppercase` and friends) and zxcvbn strength scores are deliberately +not checked; Clerk no longer recommends them, following NIST 800-63B. Length +and breach detection are what count. + +| Id | Severity | Met when | Fix | +| -------------------------- | ------------ | -------------------------------------------------------------------------------------------------------------- | --------------------------------- | +| `bot-protection` | critical | `auth_attack_protection.bot_protection.captcha_enabled` | patch | +| `breach-detection` | critical | `auth_password.disable_hibp` is false | patch | +| `user-lockout` | critical | `auth_attack_protection.user_lockout.enabled` | patch | +| `client-trust` | critical | `auth_password.device_trust.enabled` | patch | +| `mfa` | critical | authenticator app, backup codes, or SMS second factor enabled | asks `--factors` | +| `passwordless-auth` | critical | email/SMS code, passkey, web3, or a social connection is a first factor | asks `--strategy` | +| `email-verification` | critical | `auth_email.verify_at_sign_up` (only when email is a sign-up identifier) | patch | +| `breach-detection-sign-in` | recommended | `auth_password.enforce_hibp_on_sign_in` and `disable_hibp` | patch | +| `lockout-threshold` | recommended | lockout enabled and `max_attempts <= 10` | patch | +| `mfa-required` | recommended | `auth_multi_factor.required_for_sign_up` | patch, blocked until `mfa` is met | +| `passkeys` | recommended | `auth_passkey.used_for_sign_in` | patch | +| `phone-verification` | recommended | `auth_phone.verify_at_sign_up` (only when phone is a sign-up identifier) | patch | +| `password-min-length` | recommended | `auth_password.min_length >= 8` (only when passwords are enabled) | patch | +| `allowlist-on-sign-in` | recommended | `auth_access_control.allowlist_blocklist_enforced_on_sign_in` (only when an allowlist or blocklist is enabled) | patch | +| `oauth-custom-credentials` | recommended | every enabled `connection_oauth_*` has a `client_id` (production only) | manual | +| `email-link-same-client` | good-to-have | `auth_attack_protection.email_link_require_same_client` (email only) | patch | +| `session-lifetime` | good-to-have | `session_settings.maximum_lifetime.enabled` | patch | +| `block-disposable-email` | good-to-have | `auth_access_control.block_disposable_email_domains` (email only) | patch | +| `block-email-subaddresses` | good-to-have | `auth_access_control.block_email_subaddresses` (email only) | patch | + +The first sixteen mirror the Dashboard's security recommendations; the last +three (`password-min-length`, `allowlist-on-sign-in`, `oauth-custom-credentials`) +are CLI-only. + +Three states per finding: + +The **good-to-have** tier is hardening that costs users some convenience: +magic links that must open on the requesting device, and the two email +blocks. It counts toward the score like anything else, but `fix --all` and +the interactive picker leave it out unless asked (`--good-to-have`, or +ticking the rows), so a blanket `fix --all` never changes what end users +experience beyond a CAPTCHA and verification. + +- **met**: nothing to do. +- **unmet**: a real gap. Has a `patch` when the fix is a pure config change. +- **blocked**: a real gap that cannot be applied until a prerequisite is met + (`mfa-required` needs `mfa`). Still counts against the score. + +Checks that have no meaning for the instance are **not applicable** and are +left out of the report and the score entirely: the email checks when email is +not a sign-up identifier, the phone check when phone is not, the four +password checks (`breach-detection`, `breach-detection-sign-in`, +`client-trust`, `password-min-length`) when +`auth_password.enabled` is false, and the OAuth check outside production. + +Three controls depend on a Clerk billing feature and carry a `feature` key in +the report: `mfa` (`app:mfa_totp`), `passkeys` (`app:passkey`), and +`session-lifetime` (`app:custom_session_duration`). The config document does +not say which plan the application is on, so a production instance whose plan +lacks the feature learns that from the API's error when `fix` writes. They +still count against the score; run `fix --dry-run` first when that matters. + +## Score + +Weighted by severity (critical 3, recommended 2, good-to-have 1). A is 95 % +or more, B 80 %, C 60 %, D 40 %, F below. Any unmet or blocked critical +recommendation caps the grade at C. + +## Agent / CI Usage + +Agents get JSON automatically; `--json` forces it for humans too. + +```sh +clerk security checks --json # Discover ids and what they mean +clerk security audit --json --spotlight # Only the gaps +clerk security fix --yes # Apply, no prompt +clerk security fix --all --dry-run # Server-validated preview +``` + +The report: + +```json +{ + "instance": { + "appId": "app_…", + "instanceId": "ins_…", + "environmentType": "development", + "label": "My App (development)" + }, + "score": { "grade": "C", "percent": 71, "met": 14, "total": 20, "hasCriticalGap": true }, + "fixCommand": "clerk security fix user-lockout client-trust --app app_… --instance ins_… --yes", + "findings": [ + { + "id": "user-lockout", + "title": "Brute-force lockout", + "severity": "critical", + "status": "unmet", + "description": "Lock accounts after repeated failed sign-in attempts.", + "path": "auth_attack_protection.user_lockout.enabled", + "currentValue": false, + "recommendedValue": true, + "current": "Disabled", + "recommended": "Enabled", + "patch": { "auth_attack_protection": { "user_lockout": { "enabled": true } } }, + "suggestedPatch": null, + "remedy": "Run `clerk security fix user-lockout --app app_… --instance ins_…`.", + "docsUrl": "https://clerk.com/docs/guides/secure/user-lockout.md", + "dashboardUrl": "https://dashboard.clerk.com/apps/app_…/instances/ins_…/user-authentication" + }, + { + "id": "mfa", + "severity": "critical", + "status": "unmet", + "feature": "app:mfa_totp", + "patch": null, + "suggestedPatch": { + "auth_multi_factor": { + "authenticator_app": { "enabled": true }, + "backup_code": { "enabled": true } + } + }, + "decision": { + "flag": "factors", + "multiple": true, + "options": ["authenticator", "backup-code", "sms"], + "suggested": ["authenticator", "backup-code"] + }, + "remedy": "Run `clerk security fix mfa --factors authenticator,backup-code --app app_… --instance ins_… --yes` (or pick other authenticator, backup-code, sms).", + "…": "…" + } + ] +} +``` + +- `patch` is the literal payload `clerk config patch --json` accepts, so an + agent can apply it through any path. It is `null` for met, blocked, and + manual findings. +- `decision` is set on the two findings that need a choice: `{ flag, multiple, +options, suggested }`. Pass the values with `--` to `fix`; `remedy` + already spells out the command with the suggested values, e.g. + `clerk security fix mfa --factors authenticator,backup-code --app … --instance …`. + `suggestedPatch` is the config patch those suggested values produce, for + agents that prefer `clerk config patch`. Confirm the choice with the user + when it matters (SMS costs money, passkeys need client support). +- `feature` names the billing feature a control depends on (see Checks). +- `fixCommand` lists the fixable critical and recommended gaps; good-to-have + ids are applied only when named explicitly or via `--all --good-to-have`. + It and every `remedy` pin `--app` and `--instance` to the audited instance, + so a copied command cannot drift to another one. In agent mode they also + carry `--yes`, which `fix` requires there. +- `docsUrl` points at the raw markdown (`.md`) in agent mode, like `CliError`. +- `fix` prints `{ changed, dryRun, applied, skipped, score: { before, after }, remaining }` + on stdout in agent mode or with `--json`; see the fix section above. +- Errors are JSON on stderr with a `code`; stdout stays a single JSON document. + +Findings are ordered critical, recommended, good-to-have, and within a severity +unmet, blocked, met. + +Check ids are validated by the command rather than by Commander's `.choices()` +so an unknown id produces a structured usage error listing the valid ids; +tab completion for the ids is registered separately in `completion/__complete.ts`. + +## Exit Codes + +| Code | Meaning | +| ---- | -------------------------------------------------------------------------------------------------------------- | +| 0 | No unmet recommendation at or above `--fail-on`; `fix` applied or had nothing to do | +| 1 | `audit`: unmet recommendations at or above `--fail-on` (error code `security_audit_failed`); or an API failure | +| 2 | Usage error: missing or unknown ids, manual/blocked ids, agent mode without `--yes` | + +## API Endpoints + +All requests go to the Clerk Platform API (default `https://api.clerk.com`, +overridable via `CLERK_PLATFORM_API_URL`), authenticated via `Bearer` token +from `CLERK_PLATFORM_API_KEY` or the stored `clerk auth login` session. + +| Method | Endpoint | Description | +| ------- | ----------------------------------------------------------------- | --------------------------------------------------------------------------------------------------- | +| `GET` | `/v1/platform/applications/{appID}/instances/{instanceID}/config` | Fetches the config document every check reads. One call per `audit` or `fix` run. | +| `GET` | `/v1/platform/applications/{appID}?include_secret_keys=true` | Only when `--app` is passed or `--instance` is a literal id, to resolve the instance's environment. | +| `PATCH` | `/v1/platform/applications/{appID}/instances/{instanceID}/config` | `fix` only. Sends `?dry_run=true` under `--dry-run`. | + +`clerk security checks` makes no requests. diff --git a/packages/cli-core/src/commands/security/audit.test.ts b/packages/cli-core/src/commands/security/audit.test.ts new file mode 100644 index 000000000..a4e1ab5c5 --- /dev/null +++ b/packages/cli-core/src/commands/security/audit.test.ts @@ -0,0 +1,238 @@ +import { test, expect, describe, beforeEach, afterEach, spyOn, mock } from "bun:test"; +import { mkdtemp, rm } from "node:fs/promises"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import { _setConfigDir, setProfile } from "../../lib/config.ts"; +import { useCaptureLog, credentialStoreStubs, gitStubs, stubFetch } from "../../test/lib/stubs.ts"; +import { INSECURE_CONFIG, INSECURE_OAUTH_CONFIG, SECURE_CONFIG } from "./fixtures.ts"; +import type { AuditOptions, AuditReport } from "./types.ts"; + +mock.module("../../lib/credential-store.ts", () => credentialStoreStubs); +mock.module("../../lib/git.ts", () => gitStubs); +mock.module("../../lib/spinner.ts", () => ({ + intro: () => {}, + outro: () => {}, + pausedOutro: () => {}, + bar: () => {}, + withGutter: async ( + _title: string, + fn: (controls: { setNextSteps: (steps: readonly string[]) => void }) => Promise, + ) => fn({ setNextSteps: () => {} }), + withSpinner: async (_msg: string, fn: () => Promise) => fn(), +})); + +const MOCK_APP = { + application_id: "app_1", + name: "My App", + instances: [ + { instance_id: "ins_dev", environment_type: "development" }, + { instance_id: "ins_prod", environment_type: "production" }, + ], +}; + +describe("security audit", () => { + const originalEnv = { ...process.env }; + const originalFetch = globalThis.fetch; + let tempDir: string; + let logSpy: ReturnType; + let errorSpy: ReturnType; + const captured = useCaptureLog(); + + function serve(config: Record) { + stubFetch(async (input) => { + const url = input.toString(); + if (url.includes("/config")) return new Response(JSON.stringify(config), { status: 200 }); + if (url.includes("/v1/platform/applications/app_1")) { + return new Response(JSON.stringify(MOCK_APP), { status: 200 }); + } + throw new Error(`Unexpected fetch: ${url}`); + }); + } + + async function link() { + await setProfile(process.cwd(), { + workspaceId: "org_1", + appId: "app_1", + instances: { development: "ins_dev", production: "ins_prod" }, + }); + } + + async function run(options: AuditOptions = {}) { + const { securityAudit } = await import("./audit.ts"); + return securityAudit(options); + } + + function report(): AuditReport { + return JSON.parse(captured.out) as AuditReport; + } + + beforeEach(async () => { + tempDir = await mkdtemp(join(tmpdir(), "clerk-security-audit-test-")); + _setConfigDir(tempDir); + process.env.CLERK_PLATFORM_API_KEY = "test_key"; + process.env.CLERK_PLATFORM_API_URL = "https://test-api.clerk.com"; + process.env.CLERK_MODE = "human"; + delete process.env.CLERK_SECRET_KEY; + logSpy = spyOn(console, "log").mockImplementation(() => {}); + errorSpy = spyOn(console, "error").mockImplementation(() => {}); + serve(INSECURE_CONFIG); + }); + + afterEach(async () => { + _setConfigDir(undefined); + process.env = { ...originalEnv }; + globalThis.fetch = originalFetch; + logSpy.mockRestore(); + errorSpy.mockRestore(); + await rm(tempDir, { recursive: true, force: true }); + }); + + test("errors when no profile is linked", async () => { + await expect(run()).rejects.toThrow("No Clerk project linked"); + }); + + test("refuses an accountless target", async () => { + process.env.CLERK_SECRET_KEY = "sk_test_local"; + await expect(run()).rejects.toThrow("claimed application"); + }); + + test("emits the JSON envelope with --json", async () => { + await link(); + await expect(run({ json: true })).rejects.toThrow( + "security recommendations unmet (--fail-on critical)", + ); + + const parsed = report(); + expect(parsed.instance).toEqual({ + appId: "app_1", + instanceId: "ins_dev", + environmentType: "development", + label: "app_1 (development)", + }); + expect(parsed.score.grade).toBe("F"); + expect(parsed.score.hasCriticalGap).toBe(true); + expect(parsed.fixCommand).toStartWith("clerk security fix "); + expect(parsed.fixCommand).not.toContain("--yes"); + expect(parsed.fixCommand).toContain("user-lockout"); + expect(parsed.fixCommand).not.toContain("block-email-subaddresses"); + + const lockout = parsed.findings.find((f) => f.id === "user-lockout")!; + expect(lockout.status).toBe("unmet"); + expect(lockout.patch).toEqual({ auth_attack_protection: { user_lockout: { enabled: true } } }); + expect(lockout.remedy).toBe( + "Run `clerk security fix user-lockout --app app_1 --instance ins_dev`.", + ); + expect(lockout.dashboardUrl).toContain("/apps/app_1/instances/ins_dev/user-authentication"); + expect(lockout.docsUrl).toBe("https://clerk.com/docs/guides/secure/user-lockout"); + expect(lockout.suggestedPatch).toBeNull(); + + const mfa = parsed.findings.find((f) => f.id === "mfa")!; + expect(mfa.patch).toBeNull(); + expect(mfa.suggestedPatch).toEqual({ + auth_multi_factor: { authenticator_app: { enabled: true }, backup_code: { enabled: true } }, + }); + expect(mfa.feature).toBe("app:mfa_totp"); + expect(mfa.remedy).toContain( + "clerk security fix mfa --factors authenticator,backup-code --app app_1 --instance ins_dev", + ); + expect(mfa.decision?.flag).toBe("factors"); + }); + + test("agent mode forces JSON, rewrites docs URLs, and adds --yes to the fix command", async () => { + process.env.CLERK_MODE = "agent"; + await link(); + await expect(run()).rejects.toThrow(); + + const parsed = report(); + expect(parsed.fixCommand).toEndWith(" --yes"); + expect(parsed.findings[0]!.docsUrl).toEndWith(".md"); + expect(captured.err).not.toContain("Grade"); + }); + + test("orders findings by severity then status", async () => { + await link(); + await expect(run({ json: true })).rejects.toThrow(); + const statuses = report().findings.map((f) => `${f.severity}:${f.status}`); + const firstRecommended = statuses.findIndex((s) => s.startsWith("recommended")); + expect(statuses.slice(0, firstRecommended).every((s) => s.startsWith("critical"))).toBe(true); + const recommended = statuses.filter((s) => s.startsWith("recommended")); + expect(recommended.indexOf("recommended:blocked")).toBeGreaterThan( + recommended.lastIndexOf("recommended:unmet"), + ); + }); + + test("--spotlight drops met findings from JSON", async () => { + serve(SECURE_CONFIG); + await link(); + await run({ json: true, spotlight: true }); + expect(report().findings).toEqual([]); + expect(report().score.grade).toBe("A"); + }); + + test("renders a grouped human report", async () => { + await link(); + await expect(run()).rejects.toThrow(); + expect(captured.err).toContain("Grade F"); + expect(captured.err).toContain("Critical"); + expect(captured.err).toContain("Brute-force lockout"); + expect(captured.err).toContain("user-lockout"); + expect(captured.err).toContain("blocked:"); + expect(captured.err).toContain("Disabled"); + expect(captured.err).toContain("(asks --factors)"); + expect(captured.err).toContain("blocked:"); + expect(captured.out).toBe(""); + }); + + test("human --spotlight hides met findings", async () => { + serve({ ...INSECURE_CONFIG, auth_password: { ...(SECURE_CONFIG.auth_password as object) } }); + await link(); + await expect(run({ spotlight: true })).rejects.toThrow(); + expect(captured.err).not.toContain("Device trust"); + }); + + test.each([ + ["critical", true], + ["recommended", true], + ["any", true], + ["none", false], + ] as const)("--fail-on %s on the insecure fixture throws: %s", async (failOn, throws) => { + await link(); + const promise = run({ json: true, failOn }); + if (throws) await expect(promise).rejects.toThrow("unmet"); + else await expect(promise).resolves.toBeUndefined(); + }); + + test("--fail-on recommended passes when only good-to-have gaps remain", async () => { + const config = { + ...SECURE_CONFIG, + session_settings: { + ...(SECURE_CONFIG.session_settings as object), + maximum_lifetime: { enabled: false, duration_seconds: 0 }, + }, + }; + serve(config); + await link(); + await expect(run({ json: true, failOn: "recommended" })).resolves.toBeUndefined(); + await expect(run({ json: true, failOn: "any" })).rejects.toThrow( + "1 security recommendation unmet", + ); + }); + + test("resolves the environment type for a literal instance id", async () => { + serve(INSECURE_OAUTH_CONFIG); + await link(); + await run({ json: true, instance: "ins_prod", failOn: "none" }); + const parsed = report(); + expect(parsed.instance.environmentType).toBe("production"); + expect(parsed.findings.some((f) => f.id === "oauth-custom-credentials")).toBe(true); + }); + + test("targets an app directly with --app", async () => { + await run({ json: true, app: "app_1", instance: "prod", failOn: "none" }); + expect(report().instance.instanceId).toBe("ins_prod"); + expect(report().fixCommand).toContain(" --app app_1 --instance ins_prod"); + for (const finding of report().findings.filter((f) => f.remedy.includes("clerk "))) { + expect(finding.remedy).toContain(" --app app_1 --instance ins_prod"); + } + }); +}); diff --git a/packages/cli-core/src/commands/security/audit.ts b/packages/cli-core/src/commands/security/audit.ts new file mode 100644 index 000000000..cbc54ca69 --- /dev/null +++ b/packages/cli-core/src/commands/security/audit.ts @@ -0,0 +1,56 @@ +import { CliError, ERROR_CODE } from "../../lib/errors.ts"; +import { log } from "../../lib/log.ts"; +import { NEXT_STEPS } from "../../lib/next-steps.ts"; +import { intro, outro } from "../../lib/spinner.ts"; +import { isAgent } from "../../mode.ts"; +import { formatReportHuman, formatReportJson } from "./format.ts"; +import { loadAudit } from "./load.ts"; +import type { AuditOptions, FailOnLevel, Finding, Severity } from "./types.ts"; + +const FAIL_ON_SEVERITIES: Record = { + critical: ["critical"], + recommended: ["critical", "recommended"], + any: ["critical", "recommended", "good-to-have"], + none: [], +}; + +export function failingFindings(findings: Finding[], failOn: FailOnLevel): Finding[] { + const severities = FAIL_ON_SEVERITIES[failOn]; + return findings.filter((f) => f.status !== "met" && severities.includes(f.severity)); +} + +export async function securityAudit(options: AuditOptions = {}): Promise { + const json = Boolean(options.json) || isAgent(); + const spotlight = Boolean(options.spotlight); + const failOn = options.failOn ?? "critical"; + + if (!json) intro("Security audit"); + const { report } = await loadAudit(options); + + if (json) { + log.data(formatReportJson(report, spotlight)); + } else { + log.blank(); + for (const line of formatReportHuman(report, spotlight)) log.info(line); + } + + const failing = failingFindings(report.findings, failOn); + if (failing.length > 0) { + const noun = failing.length === 1 ? "recommendation" : "recommendations"; + throw new CliError(`${failing.length} security ${noun} unmet (--fail-on ${failOn})`, { + code: ERROR_CODE.SECURITY_AUDIT_FAILED, + }); + } + + if (!json) { + const flags = `${options.app ? ` --app ${options.app}` : ""}${options.instance ? ` --instance ${options.instance}` : ""}`; + await outro( + report.fixCommand + ? [ + `Run \`clerk security fix${flags}\` to choose which recommendations to apply, or \`clerk security fix --all${flags}\` for every critical and recommended one`, + ...NEXT_STEPS.SECURITY_AUDIT, + ] + : NEXT_STEPS.SECURITY_AUDIT, + ); + } +} diff --git a/packages/cli-core/src/commands/security/catalog.test.ts b/packages/cli-core/src/commands/security/catalog.test.ts new file mode 100644 index 000000000..d03af910b --- /dev/null +++ b/packages/cli-core/src/commands/security/catalog.test.ts @@ -0,0 +1,349 @@ +import { test, expect, describe } from "bun:test"; +import { hasConfigChanges } from "../config/push.ts"; +import { isKnownDashboardPath } from "../open/dashboard-paths.ts"; +import { CHECKS, CHECK_IDS, findCheck } from "./catalog.ts"; +import { evaluate } from "./evaluate.ts"; +import { INSECURE_CONFIG, INSECURE_OAUTH_CONFIG, SECURE_CONFIG } from "./fixtures.ts"; +import { deepMerge } from "./merge.ts"; +import type { CheckDef, CheckInput, InstanceConfig, InstanceRef } from "./types.ts"; + +const REF: InstanceRef = { + appId: "app_1", + instanceId: "ins_prod", + environmentType: "production", + label: "My App (production)", +}; + +const production = (config: InstanceConfig): CheckInput => ({ + config, + environmentType: "production", +}); + +function withSection(config: InstanceConfig, key: string, patch: Record) { + return deepMerge(config, { [key]: patch }); +} + +const FIXABLE = CHECKS.filter((check) => check.patch); + +// The OAuth check needs an enabled social connection, which would satisfy +// the passwordless check, so the two get different insecure documents. +const insecureFor = (check: CheckDef) => + check.id === "oauth-custom-credentials" ? INSECURE_OAUTH_CONFIG : INSECURE_CONFIG; + +describe("security catalog", () => { + test("ids are unique kebab-case", () => { + expect(new Set(CHECK_IDS).size).toBe(CHECKS.length); + for (const id of CHECK_IDS) expect(id).toMatch(/^[a-z0-9]+(-[a-z0-9]+)*$/); + }); + + test.each(CHECKS)("$id links to a known dashboard path", (check) => { + expect(isKnownDashboardPath(check.dashboardPath)).toBe(true); + }); + + test.each(CHECKS)("$id is met on the secure fixture", (check) => { + expect(check.appliesTo?.(production(SECURE_CONFIG)) ?? true).toBe(true); + expect(check.evaluate(production(SECURE_CONFIG)).met).toBe(true); + }); + + test.each(CHECKS)("$id is unmet on the insecure fixture", (check) => { + expect(check.appliesTo?.(production(insecureFor(check))) ?? true).toBe(true); + expect(check.evaluate(production(insecureFor(check))).met).toBe(false); + }); + + test.each(FIXABLE)("$id patch changes the insecure fixture and satisfies the check", (check) => { + const patch = check.patch!(production(INSECURE_CONFIG)); + expect(hasConfigChanges(INSECURE_CONFIG, patch, true)).toBe(true); + const projected = deepMerge(INSECURE_CONFIG, patch); + expect(check.evaluate(production(projected)).met).toBe(true); + }); + + test.each(FIXABLE)("$id patch is a no-op on the secure fixture", (check) => { + const patch = check.patch!(production(SECURE_CONFIG)); + expect(check.evaluate(production(deepMerge(SECURE_CONFIG, patch))).met).toBe(true); + }); +}); + +describe("not applicable checks", () => { + const ids = (config: InstanceConfig, environmentType = "production") => + evaluate({ config, environmentType }, REF).map((f) => f.id); + + test("email checks drop out when email is not a sign-up identifier", () => { + const config = withSection(INSECURE_CONFIG, "auth_email", { used_for_sign_up: false }); + const result = ids(config); + for (const id of [ + "email-verification", + "email-link-same-client", + "block-disposable-email", + "block-email-subaddresses", + ]) { + expect(result).not.toContain(id); + } + }); + + test("password checks drop out when passwords are disabled", () => { + const result = ids(withSection(INSECURE_CONFIG, "auth_password", { enabled: false })); + for (const id of [ + "breach-detection", + "breach-detection-sign-in", + "client-trust", + "password-min-length", + ]) { + expect(result).not.toContain(id); + } + }); + + test.each([ + ["phone-verification", "auth_phone", { used_for_sign_up: false }], + [ + "allowlist-on-sign-in", + "auth_access_control", + { allowlist_enabled: false, blocklist_enabled: false }, + ], + ] as const)("%s drops out when its feature is off", (id, key, patch) => { + expect(ids(withSection(INSECURE_CONFIG, key, { ...patch }))).not.toContain(id); + }); + + test("oauth-custom-credentials only applies to production with a social connection", () => { + expect(ids(INSECURE_OAUTH_CONFIG, "development")).not.toContain("oauth-custom-credentials"); + expect(ids(INSECURE_CONFIG, "production")).not.toContain("oauth-custom-credentials"); + expect(ids(INSECURE_OAUTH_CONFIG, "production")).toContain("oauth-custom-credentials"); + }); + + test("oauth-custom-credentials names the providers on shared credentials", () => { + const finding = evaluate(production(INSECURE_OAUTH_CONFIG), REF).find( + (f) => f.id === "oauth-custom-credentials", + ); + expect(finding?.currentValue).toEqual(["google"]); + expect(finding?.current).toContain("google"); + }); +}); + +describe("blocked checks", () => { + test("mfa-required is blocked until a second factor is available", () => { + const finding = evaluate(production(INSECURE_CONFIG), REF).find((f) => f.id === "mfa-required"); + expect(finding?.status).toBe("blocked"); + expect(finding?.blockedBy).toBe("mfa"); + expect(finding?.patch).toBeNull(); + expect(finding?.remedy).toContain("Two-factor authentication"); + }); + + test("mfa-required is unmet once a factor is available", () => { + const config = withSection(INSECURE_CONFIG, "auth_multi_factor", { + authenticator_app: { enabled: true }, + }); + const finding = evaluate(production(config), REF).find((f) => f.id === "mfa-required"); + expect(finding?.status).toBe("unmet"); + expect(finding?.patch).toEqual({ auth_multi_factor: { required_for_sign_up: true } }); + }); +}); + +describe("patch details", () => { + test("email-verification keeps existing verification strategies", () => { + const config = withSection(INSECURE_CONFIG, "auth_email", { + verification_strategies: ["email_link"], + }); + expect(findCheck("email-verification")!.patch!(production(config))).toEqual({ + auth_email: { verify_at_sign_up: true, verification_strategies: ["email_link"] }, + }); + }); + + test("email-verification falls back to email_code", () => { + expect(findCheck("email-verification")!.patch!(production(INSECURE_CONFIG))).toEqual({ + auth_email: { verify_at_sign_up: true, verification_strategies: ["email_code"] }, + }); + }); + + test("session-lifetime keeps a valid current duration", () => { + const config = withSection(INSECURE_CONFIG, "session_settings", { + maximum_lifetime: { enabled: false, duration_seconds: 86400 }, + }); + expect(findCheck("session-lifetime")!.patch!(production(config))).toEqual({ + session_settings: { maximum_lifetime: { enabled: true, duration_seconds: 86400 } }, + }); + }); + + test("lockout-threshold reports the disabled state", () => { + const result = findCheck("lockout-threshold")!.evaluate(production(INSECURE_CONFIG)); + expect(result.current).toBe("Lockout disabled"); + expect(result.currentValue).toBeNull(); + }); +}); + +describe("passwordless detection", () => { + test.each([ + ["email code", "auth_email", { sign_in_strategies: ["email_code"] }], + ["phone code", "auth_phone", { sign_in_strategies: ["phone_code"] }], + ["passkey", "auth_passkey", { used_for_sign_in: true }], + ["web3", "auth_web3", { used_for_sign_in: true }], + ["social connection", "connection_oauth_github", { enabled: true }], + ] as const)("%s counts as passwordless", (_name, key, patch) => { + const config = withSection(INSECURE_CONFIG, key, { ...patch }); + expect(findCheck("passwordless-auth")!.evaluate(production(config)).met).toBe(true); + }); +}); + +describe("suggested patches", () => { + const suggestion = (config: InstanceConfig, id: string) => + evaluate(production(config), REF).find((f) => f.id === id)?.suggestedPatch; + + test("mfa suggests authenticator apps and backup codes", () => { + expect(suggestion(INSECURE_CONFIG, "mfa")).toEqual({ + auth_multi_factor: { authenticator_app: { enabled: true }, backup_code: { enabled: true } }, + }); + }); + + test("mfa suggestion is null once met", () => { + expect(suggestion(SECURE_CONFIG, "mfa")).toBeNull(); + }); + + test("fixable findings carry a patch, not a suggestion", () => { + const lockout = evaluate(production(INSECURE_CONFIG), REF).find( + (f) => f.id === "user-lockout", + )!; + expect(lockout.patch).not.toBeNull(); + expect(lockout.suggestedPatch).toBeNull(); + }); + + test("passwordless-auth adds an email code when email is collected", () => { + const config = withSection(INSECURE_CONFIG, "auth_email", { sign_in_strategies: ["password"] }); + expect(suggestion(config, "passwordless-auth")).toEqual({ + auth_email: { used_for_sign_in: true, sign_in_strategies: ["password", "email_code"] }, + }); + }); + + test("passwordless-auth falls back to phone code, then passkeys", () => { + const noEmail = withSection(INSECURE_CONFIG, "auth_email", { + used_for_sign_up: false, + used_for_sign_in: false, + }); + expect(suggestion(noEmail, "passwordless-auth")).toEqual({ + auth_phone: { used_for_sign_in: true, sign_in_strategies: ["phone_code"] }, + }); + const noPhone = withSection(noEmail, "auth_phone", { + used_for_sign_up: false, + used_for_sign_in: false, + }); + expect(suggestion(noPhone, "passwordless-auth")).toEqual({ + auth_passkey: { used_for_sign_in: true }, + }); + }); + + test("remedy is a fix command carrying the suggested decision", () => { + const mfa = evaluate(production(INSECURE_CONFIG), REF).find((f) => f.id === "mfa")!; + expect(mfa.remedy).toContain( + "clerk security fix mfa --factors authenticator,backup-code --app app_1 --instance ins_prod", + ); + expect(mfa.decision).toEqual({ + flag: "factors", + multiple: true, + options: ["authenticator", "backup-code", "sms"], + suggested: ["authenticator", "backup-code"], + }); + }); + + test("a blocked remedy names the combined fix command", () => { + const required = evaluate(production(INSECURE_CONFIG), REF).find( + (f) => f.id === "mfa-required", + )!; + expect(required.remedy).toContain("clerk security fix mfa mfa-required --app app_1"); + }); + + test("oauth-custom-credentials has no suggestion", () => { + expect(suggestion(INSECURE_OAUTH_CONFIG, "oauth-custom-credentials")).toBeNull(); + }); +}); + +describe("plan-gated features", () => { + test.each([ + ["mfa", "app:mfa_totp"], + ["passkeys", "app:passkey"], + ["session-lifetime", "app:custom_session_duration"], + ])("%s reports feature %s", (id, feature) => { + expect(findCheck(id)!.feature).toBe(feature); + const finding = evaluate(production(INSECURE_CONFIG), REF).find((f) => f.id === id); + expect(finding?.feature).toBe(feature); + }); + + test("checks without a feature omit the key", () => { + const finding = evaluate(production(INSECURE_CONFIG), REF).find( + (f) => f.id === "user-lockout", + )!; + expect("feature" in finding).toBe(false); + }); +}); + +describe("decision patches", () => { + const input = production(INSECURE_CONFIG); + const mfa = findCheck("mfa")!.decision!; + const passwordless = findCheck("passwordless-auth")!.decision!; + + test("mfa with sms enables the phone second factor", () => { + expect(mfa.patch(["authenticator", "sms"], input)).toEqual({ + auth_multi_factor: { authenticator_app: { enabled: true } }, + auth_phone: { used_for_second_factor: true, second_factor_strategies: ["phone_code"] }, + }); + }); + + test.each([ + ["email-code", { auth_email: { used_for_sign_in: true, sign_in_strategies: ["email_code"] } }], + ["email-link", { auth_email: { used_for_sign_in: true, sign_in_strategies: ["email_link"] } }], + ["phone-code", { auth_phone: { used_for_sign_in: true, sign_in_strategies: ["phone_code"] } }], + ["passkey", { auth_passkey: { used_for_sign_in: true } }], + ])("passwordless-auth %s", (strategy, expected) => { + expect(passwordless.patch([strategy], input)).toEqual(expected); + }); + + test.each<{ values: string[] }>([ + { values: ["authenticator", "backup-code"] }, + { values: ["sms"] }, + ])("mfa is met after applying $values", ({ values }) => { + const projected = deepMerge(INSECURE_CONFIG, mfa.patch(values, input)); + expect(findCheck("mfa")!.evaluate(production(projected)).met).toBe(true); + }); + + test.each(passwordless.options.map((o) => o.value))( + "passwordless-auth is met after %s", + (strategy) => { + const projected = deepMerge(INSECURE_CONFIG, passwordless.patch([strategy], input)); + expect(findCheck("passwordless-auth")!.evaluate(production(projected)).met).toBe(true); + }, + ); +}); + +describe("effective protection", () => { + const MATRIX = [ + { a: false, b: false }, + { a: false, b: true }, + { a: true, b: false }, + { a: true, b: true }, + ]; + + test.each(MATRIX)( + "mfa-required follows required_for_sign_up=$b, not required_for_sign_in=$a", + ({ a: signIn, b: signUp }) => { + const config = deepMerge(SECURE_CONFIG, { + auth_multi_factor: { required_for_sign_in: signIn, required_for_sign_up: signUp }, + }); + expect(findCheck("mfa-required")!.evaluate(production(config)).met).toBe(signUp); + }, + ); + + test.each(MATRIX)( + "breach-detection-sign-in needs enforce_hibp_on_sign_in=$b and disable_hibp=$a off", + ({ a: disabled, b: enforce }) => { + const config = deepMerge(SECURE_CONFIG, { + auth_password: { disable_hibp: disabled, enforce_hibp_on_sign_in: enforce }, + }); + expect(findCheck("breach-detection-sign-in")!.evaluate(production(config)).met).toBe( + !disabled && enforce, + ); + }, + ); + + test("backup codes alone do not satisfy MFA availability", () => { + const config = deepMerge(INSECURE_CONFIG, { + auth_multi_factor: { backup_code: { enabled: true } }, + }); + expect(findCheck("mfa")!.evaluate(production(config)).met).toBe(false); + }); +}); diff --git a/packages/cli-core/src/commands/security/catalog.ts b/packages/cli-core/src/commands/security/catalog.ts new file mode 100644 index 000000000..c01de38d7 --- /dev/null +++ b/packages/cli-core/src/commands/security/catalog.ts @@ -0,0 +1,500 @@ +// Pure checks over the Platform config document. `critical` is the account-takeover +// kill chain and caps the grade at C. Composition rules and zxcvbn scores are +// intentionally absent: Clerk no longer recommends them (NIST 800-63B). + +import { isRecord } from "../../lib/objects.ts"; +import type { CheckDecision, CheckDef, CheckInput, ConfigPatch, InstanceConfig } from "./types.ts"; + +const DOCS = "https://clerk.com/docs/guides"; +const DOCS_SIGN_IN_OPTIONS = `${DOCS}/configure/auth-strategies/sign-up-sign-in-options`; +const DOCS_PASSWORDS = `${DOCS}/secure/password-protection-and-rules`; +const DOCS_LOCKOUT = `${DOCS}/secure/user-lockout`; +const DOCS_SESSIONS = `${DOCS}/secure/session-options`; +const DOCS_RESTRICTIONS = `${DOCS}/secure/restricting-access`; + +// Backend minimum for session durations. +const MIN_SESSION_SECONDS = 300; +const DEFAULT_LIFETIME_SECONDS = 604800; +const MIN_PASSWORD_LENGTH = 8; + +// Plan-gated in the Dashboard. +const FEATURE_MFA = "app:mfa_totp"; +const FEATURE_PASSKEY = "app:passkey"; +const FEATURE_LIFETIME = "app:custom_session_duration"; + +const rec = (value: unknown): Record => (isRecord(value) ? value : {}); + +function at(config: InstanceConfig, path: string): unknown { + return path.split(".").reduce((node, key) => rec(node)[key], config); +} + +const flag = (config: InstanceConfig, path: string): boolean => at(config, path) === true; +const num = (config: InstanceConfig, path: string): number => { + const value = at(config, path); + return typeof value === "number" ? value : 0; +}; +const list = (config: InstanceConfig, path: string): string[] => { + const value = at(config, path); + return Array.isArray(value) ? value.filter((v): v is string => typeof v === "string") : []; +}; + +function booleanCheck( + path: string, + opts: { invert?: boolean; labels?: [string, string] } = {}, +): Pick { + const [metLabel, unmetLabel] = opts.labels ?? ["Enabled", "Disabled"]; + return { + path, + evaluate({ config }) { + const raw = flag(config, path); + const met = opts.invert ? !raw : raw; + return { + met, + currentValue: raw, + recommendedValue: !opts.invert, + current: met ? metLabel : unmetLabel, + recommended: metLabel, + }; + }, + }; +} + +export const emailEnabled = (config: InstanceConfig): boolean => + flag(config, "auth_email.used_for_sign_up"); +export const passwordEnabled = (config: InstanceConfig): boolean => + flag(config, "auth_password.enabled"); +export const phoneEnabled = (config: InstanceConfig): boolean => + flag(config, "auth_phone.used_for_sign_up"); + +export const mfaAvailable = (config: InstanceConfig): boolean => + flag(config, "auth_multi_factor.authenticator_app.enabled") || + flag(config, "auth_phone.used_for_second_factor"); + +export function enabledOAuthProviders(config: InstanceConfig): string[] { + return Object.keys(config) + .filter((key) => key.startsWith("connection_oauth_") && flag(config, `${key}.enabled`)) + .map((key) => key.slice("connection_oauth_".length)); +} + +export function passwordlessEnabled(config: InstanceConfig): boolean { + const emailCode = list(config, "auth_email.sign_in_strategies").some((s) => + ["email_code", "email_link"].includes(s), + ); + const phoneCode = list(config, "auth_phone.sign_in_strategies").includes("phone_code"); + return ( + emailCode || + phoneCode || + flag(config, "auth_passkey.used_for_sign_in") || + flag(config, "auth_web3.used_for_sign_in") || + enabledOAuthProviders(config).length > 0 + ); +} + +const union = (values: string[], value: string) => + values.includes(value) ? values : [...values, value]; + +const MFA_DECISION: CheckDecision = { + flag: "factors", + prompt: "Which second factors should users be able to enroll?", + multiple: true, + options: [ + { value: "authenticator", label: "Authenticator app (TOTP)" }, + { value: "backup-code", label: "Backup codes" }, + { value: "sms", label: "SMS code" }, + ], + defaults: () => ["authenticator", "backup-code"], + validate(values, { config }) { + if ( + values.includes("backup-code") && + !values.includes("authenticator") && + !values.includes("sms") && + !mfaAvailable(config) + ) { + return "Backup codes require another second factor. Include authenticator or sms in --factors."; + } + }, + patch(values, { config }) { + const patch: ConfigPatch = {}; + const mfa: Record = {}; + if (values.includes("authenticator")) mfa.authenticator_app = { enabled: true }; + if (values.includes("backup-code")) mfa.backup_code = { enabled: true }; + if (Object.keys(mfa).length) patch.auth_multi_factor = mfa; + if (values.includes("sms")) { + patch.auth_phone = { + used_for_second_factor: true, + second_factor_strategies: union( + list(config, "auth_phone.second_factor_strategies"), + "phone_code", + ), + }; + } + return patch; + }, +}; + +const PASSWORDLESS_DECISION: CheckDecision = { + flag: "strategy", + prompt: "Which passwordless sign-in method should be offered?", + multiple: false, + options: [ + { value: "email-code", label: "One-time code by email" }, + { value: "email-link", label: "Magic link by email" }, + { value: "phone-code", label: "One-time code by SMS" }, + { value: "passkey", label: "Passkeys" }, + ], + // Prefer an identifier already collected so sign-up keeps its shape. + defaults: ({ config }) => + emailEnabled(config) || flag(config, "auth_email.used_for_sign_in") + ? ["email-code"] + : phoneEnabled(config) || flag(config, "auth_phone.used_for_sign_in") + ? ["phone-code"] + : ["passkey"], + patch([strategy], { config }) { + if (strategy === "passkey") return { auth_passkey: { used_for_sign_in: true } }; + const section = strategy === "phone-code" ? "auth_phone" : "auth_email"; + const apiStrategy = strategy!.replace("-", "_"); + return { + [section]: { + used_for_sign_in: true, + sign_in_strategies: union(list(config, `${section}.sign_in_strategies`), apiStrategy), + }, + }; + }, +}; + +function verifyAtSignUpPatch(section: string, fallback: string) { + return ({ config }: CheckInput) => { + const strategies = list(config, `${section}.verification_strategies`); + return { + [section]: { + verify_at_sign_up: true, + verification_strategies: strategies.length ? strategies : [fallback], + }, + }; + }; +} + +export const CHECKS: CheckDef[] = [ + // --- critical --- + { + id: "bot-protection", + title: "Bot sign-up protection", + description: "Require a CAPTCHA challenge to block automated sign-ups.", + severity: "critical", + dashboardPath: "user-authentication", + docsUrl: `${DOCS}/secure/bot-protection`, + ...booleanCheck("auth_attack_protection.bot_protection.captcha_enabled"), + patch: () => ({ + auth_attack_protection: { + bot_protection: { captcha_enabled: true, captcha_widget_type: "smart" }, + }, + }), + }, + { + id: "breach-detection", + title: "Breached password detection", + description: "Reject passwords found in known data breaches (HaveIBeenPwned).", + severity: "critical", + dashboardPath: "user-authentication", + docsUrl: DOCS_PASSWORDS, + appliesTo: ({ config }) => passwordEnabled(config), + ...booleanCheck("auth_password.disable_hibp", { invert: true }), + patch: () => ({ auth_password: { disable_hibp: false, enforce_hibp_on_sign_in: true } }), + }, + { + id: "user-lockout", + title: "Brute-force lockout", + description: "Lock accounts after repeated failed sign-in attempts.", + severity: "critical", + dashboardPath: "user-authentication", + docsUrl: DOCS_LOCKOUT, + ...booleanCheck("auth_attack_protection.user_lockout.enabled"), + patch: () => ({ auth_attack_protection: { user_lockout: { enabled: true } } }), + }, + { + id: "client-trust", + title: "Device trust", + description: + "Challenge sign-ins from unrecognized devices, a key defense against credential stuffing.", + severity: "critical", + dashboardPath: "user-authentication", + docsUrl: `${DOCS}/secure/device-trust`, + appliesTo: ({ config }) => passwordEnabled(config), + ...booleanCheck("auth_password.device_trust.enabled"), + patch: () => ({ auth_password: { device_trust: { enabled: true } } }), + }, + { + id: "mfa", + title: "Two-factor authentication", + description: "Offer a second factor (authenticator app, SMS, or backup codes) to your users.", + severity: "critical", + path: "auth_multi_factor", + dashboardPath: "user-authentication", + docsUrl: DOCS_SIGN_IN_OPTIONS, + feature: FEATURE_MFA, + evaluate({ config }) { + const met = mfaAvailable(config); + return { + met, + currentValue: met, + recommendedValue: true, + current: met ? "Available" : "Not available", + recommended: "Available", + }; + }, + decision: MFA_DECISION, + }, + { + id: "passwordless-auth", + title: "Passwordless authentication available", + description: + "Offer at least one passwordless sign-in method (passkey, email or SMS code, or social) so users are not limited to passwords.", + severity: "critical", + path: "auth_email.sign_in_strategies", + dashboardPath: "user-authentication", + docsUrl: DOCS_SIGN_IN_OPTIONS, + evaluate({ config }) { + const met = passwordlessEnabled(config); + return { + met, + currentValue: met, + recommendedValue: true, + current: met ? "Available" : "None", + recommended: "At least one", + }; + }, + decision: PASSWORDLESS_DECISION, + }, + { + id: "email-verification", + title: "Verify email at sign-up", + description: "Require users to verify their email address before completing sign-up.", + severity: "critical", + dashboardPath: "user-authentication", + docsUrl: DOCS_SIGN_IN_OPTIONS, + appliesTo: ({ config }) => emailEnabled(config), + ...booleanCheck("auth_email.verify_at_sign_up", { labels: ["Required", "Not required"] }), + patch: verifyAtSignUpPatch("auth_email", "email_code"), + }, + + // --- recommended --- + { + id: "breach-detection-sign-in", + title: "Reject breached passwords on sign-in", + description: "Force a password reset when an existing password is later found in a breach.", + severity: "recommended", + dashboardPath: "user-authentication", + docsUrl: DOCS_PASSWORDS, + appliesTo: ({ config }) => passwordEnabled(config), + path: "auth_password.enforce_hibp_on_sign_in", + evaluate({ config }) { + const met = + !flag(config, "auth_password.disable_hibp") && + flag(config, "auth_password.enforce_hibp_on_sign_in"); + return { + met, + currentValue: met, + recommendedValue: true, + current: met ? "Enabled" : "Disabled", + recommended: "Enabled", + }; + }, + patch: () => ({ auth_password: { disable_hibp: false, enforce_hibp_on_sign_in: true } }), + }, + { + id: "lockout-threshold", + title: "Strict lockout threshold", + description: "Lock accounts after 10 or fewer failed attempts.", + severity: "recommended", + path: "auth_attack_protection.user_lockout.max_attempts", + dashboardPath: "user-authentication", + docsUrl: DOCS_LOCKOUT, + evaluate({ config }) { + const enabled = flag(config, "auth_attack_protection.user_lockout.enabled"); + const attempts = num(config, "auth_attack_protection.user_lockout.max_attempts"); + return { + met: enabled && attempts <= 10, + currentValue: enabled ? attempts : null, + recommendedValue: 10, + current: enabled ? `${attempts} attempts` : "Lockout disabled", + recommended: "10 or fewer", + }; + }, + patch: () => ({ + auth_attack_protection: { user_lockout: { enabled: true, max_attempts: 10 } }, + }), + }, + { + id: "mfa-required", + title: "Require two-factor authentication", + description: "Force every user to set up a second factor, not just offer it.", + severity: "recommended", + path: "auth_multi_factor.required_for_sign_up", + dashboardPath: "user-authentication", + docsUrl: DOCS_SIGN_IN_OPTIONS, + blockedBy: "mfa", + evaluate({ config }) { + // Drives the setup-mfa task; required_for_sign_in is a different setting. + const met = flag(config, "auth_multi_factor.required_for_sign_up"); + return { + met, + currentValue: met, + recommendedValue: true, + current: met ? "Required" : "Optional", + recommended: "Required", + }; + }, + patch: () => ({ auth_multi_factor: { required_for_sign_up: true } }), + }, + { + id: "passkeys", + title: "Passkeys", + description: "Offer phishing-resistant passkeys as a sign-in option for your users.", + severity: "recommended", + dashboardPath: "user-authentication", + docsUrl: DOCS_SIGN_IN_OPTIONS, + feature: FEATURE_PASSKEY, + ...booleanCheck("auth_passkey.used_for_sign_in"), + patch: () => ({ auth_passkey: { used_for_sign_in: true } }), + }, + { + id: "phone-verification", + title: "Verify phone at sign-up", + description: "Require users to verify their phone number before completing sign-up.", + severity: "recommended", + dashboardPath: "user-authentication", + docsUrl: DOCS_SIGN_IN_OPTIONS, + appliesTo: ({ config }) => phoneEnabled(config), + ...booleanCheck("auth_phone.verify_at_sign_up", { labels: ["Required", "Not required"] }), + patch: verifyAtSignUpPatch("auth_phone", "phone_code"), + }, + { + id: "password-min-length", + title: "Minimum password length", + description: `Require passwords of at least ${MIN_PASSWORD_LENGTH} characters.`, + severity: "recommended", + path: "auth_password.min_length", + dashboardPath: "user-authentication", + docsUrl: DOCS_PASSWORDS, + appliesTo: ({ config }) => passwordEnabled(config), + evaluate({ config }) { + const length = num(config, "auth_password.min_length"); + return { + met: length >= MIN_PASSWORD_LENGTH, + currentValue: length, + recommendedValue: MIN_PASSWORD_LENGTH, + current: `${length} characters`, + recommended: `${MIN_PASSWORD_LENGTH} or more`, + }; + }, + patch: () => ({ auth_password: { min_length: MIN_PASSWORD_LENGTH } }), + }, + { + id: "allowlist-on-sign-in", + title: "Enforce allowlist and blocklist on sign-in", + description: + "Apply sign-up restrictions to sign-in too, so an identifier that is later blocked cannot keep signing in.", + severity: "recommended", + dashboardPath: "user-authentication", + docsUrl: DOCS_RESTRICTIONS, + appliesTo: ({ config }) => + flag(config, "auth_access_control.allowlist_enabled") || + flag(config, "auth_access_control.blocklist_enabled"), + ...booleanCheck("auth_access_control.allowlist_blocklist_enforced_on_sign_in"), + patch: () => ({ auth_access_control: { allowlist_blocklist_enforced_on_sign_in: true } }), + }, + { + id: "oauth-custom-credentials", + title: "Custom OAuth credentials in production", + description: + "Use your own OAuth client credentials for social connections instead of Clerk's shared development credentials.", + severity: "recommended", + path: "connection_oauth_*.client_id", + dashboardPath: "user-authentication", + docsUrl: `${DOCS}/configure/auth-strategies/social-connections/overview`, + appliesTo: ({ config, environmentType }) => + environmentType === "production" && enabledOAuthProviders(config).length > 0, + evaluate({ config }) { + const shared = enabledOAuthProviders(config).filter( + (provider) => !at(config, `connection_oauth_${provider}.client_id`), + ); + return { + met: shared.length === 0, + currentValue: shared, + recommendedValue: [], + current: shared.length ? `Shared credentials: ${shared.join(", ")}` : "Custom credentials", + recommended: "Custom credentials for every provider", + }; + }, + manualRemedy: + "Register an OAuth app with each provider and set `client_id` and `client_secret` on its `connection_oauth_` config key.", + }, + + // --- good to have --- + { + id: "session-lifetime", + title: "Bounded session lifetime", + description: "Expire sessions after a maximum lifetime instead of keeping them indefinitely.", + severity: "good-to-have", + dashboardPath: "sessions", + docsUrl: DOCS_SESSIONS, + feature: FEATURE_LIFETIME, + ...booleanCheck("session_settings.maximum_lifetime.enabled"), + patch: ({ config }) => { + const current = num(config, "session_settings.maximum_lifetime.duration_seconds"); + return { + session_settings: { + maximum_lifetime: { + enabled: true, + duration_seconds: Math.max( + current >= MIN_SESSION_SECONDS ? current : DEFAULT_LIFETIME_SECONDS, + flag(config, "session_settings.inactivity_timeout.enabled") + ? num(config, "session_settings.inactivity_timeout.duration_seconds") + + MIN_SESSION_SECONDS + : MIN_SESSION_SECONDS, + ), + }, + }, + }; + }, + }, + { + id: "email-link-same-client", + title: "Email-link same-client requirement", + description: "Require magic links to be opened on the same device that requested them.", + severity: "good-to-have", + dashboardPath: "user-authentication", + docsUrl: `${DOCS}/secure/best-practices/protect-email-links`, + appliesTo: ({ config }) => emailEnabled(config), + ...booleanCheck("auth_attack_protection.email_link_require_same_client"), + patch: () => ({ auth_attack_protection: { email_link_require_same_client: true } }), + }, + { + id: "block-disposable-email", + title: "Block disposable email domains", + description: "Reject sign-ups from throwaway email providers.", + severity: "good-to-have", + dashboardPath: "user-authentication", + docsUrl: DOCS_RESTRICTIONS, + appliesTo: ({ config }) => emailEnabled(config), + ...booleanCheck("auth_access_control.block_disposable_email_domains"), + patch: () => ({ auth_access_control: { block_disposable_email_domains: true } }), + }, + { + id: "block-email-subaddresses", + title: "Block email subaddresses", + description: 'Prevent abuse from "+alias" variations of the same email address.', + severity: "good-to-have", + dashboardPath: "user-authentication", + docsUrl: DOCS_RESTRICTIONS, + appliesTo: ({ config }) => emailEnabled(config), + ...booleanCheck("auth_access_control.block_email_subaddresses"), + patch: () => ({ auth_access_control: { block_email_subaddresses: true } }), + }, +]; + +export const CHECK_IDS: string[] = CHECKS.map((check) => check.id); + +export function findCheck(id: string): CheckDef | undefined { + return CHECKS.find((check) => check.id === id); +} diff --git a/packages/cli-core/src/commands/security/evaluate.ts b/packages/cli-core/src/commands/security/evaluate.ts new file mode 100644 index 000000000..835613439 --- /dev/null +++ b/packages/cli-core/src/commands/security/evaluate.ts @@ -0,0 +1,124 @@ +import { agentDocsUrl } from "../../lib/errors.ts"; +import { isAgent } from "../../mode.ts"; +import { buildDashboardUrl } from "../open/index.ts"; +import { CHECKS } from "./catalog.ts"; +import { computeScore } from "./score.ts"; +import type { + AuditReport, + CheckDef, + CheckInput, + Finding, + FindingStatus, + InstanceRef, + Severity, +} from "./types.ts"; + +export const SEVERITY_ORDER: Severity[] = ["critical", "recommended", "good-to-have"]; +const STATUS_RANK: Record = { unmet: 0, blocked: 1, met: 2 }; + +export function targetFlags(ref: InstanceRef): string { + return ` --app ${ref.appId} --instance ${ref.instanceId}`; +} + +export function fixCommandFor(ids: string[], ref: InstanceRef): string { + return `clerk security fix ${ids.join(" ")}${targetFlags(ref)}${isAgent() ? " --yes" : ""}`; +} + +export function fixCommandWithDecision( + check: CheckDef, + values: string[], + ref: InstanceRef, +): string { + const flag = check.decision ? ` --${check.decision.flag} ${values.join(",")}` : ""; + return `clerk security fix ${check.id}${flag}${targetFlags(ref)}${isAgent() ? " --yes" : ""}`; +} + +export function remedyFor( + check: CheckDef, + status: FindingStatus, + ref: InstanceRef, + input: CheckInput, + blockedByTitle?: string, +): string { + if (status === "met") return "Nothing to do."; + if (status === "blocked") + return `Make "${blockedByTitle}" available first (\`${check.blockedBy}\`), or fix both together: \`${fixCommandFor([check.blockedBy!, check.id], ref)}\`.`; + if (check.patch) return `Run \`${fixCommandFor([check.id], ref)}\`.`; + if (check.decision) { + const values = check.decision.defaults(input); + return `Run \`${fixCommandWithDecision(check, values, ref)}\` (or pick other ${check.decision.options.map((o) => o.value).join(", ")}).`; + } + return check.manualRemedy ?? "Configure this in the Clerk Dashboard."; +} + +export function evaluate(input: CheckInput, ref: InstanceRef): Finding[] { + const applicable = CHECKS.filter((check) => check.appliesTo?.(input) ?? true); + const evaluations = new Map(applicable.map((check) => [check.id, check.evaluate(input)])); + + const findings = applicable.map((check): Finding => { + const evaluation = evaluations.get(check.id)!; + const prerequisite = check.blockedBy ? evaluations.get(check.blockedBy) : undefined; + const status: FindingStatus = evaluation.met + ? "met" + : prerequisite && !prerequisite.met + ? "blocked" + : "unmet"; + const blockedByTitle = CHECKS.find((c) => c.id === check.blockedBy)?.title; + const patch = status === "unmet" && check.patch ? check.patch(input) : null; + const decision = status === "unmet" && !check.patch ? check.decision : undefined; + const suggested = decision?.defaults(input) ?? []; + const suggestedPatch = decision ? decision.patch(suggested, input) : null; + return { + id: check.id, + title: check.title, + description: check.description, + severity: check.severity, + status, + path: check.path, + ...evaluation, + ...(check.feature && { feature: check.feature }), + ...(check.blockedBy && { blockedBy: check.blockedBy }), + patch, + suggestedPatch, + ...(decision && { + decision: { + flag: decision.flag, + multiple: decision.multiple, + options: decision.options.map((o) => o.value), + suggested, + }, + }), + remedy: remedyFor(check, status, ref, input, blockedByTitle), + docsUrl: agentDocsUrl(check.docsUrl), + dashboardUrl: buildDashboardUrl(ref.appId, ref.instanceId, check.dashboardPath), + }; + }); + + return sortFindings(findings); +} + +export function sortFindings(findings: Finding[]): Finding[] { + return [...findings].sort( + (a, b) => + SEVERITY_ORDER.indexOf(a.severity) - SEVERITY_ORDER.indexOf(b.severity) || + STATUS_RANK[a.status] - STATUS_RANK[b.status], + ); +} + +export function fixableIds(findings: Finding[], goodToHave = true): string[] { + return findings + .filter((f) => f.status === "unmet" && f.patch && (goodToHave || f.severity !== "good-to-have")) + .map((f) => f.id); +} + +export function buildReport(input: CheckInput, ref: InstanceRef): AuditReport { + const findings = evaluate(input, ref); + // Good-to-have costs users convenience; never suggested by default. + const fixable = fixableIds(findings, false); + return { + instance: ref, + score: computeScore(findings), + fixCommand: fixable.length ? fixCommandFor(fixable, ref) : null, + findings, + }; +} diff --git a/packages/cli-core/src/commands/security/fix.test.ts b/packages/cli-core/src/commands/security/fix.test.ts new file mode 100644 index 000000000..7f3695a8e --- /dev/null +++ b/packages/cli-core/src/commands/security/fix.test.ts @@ -0,0 +1,486 @@ +import { test, expect, describe, beforeEach, afterEach, spyOn, mock } from "bun:test"; +import { mkdtemp, rm } from "node:fs/promises"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import { _setConfigDir, setProfile } from "../../lib/config.ts"; +import { + useCaptureLog, + credentialStoreStubs, + gitStubs, + libPromptsStubs, + listageStubs, + stubFetch, +} from "../../test/lib/stubs.ts"; +import { INSECURE_CONFIG, SECURE_CONFIG } from "./fixtures.ts"; +import { deepMerge } from "./merge.ts"; +import type { FixOptions, FixSummary } from "./types.ts"; + +mock.module("../../lib/credential-store.ts", () => credentialStoreStubs); +mock.module("../../lib/git.ts", () => gitStubs); +mock.module("../../lib/prompts.ts", () => libPromptsStubs); +mock.module("../../lib/listage.ts", () => ({ + ...listageStubs, + select: async (config: { default?: unknown }) => config.default, +})); +mock.module("../../lib/spinner.ts", () => ({ + intro: () => {}, + outro: () => {}, + pausedOutro: () => {}, + bar: () => {}, + withGutter: async ( + _title: string, + fn: (controls: { setNextSteps: (steps: readonly string[]) => void }) => Promise, + ) => fn({ setNextSteps: () => {} }), + withSpinner: async (_msg: string, fn: () => Promise) => fn(), +})); + +interface Captured { + method: string; + url: string; + body: Record | null; +} + +describe("security fix", () => { + const originalEnv = { ...process.env }; + const originalFetch = globalThis.fetch; + let tempDir: string; + let logSpy: ReturnType; + let errorSpy: ReturnType; + let requests: Captured[]; + const captured = useCaptureLog(); + + function serve(config: Record) { + stubFetch(async (input, init) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + const body = typeof init?.body === "string" ? JSON.parse(init.body) : null; + requests.push({ method, url, body }); + if (!url.includes("/config")) throw new Error(`Unexpected fetch: ${url}`); + const result = method === "PATCH" ? deepMerge(config, body) : config; + return new Response(JSON.stringify(result), { status: 200 }); + }); + } + + async function link() { + await setProfile(process.cwd(), { + workspaceId: "org_1", + appId: "app_1", + instances: { development: "ins_dev" }, + }); + } + + async function run(ids: string[] = [], options: FixOptions = {}) { + const { securityFix } = await import("./fix.ts"); + return securityFix(ids, options); + } + + const patches = () => requests.filter((r) => r.method === "PATCH"); + + beforeEach(async () => { + tempDir = await mkdtemp(join(tmpdir(), "clerk-security-fix-test-")); + _setConfigDir(tempDir); + process.env.CLERK_PLATFORM_API_KEY = "test_key"; + process.env.CLERK_PLATFORM_API_URL = "https://test-api.clerk.com"; + process.env.CLERK_MODE = "human"; + requests = []; + logSpy = spyOn(console, "log").mockImplementation(() => {}); + errorSpy = spyOn(console, "error").mockImplementation(() => {}); + serve(INSECURE_CONFIG); + await link(); + }); + + afterEach(async () => { + _setConfigDir(undefined); + process.env = { ...originalEnv }; + globalThis.fetch = originalFetch; + logSpy.mockRestore(); + errorSpy.mockRestore(); + await rm(tempDir, { recursive: true, force: true }); + }); + + describe("argument validation", () => { + test("agent mode requires ids or --all", async () => { + process.env.CLERK_MODE = "agent"; + await expect(run()).rejects.toThrow("Pass one or more check ids, or --all"); + expect(requests).toHaveLength(0); + }); + + test("human mode with no ids opens a picker preselecting the critical and recommended gaps", async () => { + await run([], { yes: true, json: true }); + const summary = JSON.parse(captured.out) as FixSummary; + expect(summary.applied).toContain("user-lockout"); + expect(summary.applied).not.toContain("mfa"); + expect(summary.applied).not.toContain("block-email-subaddresses"); + expect(summary.remaining).toContain("block-email-subaddresses"); + expect(summary.remaining).toContain("mfa"); + }); + + test("human mode picker on a secure instance has nothing to offer", async () => { + serve(SECURE_CONFIG); + await run([], { yes: true }); + expect(captured.err).toContain("Nothing to fix"); + expect(patches()).toHaveLength(0); + }); + + test("rejects ids together with --all", async () => { + await expect(run(["user-lockout"], { all: true })).rejects.toThrow("not both"); + }); + + test("rejects unknown ids and lists the valid ones", async () => { + await expect(run(["nope"])).rejects.toThrow( + /Unknown check: nope.*Valid ids: bot-protection/s, + ); + expect(requests).toHaveLength(0); + }); + + test("agent mode requires --yes", async () => { + process.env.CLERK_MODE = "agent"; + await expect(run(["user-lockout"])).rejects.toThrow("Pass --yes"); + expect(requests).toHaveLength(0); + }); + + test("a blocked id without its prerequisite is reported before anything is written", async () => { + await expect(run(["mfa-required", "user-lockout"], { yes: true })).rejects.toThrow( + /manual change:.*mfa-required: Make "Two-factor authentication" available first/s, + ); + expect(patches()).toHaveLength(0); + }); + }); + + test("applies one merged patch for several ids", async () => { + await run(["user-lockout", "lockout-threshold", "client-trust"], { yes: true }); + expect(patches()).toHaveLength(1); + expect(patches()[0]!.body).toEqual({ + auth_attack_protection: { user_lockout: { enabled: true, max_attempts: 10 } }, + auth_password: { device_trust: { enabled: true } }, + }); + // Catalog order: prerequisites and critical checks first. + expect(captured.err).toContain("Applied: user-lockout, client-trust, lockout-threshold"); + }); + + test("--check unions with positional ids", async () => { + await run(["user-lockout"], { check: ["client-trust", "user-lockout"], yes: true }); + expect(Object.keys(patches()[0]!.body!)).toEqual(["auth_attack_protection", "auth_password"]); + }); + + test("--all fixes the critical and recommended gaps and leaves good-to-have alone", async () => { + await run([], { all: true, yes: true, json: true }); + const body = patches()[0]!.body!; + expect(body.auth_password).toMatchObject({ min_length: 8 }); + expect(body).not.toHaveProperty("auth_multi_factor"); + expect(body).not.toHaveProperty("session_settings"); + const summary = JSON.parse(captured.out) as FixSummary; + expect(summary.remaining.sort()).toEqual([ + "block-disposable-email", + "block-email-subaddresses", + "email-link-same-client", + "mfa", + "mfa-required", + "session-lifetime", + ]); + }); + + test("--all --good-to-have fixes every unmet recommendation with a patch", async () => { + await run([], { all: true, goodToHave: true, yes: true }); + const body = patches()[0]!.body!; + expect(body).toHaveProperty("session_settings"); + expect(body).toHaveProperty("auth_access_control"); + + const { evaluate } = await import("./evaluate.ts"); + const projected = deepMerge(INSECURE_CONFIG, body); + const remaining = evaluate( + { config: projected, environmentType: "development" }, + { appId: "app_1", instanceId: "ins_dev", environmentType: "development", label: "" }, + ).filter((f) => f.status !== "met"); + // Enabling passkeys also satisfies the passwordless check. + expect(remaining.map((f) => f.id).sort()).toEqual(["mfa", "mfa-required"]); + }); + + test("--dry-run sends the patch with dry_run=true", async () => { + await run(["user-lockout"], { dryRun: true }); + expect(patches()).toHaveLength(1); + expect(patches()[0]!.url).toContain("dry_run=true"); + expect(captured.err).toContain("[dry-run]"); + }); + + test("met ids are skipped and nothing is sent", async () => { + serve(SECURE_CONFIG); + await run(["user-lockout"], { yes: true }); + expect(patches()).toHaveLength(0); + expect(captured.err).toMatch(/Skipping .*user-lockout.*: already met/); + expect(captured.err).toContain("Nothing to fix"); + }); + + test("not applicable ids are skipped", async () => { + serve({ ...INSECURE_CONFIG, auth_phone: { used_for_sign_up: false } }); + await run(["phone-verification"], { yes: true }); + expect(captured.err).toMatch(/Skipping .*phone-verification.*: not applicable/); + }); + + test("agent mode prints a JSON summary with the score change on stdout", async () => { + process.env.CLERK_MODE = "agent"; + await run(["user-lockout", "bot-protection"], { yes: true }); + const summary = JSON.parse(captured.out) as FixSummary; + expect(summary).toMatchObject({ + changed: true, + dryRun: false, + applied: ["bot-protection", "user-lockout"], + skipped: [], + }); + expect(summary.score.after.met).toBe(summary.score.before.met + 2); + expect(summary.remaining).not.toContain("user-lockout"); + expect(summary.remaining).toContain("client-trust"); + expect(captured.err).toContain("Grade"); + }); + + test("--json prints the summary in human mode too", async () => { + await run(["user-lockout"], { yes: true, json: true }); + expect((JSON.parse(captured.out) as FixSummary).applied).toEqual(["user-lockout"]); + }); + + test("the after-score comes from the server's response, not the local projection", async () => { + // A server that ignores the patch must not be reported as an improvement. + stubFetch(async (input, init) => { + requests.push({ method: init?.method ?? "GET", url: input.toString(), body: null }); + return new Response(JSON.stringify(INSECURE_CONFIG), { status: 200 }); + }); + await run(["user-lockout"], { yes: true, json: true }); + const summary = JSON.parse(captured.out) as FixSummary; + expect(summary.score.after).toEqual(summary.score.before); + expect(summary.remaining).toContain("user-lockout"); + }); + + test("dry-run scores the server's `after` envelope layered over the fetched document", async () => { + // The Platform API answers a dry-run with only the touched sections. + stubFetch(async (input, init) => { + const method = init?.method ?? "GET"; + const body = typeof init?.body === "string" ? JSON.parse(init.body) : null; + requests.push({ method, url: input.toString(), body }); + if (method !== "PATCH") return new Response(JSON.stringify(INSECURE_CONFIG), { status: 200 }); + const touched = Object.keys(body); + const pick = (doc: Record) => + Object.fromEntries(touched.map((k) => [k, doc[k]])); + const envelope = { + config_version: "v1_x", + dry_run: true, + before: pick(INSECURE_CONFIG), + after: pick(deepMerge(INSECURE_CONFIG, body)), + }; + return new Response(JSON.stringify(envelope), { status: 200 }); + }); + await run(["user-lockout"], { dryRun: true, json: true }); + const summary = JSON.parse(captured.out) as FixSummary; + expect(summary.score.after.met).toBe(summary.score.before.met + 1); + expect(summary.score.after.total).toBe(summary.score.before.total); + expect(summary.remaining).not.toContain("user-lockout"); + }); + + test("a partial PATCH response is merged over the fetched document", async () => { + stubFetch(async (input, init) => { + const method = init?.method ?? "GET"; + const body = typeof init?.body === "string" ? JSON.parse(init.body) : null; + requests.push({ method, url: input.toString(), body }); + const doc = + method === "PATCH" + ? { auth_password: deepMerge(INSECURE_CONFIG, body).auth_password } + : INSECURE_CONFIG; + return new Response(JSON.stringify(doc), { status: 200 }); + }); + await run(["client-trust"], { yes: true, json: true }); + const summary = JSON.parse(captured.out) as FixSummary; + expect(summary.score.after.total).toBe(summary.score.before.total); + expect(summary.score.after.met).toBe(summary.score.before.met + 1); + }); + + test("--all with --dry-run reports the projected score", async () => { + await run([], { all: true, goodToHave: true, dryRun: true, json: true }); + const summary = JSON.parse(captured.out) as FixSummary; + expect(summary.dryRun).toBe(true); + expect(summary.changed).toBe(true); + expect(summary.remaining.sort()).toEqual(["mfa", "mfa-required"]); + expect(captured.err).toContain("[dry-run] projected"); + }); + + test("agent mode summary for nothing to fix", async () => { + process.env.CLERK_MODE = "agent"; + serve(SECURE_CONFIG); + await run(["user-lockout"], { yes: true }); + const summary = JSON.parse(captured.out) as FixSummary; + expect(summary).toMatchObject({ + changed: false, + applied: [], + skipped: [{ id: "user-lockout", reason: "met" }], + dryRun: false, + remaining: [], + }); + expect(summary.score.after).toEqual(summary.score.before); + }); + + test("a manual id in the selection points at the fixable subset", async () => { + let error: unknown; + await run(["mfa-required", "user-lockout", "client-trust"], { yes: true }).catch( + (e) => (error = e), + ); + const { examples } = error as { examples: Array<{ command: string }> }; + expect(examples[0]!.command).toBe( + "clerk security fix user-lockout client-trust --app app_1 --instance ins_dev", + ); + }); + + test("requires MFA enrollment even when sign-in second-factor verification is enabled", async () => { + serve( + deepMerge(SECURE_CONFIG, { + auth_multi_factor: { required_for_sign_in: true, required_for_sign_up: false }, + }), + ); + await run(["mfa-required"], { yes: true, json: true }); + expect(patches()[0]!.body).toEqual({ + auth_multi_factor: { required_for_sign_up: true }, + }); + expect((JSON.parse(captured.out) as FixSummary).remaining).not.toContain("mfa-required"); + }); + + test.each([true, false])( + "sign-in breach fix enables HIBP when enforcement is %s", + async (enforce) => { + serve( + deepMerge(SECURE_CONFIG, { + auth_password: { disable_hibp: true, enforce_hibp_on_sign_in: enforce }, + }), + ); + await run(["breach-detection-sign-in"], { yes: true, json: true }); + expect(patches()[0]!.body).toEqual({ + auth_password: { disable_hibp: false, enforce_hibp_on_sign_in: true }, + }); + const summary = JSON.parse(captured.out) as FixSummary; + expect(summary.remaining).not.toContain("breach-detection-sign-in"); + expect(summary.remaining).not.toContain("breach-detection"); + }, + ); + + describe("decisions", () => { + test.each([false, true])("rejects backup codes alone before writing (all=%s)", async (all) => { + await expect( + run(all ? [] : ["mfa"], { + all, + factors: ["backup-code"], + yes: true, + }), + ).rejects.toThrow("Backup codes require another second factor"); + expect(patches()).toHaveLength(0); + }); + + test("accepts SMS with backup codes", async () => { + await run(["mfa"], { factors: ["sms,backup-code"], yes: true }); + expect(patches()[0]!.body).toEqual({ + auth_multi_factor: { backup_code: { enabled: true } }, + auth_phone: { used_for_second_factor: true, second_factor_strategies: ["phone_code"] }, + }); + }); + + test("--factors applies the chosen second factors", async () => { + await run(["mfa"], { factors: ["authenticator,sms"], yes: true, json: true }); + expect(patches()[0]!.body).toEqual({ + auth_multi_factor: { authenticator_app: { enabled: true } }, + auth_phone: { used_for_second_factor: true, second_factor_strategies: ["phone_code"] }, + }); + const summary = JSON.parse(captured.out) as FixSummary; + expect(summary.decisions).toEqual({ mfa: ["authenticator", "sms"] }); + expect(summary.remaining).not.toContain("mfa"); + }); + + test("--factors may be repeated", async () => { + await run(["mfa"], { factors: ["authenticator", "backup-code"], yes: true, json: true }); + expect((JSON.parse(captured.out) as FixSummary).decisions.mfa).toEqual([ + "authenticator", + "backup-code", + ]); + }); + + test("rejects an unknown factor", async () => { + await expect(run(["mfa"], { factors: ["totp"], yes: true })).rejects.toThrow( + "Unknown --factors value for mfa: totp. Choose from authenticator, backup-code, sms.", + ); + expect(patches()).toHaveLength(0); + }); + + test("agent mode without --factors is a usage error with the suggested command", async () => { + process.env.CLERK_MODE = "agent"; + let error: unknown; + await run(["mfa"], { yes: true }).catch((e) => (error = e)); + const { message, examples } = error as { + message: string; + examples: Array<{ command: string }>; + }; + expect(message).toContain("mfa needs --factors (authenticator, backup-code, sms)"); + expect(examples[0]!.command).toBe( + "clerk security fix mfa --factors authenticator,backup-code --app app_1 --instance ins_dev --yes", + ); + expect(patches()).toHaveLength(0); + }); + + test("human mode asks for the factors, preselecting the suggestion", async () => { + await run(["mfa"], { yes: true, json: true }); + expect(patches()[0]!.body).toEqual({ + auth_multi_factor: { authenticator_app: { enabled: true }, backup_code: { enabled: true } }, + }); + }); + + test("--strategy applies a single passwordless method", async () => { + await run(["passwordless-auth"], { strategy: "passkey", yes: true, json: true }); + expect(patches()[0]!.body).toEqual({ auth_passkey: { used_for_sign_in: true } }); + }); + + test("human mode asks for the strategy, defaulting to an identifier already collected", async () => { + await run(["passwordless-auth"], { yes: true, json: true }); + expect(patches()[0]!.body).toEqual({ + auth_email: { used_for_sign_in: true, sign_in_strategies: ["email_code"] }, + }); + }); + + test("fixing mfa in the same call unblocks mfa-required, prerequisite first", async () => { + await run(["mfa-required", "mfa"], { factors: ["authenticator"], yes: true, json: true }); + expect(patches()[0]!.body).toEqual({ + auth_multi_factor: { authenticator_app: { enabled: true }, required_for_sign_up: true }, + }); + const summary = JSON.parse(captured.out) as FixSummary; + expect(summary.applied).toEqual(["mfa", "mfa-required"]); + expect(summary.remaining).not.toContain("mfa-required"); + }); + + test("--all --good-to-have with --factors includes mfa and what it unblocks", async () => { + await run([], { + all: true, + goodToHave: true, + factors: ["authenticator", "backup-code"], + yes: true, + json: true, + }); + const summary = JSON.parse(captured.out) as FixSummary; + expect(summary.applied).toContain("mfa"); + expect(summary.applied).toContain("mfa-required"); + expect(summary.remaining).toEqual([]); + expect(summary.score.after.grade).toBe("A"); + }); + }); + + describe("score line", () => { + test("shows an arrow only when the grade changes", async () => { + const { formatScoreTransition } = await import("./format.ts"); + const score = (grade: "C" | "B", met: number) => ({ + grade, + percent: 0, + met, + total: 19, + hasCriticalGap: true, + }); + const strip = (s: string) => s.replace(new RegExp(String.raw`\x1b\[[0-9;]*m`, "g"), ""); + expect(strip(formatScoreTransition(score("C", 11), score("C", 13), false))).toBe( + "Grade C · 13 of 19 recommendations met", + ); + expect(strip(formatScoreTransition(score("C", 11), score("B", 16), true))).toBe( + "[dry-run] projected Grade C → B · 16 of 19 recommendations met", + ); + }); + }); +}); diff --git a/packages/cli-core/src/commands/security/fix.ts b/packages/cli-core/src/commands/security/fix.ts new file mode 100644 index 000000000..26aed046c --- /dev/null +++ b/packages/cli-core/src/commands/security/fix.ts @@ -0,0 +1,331 @@ +import { throwUsageError, throwUserAbort } from "../../lib/errors.ts"; +import type { Example } from "../../lib/help.ts"; +import { log } from "../../lib/log.ts"; +import { NEXT_STEPS } from "../../lib/next-steps.ts"; +import { isRecord } from "../../lib/objects.ts"; +import { withGutter } from "../../lib/spinner.ts"; +import { isAgent } from "../../mode.ts"; +import { applyConfigPatch } from "../config/apply-patch.ts"; +import { CHECKS, CHECK_IDS, findCheck } from "./catalog.ts"; +import { evaluate, fixCommandFor, fixCommandWithDecision, fixableIds } from "./evaluate.ts"; +import { formatScoreTransition } from "./format.ts"; +import { loadAudit } from "./load.ts"; +import { deepMerge, projectPatches } from "./merge.ts"; +import { computeScore } from "./score.ts"; +import type { + CheckDef, + CheckInput, + Finding, + FixOptions, + FixSummary, + InstanceConfig, + InstanceRef, + SkipReason, +} from "./types.ts"; + +const EXAMPLES: Example[] = [ + { command: "clerk security fix user-lockout client-trust --yes", description: "Apply two fixes" }, + { + command: "clerk security fix mfa --factors authenticator,backup-code --yes", + description: "Enable two-factor authentication", + }, + { command: "clerk security fix --all --yes", description: "Apply every fixable recommendation" }, +]; + +const SKIP_REASON_TEXT: Record = { + met: "already met", + not_applicable: "not applicable to this instance", +}; + +interface Selection { + checks: CheckDef[]; + skipped: FixSummary["skipped"]; +} + +function suppliedDecisions(options: FixOptions): Partial> { + const factors = options.factors + ?.flatMap((v) => v.split(",")) + .map((v) => v.trim()) + .filter(Boolean); + return { + ...(factors?.length && { factors }), + ...(options.strategy && { strategy: [options.strategy] }), + }; +} + +function selectByIds(ids: string[], findings: Finding[], ref: InstanceRef): Selection { + const byId = new Map(findings.map((f) => [f.id, f])); + const problems: string[] = []; + const selection: Selection = { checks: [], skipped: [] }; + for (const id of ids) { + const finding = byId.get(id); + const check = findCheck(id)!; + if (!finding) { + selection.skipped.push({ id, reason: "not_applicable" }); + } else if (finding.status === "met") { + selection.skipped.push({ id, reason: "met" }); + } else if (finding.status === "blocked" && !ids.includes(finding.blockedBy!)) { + // Allowed when the prerequisite is fixed in the same call. + problems.push(`${id}: ${finding.remedy}`); + } else if (!check.patch && !check.decision) { + problems.push(`${id}: ${finding.remedy}`); + } else { + selection.checks.push(check); + } + } + + if (problems.length > 0) { + const fixable = selection.checks.filter((c) => c.patch).map((c) => c.id); + const examples: Example[] = fixable.length + ? [ + { + command: fixCommandFor(fixable, ref), + description: "Apply the fixable recommendations from this selection", + }, + ...EXAMPLES, + ] + : EXAMPLES; + throwUsageError( + `${problems.length === 1 ? "This recommendation needs" : "These recommendations need"} a manual change:\n ${problems.join("\n ")}`, + undefined, + undefined, + examples, + ); + } + return selection; +} + +function selectAll( + findings: Finding[], + supplied: ReturnType, + goodToHave: boolean, +): Selection { + const ids = new Set(fixableIds(findings, goodToHave)); + for (const f of findings) { + if (f.status === "unmet" && f.decision && supplied[f.decision.flag]) ids.add(f.id); + } + for (const f of findings) { + if (f.status === "blocked" && f.blockedBy && ids.has(f.blockedBy) && findCheck(f.id)?.patch) { + ids.add(f.id); + } + } + return { checks: [...ids].map((id) => findCheck(id)!), skipped: [] }; +} + +async function selectInteractively(findings: Finding[]): Promise { + const candidates = findings.filter((f) => f.status === "unmet" && (f.patch || f.decision)); + if (candidates.length === 0) return { checks: [], skipped: [] }; + + const { multiselect } = await import("../../lib/prompts.ts"); + const chosen = await multiselect({ + message: "Which recommendations should be applied?", + options: candidates.map((f) => ({ + value: f.id, + label: f.title, + hint: f.decision + ? `${f.id} · asks which ${f.decision.flag}` + : `${f.id} · ${f.current} → ${f.recommended}`, + })), + // Good-to-have offered but unticked. + initialValues: candidates + .filter((f) => f.patch && f.severity !== "good-to-have") + .map((f) => f.id), + required: false, + }); + if (chosen.length === 0) throwUserAbort(); + const ids = new Set(chosen); + for (const f of findings) { + if (f.status === "blocked" && f.blockedBy && ids.has(f.blockedBy) && findCheck(f.id)?.patch) { + ids.add(f.id); + } + } + return { checks: [...ids].map((id) => findCheck(id)!), skipped: [] }; +} + +async function resolveDecision( + check: CheckDef, + supplied: ReturnType, + input: CheckInput, + ref: InstanceRef, +): Promise { + const decision = check.decision!; + const valid = decision.options.map((o) => o.value); + const given = supplied[decision.flag]; + if (given) { + const bad = given.filter((v) => !valid.includes(v)); + if (bad.length > 0) { + throwUsageError( + `Unknown --${decision.flag} value${bad.length === 1 ? "" : "s"} for ${check.id}: ${bad.join(", ")}. Choose from ${valid.join(", ")}.`, + ); + } + if (!decision.multiple && given.length > 1) { + throwUsageError(`--${decision.flag} takes a single value for ${check.id}.`); + } + return given; + } + + const defaults = decision.defaults(input); + if (isAgent()) { + throwUsageError( + `${check.id} needs --${decision.flag} (${valid.join(", ")}).`, + undefined, + undefined, + [ + { + command: fixCommandWithDecision(check, defaults, ref), + description: `Apply ${check.id} with the suggested ${decision.flag}`, + }, + ], + ); + } + + if (decision.multiple) { + const { multiselect } = await import("../../lib/prompts.ts"); + const values = await multiselect({ + message: decision.prompt, + options: decision.options, + initialValues: defaults, + required: true, + }); + if (values.length === 0) throwUserAbort(); + return values; + } + const { select } = await import("../../lib/listage.ts"); + const value = await select({ + message: decision.prompt, + choices: decision.options.map((o) => ({ value: o.value, name: o.label })), + default: defaults[0], + }); + return [value]; +} + +// A dry-run answers `{dry_run, before, after}` and a write may echo only touched +// sections, so the server's view is layered over the fetched document. +function afterDocument( + body: InstanceConfig | undefined, + before: InstanceConfig, + projected: InstanceConfig, +): InstanceConfig { + if (!isRecord(body)) return projected; + const view = body.dry_run === true && isRecord(body.after) ? body.after : body; + const sections = Object.fromEntries( + Object.entries(view).filter(([key]) => key in before && key !== "config_version"), + ); + return Object.keys(sections).length ? deepMerge(before, sections) : projected; +} + +export async function securityFix(ids: string[] = [], options: FixOptions = {}): Promise { + const selected = [...new Set([...ids, ...(options.check ?? [])])]; + const all = Boolean(options.all); + const json = Boolean(options.json) || isAgent(); + const supplied = suppliedDecisions(options); + + if (selected.length > 0 && all) { + throwUsageError("Pass either check ids or --all, not both.", undefined, undefined, EXAMPLES); + } + const unknown = selected.filter((id) => !CHECK_IDS.includes(id)); + if (unknown.length > 0) { + throwUsageError( + `Unknown ${unknown.length === 1 ? "check" : "checks"}: ${unknown.join(", ")}.\nValid ids: ${CHECK_IDS.join(", ")}.`, + undefined, + undefined, + EXAMPLES, + ); + } + if (isAgent()) { + if (selected.length === 0 && !all) { + throwUsageError( + "Pass one or more check ids, or --all. Run `clerk security checks` to list them.", + undefined, + undefined, + EXAMPLES, + ); + } + if (!options.yes && !options.dryRun) { + throwUsageError( + "Pass --yes to apply security fixes in agent mode.", + undefined, + undefined, + EXAMPLES, + ); + } + } + + await withGutter("Applying security fixes", async ({ setNextSteps }) => { + const { target, input, report } = await loadAudit(options); + const { checks, skipped } = all + ? selectAll(report.findings, supplied, Boolean(options.goodToHave)) + : selected.length > 0 + ? selectByIds(selected, report.findings, report.instance) + : await selectInteractively(report.findings); + for (const { id, reason } of skipped) + log.info(`Skipping \`${id}\`: ${SKIP_REASON_TEXT[reason]}`); + + const dryRun = Boolean(options.dryRun); + const summary: FixSummary = { + changed: false, + dryRun, + applied: [], + decisions: {}, + skipped, + score: { before: report.score, after: report.score }, + remaining: report.findings.filter((f) => f.status !== "met").map((f) => f.id), + }; + + if (checks.length === 0) { + log.info("Nothing to fix."); + if (json) log.data(JSON.stringify(summary, null, 2)); + return; + } + + // Catalog order: prerequisites first. + const ordered = [...checks].sort((a, b) => CHECKS.indexOf(a) - CHECKS.indexOf(b)); + const resolved: CheckDef[] = []; + for (const check of ordered) { + if (check.patch || !check.decision) { + resolved.push(check); + continue; + } + const values = await resolveDecision(check, supplied, input, report.instance); + const problem = check.decision.validate?.(values, input); + if (problem) throwUsageError(problem); + summary.decisions[check.id] = values; + resolved.push({ ...check, patch: (i) => check.decision!.patch(values, i) }); + } + + const applied = resolved.map((c) => c.id); + const { payload, projected } = projectPatches(input, resolved); + let written: InstanceConfig | undefined; + const changed = await applyConfigPatch({ + target, + payload, + verb: `Applying ${applied.length === 1 ? "1 security fix" : `${applied.length} security fixes`}`, + successMessage: `Applied: ${applied.join(", ")}`, + failureContext: "Failed to apply security fixes", + yes: options.yes, + dryRun, + currentConfig: input.config, + onWritten: (body) => { + written = body; + }, + }); + + if (changed) { + const after = evaluate( + { ...input, config: afterDocument(written, input.config, projected) }, + report.instance, + ); + summary.changed = true; + summary.applied = applied; + summary.score.after = computeScore(after); + summary.remaining = after.filter((f) => f.status !== "met").map((f) => f.id); + log.info(formatScoreTransition(summary.score.before, summary.score.after, dryRun)); + } + + if (json) { + log.data(JSON.stringify(summary, null, 2)); + } else if (changed && !dryRun && summary.remaining.length > 0) { + setNextSteps(NEXT_STEPS.SECURITY_FIX_REMAINING); + } + }); +} diff --git a/packages/cli-core/src/commands/security/fixtures.ts b/packages/cli-core/src/commands/security/fixtures.ts new file mode 100644 index 000000000..d849edbaf --- /dev/null +++ b/packages/cli-core/src/commands/security/fixtures.ts @@ -0,0 +1,131 @@ +// Test-only documents covering every group the catalog reads. + +import type { InstanceConfig } from "./types.ts"; + +export const SECURE_CONFIG: InstanceConfig = { + auth_attack_protection: { + bot_protection: { captcha_enabled: true, captcha_widget_type: "smart" }, + email_link_require_same_client: true, + enumeration_protection: "bulk", + pii_protection_enabled: true, + user_lockout: { enabled: true, max_attempts: 10, duration_in_minutes: 60 }, + }, + auth_password: { + enabled: true, + required: true, + device_trust: { enabled: true }, + disable_hibp: false, + enforce_hibp_on_sign_in: true, + min_length: 12, + max_length: 0, + min_zxcvbn_strength: 3, + show_zxcvbn: true, + }, + auth_multi_factor: { + authenticator_app: { enabled: true }, + backup_code: { enabled: true }, + required_for_sign_in: true, + required_for_sign_up: true, + }, + auth_email: { + used_for_sign_up: true, + used_for_sign_in: true, + required_for_sign_up: true, + verify_at_sign_up: true, + sign_in_strategies: ["email_code"], + verification_strategies: ["email_code"], + }, + auth_phone: { + used_for_sign_up: true, + used_for_sign_in: true, + used_for_second_factor: true, + verify_at_sign_up: true, + sign_in_strategies: ["phone_code"], + second_factor_strategies: ["phone_code"], + verification_strategies: ["phone_code"], + }, + auth_passkey: { used_for_sign_in: true, satisfies_second_factor: true }, + auth_web3: { used_for_sign_in: false, sign_in_strategies: [] }, + auth_access_control: { + allowlist_enabled: true, + blocklist_enabled: false, + allowlist_blocklist_enforced_on_sign_in: true, + block_disposable_email_domains: true, + block_email_subaddresses: true, + sign_up_mode: "public", + }, + session_settings: { + inactivity_timeout: { enabled: true, duration_seconds: 1800 }, + maximum_lifetime: { enabled: true, duration_seconds: 604800 }, + multi_session_enabled: false, + }, + connection_oauth_google: { enabled: true, client_id: "custom-id", client_secret: "***" }, + connection_oauth_github: { enabled: false, client_id: "", client_secret: "" }, +}; + +export const INSECURE_CONFIG: InstanceConfig = { + auth_attack_protection: { + bot_protection: { captcha_enabled: false, captcha_widget_type: "smart" }, + email_link_require_same_client: false, + enumeration_protection: "bulk", + pii_protection_enabled: false, + user_lockout: { enabled: false, max_attempts: 50, duration_in_minutes: 0 }, + }, + auth_password: { + enabled: true, + required: true, + device_trust: { enabled: false }, + disable_hibp: true, + enforce_hibp_on_sign_in: false, + min_length: 6, + max_length: 0, + min_zxcvbn_strength: 0, + show_zxcvbn: false, + }, + auth_multi_factor: { + authenticator_app: { enabled: false }, + backup_code: { enabled: false }, + required_for_sign_in: false, + required_for_sign_up: false, + }, + auth_email: { + used_for_sign_up: true, + used_for_sign_in: true, + required_for_sign_up: true, + verify_at_sign_up: false, + sign_in_strategies: [], + verification_strategies: [], + }, + auth_phone: { + used_for_sign_up: true, + used_for_sign_in: false, + used_for_second_factor: false, + verify_at_sign_up: false, + sign_in_strategies: [], + second_factor_strategies: [], + verification_strategies: [], + }, + auth_passkey: { used_for_sign_in: false, satisfies_second_factor: false }, + auth_web3: { used_for_sign_in: false, sign_in_strategies: [] }, + auth_access_control: { + allowlist_enabled: true, + blocklist_enabled: false, + allowlist_blocklist_enforced_on_sign_in: false, + block_disposable_email_domains: false, + block_email_subaddresses: false, + sign_up_mode: "public", + }, + session_settings: { + inactivity_timeout: { enabled: false, duration_seconds: 0 }, + maximum_lifetime: { enabled: false, duration_seconds: 0 }, + multi_session_enabled: true, + }, + connection_oauth_google: { enabled: false, client_id: "", client_secret: "" }, + connection_oauth_github: { enabled: false, client_id: "", client_secret: "" }, +}; + +// Enabling a provider satisfies passwordless, so the OAuth check gets its own document. +export const INSECURE_OAUTH_CONFIG: InstanceConfig = { + ...INSECURE_CONFIG, + connection_oauth_google: { enabled: true, client_id: "", client_secret: "" }, +}; diff --git a/packages/cli-core/src/commands/security/format.ts b/packages/cli-core/src/commands/security/format.ts new file mode 100644 index 000000000..10acfc6e3 --- /dev/null +++ b/packages/cli-core/src/commands/security/format.ts @@ -0,0 +1,135 @@ +import { bold, cyan, dim, green, red, yellow } from "../../lib/color.ts"; +import { SEVERITY_ORDER } from "./evaluate.ts"; +import type { + AuditReport, + CheckDef, + Finding, + FindingStatus, + SecurityScore, + Severity, +} from "./types.ts"; + +const SEVERITY_LABEL: Record = { + critical: "Critical", + recommended: "Recommended", + "good-to-have": "Good to have", +}; + +const STATUS_ICON: Record = { + met: green("✓"), + blocked: yellow("!"), + unmet: red("✗"), +}; + +const GRADE_COLOR: Record string> = { + A: green, + B: green, + C: yellow, + D: red, + F: red, +}; + +export function formatReportJson(report: AuditReport, spotlight: boolean): string { + const findings = spotlight ? report.findings.filter((f) => f.status !== "met") : report.findings; + return JSON.stringify({ ...report, findings }, null, 2); +} + +export function formatScoreTransition( + before: SecurityScore, + after: SecurityScore, + dryRun: boolean, +): string { + const to = GRADE_COLOR[after.grade](bold(after.grade)); + const grade = + before.grade === after.grade + ? `Grade ${to}` + : `Grade ${GRADE_COLOR[before.grade](before.grade)} ${dim("→")} ${to}`; + const prefix = dryRun ? dim("[dry-run] projected ") : ""; + return `${prefix}${grade}${dim(" · ")}${after.met} of ${after.total} recommendations met`; +} + +function formatScoreLine(score: SecurityScore): string { + const grade = GRADE_COLOR[score.grade](bold(`Grade ${score.grade}`)); + const parts = [grade, `${score.percent}%`, `${score.met} of ${score.total} recommendations met`]; + if (score.hasCriticalGap) parts.push(red("critical gaps present")); + return parts.join(dim(" · ")); +} + +function formatFindingLine(finding: Finding, widths: { title: number; id: number }): string { + const title = finding.title.padEnd(widths.title); + const id = finding.id.padEnd(widths.id); + if (finding.status === "met") return `${STATUS_ICON.met} ${dim(title)} ${dim(id)}`; + if (finding.status === "blocked") { + return `${STATUS_ICON.blocked} ${title} ${cyan(id)} ${dim(`blocked: ${finding.remedy}`)}`; + } + const change = `${finding.current} ${dim("→")} ${finding.recommended}`; + const manual = finding.patch + ? "" + : finding.decision + ? dim(` (asks --${finding.decision.flag})`) + : dim(" (manual)"); + return `${STATUS_ICON.unmet} ${title} ${cyan(id)} ${change}${manual}`; +} + +export function formatReportHuman(report: AuditReport, spotlight: boolean): string[] { + const findings = spotlight ? report.findings.filter((f) => f.status !== "met") : report.findings; + const widths = { + title: Math.max(0, ...findings.map((f) => f.title.length)), + id: Math.max(0, ...findings.map((f) => f.id.length)), + }; + const lines = [formatScoreLine(report.score), ""]; + + for (const severity of SEVERITY_ORDER) { + const group = findings.filter((f) => f.severity === severity); + if (group.length === 0) continue; + lines.push(bold(SEVERITY_LABEL[severity])); + for (const finding of group) lines.push(formatFindingLine(finding, widths)); + lines.push(""); + } + + if (findings.length === 0) lines.push(green("Every recommendation is met."), ""); + return lines; +} + +export function formatCatalogJson(checks: CheckDef[]): string { + return JSON.stringify( + checks.map((check) => ({ + id: check.id, + title: check.title, + description: check.description, + severity: check.severity, + path: check.path, + fixable: Boolean(check.patch), + ...(check.decision && { + decision: { + flag: check.decision.flag, + multiple: check.decision.multiple, + options: check.decision.options.map((o) => o.value), + }, + }), + ...(check.feature && { feature: check.feature }), + ...(check.blockedBy && { blockedBy: check.blockedBy }), + docsUrl: check.docsUrl, + })), + null, + 2, + ); +} + +export function formatCatalogHuman(checks: CheckDef[]): string[] { + const width = Math.max(...checks.map((c) => c.id.length)); + const lines: string[] = []; + for (const severity of SEVERITY_ORDER) { + lines.push(bold(SEVERITY_LABEL[severity])); + for (const check of checks.filter((c) => c.severity === severity)) { + const fix = check.patch + ? "" + : check.decision + ? dim(` (asks --${check.decision.flag})`) + : dim(" (manual)"); + lines.push(` ${cyan(check.id.padEnd(width))} ${check.title}${fix}`); + } + lines.push(""); + } + return lines; +} diff --git a/packages/cli-core/src/commands/security/index.ts b/packages/cli-core/src/commands/security/index.ts new file mode 100644 index 000000000..9edffea2c --- /dev/null +++ b/packages/cli-core/src/commands/security/index.ts @@ -0,0 +1,127 @@ +import { createOption } from "@commander-js/extra-typings"; +import type { Program } from "../../cli-program.ts"; +import { collectOptionValues } from "../../lib/option-parsers.ts"; +import { securityAudit } from "./audit.ts"; +import { securityFix } from "./fix.ts"; +import { securityChecks } from "./list-checks.ts"; +import { findCheck } from "./catalog.ts"; +import { FAIL_ON_LEVELS } from "./types.ts"; + +const PASSWORDLESS_STRATEGIES = findCheck("passwordless-auth")!.decision!.options.map( + (o) => o.value, +); + +export function registerSecurity(program: Program): void { + const security = program + .command("security") + .description("Audit an instance against Clerk's security recommendations") + .setExamples([ + { command: "clerk security", description: "Audit the linked development instance" }, + { + command: "clerk security audit --instance prod --json", + description: "Machine-readable audit of production", + }, + { command: "clerk security fix --all", description: "Apply every fixable recommendation" }, + { + command: "clerk security checks", + description: "List the recommendations the audit checks", + }, + ]); + + security + .command("audit", { isDefault: true }) + .description("Evaluate the instance and report unmet recommendations") + .option("--app ", "Application ID to target (works from any directory)") + .option("--instance ", "Instance to target (dev, prod, or a full instance ID)") + .option("--json", "Output the report as JSON") + .option("--spotlight", "Only show unmet and blocked recommendations") + .addOption( + createOption( + "--fail-on ", + "Lowest severity of an unmet recommendation that makes the command exit 1", + ) + .choices(FAIL_ON_LEVELS) + .default("critical"), + ) + .setExamples([ + { command: "clerk security audit", description: "Audit the linked development instance" }, + { + command: "clerk security audit --instance prod --spotlight", + description: "Only show gaps on production", + }, + { command: "clerk security audit --json", description: "Emit the report as JSON" }, + { + command: "clerk security audit --fail-on none", + description: "Report without failing the exit code", + }, + ]) + .action(securityAudit); + + security + .command("fix") + .description("Apply the config patch for one or more recommendations") + .argument( + "[ids...]", + "Recommendation ids to fix (shown in the audit); omit to pick interactively", + ) + .option( + "--check ", + "Recommendation id to fix (repeatable; for --input-json)", + collectOptionValues, + ) + .option("--all", "Fix every unmet critical and recommended check that has an inline patch") + .option( + "--good-to-have", + "With --all, also apply the good-to-have tier (hardening that costs users some convenience)", + ) + .option( + "--factors ", + "Second factors for `mfa`: authenticator, backup-code, sms (comma-separated; asked interactively when omitted)", + collectOptionValues, + ) + .addOption( + createOption( + "--strategy ", + "Sign-in method for `passwordless-auth` (asked interactively when omitted)", + ).choices(PASSWORDLESS_STRATEGIES), + ) + .option("--app ", "Application ID to target (works from any directory)") + .option("--instance ", "Instance to target (dev, prod, or a full instance ID)") + .option("--dry-run", "Validate server-side and preview the diff without applying it") + .option("--yes", "Skip the confirmation prompt (required in agent mode)") + .option("--json", "Output the result summary as JSON") + .setExamples([ + { command: "clerk security fix", description: "Pick the recommendations to apply" }, + { + command: "clerk security fix user-lockout client-trust", + description: "Fix two recommendations by id", + }, + { + command: "clerk security fix mfa --factors authenticator,backup-code", + description: "Enable two-factor authentication with the given factors", + }, + { + command: "clerk security fix --all --dry-run", + description: "Preview every fixable change", + }, + { + command: "clerk security fix --all --instance prod --yes", + description: "Fix production without prompting", + }, + { + command: "clerk security fix --all --good-to-have", + description: "Include the good-to-have tier", + }, + ]) + .action(async (ids, options) => securityFix(ids, options)); + + security + .command("checks") + .description("List the recommendations the audit checks, without contacting Clerk") + .option("--json", "Output the catalog as JSON") + .setExamples([ + { command: "clerk security checks", description: "List recommendation ids by severity" }, + { command: "clerk security checks --json", description: "Emit the catalog as JSON" }, + ]) + .action(securityChecks); +} diff --git a/packages/cli-core/src/commands/security/list-checks.ts b/packages/cli-core/src/commands/security/list-checks.ts new file mode 100644 index 000000000..2668a544c --- /dev/null +++ b/packages/cli-core/src/commands/security/list-checks.ts @@ -0,0 +1,13 @@ +import { log } from "../../lib/log.ts"; +import { isAgent } from "../../mode.ts"; +import { CHECKS } from "./catalog.ts"; +import { formatCatalogHuman, formatCatalogJson } from "./format.ts"; + +export function securityChecks(options: { json?: boolean } = {}): void { + if (options.json || isAgent()) { + log.data(formatCatalogJson(CHECKS)); + return; + } + for (const line of formatCatalogHuman(CHECKS)) log.info(line); + log.info("Run `clerk security audit` to evaluate the linked instance."); +} diff --git a/packages/cli-core/src/commands/security/load.ts b/packages/cli-core/src/commands/security/load.ts new file mode 100644 index 000000000..6bfd5b441 --- /dev/null +++ b/packages/cli-core/src/commands/security/load.ts @@ -0,0 +1,50 @@ +import { keylessCopy } from "../../lib/copy.ts"; +import { CliError, ERROR_CODE, withApiContext } from "../../lib/errors.ts"; +import { resolveInstanceTarget, type InstanceTarget } from "../../lib/keyless-target.ts"; +import { fetchApplication, fetchInstanceConfig } from "../../lib/plapi.ts"; +import { withSpinner } from "../../lib/spinner.ts"; +import { buildReport } from "./evaluate.ts"; +import type { AuditReport, CheckInput, InstanceRef } from "./types.ts"; + +export interface LoadedAudit { + target: Extract; + input: CheckInput; + report: AuditReport; +} + +async function resolveEnvironmentType(appId: string, instanceId: string, label: string) { + if (label === "development" || label === "production") return label; + const app = await fetchApplication(appId); + return app.instances.find((i) => i.instance_id === instanceId)?.environment_type ?? "unknown"; +} + +export async function loadAudit(options: { + app?: string; + instance?: string; +}): Promise { + const target = await resolveInstanceTarget(options); + if (target.kind === "keyless") { + throw new CliError(keylessCopy.securityNeedsClaimedApplication(), { + code: ERROR_CODE.AUTH_REQUIRED, + }); + } + + const { appId, instanceId, instanceLabel } = target.ctx; + const { config, environmentType } = await withSpinner( + `Fetching config from ${target.label}...`, + async () => { + const [config, environmentType] = await Promise.all([ + withApiContext(fetchInstanceConfig(appId, instanceId), "Failed to fetch config"), + withApiContext( + resolveEnvironmentType(appId, instanceId, instanceLabel), + "Failed to fetch application", + ), + ]); + return { config, environmentType }; + }, + ); + + const ref: InstanceRef = { appId, instanceId, environmentType, label: target.label }; + const input: CheckInput = { config, environmentType }; + return { target, input, report: buildReport(input, ref) }; +} diff --git a/packages/cli-core/src/commands/security/merge.test.ts b/packages/cli-core/src/commands/security/merge.test.ts new file mode 100644 index 000000000..87e0388ac --- /dev/null +++ b/packages/cli-core/src/commands/security/merge.test.ts @@ -0,0 +1,69 @@ +import { test, expect, describe } from "bun:test"; +import { findCheck } from "./catalog.ts"; +import { INSECURE_CONFIG } from "./fixtures.ts"; +import { deepMerge, projectPatches } from "./merge.ts"; +import type { CheckDef } from "./types.ts"; + +describe("deepMerge", () => { + test("merges nested objects and keeps untouched keys", () => { + expect(deepMerge({ a: { x: 1, y: 2 }, b: 1 }, { a: { y: 3 } })).toEqual({ + a: { x: 1, y: 3 }, + b: 1, + }); + }); + + test("replaces arrays whole", () => { + expect(deepMerge({ a: [1, 2] }, { a: [3] })).toEqual({ a: [3] }); + }); + + test("later values win on conflicts", () => { + expect(deepMerge({ a: 1 }, { a: 2 })).toEqual({ a: 2 }); + }); + + test("does not mutate its inputs", () => { + const base = { a: { x: 1 } }; + deepMerge(base, { a: { y: 2 } }); + expect(base).toEqual({ a: { x: 1 } }); + }); +}); + +describe("projectPatches", () => { + const input = { config: INSECURE_CONFIG, environmentType: "production" }; + + test("later checks see earlier patches", () => { + const lockout = findCheck("user-lockout")!; + const threshold = findCheck("lockout-threshold")!; + const { payload, projected } = projectPatches(input, [lockout, threshold]); + expect(payload).toEqual({ + auth_attack_protection: { user_lockout: { enabled: true, max_attempts: 10 } }, + }); + expect(threshold.evaluate({ ...input, config: projected }).met).toBe(true); + expect(lockout.evaluate({ ...input, config: projected }).met).toBe(true); + }); + + test("a check already satisfied by an earlier patch is skipped", () => { + const lockout = findCheck("user-lockout")!; + const threshold = findCheck("lockout-threshold")!; + const { payload } = projectPatches(input, [threshold, lockout]); + expect(payload).toEqual({ + auth_attack_protection: { user_lockout: { enabled: true, max_attempts: 10 } }, + }); + }); + + test("array-touching checks compound instead of clobbering", () => { + const first: CheckDef = { + ...findCheck("email-verification")!, + id: "first", + patch: () => ({ auth_email: { verification_strategies: ["email_link"] } }), + }; + const { payload } = projectPatches(input, [first, findCheck("email-verification")!]); + expect(payload).toEqual({ + auth_email: { verify_at_sign_up: true, verification_strategies: ["email_link"] }, + }); + }); + + test("skips checks without a patch", () => { + const { payload } = projectPatches(input, [findCheck("mfa")!]); + expect(payload).toEqual({}); + }); +}); diff --git a/packages/cli-core/src/commands/security/merge.ts b/packages/cli-core/src/commands/security/merge.ts new file mode 100644 index 000000000..970cd2289 --- /dev/null +++ b/packages/cli-core/src/commands/security/merge.ts @@ -0,0 +1,36 @@ +import { isRecord } from "../../lib/objects.ts"; +import type { CheckDef, CheckInput, InstanceConfig } from "./types.ts"; + +// PATCH semantics: objects merge, arrays and primitives replace. +export function deepMerge( + base: Record, + overlay: Record, +): Record { + const result: Record = { ...base }; + for (const [key, value] of Object.entries(overlay)) { + const existing = result[key]; + result[key] = isRecord(existing) && isRecord(value) ? deepMerge(existing, value) : value; + } + return result; +} + +export interface ProjectedPatches { + payload: Record; + projected: InstanceConfig; +} + +// Each patch sees the previous ones' result, so checks touching the same array +// or object compose, and a check already satisfied is skipped regardless of order. +export function projectPatches(input: CheckInput, checks: CheckDef[]): ProjectedPatches { + let payload: Record = {}; + let projected = input.config; + for (const check of checks) { + if (!check.patch) continue; + const current = { ...input, config: projected }; + if (check.evaluate(current).met) continue; + const patch = check.patch(current); + payload = deepMerge(payload, patch); + projected = deepMerge(projected, patch); + } + return { payload, projected }; +} diff --git a/packages/cli-core/src/commands/security/score.test.ts b/packages/cli-core/src/commands/security/score.test.ts new file mode 100644 index 000000000..c5084ae47 --- /dev/null +++ b/packages/cli-core/src/commands/security/score.test.ts @@ -0,0 +1,83 @@ +import { test, expect, describe } from "bun:test"; +import { computeScore } from "./score.ts"; +import type { Finding, FindingStatus, SecurityGrade, Severity } from "./types.ts"; + +function finding(severity: Severity, status: FindingStatus): Finding { + return { + id: `${severity}-${status}`, + title: "", + description: "", + severity, + status, + path: "", + met: status === "met", + currentValue: null, + recommendedValue: null, + current: "", + recommended: "", + patch: null, + suggestedPatch: null, + remedy: "", + docsUrl: "", + dashboardUrl: "", + }; +} + +describe("computeScore", () => { + test("all met is an A", () => { + const score = computeScore([finding("critical", "met"), finding("good-to-have", "met")]); + expect(score).toEqual({ grade: "A", percent: 100, met: 2, total: 2, hasCriticalGap: false }); + }); + + test("an empty list scores 100", () => { + expect(computeScore([]).percent).toBe(100); + }); + + test("weights severities 3 / 2 / 1", () => { + const score = computeScore([ + finding("critical", "met"), + finding("recommended", "unmet"), + finding("good-to-have", "unmet"), + ]); + expect(score.percent).toBe(50); + }); + + const THRESHOLDS: Array<[number, SecurityGrade]> = [ + [0.95, "A"], + [0.8, "B"], + [0.6, "C"], + [0.4, "D"], + [0.39, "F"], + ]; + + test.each(THRESHOLDS)("ratio %d grades %s", (ratio, grade) => { + const total = 100; + const met = Math.round(ratio * total); + const findings = Array.from({ length: total }, (_, i) => + finding("good-to-have", i < met ? "met" : "unmet"), + ); + expect(computeScore(findings).grade).toBe(grade); + }); + + test("an unmet critical caps the grade at C", () => { + const findings = [ + finding("critical", "unmet"), + ...Array.from({ length: 30 }, () => finding("good-to-have", "met")), + ]; + const score = computeScore(findings); + expect(score.percent).toBeGreaterThanOrEqual(90); + expect(score.grade).toBe("C"); + expect(score.hasCriticalGap).toBe(true); + }); + + test("the cap does not raise a lower grade", () => { + const score = computeScore([finding("critical", "unmet"), finding("critical", "unmet")]); + expect(score.grade).toBe("F"); + }); + + test("blocked findings count as gaps", () => { + const score = computeScore([finding("recommended", "blocked"), finding("recommended", "met")]); + expect(score.percent).toBe(50); + expect(score.met).toBe(1); + }); +}); diff --git a/packages/cli-core/src/commands/security/score.ts b/packages/cli-core/src/commands/security/score.ts new file mode 100644 index 000000000..2d590fc29 --- /dev/null +++ b/packages/cli-core/src/commands/security/score.ts @@ -0,0 +1,43 @@ +import type { Finding, SecurityGrade, SecurityScore, Severity } from "./types.ts"; + +export const SEVERITY_WEIGHT: Record = { + critical: 3, + recommended: 2, + "good-to-have": 1, +}; + +const GRADE_ORDER: SecurityGrade[] = ["F", "D", "C", "B", "A"]; +// A green grade must mean no critical gaps, whatever the weighted ratio says. +const CRITICAL_GRADE_CAP: SecurityGrade = "C"; + +function gradeForRatio(ratio: number): SecurityGrade { + if (ratio >= 0.95) return "A"; + if (ratio >= 0.8) return "B"; + if (ratio >= 0.6) return "C"; + if (ratio >= 0.4) return "D"; + return "F"; +} + +// Blocked counts as a gap; not-applicable never reaches this list. +export function computeScore(findings: Finding[]): SecurityScore { + const totalWeight = findings.reduce((sum, f) => sum + SEVERITY_WEIGHT[f.severity], 0); + const metWeight = findings.reduce( + (sum, f) => sum + (f.status === "met" ? SEVERITY_WEIGHT[f.severity] : 0), + 0, + ); + const ratio = totalWeight === 0 ? 1 : metWeight / totalWeight; + const hasCriticalGap = findings.some((f) => f.severity === "critical" && f.status !== "met"); + + let grade = gradeForRatio(ratio); + if (hasCriticalGap && GRADE_ORDER.indexOf(grade) > GRADE_ORDER.indexOf(CRITICAL_GRADE_CAP)) { + grade = CRITICAL_GRADE_CAP; + } + + return { + grade, + percent: Math.round(ratio * 100), + met: findings.filter((f) => f.status === "met").length, + total: findings.length, + hasCriticalGap, + }; +} diff --git a/packages/cli-core/src/commands/security/types.ts b/packages/cli-core/src/commands/security/types.ts new file mode 100644 index 000000000..2d2f36857 --- /dev/null +++ b/packages/cli-core/src/commands/security/types.ts @@ -0,0 +1,143 @@ +import type { KnownDashboardPath } from "../open/dashboard-paths.ts"; + +export type Severity = "critical" | "recommended" | "good-to-have"; + +export type FindingStatus = "met" | "unmet" | "blocked"; + +export type InstanceConfig = Record; + +export type ConfigPatch = Record; + +export interface CheckInput { + config: InstanceConfig; + environmentType: string; +} + +export interface CheckEvaluation { + met: boolean; + currentValue: unknown; + recommendedValue: unknown; + current: string; + recommended: string; +} + +export interface CheckDef { + id: string; + title: string; + description: string; + severity: Severity; + path: string; + dashboardPath: KnownDashboardPath; + docsUrl: string; + /** Billing feature the control needs on production. */ + feature?: string; + /** False excludes the check from report and score. */ + appliesTo?(input: CheckInput): boolean; + evaluate(input: CheckInput): CheckEvaluation; + /** Prerequisite check id. Still scored while blocked. */ + blockedBy?: string; + patch?(input: CheckInput): ConfigPatch; + decision?: CheckDecision; + manualRemedy?: string; +} + +export type DecisionFlag = "factors" | "strategy"; + +export interface CheckDecision { + flag: DecisionFlag; + prompt: string; + multiple: boolean; + options: Array<{ value: string; label: string }>; + defaults(input: CheckInput): string[]; + /** Usage error text for a choice the backend rejects. */ + validate?(values: string[], input: CheckInput): string | undefined; + patch(values: string[], input: CheckInput): ConfigPatch; +} + +export interface FindingDecision { + flag: DecisionFlag; + multiple: boolean; + options: string[]; + suggested: string[]; +} + +export interface Finding extends CheckEvaluation { + id: string; + title: string; + description: string; + severity: Severity; + status: FindingStatus; + path: string; + feature?: string; + blockedBy?: string; + patch: ConfigPatch | null; + /** Patch the suggested decision values would produce. */ + suggestedPatch: ConfigPatch | null; + decision?: FindingDecision; + remedy: string; + docsUrl: string; + dashboardUrl: string; +} + +export type SecurityGrade = "A" | "B" | "C" | "D" | "F"; + +export interface SecurityScore { + grade: SecurityGrade; + percent: number; + met: number; + total: number; + hasCriticalGap: boolean; +} + +export interface InstanceRef { + appId: string; + instanceId: string; + environmentType: string; + label: string; +} + +export interface AuditReport { + instance: InstanceRef; + score: SecurityScore; + /** Critical and recommended gaps only; null when none. */ + fixCommand: string | null; + findings: Finding[]; +} + +export const FAIL_ON_LEVELS = ["critical", "recommended", "any", "none"] as const; +export type FailOnLevel = (typeof FAIL_ON_LEVELS)[number]; + +export interface AuditOptions { + app?: string; + instance?: string; + json?: boolean; + spotlight?: boolean; + failOn?: FailOnLevel; +} + +export interface FixOptions { + app?: string; + instance?: string; + check?: string[]; + all?: boolean; + dryRun?: boolean; + yes?: boolean; + json?: boolean; + factors?: string[]; + strategy?: string; + goodToHave?: boolean; +} + +export type SkipReason = "met" | "not_applicable"; + +export interface FixSummary { + /** False when nothing was sent. */ + changed: boolean; + dryRun: boolean; + applied: string[]; + decisions: Record; + skipped: Array<{ id: string; reason: SkipReason }>; + score: { before: SecurityScore; after: SecurityScore }; + /** Projected under --dry-run. */ + remaining: string[]; +} diff --git a/packages/cli-core/src/lib/copy.ts b/packages/cli-core/src/lib/copy.ts index e8c12933e..2c12a4a35 100644 --- a/packages/cli-core/src/lib/copy.ts +++ b/packages/cli-core/src/lib/copy.ts @@ -57,6 +57,10 @@ export const keylessCopy = { "Replacing the entire configuration is only available for a claimed application — an unclaimed accountless application has no full config document to replace.\n" + "Use `clerk config patch` to update individual settings, or run `clerk auth login` to claim the application first.", + securityNeedsClaimedApplication: (): string => + "Security recommendations are only available for a claimed application — the checks read the account-level config document (attack protection, passwords, sessions), which Clerk's Backend API does not expose to an unclaimed accountless application.\n" + + "Run `clerk auth login` to claim this application, then re-run `clerk security audit`.", + userDashboardNeedsClaim: (keySource: string, userId: string): string => `This directory holds an unclaimed accountless application (secret key from ${keySource}), which has no Dashboard page — a dashboard link needs an application ID, and one is only assigned when the application is claimed.\n` + `Run \`clerk auth login\` to claim it, then \`clerk users open ${userId}\` will work.\n` + diff --git a/packages/cli-core/src/lib/errors.ts b/packages/cli-core/src/lib/errors.ts index 247e02776..82918953c 100644 --- a/packages/cli-core/src/lib/errors.ts +++ b/packages/cli-core/src/lib/errors.ts @@ -48,6 +48,8 @@ export const ERROR_CODE = { CATALOG_ERROR: "catalog_error", /** Doctor checks found issues. */ DOCTOR_FAILED: "doctor_failed", + /** `clerk security audit` found unmet recommendations at or above `--fail-on`. */ + SECURITY_AUDIT_FAILED: "security_audit_failed", /** Frontend API request failed. */ FAPI_ERROR: "fapi_error", /** Subscription plan does not cover the dev instance's enabled features. */ @@ -184,6 +186,13 @@ interface BillingErrorOptions extends CliErrorOptions { * }); * ``` */ +/** In agent mode a Clerk docs link points at its raw markdown (`.md`) variant. */ +export function agentDocsUrl(url: string): string { + return isAgent() && url.startsWith("https://clerk.com/docs/") && !url.endsWith(".md") + ? `${url}.md` + : url; +} + export class CliError extends Error { public code?: ErrorCode; public exitCode: ExitCode; @@ -197,19 +206,7 @@ export class CliError extends Error { this.exitCode = options?.exitCode ?? EXIT_CODE.GENERAL; this.examples = options?.examples; - if (options?.docsUrl) { - this.docsUrl = options.docsUrl; - - // If we're running in agent mode and the docs URL is a Clerk docs link - // without a .md extension, add .md to get the raw markdown URL. - if ( - isAgent() && - this.docsUrl.startsWith("https://clerk.com/docs/") && - !this.docsUrl.endsWith(".md") - ) { - this.docsUrl += ".md"; - } - } + if (options?.docsUrl) this.docsUrl = agentDocsUrl(options.docsUrl); } } diff --git a/packages/cli-core/src/lib/next-steps.ts b/packages/cli-core/src/lib/next-steps.ts index e447ddd2e..748f7da01 100644 --- a/packages/cli-core/src/lib/next-steps.ts +++ b/packages/cli-core/src/lib/next-steps.ts @@ -59,6 +59,11 @@ export const NEXT_STEPS = { ], CONFIG_DRY_RUN_PATCH: ["Run `clerk config patch` without `--dry-run` to apply these changes"], CONFIG_DRY_RUN_PUT: ["Run `clerk config put` without `--dry-run` to apply these changes"], + SECURITY_AUDIT: [ + "Run `clerk security audit --instance prod` to audit production", + "Run `clerk security checks` to read what each recommendation means", + ], + SECURITY_FIX_REMAINING: ["Run `clerk security audit --spotlight` to see what is still open"], LOGOUT: ["Run `clerk auth login` to sign in again"], WHOAMI: ["Run `clerk link` to connect this directory to an application"], // An unclaimed keyless app can't be `clerk link`ed — nothing in any account diff --git a/packages/cli-core/src/test/integration/completion.test.ts b/packages/cli-core/src/test/integration/completion.test.ts index e1e10d2e1..8da3d8695 100644 --- a/packages/cli-core/src/test/integration/completion.test.ts +++ b/packages/cli-core/src/test/integration/completion.test.ts @@ -77,6 +77,15 @@ describe("generateCompletions", () => { test("completes deploy subcommands", () => { expect(completionNames("deploy", "")).toContain("status"); }); + + test("completes security subcommands and --fail-on choices", () => { + expect(completionNames("")).toContain("security"); + const names = completionNames("security", ""); + expect(names).toEqual(expect.arrayContaining(["audit", "fix", "checks"])); + expect(completionNames("security", "audit", "--fail-on", "")).toEqual( + expect.arrayContaining(["critical", "recommended", "any", "none"]), + ); + }); }); describe("impersonate completion", () => { @@ -265,6 +274,22 @@ describe("generateCompletions", () => { expect(names).toContain("--method"); }); + test("security fix: hints --factors values and --strategy choices", () => { + expect(completionNames("security", "fix", "--factors", "")).toEqual( + expect.arrayContaining(["authenticator", "backup-code", "sms"]), + ); + expect(completionNames("security", "fix", "--strategy", "")).toEqual( + expect.arrayContaining(["email-code", "email-link", "phone-code", "passkey"]), + ); + }); + + test("security fix: suggests check ids", () => { + const names = completionNames("security", "fix", ""); + expect(names).toContain("user-lockout"); + expect(names).toContain("mfa-required"); + expect(names).toContain("--all"); + }); + test("open dashboard: suggests known subpaths", () => { const names = completionNames("open", "dashboard", ""); expect(names).toContain("users"); diff --git a/packages/cli-core/src/test/integration/lib/harness.ts b/packages/cli-core/src/test/integration/lib/harness.ts index 6a99749fb..64878225e 100644 --- a/packages/cli-core/src/test/integration/lib/harness.ts +++ b/packages/cli-core/src/test/integration/lib/harness.ts @@ -115,7 +115,7 @@ mock.module( // ── Prompt queue (drives lib/prompts.ts and lib/listage.ts mocks) ──────────── -type PromptType = "select" | "search" | "input" | "confirm" | "password" | "editor"; +type PromptType = "select" | "search" | "input" | "confirm" | "password" | "editor" | "multiselect"; const promptQueues: Record = { select: [], @@ -124,6 +124,7 @@ const promptQueues: Record = { confirm: [], password: [], editor: [], + multiselect: [], }; function dequeuePrompt(name: PromptType) { @@ -164,6 +165,7 @@ export const mockPrompts = { input: (...responses: string[]) => promptQueues.input.push(...responses), password: (...responses: string[]) => promptQueues.password.push(...responses), editor: (...responses: string[]) => promptQueues.editor.push(...responses), + multiselect: (...responses: unknown[][]) => promptQueues.multiselect.push(...responses), }; function resetPromptQueues() { @@ -208,6 +210,7 @@ mock.module("../../../lib/prompts.ts", () => ({ text: dequeuePrompt("input"), password: dequeuePrompt("password"), editor: dequeuePrompt("editor"), + multiselect: dequeuePrompt("multiselect"), })); mock.module( diff --git a/packages/cli-core/src/test/integration/security.test.ts b/packages/cli-core/src/test/integration/security.test.ts new file mode 100644 index 000000000..36fbdf519 --- /dev/null +++ b/packages/cli-core/src/test/integration/security.test.ts @@ -0,0 +1,204 @@ +/** + * `clerk security` end to end through the real program. + * Agents get a JSON report on stdout and JSON errors on stderr. + */ + +import { test, expect, beforeEach } from "bun:test"; +import { INSECURE_CONFIG, SECURE_CONFIG } from "../../commands/security/fixtures.ts"; +import type { AuditReport } from "../../commands/security/types.ts"; +import { deepMerge } from "../../commands/security/merge.ts"; +import { + useIntegrationTestHarness, + http, + setProfile, + clerk, + getInstance, + mockPrompts, + MOCK_APP, +} from "./lib/harness.ts"; + +useIntegrationTestHarness(); + +const devInstance = getInstance(MOCK_APP, "development"); + +function serveConfig(config: Record) { + http.stub(async (url, init) => { + if (!url.includes("/config")) return new Response("{}", { status: 404 }); + const body = typeof init?.body === "string" ? JSON.parse(init.body) : {}; + const result = init?.method === "PATCH" ? deepMerge(config, body) : config; + return new Response(JSON.stringify(result), { status: 200 }); + }); +} + +beforeEach(async () => { + await setProfile("github.com/test/project", { + workspaceId: "", + appId: MOCK_APP.application_id, + instances: { development: devInstance.instance_id }, + }); + serveConfig(INSECURE_CONFIG); +}); + +test("security checks lists the catalog without any request", async () => { + const { stdout } = await clerk("--mode", "agent", "security", "checks"); + const parsed = JSON.parse(stdout) as Array<{ id: string; fixable: boolean }>; + expect(parsed.some((c) => c.id === "user-lockout" && c.fixable)).toBe(true); + expect(http.requests).toHaveLength(0); +}); + +test("security audit in agent mode exits 1 with the report on stdout and a JSON error on stderr", async () => { + const result = await clerk.raw("--mode", "agent", "security", "audit"); + expect(result.exitCode).toBe(1); + const report = JSON.parse(result.stdout) as AuditReport; + expect(report.score.hasCriticalGap).toBe(true); + expect(report.findings.length).toBeGreaterThan(0); + const error = JSON.parse(result.stderr).error; + expect(error.code).toBe("security_audit_failed"); + expect(http.requests.filter((r) => r.method === "GET")).toHaveLength(1); +}); + +test("security alone runs the audit", async () => { + const result = await clerk.raw("--mode", "agent", "security", "--fail-on", "none"); + expect(result.exitCode).toBe(0); + expect((JSON.parse(result.stdout) as AuditReport).findings.length).toBeGreaterThan(0); +}); + +test.each([{ mode: "human" }, { mode: "agent" }])( + "security audit exits 0 on a secure instance ($mode mode)", + async ({ mode }) => { + serveConfig(SECURE_CONFIG); + const result = await clerk.raw("--mode", mode, "security", "audit"); + expect(result.exitCode).toBe(0); + }, +); + +test("human audit prints the grouped report on stderr", async () => { + const result = await clerk.raw("--mode", "human", "security", "audit", "--spotlight"); + expect(result.exitCode).toBe(1); + expect(result.stderr).toContain("Grade"); + expect(result.stderr).toContain("Critical"); + expect(result.stderr).toContain("error:"); + expect(result.stdout).toBe(""); +}); + +test("security fix without ids exits 2 in agent mode", async () => { + const result = await clerk.raw("--mode", "agent", "security", "fix"); + expect(result.exitCode).toBe(2); + const error = JSON.parse(result.stderr).error; + expect(error.code).toBe("usage_error"); + expect(error.examples).toBeDefined(); + expect(http.requests).toHaveLength(0); +}); + +test("security fix requires --yes in agent mode", async () => { + const result = await clerk.raw("--mode", "agent", "security", "fix", "--all"); + expect(result.exitCode).toBe(2); + expect(result.stderr).toContain("Pass --yes"); +}); + +test.each([{ mode: "human" }, { mode: "agent" }])( + "security fix --all --dry-run sends PATCH with ?dry_run=true ($mode mode)", + async ({ mode }) => { + const { stderr } = await clerk("--mode", mode, "security", "fix", "--all", "--dry-run"); + const patchReqs = http.requests.filter((r) => r.method === "PATCH"); + expect(patchReqs).toHaveLength(1); + expect(patchReqs[0]!.url).toContain("dry_run=true"); + expect(stderr).toContain("[dry-run]"); + }, +); + +test("security fix applies the patch and reports it as JSON in agent mode", async () => { + const { stdout } = await clerk( + "--mode", + "agent", + "security", + "fix", + "user-lockout", + "client-trust", + "--yes", + ); + const summary = JSON.parse(stdout); + expect(summary).toMatchObject({ + changed: true, + applied: ["user-lockout", "client-trust"], + skipped: [], + dryRun: false, + }); + expect(summary.score.after.met).toBe(summary.score.before.met + 2); + const patch = http.requests.find((r) => r.method === "PATCH")!; + expect(JSON.parse(patch.body as string)).toEqual({ + auth_attack_protection: { user_lockout: { enabled: true } }, + auth_password: { device_trust: { enabled: true } }, + }); +}); + +test("security fix accepts ids through --input-json", async () => { + await clerk( + "--mode", + "agent", + "security", + "fix", + "--input-json", + '{"check":["user-lockout","bot-protection"],"yes":true}', + ); + const patch = http.requests.find((r) => r.method === "PATCH")!; + expect(Object.keys(JSON.parse(patch.body as string))).toEqual(["auth_attack_protection"]); +}); + +test("bare security fix in human mode applies the picked recommendations", async () => { + mockPrompts.multiselect(["user-lockout", "bot-protection"]); + mockPrompts.confirm(true); + const { stderr } = await clerk("--mode", "human", "security", "fix"); + const patch = http.requests.find((r) => r.method === "PATCH")!; + expect(JSON.parse(patch.body as string)).toEqual({ + auth_attack_protection: { + user_lockout: { enabled: true }, + bot_protection: { captcha_enabled: true, captcha_widget_type: "smart" }, + }, + }); + expect(stderr).toContain("Applied: bot-protection, user-lockout"); + expect(stderr).toContain("Grade"); +}); + +test("human audit rows show the id next to the title", async () => { + const result = await clerk.raw("--mode", "human", "security", "audit", "--spotlight"); + expect(result.stderr).toContain("Brute-force lockout"); + expect(result.stderr).toContain("user-lockout"); +}); + +test("security fix mfa --factors applies the chosen factors in agent mode", async () => { + const { stdout } = await clerk( + "--mode", + "agent", + "security", + "fix", + "mfa", + "--factors", + "authenticator,backup-code", + "--yes", + ); + expect(JSON.parse(stdout).decisions).toEqual({ mfa: ["authenticator", "backup-code"] }); + const patch = http.requests.find((r) => r.method === "PATCH")!; + expect(JSON.parse(patch.body as string)).toEqual({ + auth_multi_factor: { authenticator_app: { enabled: true }, backup_code: { enabled: true } }, + }); +}); + +test("security fix mfa without --factors exits 2 in agent mode with the suggested command", async () => { + const result = await clerk.raw("--mode", "agent", "security", "fix", "mfa", "--yes"); + expect(result.exitCode).toBe(2); + const error = JSON.parse(result.stderr).error; + expect(error.message).toContain("needs --factors"); + expect(error.examples[0].command).toContain("--factors authenticator,backup-code"); + expect(http.requests.filter((r) => r.method === "PATCH")).toHaveLength(0); +}); + +test("security fix mfa in human mode asks which factors", async () => { + mockPrompts.multiselect(["sms"]); + mockPrompts.confirm(true); + await clerk("--mode", "human", "security", "fix", "mfa"); + const patch = http.requests.find((r) => r.method === "PATCH")!; + expect(JSON.parse(patch.body as string)).toEqual({ + auth_phone: { used_for_second_factor: true, second_factor_strategies: ["phone_code"] }, + }); +}); diff --git a/packages/cli-core/src/test/lib/stubs.ts b/packages/cli-core/src/test/lib/stubs.ts index b45e49c42..028a8b2f5 100644 --- a/packages/cli-core/src/test/lib/stubs.ts +++ b/packages/cli-core/src/test/lib/stubs.ts @@ -225,6 +225,9 @@ export const libPromptsStubs = { text: async () => "", password: async () => "", editor: async () => "{}", + /** Accepts whatever the caller preselected. */ + multiselect: async (config: { initialValues?: T[] }): Promise => + config.initialValues ?? [], }; export const promptsStubs = libPromptsStubs; diff --git a/test/e2e/security-audit.test.ts b/test/e2e/security-audit.test.ts new file mode 100644 index 000000000..88cf26d10 --- /dev/null +++ b/test/e2e/security-audit.test.ts @@ -0,0 +1,85 @@ +/** + * Live-PLAPI test for `clerk security audit`. Pins the report envelope + * against the real config document, so a renamed config key shows up here + * instead of silently turning a check into a permanent "unmet". + * + * Read-only: `--fail-on none` keeps the exit code at 0 whatever the test + * instance's posture, and `fix` only runs under `--dry-run`, which the + * Platform API validates without persisting, so every patch payload is + * checked against the real schema without mutating the shared instance. + * + * Requires `CLERK_PLATFORM_API_KEY` and `CLERK_CLI_TEST_APP_ID`. Locally, + * run via `bun run test:e2e:op` so 1Password resolves both in-memory. + */ + +import { test, expect, afterAll, beforeAll } from "bun:test"; +import { join } from "node:path"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import type { + AuditReport, + FixSummary, +} from "../../packages/cli-core/src/commands/security/types.ts"; +import { CHECK_IDS } from "../../packages/cli-core/src/commands/security/catalog.ts"; + +const CLI_PATH = join(import.meta.dir, "../../packages/cli-core/src/cli.ts"); + +let APP_ID: string; +let configDir: string; + +beforeAll(() => { + const appId = process.env.CLERK_CLI_TEST_APP_ID; + const platformKey = process.env.CLERK_PLATFORM_API_KEY; + if (!appId || !platformKey) { + throw new Error( + "CLERK_CLI_TEST_APP_ID and CLERK_PLATFORM_API_KEY are required. " + + "Run via `bun run test:e2e:op` for local 1Password injection.", + ); + } + APP_ID = appId; + configDir = mkdtempSync(join(tmpdir(), "clerk-cli-e2e-security-")); +}); + +afterAll(() => { + rmSync(configDir, { recursive: true, force: true }); +}); + +test("security audit --json returns a graded report over the live config document", async () => { + const result = await Bun.$`bun ${CLI_PATH} security audit --json --fail-on none --app ${APP_ID}` + .env({ ...process.env, CLERK_CONFIG_DIR: configDir, CLERK_TELEMETRY_DISABLED: "1" }) + .quiet(); + + const report = JSON.parse(result.stdout.toString()) as AuditReport; + expect(report.instance.appId).toBe(APP_ID); + expect(report.instance.environmentType).toBe("development"); + expect(["A", "B", "C", "D", "F"]).toContain(report.score.grade); + expect(report.score.total).toBe(report.findings.length); + expect(report.findings.length).toBeGreaterThan(0); + + for (const finding of report.findings) { + expect(CHECK_IDS).toContain(finding.id); + expect(["met", "unmet", "blocked"]).toContain(finding.status); + expect(finding.dashboardUrl).toContain( + `/apps/${APP_ID}/instances/${report.instance.instanceId}/`, + ); + } + // Every fixable gap advertises the patch an agent would apply. + for (const finding of report.findings.filter((f) => f.status === "unmet")) { + expect(finding.patch !== null || finding.remedy.length > 0).toBe(true); + } +}); + +test("security fix --all --dry-run validates every fixable patch server-side", async () => { + const result = await Bun.$`bun ${CLI_PATH} security fix --all --dry-run --json --app ${APP_ID}` + .env({ ...process.env, CLERK_CONFIG_DIR: configDir, CLERK_TELEMETRY_DISABLED: "1" }) + .quiet(); + + const summary = JSON.parse(result.stdout.toString()) as FixSummary; + expect(summary.dryRun).toBe(true); + expect(summary.score.after.total).toBe(summary.score.before.total); + expect(summary.score.after.met).toBeGreaterThanOrEqual(summary.score.before.met); + if (summary.changed) { + expect(summary.applied.length).toBeGreaterThan(0); + for (const id of summary.applied) expect(summary.remaining).not.toContain(id); + } +}); From 219991d33c65ec15206661f33f83e1fd37a86dbf Mon Sep 17 00:00:00 2001 From: Dominic Couture Date: Wed, 9 Sep 2026 22:49:50 +0100 Subject: [PATCH 2/5] fix(security): address review: device-trust id, unknown lockout threshold, unowned instance, variadic completion, e2e guards Co-Authored-By: Claude Fable 5.1 --- .../src/commands/completion/__complete.ts | 13 ++++++++----- packages/cli-core/src/commands/security/README.md | 14 +++++++------- .../cli-core/src/commands/security/audit.test.ts | 7 +++++++ .../src/commands/security/catalog.test.ts | 12 +++++++++++- .../cli-core/src/commands/security/catalog.ts | 15 ++++++++++----- .../cli-core/src/commands/security/fix.test.ts | 14 +++++++------- packages/cli-core/src/commands/security/fix.ts | 2 +- packages/cli-core/src/commands/security/index.ts | 2 +- packages/cli-core/src/commands/security/load.ts | 8 +++++++- .../src/test/integration/completion.test.ts | 6 ++++++ .../src/test/integration/security.test.ts | 4 ++-- test/e2e/security-audit.test.ts | 10 +++++----- 12 files changed, 72 insertions(+), 35 deletions(-) diff --git a/packages/cli-core/src/commands/completion/__complete.ts b/packages/cli-core/src/commands/completion/__complete.ts index 68d1f49d8..f0c8e6002 100644 --- a/packages/cli-core/src/commands/completion/__complete.ts +++ b/packages/cli-core/src/commands/completion/__complete.ts @@ -219,11 +219,14 @@ function completeArguments( consumedCount: number, ): CompletionResult { const registeredArgs = cmd.registeredArguments; - if (consumedCount >= registeredArgs.length) { - return EMPTY_NO_FILE; - } - - const arg = registeredArgs[consumedCount]; + const last = registeredArgs.at(-1); + const arg = + consumedCount < registeredArgs.length + ? registeredArgs[consumedCount] + : last?.variadic + ? last + : undefined; + if (!arg) return EMPTY_NO_FILE; // Prefer strict Commander choices when available. if (arg?.argChoices) { diff --git a/packages/cli-core/src/commands/security/README.md b/packages/cli-core/src/commands/security/README.md index 66aaa9902..0a9d8f039 100644 --- a/packages/cli-core/src/commands/security/README.md +++ b/packages/cli-core/src/commands/security/README.md @@ -99,7 +99,7 @@ agent mode it prints a summary: { "changed": true, "dryRun": false, - "applied": ["user-lockout", "client-trust"], + "applied": ["user-lockout", "device-trust"], "decisions": {}, "skipped": [{ "id": "bot-protection", "reason": "met" }], "score": { "before": { "grade": "F", "…": "…" }, "after": { "grade": "C", "…": "…" } }, @@ -142,7 +142,7 @@ and breach detection are what count. | `bot-protection` | critical | `auth_attack_protection.bot_protection.captcha_enabled` | patch | | `breach-detection` | critical | `auth_password.disable_hibp` is false | patch | | `user-lockout` | critical | `auth_attack_protection.user_lockout.enabled` | patch | -| `client-trust` | critical | `auth_password.device_trust.enabled` | patch | +| `device-trust` | critical | `auth_password.device_trust.enabled` | patch | | `mfa` | critical | authenticator app, backup codes, or SMS second factor enabled | asks `--factors` | | `passwordless-auth` | critical | email/SMS code, passkey, web3, or a social connection is a first factor | asks `--strategy` | | `email-verification` | critical | `auth_email.verify_at_sign_up` (only when email is a sign-up identifier) | patch | @@ -159,9 +159,9 @@ and breach detection are what count. | `block-disposable-email` | good-to-have | `auth_access_control.block_disposable_email_domains` (email only) | patch | | `block-email-subaddresses` | good-to-have | `auth_access_control.block_email_subaddresses` (email only) | patch | -The first sixteen mirror the Dashboard's security recommendations; the last -three (`password-min-length`, `allowlist-on-sign-in`, `oauth-custom-credentials`) -are CLI-only. +Every check except `password-min-length`, `allowlist-on-sign-in`, and +`oauth-custom-credentials` mirrors the Dashboard's security recommendations; +those three are CLI-only. Three states per finding: @@ -181,7 +181,7 @@ Checks that have no meaning for the instance are **not applicable** and are left out of the report and the score entirely: the email checks when email is not a sign-up identifier, the phone check when phone is not, the four password checks (`breach-detection`, `breach-detection-sign-in`, -`client-trust`, `password-min-length`) when +`device-trust`, `password-min-length`) when `auth_password.enabled` is false, and the OAuth check outside production. Three controls depend on a Clerk billing feature and carry a `feature` key in @@ -219,7 +219,7 @@ The report: "label": "My App (development)" }, "score": { "grade": "C", "percent": 71, "met": 14, "total": 20, "hasCriticalGap": true }, - "fixCommand": "clerk security fix user-lockout client-trust --app app_… --instance ins_… --yes", + "fixCommand": "clerk security fix user-lockout device-trust --app app_… --instance ins_… --yes", "findings": [ { "id": "user-lockout", diff --git a/packages/cli-core/src/commands/security/audit.test.ts b/packages/cli-core/src/commands/security/audit.test.ts index a4e1ab5c5..730455385 100644 --- a/packages/cli-core/src/commands/security/audit.test.ts +++ b/packages/cli-core/src/commands/security/audit.test.ts @@ -227,6 +227,13 @@ describe("security audit", () => { expect(parsed.findings.some((f) => f.id === "oauth-custom-credentials")).toBe(true); }); + test("rejects a literal instance id the application does not own", async () => { + await link(); + await expect(run({ json: true, instance: "ins_other", failOn: "none" })).rejects.toThrow( + "does not belong to application app_1", + ); + }); + test("targets an app directly with --app", async () => { await run({ json: true, app: "app_1", instance: "prod", failOn: "none" }); expect(report().instance.instanceId).toBe("ins_prod"); diff --git a/packages/cli-core/src/commands/security/catalog.test.ts b/packages/cli-core/src/commands/security/catalog.test.ts index d03af910b..88610c24b 100644 --- a/packages/cli-core/src/commands/security/catalog.test.ts +++ b/packages/cli-core/src/commands/security/catalog.test.ts @@ -85,7 +85,7 @@ describe("not applicable checks", () => { for (const id of [ "breach-detection", "breach-detection-sign-in", - "client-trust", + "device-trust", "password-min-length", ]) { expect(result).not.toContain(id); @@ -162,6 +162,16 @@ describe("patch details", () => { }); }); + test("lockout-threshold treats a missing max_attempts as unmet, not zero", () => { + const config = withSection(INSECURE_CONFIG, "auth_attack_protection", { + user_lockout: { enabled: true, max_attempts: undefined }, + }); + const result = findCheck("lockout-threshold")!.evaluate(production(config)); + expect(result.met).toBe(false); + expect(result.current).toBe("Threshold unknown"); + expect(result.currentValue).toBeNull(); + }); + test("lockout-threshold reports the disabled state", () => { const result = findCheck("lockout-threshold")!.evaluate(production(INSECURE_CONFIG)); expect(result.current).toBe("Lockout disabled"); diff --git a/packages/cli-core/src/commands/security/catalog.ts b/packages/cli-core/src/commands/security/catalog.ts index c01de38d7..2f475e8d6 100644 --- a/packages/cli-core/src/commands/security/catalog.ts +++ b/packages/cli-core/src/commands/security/catalog.ts @@ -212,7 +212,7 @@ export const CHECKS: CheckDef[] = [ patch: () => ({ auth_attack_protection: { user_lockout: { enabled: true } } }), }, { - id: "client-trust", + id: "device-trust", title: "Device trust", description: "Challenge sign-ins from unrecognized devices, a key defense against credential stuffing.", @@ -311,12 +311,17 @@ export const CHECKS: CheckDef[] = [ docsUrl: DOCS_LOCKOUT, evaluate({ config }) { const enabled = flag(config, "auth_attack_protection.user_lockout.enabled"); - const attempts = num(config, "auth_attack_protection.user_lockout.max_attempts"); + const raw = at(config, "auth_attack_protection.user_lockout.max_attempts"); + const attempts = typeof raw === "number" ? raw : undefined; return { - met: enabled && attempts <= 10, - currentValue: enabled ? attempts : null, + met: enabled && attempts !== undefined && attempts <= 10, + currentValue: enabled ? (attempts ?? null) : null, recommendedValue: 10, - current: enabled ? `${attempts} attempts` : "Lockout disabled", + current: !enabled + ? "Lockout disabled" + : attempts === undefined + ? "Threshold unknown" + : `${attempts} attempts`, recommended: "10 or fewer", }; }, diff --git a/packages/cli-core/src/commands/security/fix.test.ts b/packages/cli-core/src/commands/security/fix.test.ts index 7f3695a8e..3ccd8d1bc 100644 --- a/packages/cli-core/src/commands/security/fix.test.ts +++ b/packages/cli-core/src/commands/security/fix.test.ts @@ -148,18 +148,18 @@ describe("security fix", () => { }); test("applies one merged patch for several ids", async () => { - await run(["user-lockout", "lockout-threshold", "client-trust"], { yes: true }); + await run(["user-lockout", "lockout-threshold", "device-trust"], { yes: true }); expect(patches()).toHaveLength(1); expect(patches()[0]!.body).toEqual({ auth_attack_protection: { user_lockout: { enabled: true, max_attempts: 10 } }, auth_password: { device_trust: { enabled: true } }, }); // Catalog order: prerequisites and critical checks first. - expect(captured.err).toContain("Applied: user-lockout, client-trust, lockout-threshold"); + expect(captured.err).toContain("Applied: user-lockout, device-trust, lockout-threshold"); }); test("--check unions with positional ids", async () => { - await run(["user-lockout"], { check: ["client-trust", "user-lockout"], yes: true }); + await run(["user-lockout"], { check: ["device-trust", "user-lockout"], yes: true }); expect(Object.keys(patches()[0]!.body!)).toEqual(["auth_attack_protection", "auth_password"]); }); @@ -229,7 +229,7 @@ describe("security fix", () => { }); expect(summary.score.after.met).toBe(summary.score.before.met + 2); expect(summary.remaining).not.toContain("user-lockout"); - expect(summary.remaining).toContain("client-trust"); + expect(summary.remaining).toContain("device-trust"); expect(captured.err).toContain("Grade"); }); @@ -286,7 +286,7 @@ describe("security fix", () => { : INSECURE_CONFIG; return new Response(JSON.stringify(doc), { status: 200 }); }); - await run(["client-trust"], { yes: true, json: true }); + await run(["device-trust"], { yes: true, json: true }); const summary = JSON.parse(captured.out) as FixSummary; expect(summary.score.after.total).toBe(summary.score.before.total); expect(summary.score.after.met).toBe(summary.score.before.met + 1); @@ -318,12 +318,12 @@ describe("security fix", () => { test("a manual id in the selection points at the fixable subset", async () => { let error: unknown; - await run(["mfa-required", "user-lockout", "client-trust"], { yes: true }).catch( + await run(["mfa-required", "user-lockout", "device-trust"], { yes: true }).catch( (e) => (error = e), ); const { examples } = error as { examples: Array<{ command: string }> }; expect(examples[0]!.command).toBe( - "clerk security fix user-lockout client-trust --app app_1 --instance ins_dev", + "clerk security fix user-lockout device-trust --app app_1 --instance ins_dev", ); }); diff --git a/packages/cli-core/src/commands/security/fix.ts b/packages/cli-core/src/commands/security/fix.ts index 26aed046c..32ccdc49b 100644 --- a/packages/cli-core/src/commands/security/fix.ts +++ b/packages/cli-core/src/commands/security/fix.ts @@ -24,7 +24,7 @@ import type { } from "./types.ts"; const EXAMPLES: Example[] = [ - { command: "clerk security fix user-lockout client-trust --yes", description: "Apply two fixes" }, + { command: "clerk security fix user-lockout device-trust --yes", description: "Apply two fixes" }, { command: "clerk security fix mfa --factors authenticator,backup-code --yes", description: "Enable two-factor authentication", diff --git a/packages/cli-core/src/commands/security/index.ts b/packages/cli-core/src/commands/security/index.ts index 9edffea2c..b05ec97ce 100644 --- a/packages/cli-core/src/commands/security/index.ts +++ b/packages/cli-core/src/commands/security/index.ts @@ -93,7 +93,7 @@ export function registerSecurity(program: Program): void { .setExamples([ { command: "clerk security fix", description: "Pick the recommendations to apply" }, { - command: "clerk security fix user-lockout client-trust", + command: "clerk security fix user-lockout device-trust", description: "Fix two recommendations by id", }, { diff --git a/packages/cli-core/src/commands/security/load.ts b/packages/cli-core/src/commands/security/load.ts index 6bfd5b441..790a4834d 100644 --- a/packages/cli-core/src/commands/security/load.ts +++ b/packages/cli-core/src/commands/security/load.ts @@ -15,7 +15,13 @@ export interface LoadedAudit { async function resolveEnvironmentType(appId: string, instanceId: string, label: string) { if (label === "development" || label === "production") return label; const app = await fetchApplication(appId); - return app.instances.find((i) => i.instance_id === instanceId)?.environment_type ?? "unknown"; + const instance = app.instances.find((i) => i.instance_id === instanceId); + if (!instance) { + throw new CliError(`Instance ${instanceId} does not belong to application ${appId}.`, { + code: ERROR_CODE.INSTANCE_NOT_FOUND, + }); + } + return instance.environment_type; } export async function loadAudit(options: { diff --git a/packages/cli-core/src/test/integration/completion.test.ts b/packages/cli-core/src/test/integration/completion.test.ts index 8da3d8695..1edfbb05b 100644 --- a/packages/cli-core/src/test/integration/completion.test.ts +++ b/packages/cli-core/src/test/integration/completion.test.ts @@ -283,6 +283,12 @@ describe("generateCompletions", () => { ); }); + test("security fix: keeps completing ids after the first one", () => { + const names = completionNames("security", "fix", "user-lockout", ""); + expect(names).toContain("device-trust"); + expect(names).toContain("--all"); + }); + test("security fix: suggests check ids", () => { const names = completionNames("security", "fix", ""); expect(names).toContain("user-lockout"); diff --git a/packages/cli-core/src/test/integration/security.test.ts b/packages/cli-core/src/test/integration/security.test.ts index 36fbdf519..cb36ef37c 100644 --- a/packages/cli-core/src/test/integration/security.test.ts +++ b/packages/cli-core/src/test/integration/security.test.ts @@ -114,13 +114,13 @@ test("security fix applies the patch and reports it as JSON in agent mode", asyn "security", "fix", "user-lockout", - "client-trust", + "device-trust", "--yes", ); const summary = JSON.parse(stdout); expect(summary).toMatchObject({ changed: true, - applied: ["user-lockout", "client-trust"], + applied: ["user-lockout", "device-trust"], skipped: [], dryRun: false, }); diff --git a/test/e2e/security-audit.test.ts b/test/e2e/security-audit.test.ts index 88cf26d10..c3856b622 100644 --- a/test/e2e/security-audit.test.ts +++ b/test/e2e/security-audit.test.ts @@ -25,7 +25,7 @@ import { CHECK_IDS } from "../../packages/cli-core/src/commands/security/catalog const CLI_PATH = join(import.meta.dir, "../../packages/cli-core/src/cli.ts"); let APP_ID: string; -let configDir: string; +let configDir: string | undefined; beforeAll(() => { const appId = process.env.CLERK_CLI_TEST_APP_ID; @@ -41,8 +41,8 @@ beforeAll(() => { }); afterAll(() => { - rmSync(configDir, { recursive: true, force: true }); -}); + if (configDir) rmSync(configDir, { recursive: true, force: true }); +}, 60_000); test("security audit --json returns a graded report over the live config document", async () => { const result = await Bun.$`bun ${CLI_PATH} security audit --json --fail-on none --app ${APP_ID}` @@ -67,7 +67,7 @@ test("security audit --json returns a graded report over the live config documen for (const finding of report.findings.filter((f) => f.status === "unmet")) { expect(finding.patch !== null || finding.remedy.length > 0).toBe(true); } -}); +}, 60_000); test("security fix --all --dry-run validates every fixable patch server-side", async () => { const result = await Bun.$`bun ${CLI_PATH} security fix --all --dry-run --json --app ${APP_ID}` @@ -82,4 +82,4 @@ test("security fix --all --dry-run validates every fixable patch server-side", a expect(summary.applied.length).toBeGreaterThan(0); for (const id of summary.applied) expect(summary.remaining).not.toContain(id); } -}); +}, 60_000); From caa20d5a6109b5bf112e9dc7b325b770cf3d199f Mon Sep 17 00:00:00 2001 From: Dominic Couture Date: Wed, 9 Sep 2026 22:52:51 +0100 Subject: [PATCH 3/5] docs(security): drop the catalog provenance note Co-Authored-By: Claude Fable 5.1 --- packages/cli-core/src/commands/security/README.md | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/cli-core/src/commands/security/README.md b/packages/cli-core/src/commands/security/README.md index 0a9d8f039..a7a990535 100644 --- a/packages/cli-core/src/commands/security/README.md +++ b/packages/cli-core/src/commands/security/README.md @@ -159,10 +159,6 @@ and breach detection are what count. | `block-disposable-email` | good-to-have | `auth_access_control.block_disposable_email_domains` (email only) | patch | | `block-email-subaddresses` | good-to-have | `auth_access_control.block_email_subaddresses` (email only) | patch | -Every check except `password-min-length`, `allowlist-on-sign-in`, and -`oauth-custom-credentials` mirrors the Dashboard's security recommendations; -those three are CLI-only. - Three states per finding: The **good-to-have** tier is hardening that costs users some convenience: From 1f32698b113d58bf1d56ea820c785cc464a4e44f Mon Sep 17 00:00:00 2001 From: Dominic Couture Date: Thu, 10 Sep 2026 12:26:38 +0100 Subject: [PATCH 4/5] docs(security): backup codes alone do not satisfy mfa Co-Authored-By: Claude Fable 5.1 --- packages/cli-core/src/commands/security/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli-core/src/commands/security/README.md b/packages/cli-core/src/commands/security/README.md index a7a990535..c9890cc14 100644 --- a/packages/cli-core/src/commands/security/README.md +++ b/packages/cli-core/src/commands/security/README.md @@ -143,7 +143,7 @@ and breach detection are what count. | `breach-detection` | critical | `auth_password.disable_hibp` is false | patch | | `user-lockout` | critical | `auth_attack_protection.user_lockout.enabled` | patch | | `device-trust` | critical | `auth_password.device_trust.enabled` | patch | -| `mfa` | critical | authenticator app, backup codes, or SMS second factor enabled | asks `--factors` | +| `mfa` | critical | authenticator app or SMS second factor enabled (backup codes alone do not count) | asks `--factors` | | `passwordless-auth` | critical | email/SMS code, passkey, web3, or a social connection is a first factor | asks `--strategy` | | `email-verification` | critical | `auth_email.verify_at_sign_up` (only when email is a sign-up identifier) | patch | | `breach-detection-sign-in` | recommended | `auth_password.enforce_hibp_on_sign_in` and `disable_hibp` | patch | From 4f413955ff6251f2c7844a3fac0f29b9511465d2 Mon Sep 17 00:00:00 2001 From: Dominic Couture Date: Thu, 10 Sep 2026 14:27:17 +0100 Subject: [PATCH 5/5] docs(security): describe what fix --all applies instead of claiming no user impact Co-Authored-By: Claude Fable 5.1 --- .../cli-core/src/commands/security/README.md | 18 +++++++++++------- .../cli-core/src/commands/security/evaluate.ts | 2 +- .../cli-core/src/commands/security/index.ts | 5 +---- 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/packages/cli-core/src/commands/security/README.md b/packages/cli-core/src/commands/security/README.md index c9890cc14..4ae242456 100644 --- a/packages/cli-core/src/commands/security/README.md +++ b/packages/cli-core/src/commands/security/README.md @@ -161,18 +161,22 @@ and breach detection are what count. Three states per finding: -The **good-to-have** tier is hardening that costs users some convenience: -magic links that must open on the requesting device, and the two email -blocks. It counts toward the score like anything else, but `fix --all` and -the interactive picker leave it out unless asked (`--good-to-have`, or -ticking the rows), so a blanket `fix --all` never changes what end users -experience beyond a CAPTCHA and verification. - - **met**: nothing to do. - **unmet**: a real gap. Has a `patch` when the fix is a pure config change. - **blocked**: a real gap that cannot be applied until a prerequisite is met (`mfa-required` needs `mfa`). Still counts against the score. +`fix --all` applies the unmet critical and recommended checks that have a +patch: bot protection (CAPTCHA on sign-up), breached-password detection on +sign-up and sign-in, brute-force lockout and its threshold, device trust (a +second-factor challenge for password sign-ins from new devices), email and +phone verification at sign-up, passkey sign-in, an 8-character password +minimum, and allowlist enforcement on sign-in. Several of these change how +users sign in; preview with `--dry-run` first. The **good-to-have** tier +(same-device magic links, the two email blocks, and a bounded session +lifetime) counts toward the score but is left out of `--all` and unticked in +the picker; opt in with `--good-to-have` or by naming ids. + Checks that have no meaning for the instance are **not applicable** and are left out of the report and the score entirely: the email checks when email is not a sign-up identifier, the phone check when phone is not, the four diff --git a/packages/cli-core/src/commands/security/evaluate.ts b/packages/cli-core/src/commands/security/evaluate.ts index 835613439..fde8649be 100644 --- a/packages/cli-core/src/commands/security/evaluate.ts +++ b/packages/cli-core/src/commands/security/evaluate.ts @@ -113,7 +113,7 @@ export function fixableIds(findings: Finding[], goodToHave = true): string[] { export function buildReport(input: CheckInput, ref: InstanceRef): AuditReport { const findings = evaluate(input, ref); - // Good-to-have costs users convenience; never suggested by default. + // Good-to-have is opt-in. const fixable = fixableIds(findings, false); return { instance: ref, diff --git a/packages/cli-core/src/commands/security/index.ts b/packages/cli-core/src/commands/security/index.ts index b05ec97ce..4a66b27ff 100644 --- a/packages/cli-core/src/commands/security/index.ts +++ b/packages/cli-core/src/commands/security/index.ts @@ -70,10 +70,7 @@ export function registerSecurity(program: Program): void { collectOptionValues, ) .option("--all", "Fix every unmet critical and recommended check that has an inline patch") - .option( - "--good-to-have", - "With --all, also apply the good-to-have tier (hardening that costs users some convenience)", - ) + .option("--good-to-have", "With --all, also apply the good-to-have tier") .option( "--factors ", "Second factors for `mfa`: authenticator, backup-code, sms (comma-separated; asked interactively when omitted)",