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

[K9VULN-2229] Various SARIF mod code cleanups #565

Merged
merged 2 commits into from
Dec 10, 2024
Merged

[K9VULN-2229] Various SARIF mod code cleanups #565

merged 2 commits into from
Dec 10, 2024

Conversation

jasonforal
Copy link
Collaborator

(For better organization, this PR is split off from one that implement SARIF artifacts).

What problem are you trying to solve?

We'll be adding the run/artifacts property from the SARIF spec in an upcoming PR.

While implementing that, I ran into some minor annoyances:

  • Adding a new property to output broke unrelated tests because those tests hardcode the entire expected SARIF. Ideally, each unit test only targets a specific behavior.
  • For each file, we currently have three different serialized forms (all opaquely using the String type).
    • The path as reported by the walkdir (which will be the OS-native form)
    • The unix path (i.e. on Windows systems, the path from above will be converted to unix format)
    • A percent-encoded path (e.g. converting [ to %5B)
      As-is, these transformations are performed by functions with vague names.

What is your solution?

  • Narrow down each unit test to only test specific behavior.
  • Rename functions to be more descriptive (and a small nit to reduce allocations)

Alternatives considered

What the reviewer should know

  • A better solution for the path issue would be to newtype each of the 3 formats for paths so we can have compile-time safety. However, this is foregone to keep changes as minimal as possible.

@jasonforal jasonforal requested a review from juli1 December 9, 2024 15:19
@jasonforal jasonforal requested a review from a team as a code owner December 9, 2024 15:19
@jasonforal jasonforal requested review from amaanq and removed request for juli1 December 9, 2024 15:22
}
],
}
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a blocker, but I'm not sure if the indentation for the closing paren was expected (if it is for visual distinction from the json punctuation, that's totally fine)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was just keeping it consistent with existing usages of that macro, e.g.

}
}}],
"version":"2.1.0"
}
);
assert_json_eq!(expected_json, sarif_report_to_string);

I agree that it looks kinda weird, but we should handle any formatting changes in a separate PR if desired.

@jasonforal jasonforal merged commit 76e57b0 into main Dec 10, 2024
71 checks passed
@jasonforal jasonforal deleted the K9VULN-2229 branch December 10, 2024 15:57
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.

2 participants