upload-sarif: relative paths beginning with `./` are not handled correctly

Offen
#2,829 2 Kommentare 1 Reaktion 0 zugewiesene Personen Auf GitHub ansehen

Dieses Issue hat noch niemand übernommen.

Bewertung

Schwierigkeit
5/5
Geschätzter Aufwand
Über eine Woche
Anfängerfreundlichkeit
25/100
Issue-Typ
Bug
Klarheit
Größtenteils klar
Aktivitätsstatus
Veraltet
Tech-Stack
github-actions
Bereich
security

Rechercherichtung

Beginne mit dem Einstiegspunkt upload-sarif und reproduziere den gemeldeten Unterschied zwischen foo/bar und ./foo/bar, unter Verwendung der verknüpften Hinweise zu SARIF-Pfaden und des Kontexts von RFC 3986. Untersuche, ob sich die relevante Implementierung in diesem Repository oder in GitHubs SARIF-Ingestion-Mechanismus befindet; als abgeschlossen gilt die Aufgabe, wenn relative Verweise konsistent zu Repository-Dateien aufgelöst werden, Vorschauen gerendert werden und in Pull Requests erscheinen.

Vom Indexierungsmodell aus dem Issue-Text verfasst.

Beschreibung

Hello!

I'm reporting what I believe to be a bug in GitHub's SARIF consumption. This bug is probably happening somewhere deeper in GitHub's SARIF consumption machinery but upload-sarif is the main user-facing entrypoint for that machinery, so I'm filing the report here. Please let me know if a better discussion venue exists and I'd be happy to continue this elsewhere 🙂

Description

GitHub's documentation says the following about paths in SARIF inputs:

Code scanning interprets results that are reported with relative paths as relative to the root of the repository analyzed. If a result contains an absolute URI, the URI is converted to a relative URI. The relative URI can then be matched against a file committed to the repository.

Ref: https://docs.github.com/en/code-security/code-scanning/integrating-with-code-scanning/sarif-support-for-code-scanning#specifying-the-root-for-source-files

This, combined with the fact that SARIF stipulates RFC 3986 for artifactLocation URIs suggests that relative paths should be able to include . and .., as these path components are well-defined in RFC 3986.

However, if a SARIF file contains a relative URI like ./foo/bar, GitHub appears to fail to resolve that URL to a path in the repository being scanned. This results in suboptimal finding presentation:

  1. Findings are not rendered on PRs, since GitHub doesn't think the finding URLs match repository paths;
  2. Findings are rendered under "Code scanning alerts" but show "Preview unavailable" instead of a proper synopsis/extraction of the source file.

Here is a screenshot of the above behavior, demonstrating the "Preview unavailable" behavior because the path (./tests/integration/test-data/issue-612-repro/action.yml) starts with a ./:

Image

For contrast, here's a similar finding rendering correctly because it doesn't begin with ./:

Image

Expected behavior

I expect GitHub's SARIF ingestion to handle URIs that begin with (or contain) relative references, since SARIF stipulates RFC 3986 for URIs and RFC 3986 permits relative references. More generally, I believe many code-scanning tools produce relative references (like ./foo) by default and would benefit from not having to specialize their handling for GitHub's SARIF consumer.

Actual behavior

Relative URIs like foo/bar work correctly (resolving relative to the repository root), while relative URIs like ./foo/bar do not work correctly.

Workarounds

The primary workarounds here are:

  1. "Normalize" relative URIs from ./foo/bar to foo/bar. This is possible, but non-trivial in the general case (e.g. ./foo/bar/../baz), and requires more pre-processing on the generated SARIF than the specification stipulates.
  2. Switch entirely to absolute URIs, and use SARIF's features (like invocations[0].workingDirectory.uri) to help GitHub ingest and transform paths into appropriate relative URIs. I've had limited success making this work: the SARIF ingestor appears to be very fickle about absolute paths.

Overall, I think both of these workarounds are non-ideal, and add additional burden to SARIF producers to pre-process their inputs/offer a GitHub specific "quirks mode" beyond what SARIF stipulates. I think it would be fantastic if GitHub could instead support these kinds of relative paths!

Additional context

I ran into this behavior within zizmor, which supports SARIF as an output format and encourages people to use it when integrating with GitHub. Some original issue context: https://github.com/woodruffw/zizmor/issues/604, https://github.com/woodruffw/zizmor/issues/571.

Additionally, I've observed that GitHub's behavior around equivalent relative paths (e.g. foo/bar and ./foo/bar) is somewhat mixed: ./foo/bar doesn't produce a preview, but results for foo/bar and ./foo/bar are deduplicated against each other. That makes me think that some degree of normalization/equivalence checking is happening in GitHub's SARIF processing, just not at the point needed to handle previews correctly.

Please let me know if there's any other information I can provide!

CC @kommendorkapten

Vorherrschende Sprache
TypeScript
Sterne
1.6k
Forks
493
Ø Merge
1 T. 13 Std.
Gemergte PRs (30 T.)
44

Beitragsleitfaden

Beitragsleitfaden öffnen

Erste Schritte

  1. Lesen Sie das ganze Issue und danach den Beitragsleitfaden des Projekts.
  2. Schreiben Sie ins Issue, dass Sie es übernehmen — das erspart doppelte Arbeit.
  3. Forken Sie das Repository und arbeiten Sie in einem Branch.
  4. Öffnen Sie einen Pull Request, der die Issue-Nummer nennt.

Mehr aus github/codeql-action

Alle Issues in github/codeql-action

Ähnliche Issues

Weitere Issues zu TypeScript

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.