Skip to content

fix(cli): sanitize terminal output - #32

Open
zoya-brd wants to merge 2 commits into
mainfrom
fix/cli-terminal-escape-injection
Open

fix(cli): sanitize terminal output#32
zoya-brd wants to merge 2 commits into
mainfrom
fix/cli-terminal-escape-injection

Conversation

@zoya-brd

@zoya-brd zoya-brd commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

@zoya-brd
zoya-brd marked this pull request as ready for review September 1, 2026 13:55
@zoya-brd
zoya-brd requested a review from artemo-brd September 1, 2026 13:55
@artemo-brd

Copy link
Copy Markdown
Contributor

Please fix the items below:

  • Incomplete coverage. print() is sanitized, but print_table() still writes API-controlled values directly via console.log(). (e.g. discover titles and URLs). Please audit other direct console.log/error and stdout/stderr.write paths, and sanitize untrusted values before applying CLI styling (e.g. success/warn/info/fail functions).

  • Do not sanitize non-TTY stdout. The current change makes -o file preserve escapes while > file and pipes strip them, potentially corrupting raw/CSV/MD/HTML output. Sanitize only when is_tty. Please also transform is_tty to function (const is_tty = () => process.stdout.isTTY === true;) because currently it's not a live check and tests can't flip it after import. This constant is used in other files (spinner.ts, discover.ts, scraper.ts), so it needs to be updated there as well.

  • Add tests for:

  1. sanitized TTY output
  2. preserved piped/redirected output
  3. malicious values passed through print_table()

Also stripVTControlCharacters() doesn't handle standalone controls such as \r, \b, \x07.
Suggested:

const sanitize_terminal = (value: string): string =>
    stripVTControlCharacters(value)
        .replace(/\r\n?/g, '\n')
        .replace(/[\x00-\x08\x0B-\x1F\x7F-\x9F]/g, '');

Normalize carriage return before applying the character class.
In print_table(), sanitize and flatten cells before computing widths to not break table alignment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants