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

Jump to bottom

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

wants to merge 12 commits into from

Conversation

marcamillion
Copy link

This PR fixes the issues described in this issue and in this comment.

Expected behavior

When a Rails or Sprockets project is placed in a directory with spaces in the folder names in the path, it shouldn't throw any errors. It should just escape the path name and work like normal.

Actual behavior

Once you put your project in a folder that has spaces in the path name, it throws the following error:

bad URI(is not URI?): "file-digest:///Users/username/Company Name Dropbox/User Name/ProjectName/myapp/app/assets/config/bootstrap"`

System configuration

  • Sprockets 4.0.2 (also been able to replicate it in 3.7.2)
  • Ruby 2.7.1 (also been able to replicate it in 2.5.1)
  • Rails 6.0.3.2 (also been able to replicate it in 5.2.4.3)

===========

This issue surfaced once I upgraded my Dropbox account (which my Rails project is in) from a personal account (with no spaces in the name of the Dropbox folder) to a business account that has spaces -- which I couldn't control.

rafaelfranca
rafaelfranca requested changes Jul 6, 2020
View reviewed changes
lib/sprockets/uri_utils.rb Outdated Show resolved Hide resolved
lib/sprockets/uri_utils.rb Outdated Show resolved Hide resolved
lib/sprockets/uri_utils.rb Outdated Show resolved Hide resolved
lib/sprockets/uri_utils.rb Outdated Show resolved Hide resolved
lib/sprockets/uri_utils.rb Outdated Show resolved Hide resolved
lib/sprockets/uri_utils.rb
#
# Returns uri
def escaped_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.

marcamillion and others added 6 commits July 6, 2020 15:40
@marcamillion @rafaelfranca
Update lib/sprockets/uri_utils.rb
eba2602 
Co-authored-by: Rafael França <rafael@franca.dev>
@marcamillion @rafaelfranca
Update lib/sprockets/uri_utils.rb
7d428a1 
Co-authored-by: Rafael França <rafael@franca.dev>
@marcamillion @rafaelfranca
Update lib/sprockets/uri_utils.rb
2284a28 
Co-authored-by: Rafael França <rafael@franca.dev>
@marcamillion @rafaelfranca
Update lib/sprockets/uri_utils.rb
e975f8e 
Co-authored-by: Rafael França <rafael@franca.dev>
@marcamillion @rafaelfranca
Update lib/sprockets/uri_utils.rb
19c8a8f 
Co-authored-by: Rafael França <rafael@franca.dev>
@marcamillion
Merge branch 'master' into encode-url-paths-with-spaces
d9e171d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rafaelfranca rafaelfranca rafaelfranca requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@marcamillion @rafaelfranca