From 68bf6998de7ed53693b41f428e333f6d10eeb753 Mon Sep 17 00:00:00 2001 From: Altay Date: Sat, 9 May 2026 02:16:41 +0300 Subject: [PATCH 1/2] fix(security): harden cli trust boundaries --- .github/workflows/backfill-release-assets.yml | 18 +-- AGENTS.md | 2 +- README.md | 4 +- install.sh | 18 ++- skills/putio-cli/references/reads.md | 4 +- src/command-paths.test.ts | 52 +++++++++ src/internal/command.test.ts | 47 ++++++++ src/internal/command.ts | 106 +++++++++++++++++- src/internal/output-service.ts | 22 +++- src/internal/output.test.ts | 19 ++++ 10 files changed, 270 insertions(+), 22 deletions(-) diff --git a/.github/workflows/backfill-release-assets.yml b/.github/workflows/backfill-release-assets.yml index dddb514..fb32a27 100644 --- a/.github/workflows/backfill-release-assets.yml +++ b/.github/workflows/backfill-release-assets.yml @@ -15,16 +15,20 @@ jobs: timeout-minutes: 5 permissions: contents: read + outputs: + tag_name: ${{ steps.validate.outputs.tag_name }} steps: - name: Validate tag name + id: validate env: TAG_NAME: ${{ inputs.tag_name }} run: | if [[ ! "$TAG_NAME" =~ ^v[0-9]+\.[0-9]+\.[0-9]+([-+][0-9A-Za-z.-]+)?$ ]]; then - echo "Invalid release tag: $TAG_NAME" >&2 + echo "Invalid release tag." >&2 exit 1 fi + printf 'tag_name=%s\n' "$TAG_NAME" >> "$GITHUB_OUTPUT" build-unix-binaries: name: Build ${{ matrix.os }} release assets @@ -58,7 +62,7 @@ jobs: uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 with: fetch-depth: 0 - ref: ${{ inputs.tag_name }} + ref: ${{ needs.validate-release-tag.outputs.tag_name }} - name: Set up Vite+ uses: voidzero-dev/setup-vp@45e5c098f1095cc6b65fd92534603e7be70386c1 # v1 @@ -78,7 +82,7 @@ jobs: - name: Package release assets shell: pwsh env: - TAG_NAME: ${{ inputs.tag_name }} + TAG_NAME: ${{ needs.validate-release-tag.outputs.tag_name }} run: | $version = $env:TAG_NAME.TrimStart("v") $assetBase = "putio-cli-$version-${{ matrix.asset_os }}-${{ matrix.asset_arch }}" @@ -103,7 +107,7 @@ jobs: uses: softprops/action-gh-release@b4309332981a82ec1c5618f44dd2e27cc8bfbfda # v3 with: token: ${{ steps.release-bot.outputs.token }} - tag_name: ${{ inputs.tag_name }} + tag_name: ${{ needs.validate-release-tag.outputs.tag_name }} files: | .artifacts/release/* @@ -129,7 +133,7 @@ jobs: uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 with: fetch-depth: 0 - ref: ${{ inputs.tag_name }} + ref: ${{ needs.validate-release-tag.outputs.tag_name }} - name: Set up Vite+ uses: voidzero-dev/setup-vp@45e5c098f1095cc6b65fd92534603e7be70386c1 # v1 @@ -149,7 +153,7 @@ jobs: - name: Package release assets shell: pwsh env: - TAG_NAME: ${{ inputs.tag_name }} + TAG_NAME: ${{ needs.validate-release-tag.outputs.tag_name }} run: | $version = $env:TAG_NAME.TrimStart("v") $assetBase = "putio-cli-$version-windows-amd64" @@ -169,6 +173,6 @@ jobs: uses: softprops/action-gh-release@b4309332981a82ec1c5618f44dd2e27cc8bfbfda # v3 with: token: ${{ steps.release-bot.outputs.token }} - tag_name: ${{ inputs.tag_name }} + tag_name: ${{ needs.validate-release-tag.outputs.tag_name }} files: | .artifacts/release/* diff --git a/AGENTS.md b/AGENTS.md index d3167a7..54e384b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -32,7 +32,7 @@ Focused: Runtime proofs: - `./dist/bin.mjs describe` -- `./dist/bin.mjs whoami --output json` +- `./dist/bin.mjs whoami --fields auth --output json` ## Development Guidance diff --git a/README.md b/README.md index d9f01bd..0829e32 100644 --- a/README.md +++ b/README.md @@ -100,10 +100,10 @@ Link your account: putio auth login ``` -Check the account: +Check the auth source: ```bash -putio whoami --output json +putio whoami --fields auth --output json ``` Read a small JSON result: diff --git a/install.sh b/install.sh index 651f89a..310c80a 100755 --- a/install.sh +++ b/install.sh @@ -113,8 +113,21 @@ verify_checksum() { } ensure_install_dir() { - mkdir -p "$INSTALL_DIR" + if [ ! -d "$INSTALL_DIR" ]; then + mkdir -p "$INSTALL_DIR" + chmod 0755 "$INSTALL_DIR" + fi + [ -w "$INSTALL_DIR" ] || fail "install directory is not writable: $INSTALL_DIR" + + permissions="$(ls -ld "$INSTALL_DIR" | awk '{print $1}')" + group_write="$(printf '%s' "$permissions" | cut -c6)" + other_write="$(printf '%s' "$permissions" | cut -c9)" + + if { [ "$group_write" = "w" ] || [ "$other_write" = "w" ]; } && + [ "${PUTIO_CLI_ALLOW_SHARED_INSTALL_DIR:-}" != "1" ]; then + fail "install directory is group/world-writable: $INSTALL_DIR. Set PUTIO_CLI_ALLOW_SHARED_INSTALL_DIR=1 to allow this intentionally." + fi } print_path_hint() { @@ -156,8 +169,9 @@ printf '%s\n' "putio installer: verifying checksum" verify_checksum "$checksum_path" "$archive_path" tar -xzf "$archive_path" -C "$work_dir" -chmod +x "$work_dir/putio" +chmod 0755 "$work_dir/putio" mv "$work_dir/putio" "$INSTALL_DIR/putio" +chmod 0755 "$INSTALL_DIR/putio" printf '%s\n' "putio installer: installed to $INSTALL_DIR/putio" print_path_hint diff --git a/skills/putio-cli/references/reads.md b/skills/putio-cli/references/reads.md index 751dee8..acd4145 100644 --- a/skills/putio-cli/references/reads.md +++ b/skills/putio-cli/references/reads.md @@ -3,14 +3,14 @@ Prefer structured output: ```bash -putio whoami +putio whoami --fields auth --output json putio files list --output json ``` Use `--fields` with top-level keys only: ```bash -putio whoami --fields auth,info +putio whoami --fields auth --output json putio files list --fields files,total --output json ``` diff --git a/src/command-paths.test.ts b/src/command-paths.test.ts index 850b069..315ac3a 100644 --- a/src/command-paths.test.ts +++ b/src/command-paths.test.ts @@ -929,6 +929,58 @@ describe("cli command paths", () => { ); }); + it("rejects repeated cursors while streaming file list pages", async () => { + mocks.listFilesMock.mockReturnValueOnce( + Effect.succeed({ + cursor: "cursor-1", + files: [{ id: 1, name: "Movies" }], + total: 2, + }), + ); + mocks.continueFilesMock.mockReturnValueOnce( + Effect.succeed({ + cursor: "cursor-1", + files: [{ id: 2, name: "Shows" }], + total: 2, + }), + ); + + await expect( + runCliInTest(["putio", "files", "list", "--page-all", "--output", "ndjson"]), + ).rejects.toMatchObject({ + message: "`files list` pagination returned a repeated cursor.", + }); + + expect(mocks.continueFilesMock).toHaveBeenCalledTimes(1); + expect(mocks.writeOutputMock).toHaveBeenCalledTimes(1); + }); + + it("rejects cumulative item overflow while streaming file list pages", async () => { + mocks.listFilesMock.mockReturnValueOnce( + Effect.succeed({ + cursor: "cursor-1", + files: Array.from({ length: 60_000 }, (_value, id) => ({ id })), + total: 110_001, + }), + ); + mocks.continueFilesMock.mockReturnValueOnce( + Effect.succeed({ + cursor: null, + files: Array.from({ length: 50_001 }, (_value, id) => ({ id: id + 60_000 })), + total: 110_001, + }), + ); + + await expect( + runCliInTest(["putio", "files", "list", "--page-all", "--output", "ndjson"]), + ).rejects.toMatchObject({ + message: "`files list` pagination exceeded 100000 items.", + }); + + expect(mocks.continueFilesMock).toHaveBeenCalledTimes(1); + expect(mocks.writeOutputMock).toHaveBeenCalledTimes(1); + }); + it("executes files rename", async () => { await expect( runCliInTest([ diff --git a/src/internal/command.test.ts b/src/internal/command.test.ts index b036259..ad68c23 100644 --- a/src/internal/command.test.ts +++ b/src/internal/command.test.ts @@ -185,6 +185,25 @@ describe("resolveReadOutputControls", () => { expect(String(exit.cause)).toContain("cannot include `?` or `#` fragments"); } }); + + it("does not reflect control-bearing field selectors in errors", async () => { + const payload = "\u001B]52;c;cHduZWQ=\u0007"; + const exit = await Effect.runPromiseExit( + provideRuntime( + resolveReadOutputControls({ + fields: Option.some(payload), + output: "json", + }), + ), + ); + + expect(exit._tag).toBe("Failure"); + if (exit._tag === "Failure") { + const cause = String(exit.cause); + expect(cause).toContain("`--fields` selector cannot contain control characters"); + expect(cause).not.toContain(payload); + } + }); }); describe("selectTopLevelFields", () => { @@ -272,6 +291,34 @@ describe("collectAllCursorPages", () => { total: 3, }); }); + + it("rejects repeated cursors", async () => { + const continueWithCursor = () => + Effect.succeed({ + cursor: "cursor-1", + files: [{ id: 2 }], + total: 2, + }); + + const exit = await Effect.runPromiseExit( + collectAllCursorPages({ + command: "files list", + continueWithCursor, + initial: { + cursor: "cursor-1", + files: [{ id: 1 }], + total: 2, + }, + itemKey: "files", + pageAll: true, + }), + ); + + expect(exit._tag).toBe("Failure"); + if (exit._tag === "Failure") { + expect(String(exit.cause)).toContain("pagination returned a repeated cursor"); + } + }); }); describe("agent-safe string validation", () => { diff --git a/src/internal/command.ts b/src/internal/command.ts index 951d01d..0357571 100644 --- a/src/internal/command.ts +++ b/src/internal/command.ts @@ -134,6 +134,8 @@ type ReadOutputControls = { const PATH_TRAVERSAL_PATTERN = /(?:^|[\\/])\.\.(?:[\\/]|$)|%2e/iu; const QUERY_OR_FRAGMENT_PATTERN = /[?#]/u; const TOP_LEVEL_FIELD_PATTERN = /^[A-Za-z0-9_-]+$/u; +const MAX_CURSOR_PAGES = 1_000; +const MAX_CURSOR_ITEMS = 100_000; const ownKeys = (value: Record) => Object.keys(value); const hasControlCharacters = (value: string) => @@ -204,7 +206,7 @@ const parseRequestedFields = (raw: string) => { ...new Set( parts.map((part) => validateSafeString({ - label: `\`--fields\` selector \`${part}\``, + label: "`--fields` selector", pattern: TOP_LEVEL_FIELD_PATTERN, patternMessage: "`--fields` only accepts top-level field names without dots, brackets, or slashes.", @@ -236,6 +238,50 @@ const readPageItems = (value: Record, itemKey: string, command: return items; }; +const assertCursorPageBudget = (input: { + readonly command: string; + readonly itemCount: number; + readonly pageCount: number; +}) => { + if (input.pageCount > MAX_CURSOR_PAGES) { + throw new CliCommandInputError({ + message: `\`${input.command}\` pagination exceeded ${MAX_CURSOR_PAGES} pages.`, + }); + } + + if (input.itemCount > MAX_CURSOR_ITEMS) { + throw new CliCommandInputError({ + message: `\`${input.command}\` pagination exceeded ${MAX_CURSOR_ITEMS} items.`, + }); + } +}; + +const assertCursorNotSeen = (input: { + readonly command: string; + readonly cursor: string; + readonly seenCursors: Set; +}) => { + if (input.seenCursors.has(input.cursor)) { + throw new CliCommandInputError({ + message: `\`${input.command}\` pagination returned a repeated cursor.`, + }); + } + + input.seenCursors.add(input.cursor); +}; + +const assertCursorNotRepeated = (input: { + readonly command: string; + readonly cursor: string | null; + readonly seenCursors: Set; +}) => { + if (input.cursor !== null && input.seenCursors.has(input.cursor)) { + throw new CliCommandInputError({ + message: `\`${input.command}\` pagination returned a repeated cursor.`, + }); + } +}; + const integerPattern = /^-?\d+$/; export const parseRepeatedIntegers = ( @@ -404,12 +450,39 @@ export const collectAllCursorPages = , E, R>(i } const collectedItems = [...readPageItems(input.initial, input.itemKey, input.command)]; + const seenCursors = new Set(); let cursor = readCursor(input.initial); + let pageCount = 1; + + assertCursorPageBudget({ + command: input.command, + itemCount: collectedItems.length, + pageCount, + }); while (cursor !== null) { + assertCursorNotSeen({ + command: input.command, + cursor, + seenCursors, + }); + const nextPage = yield* input.continueWithCursor(cursor); - collectedItems.push(...readPageItems(nextPage, input.itemKey, input.command)); - cursor = readCursor(nextPage); + const nextCursor = readCursor(nextPage); + assertCursorNotRepeated({ + command: input.command, + cursor: nextCursor, + seenCursors, + }); + const pageItems = readPageItems(nextPage, input.itemKey, input.command); + pageCount += 1; + collectedItems.push(...pageItems); + assertCursorPageBudget({ + command: input.command, + itemCount: collectedItems.length, + pageCount, + }); + cursor = nextCursor; } return { @@ -477,8 +550,20 @@ export const writeReadPages = , E, R>(input: { } let current = input.initial; + const seenCursors = new Set(); + let pageCount = 1; + let streamedItemCount = 0; while (true) { + if (input.itemKey) { + streamedItemCount += readPageItems(current, input.itemKey, input.command).length; + assertCursorPageBudget({ + command: input.command, + itemCount: streamedItemCount, + pageCount, + }); + } + const selectedValue = yield* selectTopLevelFields({ command: input.command, requestedFields: input.controls.requestedFields, @@ -497,7 +582,20 @@ export const writeReadPages = , E, R>(input: { return; } - current = yield* input.continueWithCursor(cursor); + assertCursorNotSeen({ + command: input.command, + cursor, + seenCursors, + }); + + const nextPage = yield* input.continueWithCursor(cursor); + assertCursorNotRepeated({ + command: input.command, + cursor: readCursor(nextPage), + seenCursors, + }); + current = nextPage; + pageCount += 1; } }); diff --git a/src/internal/output-service.ts b/src/internal/output-service.ts index f78192b..e59eff2 100644 --- a/src/internal/output-service.ts +++ b/src/internal/output-service.ts @@ -65,11 +65,19 @@ const SAFE_SENSITIVE_SENTINEL_VALUES = new Set([ ]); const PROMPT_INJECTION_PATTERN = /\b(ignore (?:all|any|previous|above)|system prompt|developer message|tool call|function call|follow these instructions|you are chatgpt|you are an ai)\b/iu; +const TERMINAL_ESCAPE_PATTERN = new RegExp( + String.raw`\u001B(?:\][^\u0007]*(?:\u0007|\u001B\\)|\[[0-?]*[ -/]*[@-~]|[@-Z\\-_])`, + "gu", +); +const TERMINAL_CONTROL_PATTERN = new RegExp( + String.raw`[\u0000-\u0008\u000B-\u001F\u007F-\u009F]`, + "gu", +); const isPlainObject = (value: unknown): value is Record => typeof value === "object" && value !== null && !Array.isArray(value); -export const sanitizeTerminalText = (value: string) => +const redactSensitiveText = (value: string) => value .replace( /(Authorization\s*:\s*Bearer\s+)([A-Za-z0-9._~+/=-]+)/gi, @@ -88,6 +96,11 @@ export const sanitizeTerminalText = (value: string) => (_match, prefix: string) => `${prefix}${REDACTED_VALUE}`, ); +export const sanitizeTerminalText = (value: string) => + redactSensitiveText(value) + .replace(TERMINAL_ESCAPE_PATTERN, "") + .replace(TERMINAL_CONTROL_PATTERN, ""); + export const sanitizeTerminalValue = (value: unknown): unknown => { if (typeof value === "string") { return sanitizeTerminalText(value); @@ -132,7 +145,7 @@ const sanitizeStructuredValueInternal = ( path: ReadonlyArray, ): StructuredSanitization => { if (typeof value === "string") { - const sanitized = sanitizeTerminalText(value); + const sanitized = redactSensitiveText(value); return { untrustedTextPaths: PROMPT_INJECTION_PATTERN.test(sanitized) ? [joinPath(path)] : [], @@ -213,7 +226,7 @@ export const renderJson = (value: unknown) => export const renderNdjson = (value: unknown) => JSON.stringify(sanitizeStructuredValue(value)); export const renderTerminal = (value: A, renderTerminalValue: (value: A) => string) => - sanitizeTerminalText(renderTerminalValue(value)); + redactSensitiveText(renderTerminalValue(sanitizeTerminalValue(value) as A)); type LocalizedErrorMeta = { readonly details?: ReadonlyArray; @@ -266,8 +279,9 @@ const localizeError = (error: unknown) => export const formatCliError = (error: unknown) => { const localized = localizeError(error); + const view = sanitizeTerminalValue(toCliErrorView(localized)) as CliTerminalErrorView; - return sanitizeTerminalText(renderCliErrorTerminal(toCliErrorView(localized))); + return redactSensitiveText(renderCliErrorTerminal(view)); }; export const formatCliErrorJson = (error: unknown) => { diff --git a/src/internal/output.test.ts b/src/internal/output.test.ts index 5a4677b..396f1dd 100644 --- a/src/internal/output.test.ts +++ b/src/internal/output.test.ts @@ -51,6 +51,12 @@ describe("sanitizeTerminalText", () => { ), ).toBe("https://api.put.io/v2/files/1/download/file.txt?oauth_token=[REDACTED]&x=1"); }); + + it("removes terminal control characters", () => { + expect(sanitizeTerminalText("safe\u001B]52;c;cHduZWQ=\u0007\u001B[2J\rspoof")).toBe( + "safespoof", + ); + }); }); describe("renderJson", () => { @@ -76,6 +82,10 @@ describe("renderJson", () => { ); }); + it("preserves terminal control characters as escaped json data", () => { + expect(renderJson({ name: "safe\u001B[2J" })).toContain('"name": "safe\\u001b[2J"'); + }); + it("preserves suspicious api text and attaches machine-readable safety metadata", () => { const output = renderJson({ name: "movie.mkv", @@ -156,6 +166,15 @@ describe("renderTerminal", () => { renderTerminal({ token: "secret-token" }, () => "Authorization: Bearer secret-token"), ).toBe("Authorization: Bearer [REDACTED]"); }); + + it("removes terminal controls from rendered values", () => { + expect( + renderTerminal( + { name: "safe\u001B]52;c;cHduZWQ=\u0007\u001B[2J\rspoof" }, + (value) => value.name, + ), + ).toBe("safespoof"); + }); }); describe("detectOutputModeFromArgv", () => { From edf1df86937fa86e5bf1af96272b6eb5ecc488b3 Mon Sep 17 00:00:00 2001 From: Altay Date: Sat, 9 May 2026 02:25:44 +0300 Subject: [PATCH 2/2] fix(security): address review hardening feedback --- install.sh | 3 ++- src/internal/command.test.ts | 20 +++++++++++++++++++- src/internal/command.ts | 7 +++---- src/internal/output-service.ts | 10 ++++++++-- src/internal/output.test.ts | 14 ++++++++++++++ 5 files changed, 46 insertions(+), 8 deletions(-) diff --git a/install.sh b/install.sh index 310c80a..06d78ff 100755 --- a/install.sh +++ b/install.sh @@ -120,7 +120,8 @@ ensure_install_dir() { [ -w "$INSTALL_DIR" ] || fail "install directory is not writable: $INSTALL_DIR" - permissions="$(ls -ld "$INSTALL_DIR" | awk '{print $1}')" + install_dir_target="$(cd "$INSTALL_DIR" && pwd -P)" || fail "unable to resolve install directory: $INSTALL_DIR" + permissions="$(ls -ld "$install_dir_target" | awk '{print $1}')" group_write="$(printf '%s' "$permissions" | cut -c6)" other_write="$(printf '%s' "$permissions" | cut -c9)" diff --git a/src/internal/command.test.ts b/src/internal/command.test.ts index ad68c23..e8bccbf 100644 --- a/src/internal/command.test.ts +++ b/src/internal/command.test.ts @@ -200,10 +200,28 @@ describe("resolveReadOutputControls", () => { expect(exit._tag).toBe("Failure"); if (exit._tag === "Failure") { const cause = String(exit.cause); - expect(cause).toContain("`--fields` selector cannot contain control characters"); + expect(cause).toContain("`--fields` selector #1 cannot contain control characters"); expect(cause).not.toContain(payload); } }); + + it("identifies the invalid field selector by position without echoing it", async () => { + const exit = await Effect.runPromiseExit( + provideRuntime( + resolveReadOutputControls({ + fields: Option.some("auth,bad.field"), + output: "json", + }), + ), + ); + + expect(exit._tag).toBe("Failure"); + if (exit._tag === "Failure") { + const cause = String(exit.cause); + expect(cause).toContain("`--fields` selector #2 only accepts top-level field names"); + expect(cause).not.toContain("bad.field"); + } + }); }); describe("selectTopLevelFields", () => { diff --git a/src/internal/command.ts b/src/internal/command.ts index 0357571..44c9182 100644 --- a/src/internal/command.ts +++ b/src/internal/command.ts @@ -204,12 +204,11 @@ const parseRequestedFields = (raw: string) => { return [ ...new Set( - parts.map((part) => + parts.map((part, index) => validateSafeString({ - label: "`--fields` selector", + label: `\`--fields\` selector #${index + 1}`, pattern: TOP_LEVEL_FIELD_PATTERN, - patternMessage: - "`--fields` only accepts top-level field names without dots, brackets, or slashes.", + patternMessage: `\`--fields\` selector #${index + 1} only accepts top-level field names without dots, brackets, or slashes.`, value: part, }), ), diff --git a/src/internal/output-service.ts b/src/internal/output-service.ts index e59eff2..88e03ff 100644 --- a/src/internal/output-service.ts +++ b/src/internal/output-service.ts @@ -74,8 +74,14 @@ const TERMINAL_CONTROL_PATTERN = new RegExp( "gu", ); -const isPlainObject = (value: unknown): value is Record => - typeof value === "object" && value !== null && !Array.isArray(value); +const isPlainObject = (value: unknown): value is Record => { + if (typeof value !== "object" || value === null || Array.isArray(value)) { + return false; + } + + const prototype = Object.getPrototypeOf(value); + return prototype === Object.prototype || prototype === null; +}; const redactSensitiveText = (value: string) => value diff --git a/src/internal/output.test.ts b/src/internal/output.test.ts index 396f1dd..98c8973 100644 --- a/src/internal/output.test.ts +++ b/src/internal/output.test.ts @@ -33,6 +33,12 @@ describe("sanitizeTerminalValue", () => { safe: "visible", }); }); + + it("preserves non-plain objects for renderers", () => { + const date = new Date("2026-01-01T00:00:00.000Z"); + + expect(sanitizeTerminalValue(date)).toBe(date); + }); }); describe("sanitizeTerminalText", () => { @@ -175,6 +181,14 @@ describe("renderTerminal", () => { ), ).toBe("safespoof"); }); + + it("does not replace class instances before rendering", () => { + class RenderableValue { + constructor(readonly name: string) {} + } + + expect(renderTerminal(new RenderableValue("safe"), (value) => value.name)).toBe("safe"); + }); }); describe("detectOutputModeFromArgv", () => {