Skip to content

Commit

Permalink
feat(frontend): improved UX for missing vulnerabilities
Browse files Browse the repository at this point in the history
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
  • Loading branch information
andrewpollock committed Nov 12, 2024
1 parent d58c1f6 commit d0cbe01
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 16 deletions.
10 changes: 10 additions & 0 deletions docs/faq.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
22 changes: 22 additions & 0 deletions gcp/appengine/frontend3/src/templates/404.html
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,27 @@
{% block top_bar %} {% endblock %}

{% block content %}
{% if vuln_id %}
<div class="mdc-layout-grid not-found-page">
<div class="mdc-layout-grid__inner">
<div class="mdc-layout-grid__cell--span-12 text-info">
<h2 class="heading">{{ vuln_id }} Not Found!</h2>
<p class="description">
{% if has_importfindings %}
The record has not imported successfully from its source<br/>
{% endif %}
Please see our
<a href="https://google.github.io/osv.dev/faq/">FAQ</a> for more information.
</p>
<div class="cta">
<a class="cta-primary link-button" href="{{ url_for('frontend_handlers.index') }}"
aria-label="Get me back Home!">Back to Home</a>
</div>
</div>
</div>
<footer></footer>
</div>
{% else %}
<div class="mdc-layout-grid not-found-page">
<div class="mdc-layout-grid__inner">
<div class="mdc-layout-grid__cell--span-12 text-info">
Expand All @@ -29,4 +50,5 @@ <h2 class="heading">Oops! Page Not Found!</h2>
</div>
<footer></footer>
</div>
{% endif %}
{% endblock %}
2 changes: 1 addition & 1 deletion gcp/appengine/frontend3/src/templates/list.html
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ <h1 class="title">Vulnerabilities</h1>
</div>
{%- endfor -%}
{%- if vulnerabilities | length == 0 -%}
<span class="no-results">No results</span>
<span class="no-results">No results (check our <a href="https://google.github.io/osv.dev/faq/">FAQ</a> if this is unexpected)</span>
{%- endif -%}
{%- if page < total_pages -%} <turbo-frame id="vulnerability-table-page{{ page + 1 }}"
data-turbo-action="advance" target="_top" class="next-page-frame">
Expand Down
52 changes: 37 additions & 15 deletions gcp/appengine/frontend_handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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'))
Expand Down Expand Up @@ -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}')

Check warning

Code scanning / CodeQL

URL redirection from remote source Medium

Untrusted URL redirection depends on a
user-provided value
.


@blueprint.route('/<potential_vuln_id>.json')
Expand All @@ -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'
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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


Expand Down

0 comments on commit d0cbe01

Please sign in to comment.