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

Fix wrap-ssl-redirect calling handler on redirect #9

Merged

Conversation

mainej
Copy link

@mainej mainej commented Sep 2, 2016

Commit a495828 introduced a bug in which the handler passed to
wrap-ssl-redirect was always called, regardless of whether the request
was over SSL. This is a bit like launching the missles, then saying "I'm
sorry I can't launch, you have the wrong code."

This bug wasn't caught by the tests because, until now none of them
checked whether the inner handler is called.

@weavejester
Copy link
Member

Thanks for the fix!

Could you possibly add a docstring to ssl-redirect, perhaps something like:

  "Given a HTTP request, return a redirect response to the equivalent HTTPS URL.
  See: wrap-ssl-redirect."

Also, could you change the commit message to:

Fix wrap-ssl-redirect calling handler on redirect

Handler should not be called when a redirect response is returned.

This explains the issue more concisely, and sticks to imperative mood in the summary line.

(let [handler (wrap-ssl-redirect (fn [_ respond _] (respond (response ""))))
(let [effect (promise)
handler (wrap-ssl-redirect (fn [_ respond _] (respond (do (effect "performed")
(response "")))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation needs fixing, here?

@mainej
Copy link
Author

mainej commented Sep 2, 2016

Yep, good suggestions. I'll push those changes shortly.

Also, should the ssl-redirect function be named ssl-redirect-response? I'm not sure of the convention here. And finally, should the shared conditional be extracted, something like ssl-request?

Thanks for writing and maintaining so many useful Clojure libraries!

@mainej mainej force-pushed the bypass-handler-on-insecure-requests branch from f336854 to acc6b17 Compare September 2, 2016 15:20
@weavejester
Copy link
Member

The convention I'm using is that if I have a middleware function wrap-foo, then the "request" part of it should be handled by a function foo-request, and the response part handled by foo-response.

In this case it only makes sense to call the response part if the request is HTTP, so... yes, on reflection I think ssl-redirect should be renamed to ssl-redirect-response.

Handler should not be called when a redirect response is returned.
@mainej mainej force-pushed the bypass-handler-on-insecure-requests branch from acc6b17 to 92349c2 Compare September 3, 2016 18:25
@mainej
Copy link
Author

mainej commented Sep 3, 2016

ssl-redirect renamed to ssl-redirect-response

@weavejester weavejester changed the title Inner handler shouldn't be called on HTTP requests Fix wrap-ssl-redirect calling handler on redirect Sep 4, 2016
@weavejester weavejester merged commit c559141 into ring-clojure:master Sep 4, 2016
@mainej mainej deleted the bypass-handler-on-insecure-requests branch April 29, 2021 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants