fix(report): check the URI before converting it to a string - #11064
Closed
Eljees wants to merge 1 commit into
Closed
fix(report): check the URI before converting it to a string#11064Eljees wants to merge 1 commit into
Eljees wants to merge 1 commit into
Conversation
`toUri` logs the error and returns nil when `url.Parse` rejects its input, but `addSarifResult` dereferenced the returned pointer without a check, so writing a SARIF report panicked with a nil pointer dereference. `url.Parse` rejects any path that contains a bare `%` (e.g. `100%.tf` -> `invalid URL escape "%.t"`), so scanning a repository that has such a file crashed `--format sarif`. The sibling field `data.url` was already guarded a few lines above; this moves both conversions to a small `uriString` helper so the two are handled the same way. Closes aquasecurity#8154
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
toUrilogs the error and returnsnilwhenurl.Parserejects its input, butaddSarifResultdereferenced that pointer without a check, so writing a SARIF report panicked.url.Parserejects any path that contains a bare%(100%.tf->invalid URL escape "%.t"), so scanning a repository that happens to contain such a file crashes--format sarif.The sibling field
data.urlwas already guarded a few lines above (sarif.go:91); this routes both conversions through a smalluriStringhelper so the two are handled the same way. Behaviour for parsable URIs is unchanged.Both call sites of
addSarifResultwere affected, the vulnerability branch (sarif.go:160) and the misconfiguration branch (sarif.go:184); the added test covers both.Before
100%.tfcontaining anaws_s3_bucketresource, thentrivy fs --scanners misconfig --format sarif .:After
Same command, exit code 0, valid SARIF:
One point I would like your opinion on: an unparsable path now produces an empty
artifactLocation.uri, and the raw path survives only in the locationmessage. The alternative is to fall back to an escaped path (&url.URL{Path: str}, which renders100%.tfas100%25.tf) so the location is preserved. I kept the minimal change because the issue describes "log this and return nil" as the intended contract, but I am happy to switch to the fallback if you prefer it.Related issues
Checklist