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

Add support to vulnerability aliases #220

Merged
merged 1 commit into from
Jul 11, 2024
Merged

Conversation

macedogm
Copy link
Contributor

@macedogm macedogm commented Jul 9, 2024

This is an initial implementation that adds support to vulnerability aliases as documented in the spec.

Note that support is only added when creating a new Vex document or adding a new statement. It doesn't support filtering or merging based on aliases.

Closes #219

Copy link
Member

@puerco puerco left a comment

Choose a reason for hiding this comment

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

Nice! Thank you @macedogm

Just one nit, can you add a simple validation to vexStatementOptions.Validate() ? I think for now checking we are not duplicating identifiers in vexStatementOptions.Aliases and vexStatementOptions.Vulnerability should be enough. (Oh and also add a test case to options_test.go )

// Validate checks that the statement options are coherent
func (so *vexStatementOptions) Validate() error {
if so.Status != string(vex.StatusAffected) &&
(so.ActionStatement != vex.NoActionStatementMsg && so.ActionStatement != "") {
return errors.New("action statement can only be set when status = \"affected\" ")
}

@macedogm
Copy link
Contributor Author

@puerco thanks for the quick review and comments. Yes, I'll add the improvements that you requested, just a note that I'll have to travel for a few days, so there might be some delay before I update the PR.

@puerco
Copy link
Member

puerco commented Jul 10, 2024

@macedogm if you need this change we can handle the validation in a follow up. If you need this one quickly, just rebase the PR, we merge it and I can add the validation logic :)

@macedogm
Copy link
Contributor Author

@puerco I can wait a few days, no rush on that. It's more up to you if you prefer to merge this one quickly and do the rest on a follow up PR.

@macedogm macedogm force-pushed the add-aliases-flag branch 2 times, most recently from 7b1da57 to 8770741 Compare July 11, 2024 02:36
@puerco
Copy link
Member

puerco commented Jul 11, 2024

Ah the linter is not happy, otherwise LGTM

@macedogm
Copy link
Contributor Author

@puerco hopefully I addressed your comments. PTAL and let me know if the style is as expected.

Additionally, should we consider adding a validation in internal/cmd/add.go to check if an alias being added is repeated with another aliases or vulnerabilities ID already present in the document? Would this validation make sense at the moment?

@macedogm macedogm requested a review from puerco July 11, 2024 03:53
@puerco
Copy link
Member

puerco commented Jul 11, 2024

The vexStatementOptions are embedded in the addOptions, so your change should work in both :)

return errors.Join(
o.vexStatementOptions.Validate(),
o.outFileOption.Validate(),
fileError, docError,
)

Thank you!

@puerco puerco enabled auto-merge July 11, 2024 05:22
@puerco puerco merged commit 4067f8e into openvex:main Jul 11, 2024
10 checks passed
@macedogm macedogm deleted the add-aliases-flag branch July 11, 2024 10:59
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.

Support for vulnerability with multilple aliases
2 participants