Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement redirects from old % encoding to new dash encoding #1650

Closed
simonw opened this issue Mar 6, 2022 · 5 comments
Closed

Implement redirects from old % encoding to new dash encoding #1650

simonw opened this issue Mar 6, 2022 · 5 comments

Comments

@simonw
Copy link
Owner

simonw commented Mar 6, 2022

One big advantage to this scheme is that redirecting old links to %2F pages (e.g. https://fivethirtyeight.datasettes.com/fivethirtyeight/twitter-ratio%2Fsenators) is easy - if you see a % in the raw_path, redirect to that page with the % replaced by -.

Originally posted by @simonw in #1439 (comment)

@simonw
Copy link
Owner Author

simonw commented Mar 7, 2022

This is a bit tricky.

I tried this, sending a redirect only if a 404 happens:

diff --git a/datasette/app.py b/datasette/app.py
index 8c5480c..420664c 100644
--- a/datasette/app.py
+++ b/datasette/app.py
@@ -1211,6 +1211,10 @@ class DatasetteRouter:
         return await self.handle_404(request, send)
 
     async def handle_404(self, request, send, exception=None):
+        # If path contains % encoding, redirect to dash encoding
+        if '%' in request.scope["path"]:
+            await asgi_send_redirect(send, request.scope["path"].replace("%", "-"))
+            return
         # If URL has a trailing slash, redirect to URL without it
         path = request.scope.get(
             "raw_path", request.scope["path"].encode("utf8")

But this URL didn't work:

I was expecting that to redirect to this page:

But instead it took me to another 404:

This is because that URL contains both a %-escaped / AND a plain - - which was not escaped in the old system but is escaped in the new system.

@simonw
Copy link
Owner Author

simonw commented Mar 7, 2022

This doesn't seem to work.

https://latest.datasette.io/fixtures/table%2Fwith%2Fslashes.csv should be redirecting now that this is deployed - which it is, because https://latest.datasette.io/-/versions shows 644d25d - but I'm not getting that redirect.

@simonw simonw reopened this Mar 7, 2022
@simonw
Copy link
Owner Author

simonw commented Mar 7, 2022

Same problem here: https://fivethirtyeight.datasettes.com/fivethirtyeight/ahca-2Dpolls%2Fahca_polls should redirect but doesn't.

@simonw
Copy link
Owner Author

simonw commented Mar 7, 2022

The problem seems to be that http://127.0.0.1:8002/fixtures/table%2Fwith%2Fslashes.csv doesn't result in a 404 at all. If it did, it would be redirected.

@simonw
Copy link
Owner Author

simonw commented Mar 7, 2022

Here's the problem:

def dash_decode(s: str) -> str:
"Decodes a dash-encoded string, so ``-2Ffoo-2Fbar`` -> ``/foo/bar``"
return urllib.parse.unquote(s.replace("-", "%"))

Which is called here:

table, _ext_format = await resolve_table_and_format(
table_and_format=dash_decode(args["table_and_format"]),
table_exists=async_table_exists,
allowed_formats=self.ds.renderers.keys(),
)

So table%2Fwith%2Fslashes ends up decoded as if it was using dash encoding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant