From d0cbe01da720a7a01084e0078664a99c56071f84 Mon Sep 17 00:00:00 2001 From: Andrew Pollock Date: Tue, 12 Nov 2024 06:39:46 +0000 Subject: [PATCH] feat(frontend): improved UX for missing vulnerabilities This commit modifies the 404 flow so that: - when a request for a non-existent vulnerability is received, the 404 page reflects the nature of the failed request - when it is known that the vulnerability failed to import, this is explicitly surfaced A new FAQ entry discussing this is also added. Additionally, the 404 generation was tidied up to make it easier to understand where the 404 comes from. There were many unreachable paths that called `abort()`, which made the code unnecessarily unreadable. It also addresses requests for `/favicon.ico` generating 404's from the redirector by adding an explicit handler for it. Part of #2190 --- docs/faq.md | 10 ++++ .../frontend3/src/templates/404.html | 22 ++++++++ .../frontend3/src/templates/list.html | 2 +- gcp/appengine/frontend_handlers.py | 52 +++++++++++++------ 4 files changed, 70 insertions(+), 16 deletions(-) diff --git a/docs/faq.md b/docs/faq.md index 3de099cdab4..dac8b0dff2b 100644 --- a/docs/faq.md +++ b/docs/faq.md @@ -91,6 +91,16 @@ If you have any questions, please feel free to create an issue! ## Data +### Why isn't a record available? + +If you are unable to retrieve a record you are expecting to be able to find, it +may be because it has not imported successfully, due to there being a quality issue with it. + +If a record has not met [OSV.dev's quality bar](data_quality.md), the 404 page +for the vulnerability will state this. + +You should raise this with the home database responsible for the record. + ### I've found something wrong with the data Data quality is very important to us. Please remember that OSV.dev is an diff --git a/gcp/appengine/frontend3/src/templates/404.html b/gcp/appengine/frontend3/src/templates/404.html index 76186370223..b1ab3107a5f 100644 --- a/gcp/appengine/frontend3/src/templates/404.html +++ b/gcp/appengine/frontend3/src/templates/404.html @@ -12,6 +12,27 @@ {% block top_bar %} {% endblock %} {% block content %} +{% if vuln_id %} +
+
+
+

{{ vuln_id }} Not Found!

+

+ {% if has_importfindings %} + The record has not imported successfully from its source
+ {% endif %} + Please see our + FAQ for more information. +

+
+ Back to Home +
+
+
+
+
+{% else %}
@@ -29,4 +50,5 @@

Oops! Page Not Found!

+{% endif %} {% endblock %} diff --git a/gcp/appengine/frontend3/src/templates/list.html b/gcp/appengine/frontend3/src/templates/list.html index 6f9dedf324f..d1ead29022a 100644 --- a/gcp/appengine/frontend3/src/templates/list.html +++ b/gcp/appengine/frontend3/src/templates/list.html @@ -128,7 +128,7 @@

Vulnerabilities

{%- endfor -%} {%- if vulnerabilities | length == 0 -%} - No results + No results (check our FAQ if this is unexpected) {%- endif -%} {%- if page < total_pages -%} diff --git a/gcp/appengine/frontend_handlers.py b/gcp/appengine/frontend_handlers.py index a0b0dc2a9cb..753cad7fed3 100644 --- a/gcp/appengine/frontend_handlers.py +++ b/gcp/appengine/frontend_handlers.py @@ -55,6 +55,19 @@ _DEPS_BASE_URL = 'https://deps.dev' _FIRST_CVSS_CALCULATOR_BASE_URL = 'https://www.first.org/cvss/calculator' + +class VulnerabilityNotFound(exceptions.NotFound): + """Plumb the vulnerability ID not found to the error handler.""" + + def __init__(self, vuln_id: str) -> None: + super().__init__() + self.vuln_id = vuln_id + + +class VulnerabilityNotImported(VulnerabilityNotFound): + """Plumb the vulnerability ID with import findings to the error handler.""" + + if utils.is_prod(): redis_host = os.environ.get('REDISHOST', 'localhost') redis_port = int(os.environ.get('REDISPORT', 6379)) @@ -126,6 +139,11 @@ def robots(): return response +@blueprint.route('/favicon.ico') +def favicon(): + return current_app.send_static_file('img/favicon-32x32.png') + + @blueprint.route('/blog/', strict_slashes=False) def blog(): return render_template('blog.html', index=_load_blog_content('index.html')) @@ -251,15 +269,13 @@ def vulnerability_redirector(potential_vuln_id): # AlmaLinux have colons in their identifiers, which gets URL encoded. potential_vuln_id = parse.unquote(potential_vuln_id) if not _VALID_VULN_ID.match(potential_vuln_id): - abort(404) + abort(400) return None - vuln = osv_get_by_id(potential_vuln_id) - if vuln: - return redirect(f'/vulnerability/{potential_vuln_id}') + # This may raise an exception directly or via abort() for failed retrievals. + _ = osv_get_by_id(potential_vuln_id) - abort(404) - return None + return redirect(f'/vulnerability/{potential_vuln_id}') @blueprint.route('/.json') @@ -269,13 +285,11 @@ def vulnerability_json_redirector(potential_vuln_id): https://api.osv.dev/v1/vulns/VULN-ID. """ if not _VALID_VULN_ID.match(potential_vuln_id): - abort(404) + abort(400) return None - vuln = osv_get_by_id(potential_vuln_id) - if not vuln: - abort(404) - return None + # This calls abort() on failed retrievals. + _ = osv_get_by_id(potential_vuln_id) if utils.is_prod(): api_url = 'api.osv.dev' @@ -524,12 +538,15 @@ def osv_get_by_id(vuln_id): bug = osv.Bug.get_by_id(vuln_id) if not bug: - abort(404) - return None + if osv.ImportFinding.get_by_id(vuln_id): + # abort(404) is too simplistic for this. + raise VulnerabilityNotImported(vuln_id) + # abort(404) is too simplistic for this. + raise VulnerabilityNotFound(vuln_id) if bug.status == osv.BugStatus.UNPROCESSED: - abort(404) - return None + # abort(404) is too simplistic for this. + raise VulnerabilityNotFound(vuln_id) if not bug.public: abort(403) @@ -705,6 +722,11 @@ def list_packages(vuln_affected: list[dict]): @blueprint.app_errorhandler(404) def not_found_error(error: exceptions.HTTPException): logging.info('Handled %s - Path attempted: %s', error, request.path) + if isinstance(error, VulnerabilityNotFound): + return render_template( + '404.html', + vuln_id=error.vuln_id, + has_importfindings=isinstance(error, VulnerabilityNotImported)), 404 return render_template('404.html'), 404