Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Trailing slash removed from env['PATH_INFO'] on mounted app #3215

Closed
jmazzi opened this Issue · 31 comments

8 participants

@jmazzi

The trailing slash is being removed from env when mounting it in routes using Rails 3.1.1.rc2. This same class works fine in pure Rack.

Given the following routes

class RackTest
  def call(env)
    [200, {}, [env['PATH_INFO']]]
  end
end

RackMountRaiilsBug::Application.routes.draw do
  mount RackTest.new => "/testing"
end

When I access http://localhost:3000/testing/blah/ the output I see is

/blah

What's interesting is that https://github.com/rails/rails/blob/master/railties/lib/rails/rack/logger.rb outputs PATH_INFO with the trailing /. You can clone this project to see it in action https://github.com/jmazzi/rack_mount_rails_bug.

@tenderlove tenderlove was assigned
@jmazzi

I have a workaround for this for now.

class Rack::PathInfoFix
  def initialize(app)
    @app = app
  end

  def call(env)
    if env['PATH_INFO'].respond_to?(:sub)
      env['PATH_INFO_PRISTINE'] = env['PATH_INFO'].sub(/^\//, '')
    end
    @app.call(env)
  end
end

In config/application.rb

config.middleware.use "Rack::PathInfoFix"

Then in your class you're mounting as a route

class Rack::SomeClass
  def call(env)
    env['PATH_INFO'] = env.delete('PATH_INFO_PRISTINE')
  end
end

This will work as long as your class is the endpoint. If it isn't, just use PATH_INFO_PRISTINE directly.

@carlosantoniodasilva

Hey guys, do you mind checking if this is still an issue with 3.2 latest? Thanks.

@jmazzi

Yes, this is still an issue https://github.com/jmazzi/rack_mount_rails_bug as of 3.2.3

@tenderlove
Owner

Hi @jmazzi, thank you for creating the rails app, but I was not able to reproduce this error. Here is a video of me trying to reproduce it. Do you know where I might be going wrong? What version of Ruby are you using?

@jmazzi

Hello @tenderlove, I think there might be a misunderstanding. Your video shows the trailing / being removed from each request. That is the problem I reported :smile:

@tenderlove
Owner

Ah! I got it backwards. Cool, I will investigate. :-)

@tenderlove
Owner

Are you sure this started with Rails 3.1.1.rc2? I downgraded your demo app to 3.1.0, and it seems to exhibit the same behavior as 3.2.3.

Here is a video of my test.

Here is a fork of your repo where I downgraded.

@jmazzi

I'm honestly not sure which version of Rails this started in. I mentioned version 3.1.1.rc2 because it is the first version I tried it with.

Should rails be modifying PATH_INFO tho? I guess that's the clarification needed. That's client provided information and it seems odd for Rails to be modifying it.

@tenderlove
Owner

Honestly, I don't think it should be modifying PATH_INFO. SCRIPT_NAME, yes, PATH_INFO, no. However, I'm trying to figure out if this is a regression or a new feature. I'll try to dig around to see who is modifying it. Maybe that will give us some clues.

@jmazzi

I've tested all the way back to 3.0.0. It didn't change anything. It seems it's always worked this way since routes introduced the mount method.

@tenderlove
Owner

So, pre Rails 3.2, PATH_INFO was stripped inside rack-mount (and journey maintains this behavior). It looks like the line in rack-mount that munges this dates back to at least rack-mount version 0.6.2. The latest Rails 3.0 depends on rack-mount 0.6.14. So if this is a regression, it would have to be way back in the Rails 3.0.x series (before it depended on rack-mount > 0.6.2). Even then, this code may have existed, just in a different spot.

If this is a regression, I can't find info about the original behavior. Maybe were you on a 3.0.x version of Rails?

@tenderlove
Owner

@jmazzi cool. Sorry, I didn't refresh the page before I left my comment. :-)

Let's leave this ticket open. I'd like to discuss the impact of "not munging the rack env".

@jmazzi

@tenderlove actually this isn't mount specific. In Rails 2.3.14 the output from request.env['PATH_INFO'] would preserve the trailing '/'. That isn't the case for 3.x.

@jmazzi

Just to confirm, I've tested request.env['PATH_INFO'] with Rails 3.0.0 and it strips the trailing /.

@steveklabnik
Collaborator

@drogus any thoughts on this?

@drogus
Collaborator

@steveklabnik I would like to remove stripping as I agree that we should not change PATH_INFO, especially because 4.0 is close - it's best to do such changes on major releases.

@tenderlove as far as I understand trailing slash is removed in journey, do you have something against removing this functionality?

@tenderlove
Owner

@drogus no, I don't have anything against removing that. Do you want to remove it and see how it impacts the Rails tests?

@drogus
Collaborator

Yes, I'll try.

@drogus drogus was assigned
@drogus
Collaborator

I had some time to play with it and it seems that just removing that will break some things: https://next.travis-ci.org/rails/rails/jobs/2676402, I will take a look at those fails later.

@caius

We've just moved an app from sinatra to rails 3 and run into this as well. Sinatra was passing our URL through unchanged, where rails is stripping the trailing slash (and therefore causing us issues).

Do you guys think the tests should just be amended to expect /foo/ instead of /foo in rails with this change in journey, or should the code itself be amended to strip the trailing slash in certain instances without the tests being changed?

@drogus
Collaborator

@caius I don't understand your question about tests, could you rephrase it?

I've looked at those fails closer and the problem was connected with the fact that rails use normalize_path as well and most places it actually should remove trailing slash (mostly when adding routes to route_set through Mapper).

@tendelove the fix could look like this: drogus/journey@e19a702, all actionpack and journey tests pass with such change, but I don't like the fact that I need to parametrize normalize_path behavior. Do you think it's ok to push this fix or should I think about better solution? (in the latter case, do you have any ideas on how should it be solved?)

@caius

@drogus I think you just answered me with your comment anyway. :smile:

I don't know the intricacies of actionpack/router enough to know whether having routes added as /foo/ instead of /foo was okay. Seems it isn't, but you've got a workaround to that as well.

@drogus
Collaborator

@caius ah, ok, now I get your question :) And yes, you're right, you should be able to add /foo/ route to router - and if it would not work for some reason I would consider it a bug.

@JonRowe

Did this go anywhere? Is it still an issue?

@jmazzi

@JonRowe yes, I updated the project to 3.2.13 and the problem persists.

@jmazzi

@JonRowe same goes for rails 4.0.0.rc2

@drogus
Collaborator

Sorry for no activity, I totally forgot about this, I need to get organized ;) I think I had it somewhere on a branch in journey, I'll see what's there.

@drogus drogus referenced this issue from a commit in drogus/rails
@drogus drogus Don't remove trailing slash from PATH_INFO for mounted apps
Previously when app was mounted as following:

    class Foo
      def call(env)
        [200, {}, [env['PATH_INFO']]]
      end
    end

    RackMountRailsBug::Application.routes.draw do
      mount RackTest.new => "/foo"
    end

trailing slash was removed from PATH_INFO. For example requesting

    GET /foo/bar/

on routes defined above would result in a response containing "/foo/bar"
instead of "/foo/bar/".

This commit fixes the issue.

(closes #3215)
d2bace3
@drogus drogus closed this issue from a commit
@drogus drogus Don't remove trailing slash from PATH_INFO for mounted apps
Previously when app was mounted as following:

    class Foo
      def call(env)
        [200, {}, [env['PATH_INFO']]]
      end
    end

    RackMountRailsBug::Application.routes.draw do
      mount RackTest.new => "/foo"
    end

trailing slash was removed from PATH_INFO. For example requesting

    GET /foo/bar/

on routes defined above would result in a response containing "/foo/bar"
instead of "/foo/bar/".

This commit fixes the issue.

(closes #3215)
50311f1
@drogus drogus closed this in 50311f1
@drogus drogus referenced this issue from a commit
@drogus drogus Don't remove trailing slash from PATH_INFO for mounted apps
Previously when app was mounted as following:

    class Foo
      def call(env)
        [200, {}, [env['PATH_INFO']]]
      end
    end

    RackMountRailsBug::Application.routes.draw do
      mount RackTest.new => "/foo"
    end

trailing slash was removed from PATH_INFO. For example requesting

    GET /foo/bar/

on routes defined above would result in a response containing "/foo/bar"
instead of "/foo/bar/".

This commit fixes the issue.

(closes #3215)

Conflicts:
	actionpack/CHANGELOG.md
1193a27
@drogus
Collaborator

For reference: I've reverted this commit on 4-0-stable after getting feedback from @jeremy here: 1193a27#commitcomment-3474691

@tillsc

If the fix is reverted: shouldn't this be reopened then?

@drogus
Collaborator

@tillsc it was still commited to master, I've reverted only 4-0-stable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.