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.

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

Identify relative protocol asset URLs #83

Merged
merged 1 commit into from
Mar 7, 2022
Merged

Conversation

barrywoolgar
Copy link
Contributor

Hello. Propshaft does not appear to be detecting URLs using the double-slash relative protocol, eg:

@font-face { font-family: Inter; src:url("//fonts.gstatic.com/s/inter/abc.eot") } # abridged

This leads them to be processed as local assets, with the subsequent warning:

Unable to resolve '//fonts.gstatic.com/s/inter/abc.eot' for missing asset '/fonts.gstatic.com/s/inter/s/inter/abc.eot' in application.css

This PR adds "//" to the URL regex's negative lookahead.

Thanks!

@brenogazzola
Copy link
Collaborator

brenogazzola commented Mar 7, 2022

Looks good, but I was not aware this was a valid url format. What would the GET url look like when the browser makes the request, and what is the use case? Just want to make sure we don't have to modify something else in Propshaft too.

@barrywoolgar
Copy link
Contributor Author

No problem! As I understand it, the browser applies the same scheme/protocol used when retrieving the parent document. This "//" format allows for protocol matching without knowing in advance the method being used to retrieve child resources, as a way to avoid mixed-protocol security issues (and the warnings/errors that eventually came with them).

If you'll excuse the Stack Overflow link, this answer references several RFCs that define it going back ~25 years.

This was much more common when we had to pay for SSL certificates and the "SSL everywhere" concept wasn't as accepted as it is now. A "//" reference would work cleanly with HTTP and HTTPS.

In fairness, "//" is now regarded as an anti-pattern by some, so it may be preferable to not support it. I discovered this because a PostCSS plugin (by default) generated URLs in this format - it also provides configuration to force the protocol, but others may not be so lucky.

@brenogazzola
Copy link
Collaborator

Since Propshaft is handling url directly, I believe it should support everything that url supports, even if it's considered anti-pattern.

Thank you for the fix and for checking Propshaft!

@brenogazzola brenogazzola merged commit 5d325d7 into rails:main Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants