Skip to content

fix(output): prevent CSV formula injection - #31

Merged
zoya-brd merged 7 commits into
mainfrom
fix/csv-formula-injection
Sep 16, 2026
Merged

zoya-brd merged 7 commits into
mainfrom
fix/csv-formula-injection

Conversation

@zoya-brd

@zoya-brd zoya-brd commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator
  • Adds CSV formula-injection protection across CLI CSV output.
  • Adds global sanitize_csv config, enabled by default.
  • Sanitizes formula-like CSV cells while preserving numeric values.
  • Supports enabling/disabling with brightdata config set sanitize_csv true|false.
  • Handles pipeline CSV safely by parsing the API-generated CSV, sanitizing each cell, and re-serializing it.
  • Preserves the original UTF-8 BOM when re-serializing pipeline CSV.
  • Returns an actionable error if server-generated CSV cannot be parsed.
  • Normalizes persisted sanitize_csv string values and rejects unsupported values.
  • Covers stdout, -o file.csv, config persistence, and invalid config values with regression tests.
  • Keeps the existing server-generated CSV structure instead of rebuilding CSV from JSON.

@zoya-brd
zoya-brd requested a review from artemo-brd August 31, 2026 09:10
Comment thread src/utils/output.ts Outdated
};

const sanitize_csv_cell = (s: string): string=>{
if (/^[=+\-@\t\r]/.test(s))

@artemo-brd artemo-brd Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a workaround for formula injection if there is a space before a formula like " =cmd|'/c calc'!A0" . Please detect dangerous prefixes after leading whitespace without removing that whitespace from the exported value. For example:

if (/^[\s\u00a0]*[=+\-@]/.test(s))
    return "'" + s;

Please add tests for regular spaces, tabs, and NBSP.

This also changes numeric-looking string values such as "-100" and "+15". Is that intentional? If we want to preserve them, please define a strict numeric-string exemption and add tests. Otherwise, we should document that sanitization intentionally modifies all string cells starting with + or -.

Comment thread src/utils/output.ts
return JSON.stringify(data, null, 2);
};

const print = (data: unknown, opts: Print_opts = {})=>{

@artemo-brd artemo-brd Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/commands/dataset.ts -> handle_pipelines:

handle_pipelines() requests CSV from the API, so the result is already a string. Without -o, print() treats it as raw because format is not passed. With -o file.csv, serialize_csv() still returns the string unchanged before csv_escape.

Passing format to print() alone therefore won't fix the issue. We should choose:

  • request structured JSON and serialize it locally when CSV output is requested, or
  • parse the returned CSV and sanitize/re-serialize every cell.

Please add regression tests for both stdout redirection and -o file.csv

Comment thread src/utils/output.ts
return JSON.stringify(val);
};

const sanitize_csv_cell = (s: string): string=>{

@artemo-brd artemo-brd Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a global sanitize_csv configuration option that users can set with:

brightdata config set sanitize_csv false
brightdata config set sanitize_csv true

It should default to true. Since config values currently enter through the CLI as strings, please validate and convert "true"/"false" to actual booleans when setting or reading this option; "false" must not be treated as a truthy string. Other values should be rejected.

We can add per-command overrides later if needed (scrape, search, pipelines, discover, scraper, browser),

@artemo-brd artemo-brd changed the title fix(output): prevent CVS formula injection fix(output): prevent CSV formula injection Aug 31, 2026
Comment thread src/utils/output.ts Outdated
const csv_escape = (val: unknown): string=>{
const s = cell_to_string(val);
let s = cell_to_string(val);
if (typeof val == 'string' && get_config('sanitize_csv') !== false)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please refactor it to not call get_config('sanitize_csv') for every cell (it reads config from the disk every time). It's better to read the config just once

Comment thread src/utils/output.ts Outdated
if (!sanitize || !csv)
return csv;
const rows = parse(csv, {
bom: true,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parse(..., {bom: true}) strips the input UTF-8 BOM, while stringify() does not restore it. This can regress Excel compatibility for pipeline CSV containing non-ASCII data. Please detect and preserve the BOM when re-serializing.

example:

const has_bom = csv.charCodeAt(0) == 0xFEFF;

return stringify(sanitized, {
    bom: has_bom,
});

Comment thread src/utils/config.ts Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setter stores a boolean correctly, but load() still accepts "sanitize_csv": "false" as a string. Since "false" !== false, sanitization remains enabled, contrary to the configured value. Please normalize "true"/"false" while loading, or reject invalid non-boolean values, and add tests for persisted string values.

Comment thread src/commands/dataset.ts Outdated
Comment on lines +290 to +294
const cleaned_result = format == 'json'
? strip_nulls(result)
: format == 'csv' && typeof result == 'string'
? sanitize_serialized_csv(result)
: result;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-blocking:

Maybe replace it with if to make it more readable

Suggested change
const cleaned_result = format == 'json'
? strip_nulls(result)
: format == 'csv' && typeof result == 'string'
? sanitize_serialized_csv(result)
: result;
let cleaned_result = result;
if (format == 'json')
cleaned_result = strip_nulls(result);
else if (format == 'csv' && typeof result == 'string')
cleaned_result = sanitize_serialized_csv(result);

Comment thread src/utils/output.ts Outdated
Comment thread src/commands/config.ts
@zoya-brd
zoya-brd requested a review from artemo-brd September 14, 2026 06:55

@artemo-brd artemo-brd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@zoya-brd
zoya-brd merged commit 2a869eb into main Sep 16, 2026
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