Skip to content

fix(scan): reject unsafe finding location paths in imported JSON - #866

Open
kevin9327 wants to merge 1 commit into
openai:mainfrom
kevin9327:fix/import-json-path-validation
Open

kevin9327 wants to merge 1 commit into
openai:mainfrom
kevin9327:fix/import-json-path-validation

Conversation

@kevin9327

Copy link
Copy Markdown
Contributor

Summary

codex-security scan import has two input branches in one function. The CSV branch validates each row's path with safeFindingPath, which rejects an absolute path, a drive letter, a backslash, a control character, or a path that walks out of the repository. The JSON branch validates only against findings.schema.json, where locations[].path is {"type": "string", "minLength": 1}.

The same location path is therefore rejected by --csv and accepted by --json. Because --dry-run is documented as "Validate the input without saving a scan", the practical effect is that the dry run reports a document as importable when the import cannot succeed: the real run gets as far as finalization, where the plugin's _require_safe_relative_path rejects the path, and fails with Import finalization did not preserve every input finding; the scan was not completed. — after the source has been persisted under the state directory and a scan registered.

Changes

  • sdk/typescript/src/findings-import.ts: in the JSON branch, check each imported finding's locations[].path with the existing safeFindingPath helper, inside the per-finding loop that already reports the finding index for a duplicate occurrenceId.
  • sdk/typescript/tests-ts/findings-import.test.ts: regression test asserting both formats reject the same unsafe path.

Testing

Reproduced through the CLI, before and after, with the same location path in both formats. The CSV was produced by codex-security export --export-format csv and the JSON is the bundled examples/completed-scan/findings.json; in each the location path was changed to ../../../etc/passwd.

Before, on unmodified main:

$ codex-security scan import --csv escape.csv --dry-run
codex-security: Findings CSV row 2 has an invalid path.
exit=2

$ codex-security scan import --json escape.json --dry-run
dryRun: true
format: json
findingCount: 1
exit=0

Running that same JSON import for real:

$ codex-security scan import --json escape.json --output-dir imp
codex-security: Import finalization did not preserve every input finding; the scan was not completed.
exit=2
$ find state -type f
state/completion-locks/<id>.lock
state/imports/json-<digest>/source.json
state/workbench.sqlite3

New test against unmodified main:

$ bun test --timeout 30000 ./tests-ts/findings-import.test.ts
154 |     ).rejects.toThrow("has an invalid path");
error:
Expected promise that rejects
Received promise that resolved: Promise { <resolved> }
(fail) findings import formats > rejects an unsafe finding location path in either import format
 4 pass
 1 fail

After the change, same two commands:

$ codex-security scan import --csv escape.csv --dry-run
codex-security: Findings CSV row 2 has an invalid path.
exit=2

$ codex-security scan import --json escape.json --dry-run
codex-security: Findings JSON finding 1 has an invalid path.
exit=2

Still accepted, confirmed after the change:

  • scan import --json examples/completed-scan/findings.json --dry-runfindingCount: 1, exit 0
  • scan import --csv <exported findings.csv> --dry-runfindingCount: 1, exit 0

Checks run:

  • bun test --timeout 30000 ./tests-ts/findings-import.test.ts — 5 pass, 0 fail
  • bun test --timeout 30000 ./tests-ts/cli-scan-import.test.ts ./tests-ts/import-scan.test.ts — the same 11 failures before and after the change (diffed the failing-test lists; they are identical). They are pre-existing on this Windows machine: symlink-rejection tests that need symlink creation privilege, plus workbench-database tests.
  • tsc --noEmit (lint) — clean
  • tsc -p tsconfig.ci.json (build:ci) — clean
  • prettier --check on both changed files — All matched files use Prettier code style!

Risk and rollout

No public CLI surface change: no new command, flag, environment variable, or default, and no change to the accepted CSV set. The new error uses the wording and finding-index convention already used by the neighbouring duplicate-occurrenceId check in the same loop.

Compatibility: a findings JSON whose location paths are already repository-relative imports exactly as before. A document that would previously have been accepted by --dry-run and then failed during finalization is now rejected up front, with a message naming the finding, and with nothing persisted. The accepted set does not shrink for any input that could previously complete an import.

Public disclosure review

  • No customer, partner, prospect, or user identities, data, or identifying details are included.
  • No credentials, personal data, private source, scan findings, or nonpublic links or tickets are included.
  • I reviewed the branch name, title, description, commits, changes, comments, logs, screenshots, attachments, and links for public disclosure.

`scan import --csv` refuses a row whose `path` escapes the repository,
is absolute, or carries a drive letter, backslash or control character.
The `--json` branch of the same command validated only against the
findings JSON schema, which constrains `locations[].path` to a non-empty
string, so the same path was accepted.

`--dry-run` therefore reported such a document as importable, and the
real import failed later in finalization with a message about findings
not being preserved, after the source had already been persisted and a
scan registered.

Apply the existing `safeFindingPath` check to imported JSON finding
locations, reporting the finding index the way the duplicate
`occurrenceId` check beside it already does.
@chatgpt-codex-connector

Copy link
Copy Markdown

Note

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@github-actions github-actions Bot added the bug Something isn't working label Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant