Get rid of enforcing absolute URLs for redirects. #5
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Without it, `lein test` produces the following error when executing test/test_friend/functional.clj: ``` Syntax error (ClassNotFoundException) compiling at (ring/adapter/jetty.clj:1:1). org.eclipse.jetty.util.component.AggregateLifeCycle ```
This fixes clj-commons#4. For long, the HTTP spec has allowed relative URLs in the Location header, see https://datatracker.ietf.org/doc/html/rfc7231#section-7.1.2. Absolute URLs have been introduced here: cemerick@5b04323 It was trying to fix the issue reported in 2013: cemerick#42 where they claimed that friend doesn't follow the HTTP spec. However, this lead to all sorts of problems with the clojure app running behind an SSL/TLS proxy, e.g. cemerick#84. To sum up: Original friend implementation got it right by using relative URLs for redirects but it wasn't, at the time, strictly following the HTTP spec. However, the HTTP spec has since been updated and there's no more reason to use absolute URLs - they are brittle and break apps.
This is to avoid redirecting to HTTP when the user request was in fact to an HTTPS endpoint. Such a situation happens when you run an SSL proxy in front of your plain HTTP app server. In that case, `ring.util.request/request-url` returns _almost_ a proper endpoint, but uses "http" not "https".
jumarko
commented
Apr 21, 2022
| @@ -1,5 +1,11 @@ | |||
| ## [Friend](http://github.com/cemerick/friend) changelog | |||
|
|
|||
| ### `0.3.x` | |||
Author
There was a problem hiding this comment.
Not sure how does versioning works these days - should I replace x with something specific?
Member
There was a problem hiding this comment.
No worries, I'll take care of that at release
jumarko
commented
Apr 21, 2022
jumarko
commented
Apr 21, 2022
jumarko
commented
Apr 21, 2022
Comment on lines
15
to
20
| (defn resolve-absolute-uri | ||
| [^String uri request] | ||
| (-> (original-url request) | ||
| java.net.URI. | ||
| (.resolve uri) | ||
| str)) |
Author
There was a problem hiding this comment.
Should we be worried about an external client using this function?
Member
There was a problem hiding this comment.
Perhaps it's safer to leave it and mark it as deprecated?
jumarko
commented
Apr 21, 2022
Comment on lines
+75
to
+76
| (str (if (.contains login-uri "?") login-uri (str login-uri "?")) | ||
| param)))) |
Author
There was a problem hiding this comment.
This is basically the same implementation as it was 9 years ago: cemerick@5b04323#diff-7e2343c759ed64f4985000a745bce4778210e832d9e98b4c59e8793344cb5ef9L65-L66
jumarko
commented
Apr 21, 2022
slipset
reviewed
Apr 24, 2022
slipset
requested changes
Apr 24, 2022
Member
slipset
left a comment
slipset
left a comment
There was a problem hiding this comment.
If you could reinstate the one deleted fn and update the documentation as mentioned in comments, I'm good.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use relative URLs for redirects.
This fixes #4.
For long, the HTTP spec has allowed relative URLs in the Location header,
see https://datatracker.ietf.org/doc/html/rfc7231#section-7.1.2.
Absolute URLs have been introduced here: cemerick@5b04323
It was trying to fix the issue reported in 2013: cemerick#42
where they claimed that friend didn't follow the HTTP spec.
However, this lead to all sorts of problems with Clojure apps running behind an SSL/TLS proxy,
e.g. cemerick#84.
To sum up: The original friend implementation got it right by using
relative URLs for redirects but it wasn't, at the time, strictly following the HTTP spec.
However, the HTTP spec has since been updated and there's no more reason
to use absolute URLs - they are brittle and break apps.
UPDATE: ring-defaults vs relative redirects
I fixed all the paths in friend's code that I could find.
However, even after doing that, my application was still redirecting to HTTP, instead of HTTPS.
I found that the problem is in ring-defaults: https://github.com/ring-clojure/ring-defaults#customizing
:absolute-redirectsis set totrueinsite-defaults: https://github.com/ring-clojure/ring-defaults/blob/master/src/ring/middleware/defaults.clj#L52So to make this work, you also need to customize
wrap-defaults, e.g. like this:I posted a message about this on Clojurians slack's ring channel: https://clojurians.slack.com/archives/C0A5GSC6T/p1650524416846019