Mapping to a redirect route with a param can result in URI::InvalidURIError #5688

Closed
kylev opened this Issue Apr 1, 2012 · 8 comments

Comments

Projects
None yet
4 participants
@kylev

kylev commented Apr 1, 2012

Assuming a paramaterized redirect route like this:

match 'foo/view/:id' => redirect("/foo/%{id}")

It is possible for an errant link to cause this to throw URI::InvalidURIError via HttpHelpers#redirect. If someone forgets their closing quote in a link and the incoming URL looks like "/foo/view/1234>" (with the greater-than) the unprotected URI.parse on mapper.rb:372 will throw this exception. This URL either shouldn't make it to the mapper or should not choke.

One suggestion would be transforming the URI exception to an ActionController::RoutingError and thus a 404:

begin
  uri = URI.parse(path_proc.call(*params))
rescue URI::InvalidURIError => e
  raise ActionController::RoutingError, e.message
end

This is in actionpack for 3-0-stable.

@kaiwren

This comment has been minimized.

Show comment
Hide comment
@kaiwren

kaiwren Apr 1, 2012

Contributor

Doesn't seem like a test can easily be written for this - all the test helpers (lib/action_dispatch/testing/integration.rb in ActionPack and lib/rack/test.rb in rack-test) use URI.parse before making mock requests inadvertently validating the URI. Am I missing something?

Contributor

kaiwren commented Apr 1, 2012

Doesn't seem like a test can easily be written for this - all the test helpers (lib/action_dispatch/testing/integration.rb in ActionPack and lib/rack/test.rb in rack-test) use URI.parse before making mock requests inadvertently validating the URI. Am I missing something?

kaiwren added a commit to c42engineering/rails that referenced this issue Apr 1, 2012

Issue #5688:
* Fixed modulo based redirect to fail with a RoutingError rather than an InvalidURIError if the generated redirect URI is invalid because of a bad incoming URI
* Changed intgration test helper to not blow up on invalid URIs so that tests for invalid URIs can be written
* Still have a failing test because we still use URI.parse further down the testing stack in rack-test inadvertently validating the incoming URI for correctness
@kylev

This comment has been minimized.

Show comment
Hide comment
@kylev

kylev Apr 2, 2012

Yeah, I was going to comment that the integration tests are specifically insulated from this type of test case (or any invalid URL test case) because of the use of URI.process in the process helper of integration.rb. mapper.rb has very few direct-call tests.

kylev commented Apr 2, 2012

Yeah, I was going to comment that the integration tests are specifically insulated from this type of test case (or any invalid URL test case) because of the use of URI.process in the process helper of integration.rb. mapper.rb has very few direct-call tests.

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Apr 3, 2012

Member

I'd consider this a bug in redirect as it should be url encoding any params that will get interpolated - needs fixing in master, 3-2-stable and 3-1-stable if someone wants to have a go. I'd % escape the subdelims from RFC 3986 even though they're valid in a path because they may be used in query context, e.g:

match 'foo/view/:id' => redirect("/foo?id=%{id}")

/cc @rohit

Member

pixeltrix commented Apr 3, 2012

I'd consider this a bug in redirect as it should be url encoding any params that will get interpolated - needs fixing in master, 3-2-stable and 3-1-stable if someone wants to have a go. I'd % escape the subdelims from RFC 3986 even though they're valid in a path because they may be used in query context, e.g:

match 'foo/view/:id' => redirect("/foo?id=%{id}")

/cc @rohit

@kaiwren

This comment has been minimized.

Show comment
Hide comment
@kaiwren

kaiwren Apr 4, 2012

Contributor

@pixeltrix I'll take a stab at it just to get going if nothing else. I'll almost certainly come back with more questions, though, if that's ok.

Contributor

kaiwren commented Apr 4, 2012

@pixeltrix I'll take a stab at it just to get going if nothing else. I'll almost certainly come back with more questions, though, if that's ok.

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Apr 4, 2012

Member

@kaiwren thanks - questions are fine

Member

pixeltrix commented Apr 4, 2012

@kaiwren thanks - questions are fine

@ghost ghost assigned pixeltrix Apr 29, 2012

@pixeltrix pixeltrix closed this in 958daaa Apr 29, 2012

arunagw pushed a commit to arunagw/rails that referenced this issue Apr 30, 2012

arunagw pushed a commit to arunagw/rails that referenced this issue Apr 30, 2012

@pixeltrix pixeltrix referenced this issue in rails/journey May 17, 2012

Closed

unicode urls and redirection #28

@pwim

This comment has been minimized.

Show comment
Hide comment
@pwim

pwim May 17, 2012

Contributor

This patch causes an issue when using the following redirection strategy

constraints(:host => "www.kanjidamage.com") do
  match '*source', to: redirect("http://kanjidamage.com/%{source}")
  root to: redirect("http://kanjidamage.com/")
end

If a user visits http://www.kanjidamage.com/kanji/51, he will now be redirected to http://kanjidamage.com/kanji%2F51.

Contributor

pwim commented May 17, 2012

This patch causes an issue when using the following redirection strategy

constraints(:host => "www.kanjidamage.com") do
  match '*source', to: redirect("http://kanjidamage.com/%{source}")
  root to: redirect("http://kanjidamage.com/")
end

If a user visits http://www.kanjidamage.com/kanji/51, he will now be redirected to http://kanjidamage.com/kanji%2F51.

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix May 17, 2012

Member

@pwim this is intentional - the redirect interpolation may be used in a query argument where / needs escaping, e.g.

constraints(:host => "www.kanjidamage.com") do
  match '*source', to: redirect("http://kanjidamage.com/?page=%{source}")
  root to: redirect("http://kanjidamage.com/")
end

If you need something different just construct your own redirect proc, :e.g:

constraints(:host => "www.kanjidamage.com") do
  match '*source', to: redirect { |params, request| "http://kanjidamage.com/#{URI.escape(params[:source]}" }
  root to: redirect("http://kanjidamage.com/")
end

The URI.escape doesn't escape / unless you ask it to.

Member

pixeltrix commented May 17, 2012

@pwim this is intentional - the redirect interpolation may be used in a query argument where / needs escaping, e.g.

constraints(:host => "www.kanjidamage.com") do
  match '*source', to: redirect("http://kanjidamage.com/?page=%{source}")
  root to: redirect("http://kanjidamage.com/")
end

If you need something different just construct your own redirect proc, :e.g:

constraints(:host => "www.kanjidamage.com") do
  match '*source', to: redirect { |params, request| "http://kanjidamage.com/#{URI.escape(params[:source]}" }
  root to: redirect("http://kanjidamage.com/")
end

The URI.escape doesn't escape / unless you ask it to.

@pwim

This comment has been minimized.

Show comment
Hide comment
@pwim

pwim May 17, 2012

Contributor

@pixeltrix OK, thanks!

Contributor

pwim commented May 17, 2012

@pixeltrix OK, thanks!

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