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.

Sign up for GitHub

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: Path with spaces throws a bad URI(is not URI?) error #689

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Fix: Path with spaces throws a bad URI(is not URI?) error #689

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
bf58f04
Added a successful failing test.
marcamillion Jun 29, 2020
a0dccbd
Implemented an EscapedURI method that either returns the URI if it is…
marcamillion Jun 30, 2020
7042228
WIP: Attempting to fix Load URI with Index Alias error in Test Loader.
marcamillion Jul 3, 2020
36751bd
Fixed the not loaded error being generated by the test suite.
marcamillion Jul 6, 2020
6b2e2b4
More tidying up debugging cruft.
marcamillion Jul 6, 2020
be3b7c7
Updated CHANGELOG.
marcamillion Jul 6, 2020
eba2602
Update lib/sprockets/uri_utils.rb
marcamillion Jul 6, 2020
7d428a1
Update lib/sprockets/uri_utils.rb
marcamillion Jul 6, 2020
2284a28
Update lib/sprockets/uri_utils.rb
marcamillion Jul 6, 2020
e975f8e
Update lib/sprockets/uri_utils.rb
marcamillion Jul 6, 2020
19c8a8f
Update lib/sprockets/uri_utils.rb
marcamillion Jul 6, 2020
d9e171d
Merge branch 'master' into encode-url-paths-with-spaces
marcamillion Jul 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

Get upgrade notes from Sprockets 3.x to 4.x at https://github.com/rails/sprockets/blob/master/UPGRADING.md

## Next Release

- Fix bug where project Path with spaces throws a bad URI(is not URI?) error - Marc Gayle - https://github.com/marcamillion

## Master

- Remove remaining support for Ruby < 2.4.[#672](https://github.com/rails/sprockets/pull/672)
Expand Down
12 changes: 11 additions & 1 deletion lib/sprockets/uri_utils.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,23 @@ def join_uri(scheme, userinfo, host, port, registry, path, opaque, query, fragme
URI::Generic.new(scheme, userinfo, host, port, registry, path, opaque, query, fragment).to_s
end

# Internal: Escape the URI
#
# uri - String uri
#
# Returns String.
def escape_uri(uri)
return uri if uri == URI::Generic::DEFAULT_PARSER.escape(URI::Generic::DEFAULT_PARSER.unescape(uri))
Copy link
Member

Choose a reason for hiding this comment

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

Is there any case were this method receives an already escaped uri? The URIs are usually generated by the library, so we should be able to know when we are passing a escaped uri and in that case not call this method at all.

Always doing this check will be expensive.

Copy link
Author

@marcamillion marcamillion Jul 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Well, keep in mind that I added this new escaped_uri(uri) method and I only call it in one place.

I do know though, that the split_file_uri method that I am calling this escaped_uri method in, does receive escaped URIs otherwise.

For example, this line unescapes the path variable, which before my change didn't escape the URI input in the URI.split(uri) call.

So I assume someone had seen some URIs sent to that URI.split method that generated a path that needed to be unescaped.

Another instance is there is a test case that includes already escaped URIs sent to split_file_uri.

Does that fully address your question?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it does. Sprockets is the library that generates those URI, so we can make sure we either only accept escaped URIs on this method or we only accept unescaped URIs.

So the fix seems to be to make sure we only pass escaped URIs to split_file_uri.

That way we don't need to check if the path was already escaped and we can be sure the URI.split always pass.

Copy link
Author

Choose a reason for hiding this comment

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

Exactly, that's exactly what this method does. Previously, it was assumed that the URI was escaped -- which based on the linked issue I ran into -- wasn't always the case.

Copy link
Author

Choose a reason for hiding this comment

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

So is it ready to be merged?

Also, this is my first time doing a PR on any Rails project, so out of curiosity, how will multiple versions be handled? As in, Sprockets seems to be frozen at 3.7.2 and all of the active support seems to be happening on 4.x.

Will this fix be automatically applied to 3.7.2 also, or is there something else that needs to be done for it to be backwards compatible? If the latter, who is responsible for that?

The reason I am asking is that this PR was done on the latest master branch (i.e. 4.x).

I tried to run the test suite on branch 3.x locally, and it gave me a number of errors in the vanilla state (i.e. before I made any changes) so I haven't gone any further with it.

I have tested my version of this fix in my local rails-6 project and it fixes the issue, but when I tried it in another rails-5 project, it wouldn't install due to Gemfile conflicts based on the fact that rails-5 uses 3.7.2.

So I am trying to think through and figure out how to get both of them working as easily as possible.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

If you know the URI that fails you can do something like puts caller if uri == something and you will get the entire callstack of that argument. With that you will see which codepath is passing unescaped URI.

We need the real codepath in an application, not the codepath of the test suite. You could try to run your application that you know you can reproduce the bug against your local sprockets and see what exactly is passing an unescaped URI to that method.

Copy link
Author

Choose a reason for hiding this comment

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

To find this, I think we can start by looking what is the source of your file:///usr/Company Name Dropbox/local/bin/myapp/assets URI. Which codepath is generating a non-escaped URI, given that, by the look of the test, we are expecting URIs to be always escaped?

Not sure if this is the codepath you are looking for, but if I simply remove the escape_uri(uri) out of the split_file_uri(uri) method call and re-run it, this is the full error. Are those file names with specific lines the entire codepath you are referencing?

  1) Error:
TestURIUtils#test_split_file_uri:
URI::InvalidURIError: bad URI(is not URI?): "file:///usr/Company Name Dropbox/local/bin/myapp/assets"
    /Users/marcamillion/.rvm/rubies/ruby-2.7.1/lib/ruby/2.7.0/uri/rfc3986_parser.rb:67:in `split'
    /Users/marcamillion/.rvm/rubies/ruby-2.7.1/lib/ruby/2.7.0/uri/common.rb:197:in `split'
    /Users/marcamillion/sprockets/lib/sprockets/uri_utils.rb:57:in `split_file_uri'
    /Users/marcamillion/sprockets/test/test_uri_utils.rb:38:in `test_split_file_uri'

900 runs, 3939 assertions, 0 failures, 1 errors, 4 skips

You have skipped tests. Run with --verbose for details.
rake aborted!

Copy link
Author

@marcamillion marcamillion Jul 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We need the real codepath in an application, not the codepath of the test suite. You could try to run your application that you know you can reproduce the bug against your local sprockets and see what exactly is passing an unescaped URI to that method.

Ok below is the full stack trace from the application where it fails. I assume you just need the sprockets related paths, but I included the full framework trace just to be sure.

Let me know if I still need to do your other suggestion because this doesn't fully capture it.

/Users/marcamillion/.rvm/rubies/ruby-2.5.1/lib/ruby/2.5.0/uri/rfc3986_parser.rb:67:in `split'
/Users/marcamillion/.rvm/rubies/ruby-2.5.1/lib/ruby/2.5.0/uri/common.rb:199:in `split'
sprockets (3.7.2) lib/sprockets/uri_utils.rb:45:in `split_file_uri'
sprockets (3.7.2) lib/sprockets/uri_utils.rb:126:in `parse_file_digest_uri'
sprockets (3.7.2) lib/sprockets.rb:159:in `block in <module:Sprockets>'
sprockets (3.7.2) lib/sprockets/dependencies.rb:67:in `resolve_dependency'
sprockets (3.7.2) lib/sprockets/cached_environment.rb:23:in `block in initialize'
sprockets (3.7.2) lib/sprockets/cached_environment.rb:59:in `resolve_dependency'
sprockets (3.7.2) lib/sprockets/loader.rb:268:in `block in resolve_dependencies'
sprockets (3.7.2) lib/sprockets/loader.rb:268:in `map'
sprockets (3.7.2) lib/sprockets/loader.rb:268:in `resolve_dependencies'
sprockets (3.7.2) lib/sprockets/loader.rb:55:in `block in load'
sprockets (3.7.2) lib/sprockets/loader.rb:311:in `block in fetch_asset_from_dependency_cache'
sprockets (3.7.2) lib/sprockets/loader.rb:307:in `each'
sprockets (3.7.2) lib/sprockets/loader.rb:307:in `each_with_index'
sprockets (3.7.2) lib/sprockets/loader.rb:307:in `fetch_asset_from_dependency_cache'
sprockets (3.7.2) lib/sprockets/loader.rb:44:in `load'
sprockets (3.7.2) lib/sprockets/cached_environment.rb:20:in `block in initialize'
sprockets (3.7.2) lib/sprockets/cached_environment.rb:47:in `load'
sprockets (3.7.2) lib/sprockets/base.rb:66:in `find_asset'
sprockets (3.7.2) lib/sprockets/base.rb:73:in `find_all_linked_assets'
sprockets (3.7.2) lib/sprockets/manifest.rb:134:in `block in find'
sprockets (3.7.2) lib/sprockets/manifest.rb:133:in `each'
sprockets (3.7.2) lib/sprockets/manifest.rb:133:in `find'
sprockets-rails (3.2.1) lib/sprockets/railtie.rb:50:in `each'
sprockets-rails (3.2.1) lib/sprockets/railtie.rb:50:in `map'
sprockets-rails (3.2.1) lib/sprockets/railtie.rb:50:in `precompiled_assets'
sprockets-rails (3.2.1) lib/sprockets/railtie.rb:35:in `asset_precompiled?'
sprockets-rails (3.2.1) lib/sprockets/railtie.rb:251:in `block (3 levels) in <class:Railtie>'
sprockets-rails (3.2.1) lib/sprockets/rails/helper.rb:359:in `precompiled?'
sprockets-rails (3.2.1) lib/sprockets/rails/helper.rb:363:in `raise_unless_precompiled_asset'
sprockets-rails (3.2.1) lib/sprockets/rails/helper.rb:348:in `find_debug_asset'
sprockets-rails (3.2.1) lib/sprockets/rails/helper.rb:229:in `block in lookup_debug_asset'
sprockets-rails (3.2.1) lib/sprockets/rails/helper.rb:242:in `block in resolve_asset'
sprockets-rails (3.2.1) lib/sprockets/rails/helper.rb:241:in `each'
sprockets-rails (3.2.1) lib/sprockets/rails/helper.rb:241:in `detect'
sprockets-rails (3.2.1) lib/sprockets/rails/helper.rb:241:in `resolve_asset'
sprockets-rails (3.2.1) lib/sprockets/rails/helper.rb:228:in `lookup_debug_asset'
sprockets-rails (3.2.1) lib/sprockets/rails/helper.rb:170:in `block in stylesheet_link_tag'
sprockets-rails (3.2.1) lib/sprockets/rails/helper.rb:169:in `map'
sprockets-rails (3.2.1) lib/sprockets/rails/helper.rb:169:in `stylesheet_link_tag'
actionview (5.2.4.3) lib/action_view/template.rb:159:in `block in render'
activesupport (5.2.4.3) lib/active_support/notifications.rb:170:in `instrument'
actionview (5.2.4.3) lib/action_view/template.rb:354:in `instrument_render_template'
actionview (5.2.4.3) lib/action_view/template.rb:157:in `render'
rack-mini-profiler (1.0.2) lib/mini_profiler/profiling_methods.rb:104:in `block in profile_method'
xray-rails (0.3.2) lib/xray/engine.rb:27:in `render_with_xray'
actionview (5.2.4.3) lib/action_view/renderer/template_renderer.rb:66:in `render_with_layout'
actionview (5.2.4.3) lib/action_view/renderer/template_renderer.rb:52:in `render_template'
actionview (5.2.4.3) lib/action_view/renderer/template_renderer.rb:16:in `render'
actionview (5.2.4.3) lib/action_view/renderer/renderer.rb:44:in `render_template'
actionview (5.2.4.3) lib/action_view/renderer/renderer.rb:25:in `render'
actionview (5.2.4.3) lib/action_view/rendering.rb:103:in `_render_template'
actionpack (5.2.4.3) lib/action_controller/metal/streaming.rb:219:in `_render_template'
actionview (5.2.4.3) lib/action_view/rendering.rb:84:in `render_to_body'
actionpack (5.2.4.3) lib/action_controller/metal/rendering.rb:52:in `render_to_body'
actionpack (5.2.4.3) lib/action_controller/metal/renderers.rb:142:in `render_to_body'
actionpack (5.2.4.3) lib/abstract_controller/rendering.rb:25:in `render'
actionpack (5.2.4.3) lib/action_controller/metal/rendering.rb:36:in `render'
actionpack (5.2.4.3) lib/action_controller/metal/instrumentation.rb:46:in `block (2 levels) in render'
activesupport (5.2.4.3) lib/active_support/core_ext/benchmark.rb:14:in `block in ms'
/Users/marcamillion/.rvm/rubies/ruby-2.5.1/lib/ruby/2.5.0/benchmark.rb:308:in `realtime'
activesupport (5.2.4.3) lib/active_support/core_ext/benchmark.rb:14:in `ms'
actionpack (5.2.4.3) lib/action_controller/metal/instrumentation.rb:46:in `block in render'
actionpack (5.2.4.3) lib/action_controller/metal/instrumentation.rb:87:in `cleanup_view_runtime'
activerecord (5.2.4.3) lib/active_record/railties/controller_runtime.rb:31:in `cleanup_view_runtime'
actionpack (5.2.4.3) lib/action_controller/metal/instrumentation.rb:45:in `render'
actionpack (5.2.4.3) lib/action_controller/metal/implicit_render.rb:35:in `default_render'
actionpack (5.2.4.3) lib/action_controller/metal/basic_implicit_render.rb:6:in `block in send_action'
actionpack (5.2.4.3) lib/action_controller/metal/basic_implicit_render.rb:6:in `tap'
actionpack (5.2.4.3) lib/action_controller/metal/basic_implicit_render.rb:6:in `send_action'
actionpack (5.2.4.3) lib/abstract_controller/base.rb:194:in `process_action'
actionpack (5.2.4.3) lib/action_controller/metal/rendering.rb:30:in `process_action'
actionpack (5.2.4.3) lib/abstract_controller/callbacks.rb:42:in `block in process_action'
activesupport (5.2.4.3) lib/active_support/callbacks.rb:132:in `run_callbacks'
actionpack (5.2.4.3) lib/abstract_controller/callbacks.rb:41:in `process_action'
actionpack (5.2.4.3) lib/action_controller/metal/rescue.rb:22:in `process_action'
actionpack (5.2.4.3) lib/action_controller/metal/instrumentation.rb:34:in `block in process_action'
activesupport (5.2.4.3) lib/active_support/notifications.rb:168:in `block in instrument'
activesupport (5.2.4.3) lib/active_support/notifications/instrumenter.rb:23:in `instrument'
activesupport (5.2.4.3) lib/active_support/notifications.rb:168:in `instrument'
actionpack (5.2.4.3) lib/action_controller/metal/instrumentation.rb:32:in `process_action'
actionpack (5.2.4.3) lib/action_controller/metal/params_wrapper.rb:256:in `process_action'
activerecord (5.2.4.3) lib/active_record/railties/controller_runtime.rb:24:in `process_action'
actionpack (5.2.4.3) lib/abstract_controller/base.rb:134:in `process'
actionview (5.2.4.3) lib/action_view/rendering.rb:32:in `process'
rack-mini-profiler (1.0.2) lib/mini_profiler/profiling_methods.rb:104:in `block in profile_method'
actionpack (5.2.4.3) lib/action_controller/metal.rb:191:in `dispatch'
actionpack (5.2.4.3) lib/action_controller/metal.rb:252:in `dispatch'
actionpack (5.2.4.3) lib/action_dispatch/routing/route_set.rb:52:in `dispatch'
actionpack (5.2.4.3) lib/action_dispatch/routing/route_set.rb:34:in `serve'
actionpack (5.2.4.3) lib/action_dispatch/routing/mapper.rb:18:in `block in <class:Constraints>'
actionpack (5.2.4.3) lib/action_dispatch/routing/mapper.rb:48:in `serve'
actionpack (5.2.4.3) lib/action_dispatch/journey/router.rb:52:in `block in serve'
actionpack (5.2.4.3) lib/action_dispatch/journey/router.rb:35:in `each'
actionpack (5.2.4.3) lib/action_dispatch/journey/router.rb:35:in `serve'
actionpack (5.2.4.3) lib/action_dispatch/routing/route_set.rb:840:in `call'
xray-rails (0.3.2) lib/xray/middleware.rb:38:in `call'
warden (1.2.8) lib/warden/manager.rb:36:in `block in call'
warden (1.2.8) lib/warden/manager.rb:34:in `catch'
warden (1.2.8) lib/warden/manager.rb:34:in `call'
rack (2.2.3) lib/rack/tempfile_reaper.rb:15:in `call'
rack (2.2.3) lib/rack/etag.rb:27:in `call'
rack (2.2.3) lib/rack/conditional_get.rb:27:in `call'
rack (2.2.3) lib/rack/head.rb:12:in `call'
actionpack (5.2.4.3) lib/action_dispatch/http/content_security_policy.rb:18:in `call'
rack (2.2.3) lib/rack/session/abstract/id.rb:266:in `context'
rack (2.2.3) lib/rack/session/abstract/id.rb:260:in `call'
actionpack (5.2.4.3) lib/action_dispatch/middleware/cookies.rb:670:in `call'
activerecord (5.2.4.3) lib/active_record/migration.rb:559:in `call'
actionpack (5.2.4.3) lib/action_dispatch/middleware/callbacks.rb:28:in `block in call'
activesupport (5.2.4.3) lib/active_support/callbacks.rb:98:in `run_callbacks'
actionpack (5.2.4.3) lib/action_dispatch/middleware/callbacks.rb:26:in `call'
actionpack (5.2.4.3) lib/action_dispatch/middleware/executor.rb:14:in `call'
actionpack (5.2.4.3) lib/action_dispatch/middleware/debug_exceptions.rb:61:in `call'
web-console (3.7.0) lib/web_console/middleware.rb:135:in `call_app'
web-console (3.7.0) lib/web_console/middleware.rb:30:in `block in call'
web-console (3.7.0) lib/web_console/middleware.rb:20:in `catch'
web-console (3.7.0) lib/web_console/middleware.rb:20:in `call'
actionpack (5.2.4.3) lib/action_dispatch/middleware/show_exceptions.rb:33:in `call'
railties (5.2.4.3) lib/rails/rack/logger.rb:38:in `call_app'
railties (5.2.4.3) lib/rails/rack/logger.rb:26:in `block in call'
activesupport (5.2.4.3) lib/active_support/tagged_logging.rb:71:in `block in tagged'
activesupport (5.2.4.3) lib/active_support/tagged_logging.rb:28:in `tagged'
activesupport (5.2.4.3) lib/active_support/tagged_logging.rb:71:in `tagged'
railties (5.2.4.3) lib/rails/rack/logger.rb:26:in `call'
sprockets-rails (3.2.1) lib/sprockets/rails/quiet_assets.rb:13:in `call'
actionpack (5.2.4.3) lib/action_dispatch/middleware/remote_ip.rb:81:in `call'
actionpack (5.2.4.3) lib/action_dispatch/middleware/request_id.rb:27:in `call'
rack (2.2.3) lib/rack/method_override.rb:24:in `call'
rack (2.2.3) lib/rack/runtime.rb:22:in `call'
activesupport (5.2.4.3) lib/active_support/cache/strategy/local_cache_middleware.rb:29:in `call'
actionpack (5.2.4.3) lib/action_dispatch/middleware/executor.rb:14:in `call'
actionpack (5.2.4.3) lib/action_dispatch/middleware/static.rb:127:in `call'
rack (2.2.3) lib/rack/sendfile.rb:110:in `call'
rack-mini-profiler (1.0.2) lib/mini_profiler/profiler.rb:281:in `call'
railties (5.2.4.3) lib/rails/engine.rb:524:in `call'
puma (3.12.1) lib/puma/configuration.rb:227:in `call'
puma (3.12.1) lib/puma/server.rb:660:in `handle_request'
puma (3.12.1) lib/puma/server.rb:474:in `process_client'
puma (3.12.1) lib/puma/server.rb:334:in `block in run'
puma (3.12.1) lib/puma/thread_pool.rb:135:in `block in spawn_thread'

Edit 1

The application fails at the stylesheet_link_tag call in my application.html.erb:

<%= stylesheet_link_tag 'application', media: 'all', 'data-turbolinks-track': 'reload' %>

This is the error:

bad URI(is not URI?): file-digest:///Users/marcamillion/Company Name Dropbox/User/myapp/app/assets/stylesheets/trestle/_variables.scss

Copy link
Author

Choose a reason for hiding this comment

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

bump @rafaelfranca

Copy link
Author

Choose a reason for hiding this comment

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

Hey @rafaelfranca I know you are prolly busy with a bunch of other stuff, and we don't need to solve this completely right now. But I would love to get my app working.

If I use my fork, with my fixes, in my Rails 6 project that works fine, but when I try to use it in my Rails 5.2 project, it doesn't work (due to backwards compatibility issues with sprockets 3.7.2).

I tried pulling the 3.x branch of this gem and simply running the test suite but all the tests failed -- so I can't even manually apply these changes to that branch to generate a version that works for my rails 5.2 app.

Do you have any suggestions for how I might get around this to get my app working again by any chance? It's been out of commission for more than 2 weeks, so any assistance you can give me to get that operational would be greatly appreciated.

Thanks.

URI::Generic::DEFAULT_PARSER.escape(uri)
end

# Internal: Parse file: URI into component parts.
#
# uri - String uri
#
# Returns [scheme, host, path, query].
def split_file_uri(uri)
scheme, _, host, _, _, path, _, query, _ = URI.split(uri)
scheme, _, host, _, _, path, _, query, _ = URI.split(escape_uri(uri))

path = URI::Generic::DEFAULT_PARSER.unescape(path)
path.force_encoding(Encoding::UTF_8)
Expand Down
2 changes: 1 addition & 1 deletion test/test_loader.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ def setup
test 'load uri with index alias' do
filename = fixture_path('default/coffee/index.js')
index_alias = fixture_path('default/coffee.js')
assert asset = @env.load("file://#{uri_path(filename)}?type=application/javascript&index_alias=#{Rack::Utils.escape(index_alias)}")
assert asset = @env.load("file://#{uri_path(filename)}?type=application/javascript&index_alias=#{index_alias}")
assert_equal filename, asset.filename, asset.inspect
assert_equal 'coffee.js', asset.logical_path, asset.inspect
assert_equal fixture_path('default'), asset.to_hash[:load_path], asset.inspect
Expand Down
3 changes: 3 additions & 0 deletions test/test_uri_utils.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ def test_inverse_uri_functions
end

def test_split_file_uri
parts = split_file_uri("file:///usr/Company Name Dropbox/local/bin/myapp/assets")
assert_equal ['file', nil, '/usr/Company Name Dropbox/local/bin/myapp/assets', nil], parts

parts = split_file_uri("file://localhost/etc/fstab")
assert_equal ['file', 'localhost', '/etc/fstab', nil], parts

Expand Down