search

ring-clojure / ring

Clojure HTTP server abstraction
MIT License
3.75k stars 519 forks source link

Return correct request urls for proxied requests #435

Closed ryfow closed 3 years ago

ryfow commented 3 years ago

We're having problems with wrap-absolute-redirects sending the browser to http: and the underlying problem is that ring.util.request/request-url doesn't take x-forwarded-proto into account. I'm happy to move the change into wrap-absolute-redirects or make this fn multi-arity if you think this is too heavy handed.

weavejester commented 3 years ago

This is the wrong place for it. The behaviour of core functions shouldn't be changed; rather, we should add in new functions, or new middleware to accommodate. For example, a ring.middleware.proxy-headers/wrap-forwarded-scheme function might be a better fit.

ryfow commented 3 years ago

Ahh, thanks. Looks like it's already handled but ring-defaults doesn't take advantage of it. I can manage in my own codebase with configuration but do you think site-defaults and api-defaults should set :proxy to true?

weavejester commented 3 years ago

Ahh, thanks. Looks like it's already handled but ring-defaults doesn't take advantage of it.

You may have misunderstood my comment. I'm saying that a function like wrap-forwarded-scheme would be a good addition, not that it already exists.

do you think site-defaults and api-defaults should set :proxy to true?

I don't see any reason why they should be.

ryfow commented 3 years ago

I think I understand you, but it does already exist in ring.middleware.ssl/wrap-forwarded-scheme.

The reason changing the defaults makes some sense to me is that depending on STS, Chrome & Firefox treat redirects to http:// differently so devs may run into weird bugs if they only use Chrome. I don't feel strongly about it though, so close this if you want to end the conversation.

Thanks again, Ryan

weavejester commented 3 years ago

I think I understand you, but it does already exist in ring.middleware.ssl/wrap-forwarded-scheme.

Ah, so it does. I had completely forgotten about that. Well done finding it.

The reason changing the defaults makes some sense to me is that depending on STS, Chrome & Firefox treat redirects to http:// differently so devs may run into weird bugs if they only use Chrome.

I'd rather make it explicit, for three reasons:

  1. Changing the behaviour may break existing apps
  2. A proxy by default may introduce potential security issues for apps that don't use a proxy
  3. We shouldn't assume people are using a particular technology by default
ryfow commented 3 years ago

fair enough