On 3/24/21 2:26 PM, Magnus Hagander wrote:
> On Mon, Mar 22, 2021 at 4:43 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:
>>
>> Hi,
>>
>> When we have a release that contains CVEs, we currently link to a CVE
>> authority to display the full details about that CVE. This has presented
>> a few issues:
>>
>> - The CVE authority does not publish the CVE details when the release is
>> made; the window for this happening can vary
>> - As a result, we can't link to that page from the news announcement;
>> when we have in the past, we'll get reports about the URL 404ing
>>
>> This patchset aims to remedy this by creating a page that houses the
>> details about a CVE. It includes the additional description that is
>> provided to the CVE authority and allows for the details to be published
>> as soon as the CVE is published. See attached screenshot.
>>
>> 0001 updates the current CVE ID validator to match what MITRE has put
>> forth on the numbering (7 digits! It does say in places it can be
>> "arbitrary amounts" but the official examples go up to 7 digits), and
>
> This one should probably change the error message as well?
Yeah, though I think there's only one person who reads it at this point.
However, for that one person, I have adjusted the message.
>> 0002 refactors a function we used to generate our internal CVE IDs so it
>> can be used in multiple places, e.g. its use in 0003.
>
> I applaud you for adding what may be the first docstring in pgweb :)
There's some others that I've added! This may be the first one you caught ;)
> I don't think you need to be consistent with the previous error since
> it's a "never happens" error, you can just let the ValidationError
> through. I also don't mind if you prefer keeping it :)
I prefer brevity, so I removed the reraise and updated the comment.
> 0003
> * can we make the purging a bit more specific? That is only purge the
> actually edited one? See for example how news/ does it.
Done.
> * is there really a need to support case insensitive cve in the URL?
...I'm not quite sure what possessed me there. I think it's the fact
that most sites tend to use the capital letters for CVE, both in the
URLs and the listings, so one copying/pasting would copy that directly.
> We don't support case insensitive URLs anywhere else... I suggest also
> making the URLs we generate ourselves be lowercase, even if we keep
> the insensitivity in the matching
I would suggest the opposite, that we keep it uppercase as this seems
consistent with how the others in the CVE game do it, e.g.
https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2018-10925
https://nvd.nist.gov/vuln/detail/CVE-2018-10925
https://access.redhat.com/security/cve/CVE-2018-1058
I've modified the URL matching to be all uppercase, but keeping our
matching logic case insensitive.
> * The query for "versions" needs a .elect_related('version')
That I do agree with and somehow missed that. Thanks!
> Rest LGTM. (did not review the HTML itself, but since the output looks
> good and has already been approved..)
Cool. Please see updated patches.
Thanks,
Jonathan