Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
base: main
Are you sure you want to change the base?
Fix: Path with spaces throws a bad URI(is not URI?) error #689
Changes from 6 commits
bf58f04
a0dccbd
7042228
36751bd
6b2e2b4
be3b7c7
eba2602
7d428a1
2284a28
e975f8e
19c8a8f
d9e171d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 thisescaped_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 theURI.split(uri)
call.So I assume someone had seen some URIs sent to that
URI.split
method that generated apath
that needed to beunescaped
.Another instance is there is a test case that includes already escaped URIs sent to
split_file_uri
.Does that fully address your question?
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 on4.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 anotherrails-5
project, it wouldn't install due to Gemfile conflicts based on the fact thatrails-5
uses3.7.2
.So I am trying to think through and figure out how to get both of them working as easily as possible.
Thoughts?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is the codepath you are looking for, but if I simply remove the
escape_uri(uri)
out of thesplit_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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Edit 1
The application fails at the
stylesheet_link_tag
call in myapplication.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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump @rafaelfranca
There was a problem hiding this comment.
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 myRails 5.2
project, it doesn't work (due to backwards compatibility issues withsprockets 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 myrails 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.