Skip to content

fix: encode SARIF artifact URIs per RFC 3986 and route batch progress to stderr - #789

Open
dajiaohuang wants to merge 3 commits into
NVIDIA:mainfrom
dajiaohuang:fix/sarif-uri-encoding-and-batch-stderr
Open

dajiaohuang wants to merge 3 commits into
NVIDIA:mainfrom
dajiaohuang:fix/sarif-uri-encoding-and-batch-stderr

Conversation

@dajiaohuang

Copy link
Copy Markdown

Summary

Two minimal fixes:

  1. Fix SARIF artifact URIs do not encode literal filename characters #757: Encode file paths in SARIF artifactLocation.uri using urllib.parse.quote to handle special characters (spaces, unicode, etc.) per RFC 3986. This ensures SARIF consumers correctly parse artifact URIs that contain reserved or non-ASCII characters.

  2. Fix Batch scanner mixes progress messages into JSON stdout #753: Route all batch scanner progress, header, and status messages to stderr so JSON/markdown output on stdout remains machine-parseable. Previously, progress lines like [1/10] skill-name → 80/100 HIGH (2 issues) were printed to stdout, corrupting piped JSON output.

Testing

  • Python syntax verification passed for both modified files
  • Changes are minimal and targeted:
    • src/skillspector/nodes/report.py: Added urllib.parse.quote import + URI encoding in _sarif_artifact_location
    • contrib/batch_scan/batch_scan.py: Added file=sys.stderr to all progress/status _print calls

Signed-off-by: dajiaohuang dajiaohuang@bytedance.com

… to stderr

- Fix NVIDIA#757: Encode file paths in SARIF artifactLocation.uri using urllib.parse.quote
  to handle special characters (spaces, unicode, etc.) per RFC 3986
- Fix NVIDIA#753: Route all batch scanner progress, header, and status messages to stderr
  so JSON/markdown output on stdout remains machine-parseable

Signed-off-by: dajiaohuang <dajiaohuang@bytedance.com>

@yashrajp22 yashrajp22 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Two paths in these fixes still need attention: batch diagnostics with Rich installed, and SARIF notification locations. Regular/suppressed finding URI encoding and the plain stderr path work.

Verified this commit with fresh BASE/HEAD wheels, source/install identity checks, 48 sample scans, 12 focused SARIF scans, stream/exit probes, and selected existing SARIF/batch tests. Existing tests: BASE 178 passed per mode; HEAD 177 passed and 1 failed per mode. The failure expects the old plain-output error stream and needs its assertion updated.

One BASE/source Markdown batch invocation recorded a worker timeout; I have not attributed it to this PR or claimed complete batch parity. These offline checks do not establish live-provider behavior or global detection accuracy.

f"{len(skill_dirs)} skill(s) in [dim]{display(root)}[/dim]"
f" ([cyan]{args.workers} workers[/cyan]{pool_note})\n"
f" ([cyan]{args.workers} workers[/cyan]{pool_note})\n",
file=sys.stderr,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could you make _print honor file=sys.stderr when Rich is available? Its Rich branch drops the file argument and uses the stdout-backed Console. With the normal Rich dependency installed, I reproduced --format json emitting the banner/progress before the JSON with empty stderr, so json.loads(stdout) still fails; Markdown has the same prefix. Please route diagnostics through a stderr-backed console and cover both Rich/plain branches. The existing plain-error test also needs to read stderr.

occurrence = occurrence or {}
file_path = str(occurrence.get("file", finding.file)).replace("\\", "/").lstrip("/")
raw_file_path = str(occurrence.get("file", finding.file)).replace("\\", "/").lstrip("/")
file_path = quote(raw_file_path, safe="/")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could you apply this encoding to invocation notification locations too? _build_sarif.notification_from_exception still constructs SarifArtifactLocation(uri=path) directly. Scanning a malformed file named broken#old.zip produces error/warning notifications whose URI is still broken#old.zip, so SARIF consumers treat old.zip as a fragment instead of part of the filename. I reproduced this from source and the installed wheel; regular finding URIs are correctly encoded. Please use the same path-normalization/encoding policy for notification locations.

Signed-off-by: dajiaohuang <mikewushuwen@outlook.com>
@dajiaohuang

Copy link
Copy Markdown
Author

Addressed both requested paths. _print now honors file= with Rich and the plain fallback; the old error test expects stderr, and the batch tests require JSON and Markdown stdout to remain clean in both formatter branches. SARIF finding and notification artifact locations now share path normalization/percent-encoding, with #, spaces, and backslashes covered.\n\nValidation: 13 focused tests passed on Windows; Ruff lint passed for the changed files. Ruff format reports existing formatting differences elsewhere in contrib/batch_scan/batch_scan.py; I left those unrelated lines unchanged.

Signed-off-by: dajiaohuang <mikewushuwen@outlook.com>
@dajiaohuang

Copy link
Copy Markdown
Author

Updated the nested archive inspection-limit regression to assert the percent-encoded URI in the SARIF notification location, while retaining the raw display-path assertion for terminal, JSON and Markdown. This addresses the sole unit-test failure in run 37672509169.

Validation: 7 relevant report tests passed; Ruff check src/ tests/ passed; formatting check for the changed test passed. The full test suite and live-provider integration tests were not rerun locally. Current-head CI and re-review remain pending.

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.

SARIF artifact URIs do not encode literal filename characters Batch scanner mixes progress messages into JSON stdout

2 participants