google / osv.dev

Open source vulnerability DB and triage service.
https://osv.dev
Apache License 2.0
1.53k stars 188 forks source link

Clarify that `ecosystem` must be omitted when querying with a `purl` #1443

Closed sschuberth closed 1 year ago

sschuberth commented 1 year ago

I was wondering why a batch query fails with HTTP 400 (bad request) as soon as I specify a purl in addition to the ecosystem. For a batch query, the response message is empty, for for a single query like

curl -d '{"version": "1.4", "package": {"name": "com.jfinal:jfinal", "ecosystem": "Maven", "purl": "pkg:maven/com.jfinal/jfinal@1.4"}}' "https://api.osv.dev/v1/query"

I do get the hint

{"code":3,"message":"Ecosystem specified in a purl query"}

I believe several things could be improved here:

andrewpollock commented 1 year ago

https://github.com/google/osv.dev/blob/d615cac32ba42fff41e39facaf9b8d3f45a80d21/gcp/api/server.py#L814 is where this happens, and that was only recently added by @another-rex in https://github.com/google/osv.dev/pull/1354

another-rex commented 1 year ago

We are disallowing ecosystem in purl queries because of the reason you specified, it's unclear what should be done if they are different.

Firstly, https://ossf.github.io/osv-schema/#affectedpackage-field says "This field [purl] is optional but recommended", but does not say that ecosystem must be omitted at the same time.

Also, the name is also part of the purl, but the name must still be specified.

I think there's some confusion here between the osv schema and the osv.dev API. The schema is specifying requirements for the publication of OSV entries, where the purl field is recommended but optional.

The API docs however are located here: https://google.github.io/osv.dev/post-v1-query/, where it says:

Attribute Type Description
purl string The package URL for this package. If purl is used to specify the package, name and ecosystem should not be set.

Currently we don't error if name is in the query alongside the PURL, though it has the same problem of it might be different from the PURL name, so it's probably a good thing to check and return 400 for as well.

@hayleycd Can we add an additional link to the API doc on the README, it's not clear right now that the readme docs link is associated with the API.

sschuberth commented 1 year ago

We are disallowing ecosystem in purl queries because of the reason you specified, it's unclear what should be done if they are different.

Would it maybe make sense to instead always prefer a PURL if specified, and disregard name and ecosystem then, without giving an error?

oliverchang commented 1 year ago

We believe it's best practice to break loudly in such cases to let users know of potential issues with the way they're generating these query parameters, rather than try to be too clever with guessing what the user intends. This may also lead to more confusion in the future.

Thanks for the feedback -- as @another-rex it does seem like we're lacking some clear API documentation links from the root GitHub README. We'll get that added.

sschuberth commented 1 year ago

We believe it's best practice to break loudly in such cases

it does seem like we're lacking some clear API documentation links

Then, in addition to the API docs, as mentioned you should also break if a name is specified in addition to a purl.

Edit: Actually same for the version, too.

oliverchang commented 1 year ago

Thanks, created https://github.com/google/osv.dev/issues/1451 to track them.

version is a bit more nuanced -- it's valid to provide a PURL without a version in the PURL itself and separately provide the version as a separate parameter.

goneall commented 10 months ago

@oliverchang - It would be nice to also document version even though it is a bit nuanced. It took me a while to figure out why I was getting the 400 responses that was due to the versions being specified in the purl and in the JSON API.

oliverchang commented 10 months ago

Thanks for the feedback @goneall ! I've opened https://github.com/google/osv.dev/issues/1900 for the documentation fix.