openstreetmap-website icon indicating copy to clipboard operation
openstreetmap-website copied to clipboard

Add localhost to allowed http redirect_uris for OAuth

Open spwoodcock opened this issue 1 year ago • 8 comments

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.

spwoodcock avatar Oct 12 '23 10:10 spwoodcock

The reasons for not allowing this are discussed in that ticket - has something changed since then?

tomhughes avatar Oct 12 '23 10:10 tomhughes

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 👍

spwoodcock avatar Oct 12 '23 10:10 spwoodcock

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.

Zverik avatar Oct 14 '23 11:10 Zverik

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 👍

spwoodcock avatar Oct 24 '23 12:10 spwoodcock

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.

tomhughes avatar Oct 24 '23 12:10 tomhughes

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.

spwoodcock avatar Dec 12 '23 15:12 spwoodcock

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.

tomhughes avatar Dec 12 '23 18:12 tomhughes

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!

spwoodcock avatar Dec 14 '23 14:12 spwoodcock