Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix incorrect assert_redirected_to failure message #10362

Merged
merged 2 commits into from Sep 19, 2013

Conversation

Projects
None yet
4 participants
Contributor

derekprior commented Apr 26, 2013

assert_redirected_to returns incorrect and misleading failure messages in certain scenarios. The assertion itself is correct, but when there is a failure, it calculates the URLs to use in the messages differently than redirect_to calculates the actual url. This is most easily seen with protocol relative URLs.

The underlying cause was duplication and eventual divergence of the code used to calculate the redirect url from options. This change centralizes that code in ActionController::Redirecting.

The change also revealed that the regular expression previously used in ActionController::Redirecting was too permissive in that it was allowing _ in schemes. This has also been addressed.

Left as three commits for now to make for an easier review.

Contributor

derekprior commented May 16, 2013

@pixeltrix, @marcel, @tenderlove : According to git blame you guys seem pretty active in these files. What do you think of this?

@pixeltrix pixeltrix commented on an outdated diff May 16, 2013

...ck/lib/action_dispatch/testing/assertions/response.rb
- normalized = case fragment
- when Regexp
- fragment
- when %r{^\w[A-Za-z\d+.-]*:.*}
- fragment
- when String
- @request.protocol + @request.host_with_port + fragment
- when :back
- raise RedirectBackError unless refer = @request.headers["Referer"]
- refer
- else
- @controller.url_for(fragment)
- end
-
- normalized.respond_to?(:delete) ? normalized.delete("\0\r\n") : normalized
+ if fragment.class == Regexp
@pixeltrix

pixeltrix May 16, 2013

Owner

You can rewrite this as Regexp === fragment

Owner

pixeltrix commented May 16, 2013

@derekprior you might want to check the last commit date before paging someone in a comment - Marcel's last Rails commit was over 5 years ago.

It'll need a CHANGELOG entry for the addition of protocol relative support to redirect_to as well and once you've done that can you squash the commits.

Otherwise, it's a 👍 - thanks!

Contributor

derekprior commented May 16, 2013

Thanks @pixeltrix. I didn't add support for protocol-relative: it was already there. All this did was correct the assertion failure message for protocol relative urls (and likely other urls where the previously duplicated implementation diverged) and then very subtly tweak the scheme regex in redirect_to to conform to spec.

I'll make the suggested edit, changelog entry, and squash.

Contributor

derekprior commented May 16, 2013

Rebased and squashed.

Is master the right target for this?

Owner

pixeltrix commented May 16, 2013

Is master the right target for this?

Yes, I'll cherry pick if necessary.

Contributor

pftg commented Aug 6, 2013

@derekprior I think changelog should be rebased and your changes moved to top

Contributor

derekprior commented Sep 9, 2013

Rebased again @pftg, @pixeltrix

Contributor

pftg commented Sep 9, 2013

@derekprior sorry, I was not clear. Please move your Changelog changes:

+ *   Fix incorrect `assert_redirected_to` failure message for protocol-relative
+    URLs.
+
+    *Derek Prior*
+
+*   Fix regex used to detect URI schemes in `redirect_to` to be consistent with
+    RFC 3986.
+
+    *Derek Prior*

To the first line.

Contributor

derekprior commented Sep 9, 2013

Yeah, I caught that after I pinged you. Should be all set.

Contributor

pftg commented Sep 9, 2013

Should be:

1:*   Fix incorrect `assert_redirected_to` failure message for protocol-relative
...
9:    *Derek Prior*
10:
11:*   Introduce `BasicRendering` which is the most basic rendering implementation. It
12:    allows to `render :text` and `render :nothing` without depending on Action View.
13:
14:    *Łukasz Strzałkowski*
Contributor

pftg commented Sep 9, 2013

👍

Member

robin850 commented Sep 14, 2013

@derekprior : Thanks for your contribution! Since you are adding two changelog entries, I think you could better split your commit into two, what do you think ? It's easier for reverting/point commits when debugging. Also, it will need a rebase.

derekprior added some commits Apr 26, 2013

@derekprior derekprior Fix incorrect assert_redirected_to failure message
In some instances, `assert_redirected_to` assertion was returning an
incorrect and misleading failure message when the assertion failed.
This was due to a disconnect in how the assertion computes the redirect
string for the failure message and how `redirect_to` computes the
string that is actually used for redirection.

I made the `_compute_redirect_to_loaction` method used by `redirect_to`
public and call that from the method `assert_redirect_to` uses to
calculate the URL.

The reveals a new test failure due to the regex used by
`_compute_redirect_to_location` allow `_` in the URL scheme.
1dacfba
@derekprior derekprior Fix regex used to find URI schemes in redirect_to
The previous regex was allowing `_` in the URI scheme, which is not
allowed by RFC 3986. This change brings the regex in line with the RFC.
a78c10d
Contributor

derekprior commented Sep 19, 2013

@robin850, @pftg, @pixeltrix : I split the commit as suggested and rebased once again. Ready to merge?

@pixeltrix pixeltrix added a commit that referenced this pull request Sep 19, 2013

@pixeltrix pixeltrix Merge pull request #10362 from derekprior/dp-fix-assert-redirect-to
Fix incorrect assert_redirected_to failure message
acd83a7

@pixeltrix pixeltrix merged commit acd83a7 into rails:master Sep 19, 2013

Owner

pixeltrix commented Sep 19, 2013

@derekprior thanks - sorry it took so long to get merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment