briandfoy / cpan-security-advisory

CPAN Security Advisory Database
Artistic License 2.0
22 stars 14 forks source link

darkpan consistency updates #126

Closed garu closed 1 year ago

garu commented 1 year ago

External apps (i.e. not from CPAN) were assigned a CPANSA id as well, so we need to make sure they are all easy to spot and to parse.

This patch nullifies their 'distribution' value, sets the 'darkpan' attribute to true and provides a link to the external resource.

briandfoy commented 1 year ago

I see what you are going for here, but I'm going to think about this for a bit. I think there might be a better way to handle this, and no one has reported any issues where these reports being present have caused a problem. If you have noticed a problem, please not the details of that somewhere.

garu commented 1 year ago

We have noticed it when parsing the data from CPAN::Audit::DB into a json that will be used to add CPANSA information to MetaCPAN. Without a standardized way to identify whether the vulnerability on CPANSA is referring to an actual indexed CPAN module that is visible on MetaCPAN, we need to rely on some complex and error-prone heuristics.

I don't mean to rush you at all, and I agree it may not be the best solution - I still argue that third-party non-CPAN modules like ActivePerl and MT should not get a CPANSA id in the first place, it would make all this be a non-issue - but any decisions that would give us this index in a simple(r) way would allow us to move forward with this still during PTS 2023, when everything moves much faster.

briandfoy commented 1 year ago

FWIW, it's usually better to start with what you are doing and what motivated a change.

I think MetaCPAN should digest the reports directly instead of going through the cumbersome CPAN::Audit::DB. When I have time, that's going to change dramatically. Only CPAN::Audit and its internals should care what that looks like. I don't think any external consumer should depend on it because that means I'd never been able to change it.

I'm completely happy for reports in this repo to have an x-metacpan hash where MetaCPAN can add anything it likes and wants to use. Since only MetaCPAN cares about that key, it doesn't disturb anything else or mess up anything I'm doing now to track CVEs.

garu commented 1 year ago

oh, I thought you knew! We are all here tweaking stuff :)

Being that there are currently only 2 of those (MT and urxvt) under cpansa/, I think a simple tag saying "this is not on CPAN" should suffice. Isn't this what darkpan: true meant? We can use that.

The url bit I added just so people can refer to it, I'll remove it if that's a problem.

I'm guessing what you didn't like about this PR is changing the distribution field no null? If so, I can revert it, it's fine - as long as the issue of "if someone uploads a module to CPAN with that same name and it contains a logged vulnerability, the primary keys will clash" is something that you are willing to cope with if/when the time comes.

briandfoy commented 1 year ago

I've already merged the sperl commits, and I'm going to reject the last one.