openstreetmap-website
openstreetmap-website copied to clipboard
Add localhost to allowed http redirect_uris for OAuth
Fixes #3613
One line change to add "localhost" to the list of allowed OAuth2 http redirects (along with 127.0.0.1 currently).
As suggested by this comment: https://github.com/openstreetmap/openstreetmap-website/issues/3613#issuecomment-1193399513 the security implications should be minimal.
localhost should resolve to loopback 127.0.0.1 on most systems and the auth code obtained should not be leaked externally. The circumstance where this is in issue is on compromised systems, where the DNS resolution can be manipulated.
The reasons for not allowing this are discussed in that ticket - has something changed since then?
Oh, my apologies @tomhughes - the discussion didn't seem resolved to me & the ticket was left open, so I thought I would PR 😅
If there are security implications that are too great to reconcile then feel free to close 👍
I see the reasons are very vague — in my opinion, it's really far-fetched to assume the localhost address would be redirected. Like, OSM token theft would be the least worry then.
Further context to understand the use case
- I am developing an application that uses OSM OAuth for login.
- During development I manage my services in containers.
- There is an Nginx reverse proxy that sits in front and proxies services to domains like:
app.localhost --> frontend
backend.app.localhost --> backend
s3.app.localhost --> minio
service3.app.localhost --> service3
- I need to be able to set the redirect URL in the OpenStreetMap settings to
http://app.localhost/oauth
, which loops back to 127.0.0.1.
Current workaround
- I get around this by binding the port for the app to my host machine localhost, e.g.
-p 7050:7050
. - I then set the redirect URL to http://127.0.0.1:7050.
- The service is accessible via http://app.localhost, but the login redirects to http://127.0.0.1 and it still works.
Steps forward
By the sounds of it there is no interest in proceeding with this PR.
If the extra context has helped clarify why this is useful, then I will happily update the PR to include redirect urls that end in .localhost
.
Otherwise, please close 👍
I wouldn't say there is no interest - clearly there is interest.
The only concern is the potential impact on security.
Even if we allowed localhost
though I don't think we would want to extend that to other names and in a sense what you're trying to do is exactly the kind of thing we want to protect against, with requests being redirected into different environments.
I understand your concerns.
OAuth RFC does in fact recommend not to use localhost, but use 127.0.0.1
instead.
However, due to:
- Improved developer experience (easy, flexible)
- Small attack surface
using the localhost
loopback address for OAuth callbacks during development is a common accepted approach:
Google Microsoft Amazon I'm sure I could find many more.
I updated so that localhost
and 127.0.0.1
are hardcoded, so devs should not get the idea to extend to additional domains in the future.
Was there a reason for fixing the lint error in that way rather than he way it suggested? I assume that was what you meant by "hardcoding" them, though it doesn't really seem any more hardcoded than the previous method.
What we should definitely do is to include ::1
in the list of allowed domains for IPv6 compatibility as mentioned in the RFC.
Apologies - I didn't see the lint error before! I can update if you can post the previous lint message 😄
By 'hardcoding' I meant I just removed the list based approach, so people don't get the idea to add new domains. But as we also want ::1
maybe I can revert that!