Fix CLI security hardening findings#5
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Addresses CLI security hardening findings across terminal output handling, pagination guardrails, installer permissions, and release tooling/doc guidance to reduce injection and runaway read risks.
Changes:
- Harden terminal rendering by stripping control/escape sequences from untrusted, terminal-rendered values while keeping structured output safely escaped.
- Add pagination guardrails for
--page-all(max pages/items) and detect repeated cursors for both aggregate and streaming modes. - Tighten installer-created permissions and validate release tag inputs/propagation in the backfill workflow; update docs to use scoped
whoamireads.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/internal/output-service.ts | Adds terminal escape/control stripping, separates redaction from structured sanitization, and adjusts terminal/error rendering paths. |
| src/internal/output.test.ts | Adds regression tests for control stripping in terminal mode and safe escaping in JSON output. |
| src/internal/command.ts | Adds cursor pagination budgets and repeated-cursor detection; adjusts --fields validation labeling. |
| src/internal/command.test.ts | Adds tests for non-reflection of control-bearing --fields and repeated cursor rejection. |
| src/command-paths.test.ts | Adds end-to-end CLI path tests for repeated cursors and item-budget overflow during streamed paging. |
| install.sh | Normalizes install dir/binary permissions and rejects group/world-writable install dirs unless opted in. |
| .github/workflows/backfill-release-assets.yml | Validates tag name once and reuses validated output for checkout/packaging/upload; reduces reflection in validation error. |
| README.md | Updates whoami example to a scoped, structured read. |
| AGENTS.md | Narrows runtime proof command to whoami --fields auth --output json. |
| skills/putio-cli/references/reads.md | Updates read guidance examples to use scoped whoami and structured output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
77
to
79
| const isPlainObject = (value: unknown): value is Record<string, unknown> => | ||
| typeof value === "object" && value !== null && !Array.isArray(value); | ||
|
|
|
|
||
| export const renderTerminal = <A>(value: A, renderTerminalValue: (value: A) => string) => | ||
| sanitizeTerminalText(renderTerminalValue(value)); | ||
| redactSensitiveText(renderTerminalValue(sanitizeTerminalValue(value) as A)); |
Comment on lines
206
to
212
| ...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.", |
Comment on lines
+116
to
+130
| 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 |
Contributor
|
🎉 This PR is included in version 1.0.12 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the current
putio-clisecurity hardening findings around installer permissions, terminal control handling, pagination bounds, non-PII agent runtime proofs, and manual release backfill input handling.Changed
0755and reject group/world-writable install directories unless explicitly opted in--fieldspayloads--page-allpagination by pages/items and reject repeated cursorswhoami --fields auth --output jsonRisks
--page-allreads now fail once they exceed the configured page/item guardrailsVerification
vp run verify--fieldspayload with no raw control bytes in stderr755underumask 000.github/workflows/backfill-release-assets.ymlparsed as YAMLComplexity
Reduced: the release/input trust boundaries and runtime safety checks are explicit and covered by focused regression tests.