Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: adding sarif file extension validation support #217

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

arthurflame
Copy link

As discussed in this PR #183:

  • Parsing the report as JSON
  • Validating that schema is specified
  • Validating that the content of the .sarif file matches the schema

Content of .sarif files was taken from these samples.

pkg/filetype/file_type.go Outdated Show resolved Hide resolved
pkg/validator/sarif.go Outdated Show resolved Hide resolved
pkg/validator/sarif.go Outdated Show resolved Hide resolved
pkg/validator/sarif.go Outdated Show resolved Hide resolved
pkg/validator/sarif.go Outdated Show resolved Hide resolved
pkg/validator/sarif.go Outdated Show resolved Hide resolved
* schemaUrl variable changed to schemaURL
* error formatting when schema is absent, and when schema isn't valid
* validation that schema is a string
* formatError function refactor, typo fix, and return multiple errors (if applicable)
@kehoecj kehoecj added OSS Community Contribution Contributions from the OSS Community waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers labels Dec 3, 2024
@kehoecj kehoecj self-requested a review December 10, 2024 15:31
return false, errors.New("error - schema isn't a string")
}

loadedSchema := gojsonschema.NewReferenceLoader(schemaURL.(string))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will fail in airgapped environments or corporate networks without a proxy. I think we need a separate error if it can't download the schema from the URL. The current error is error: error - schema isn't valid: https://docs.oasis-open.org/sarif/sarif/v2.1.0/errata01/os/schemas/sarif-schema-2.1.0.json which is confusing

Copy link
Author

Choose a reason for hiding this comment

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

That's a great point.
What do you think if we add an additional function that will verify that the schema's hostname is resolvable and try to establish a TCP connection prior to loading it?

If it's not resolvable or the connection can't be established, we might return with an error that indicates that schema couldn't be loaded.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think if we add an additional function that will verify that the schema's hostname is resolvable and try to establish a TCP connection prior to loading it? If it's not resolvable or the connection can't be established, we might return with an error that indicates that schema couldn't be loaded.

I like that approach

@kehoecj kehoecj added pr-action-requested PR is awaiting feedback from the submitting developer and removed waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers labels Dec 10, 2024
Copy link
Collaborator

@kehoecj kehoecj left a comment

Choose a reason for hiding this comment

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

@arthurflame Looks good! Please take care of the golint-ci findings and resolve conflicts from the dependency changes in main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OSS Community Contribution Contributions from the OSS Community pr-action-requested PR is awaiting feedback from the submitting developer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants