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

request.url and request.scheme should obey force_https_urls config setting #781

Closed
simonw opened this issue May 28, 2020 · 3 comments
Closed
Labels

Comments

@simonw
Copy link
Owner

simonw commented May 28, 2020

I'm trying to get the https://www.niche-museums.com/browse/feed.atom feed to validate and I git this from https://validator.w3.org/feed/check.cgi?url=https%3A%2F%2Fwww.niche-museums.com%2Fbrowse%2Ffeed.atom

This feed is valid, but interoperability with the widest range of feed readers could be improved by implementing the following recommendations.

line 6, column 73: Self reference doesn't match document location [help]

<link href="http://www.niche-museums.com/browse/feed.atom" rel="self"/>

I tried to fix this using force_https_urls (commit) but it didn't work - because that setting isn't respected by the Request class:

class Request:
def __init__(self, scope, receive):
self.scope = scope
self.receive = receive
@property
def method(self):
return self.scope["method"]
@property
def url(self):
return urlunparse(
(self.scheme, self.host, self.path, None, self.query_string, None)
)
@property
def scheme(self):
return self.scope.get("scheme") or "http"

@simonw simonw added the bug label May 28, 2020
@simonw
Copy link
Owner Author

simonw commented May 28, 2020

I think the right way to fix this is to modify the scope["scheme"] ASGI key before the Request object is constructed - otherwise that Request class will have to gain knowledge of Datasette's configuration mechanism.

@simonw
Copy link
Owner Author

simonw commented May 28, 2020

I'm inclined to do this at the earliest possible moment. I think that's probably in the DatasetteRouter class, which should see every scope first and be able to apply that setting to it. It already has some special case logic to deal with the base_url setting here:

datasette/datasette/app.py

Lines 779 to 789 in 40885ef

class DatasetteRouter(AsgiRouter):
def __init__(self, datasette, routes):
self.ds = datasette
super().__init__(routes)
async def route_path(self, scope, receive, send, path):
# Strip off base_url if present before routing
base_url = self.ds.config("base_url")
if base_url != "/" and path.startswith(base_url):
path = "/" + path[len(base_url) :]
return await super().route_path(scope, receive, send, path)

@simonw simonw closed this as completed in 7bb30c1 May 28, 2020
simonw added a commit to simonw/museums that referenced this issue May 28, 2020
@simonw
Copy link
Owner Author

simonw commented May 28, 2020

simonw added a commit that referenced this issue Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant