Skip to content

Commit

Permalink
Normalize subpath redirects on page visit
Browse files Browse the repository at this point in the history
This fixes an edge case in the FlaskClient (werkzeug.test.Client), used in the Grip.render method, that
results in different behavior when calling client.get() with an argument that doesn't have a leading slash.
This behaves differently on newer versions of Flask/Werkzeug (likely more strict, which is for the better).

This edge case was discovered by a test (namely, testing the '/x/../' route - line 262 in test_api.py). The
path gets normalized to '.' since it's relative (then resolves to a path or file in the current directory).
On older versions, this would ultimately redirect to '/' and render the specified README and pass the test.
In later versions, this the test client would respond with the 404 page since '.' doesn't match any routes.

This fix smooths over the difference in behavior by adding the leading slash (making it an absolute path
and therefore, will properly match one of the defined routes). This passes the test in both new and old
versions of Flask and Werkzeug.

A future release may require leading slashes in routes passed to Grip.render to prevent this edge case from
leaking and causing surprises in external uses of the function.
  • Loading branch information
joeyespo committed Mar 30, 2022
1 parent b915078 commit 3949fa5
Showing 1 changed file with 11 additions and 2 deletions.
13 changes: 11 additions & 2 deletions grip/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,15 @@ def __init__(self, source=None, auth=None, renderer=None,
self._render_rate_limit_page)
self.errorhandler(403)(self._render_rate_limit_page)

def _redirect_to_subpath(self, subpath=None):
"""
Redirects to the specified subpath, which is the relative path
part of the root location (i.e. the current working directory
or the path part of a URL excluding the initial '/').
"""
route = posixpath.normpath('/' + (subpath or '').lstrip('/'))
return redirect(route)

def _render_asset(self, subpath):
"""
Renders the specified cache file.
Expand All @@ -163,7 +172,7 @@ def _render_page(self, subpath=None):
# Normalize the subpath
normalized = self.reader.normalize_subpath(subpath)
if normalized != subpath:
return redirect(normalized)
return self._redirect_to_subpath(normalized)

# Read the Readme text or asset
try:
Expand Down Expand Up @@ -217,7 +226,7 @@ def _render_refresh(self, subpath=None):
# Normalize the subpath
normalized = self.reader.normalize_subpath(subpath)
if normalized != subpath:
return redirect(normalized)
return self._redirect_to_subpath(normalized)

# Get the full filename for display
filename = self.reader.filename_for(subpath)
Expand Down

0 comments on commit 3949fa5

Please sign in to comment.