WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 22 months ago

#16560 closed enhancement (fixed)

let wp_enqueue_script and wp_enqueue_style use '//' as scheme

Reported by: mimecine Owned by: nacin
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.1
Component: General Keywords: has-patch 3.4-early commit
Focuses: Cc:

Description

When loading external resources, most browsers (need to be confirmed) will auto select between https:// and http:// if the url-scheme is simply //. For example, //ajax.googleapis.com/ajax/libs/jquery/1.5.0/jquery.min.js would load over https if the containing page was https itself, but over http if wasn't.

class.wp-scripts.php and class.wp-styles.php does a regex check for ^https?:// to determine if it is an external resource or not, but should really use ^(https?:)?// - or even ^(\w+:)?// (since some loony *could* use ftp for example).

Attachments (4)

external-protocol-regex.diff (1.1 KB) - added by mimecine 3 years ago.
Diff to change the regex for checking if a link is using a protocol.
16560.diff (1.1 KB) - added by georgestephanis 2 years ago.
Allowing styles and scripts to be referenced by http:// https:// and
16560-unit-test.patch (4.2 KB) - added by kurtpayne 22 months ago.
Unit tests
16560-unit-test.2.patch (4.9 KB) - added by kurtpayne 22 months ago.
Asserting bad protocols (e.g. FTP) get prepended with current site's URL

Download all attachments as: .zip

Change History (25)

comment:1 scribu3 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

Didn't know about the '' trick. Should get this in.

Version 0, edited 3 years ago by scribu (next)

comment:2 follow-up: mimecine3 years ago

I could provide a patch I think -- what format would you like it in? general diff?

comment:3 in reply to: ↑ 2 sivel3 years ago

Replying to mimecine:

I could provide a patch I think -- what format would you like it in? general diff?

Patch against trunk from svn. If using the command line version of subversion, an "svn diff" from the root of the WP install will be sufficient.

mimecine3 years ago

Diff to change the regex for checking if a link is using a protocol.

comment:4 mimecine3 years ago

That was easy:) I did it on a newly checked out copy, so I haven't yet run the code to make sure everything works alright, but changing from 'https?:' to '(\w+:)?' should do the trick with http://, https:// - even ftp://, file:/// or whatever shows up ahead ( gfile://, spdy:// etc.) - sure thing, if someone use file:/// he/she is toast, but that shouldn't be our concern...

comment:5 nacin3 years ago

//: is something that's been suggested to me a few times -- not just here, but really in all of our goofy is_ssl() checks and even for URLs and what not stored in the database. I hadn't heard of it either until months ago.

comment:6 mimecine3 years ago

I have been using it for quite some time now. I think I first spotted it on some page at google's -- of course...
It is quite nice, especially since the price of certificates are reachable for most, and it makes sense to secure even the tiniest blog as long as it has it's own ip-address (or running on a webserver that can handle the equivalent of vhosting for certificates).

I actually had a whole night session trying to wrap my head around why is_ssl didn't work for me on nginx, but it turned out I had just configured nginx wrong...

Anyway, this patch itself doesn't fix many of the problems with protocol mismatch that comes from within wordpress or from plugins, but at least it enables you to start using -urls when you are ready.

I still have to test on more browsers though -- pretty sure all modern ones support it, including IE from a few years back, but a more comprehensive test would be great, just to know.

comment:7 hakre3 years ago

Thanks for reporting and the fix.

Also known as protocol-relative URL/URI/LINK - see Uniform Resource Identifiers (URI): Generic Syntax - RFC2396 (updates the older RFC1808), esp. 3. URI Syntactic Components and 5. Relative URI References.

comment:8 phill_brown3 years ago

Patches look good. Worth baring in mind that stylesheets get downloaded twice in IE7 and 8 if this trick ever gets used in the core.

comment:9 xyzzy3 years ago

I'm happy to see this change. Thanks.

Would it make sense to also allow relative URLs -- without a protocol -- for scripts and CSS files? That way, we would not have to worry about the hostname.

This would allow something like:

wp_register_script( 'myscript', '/path/to/my/script.js');

comment:10 SergeyBiryukov3 years ago

  • Keywords has-patch added; needs-patch removed

comment:11 SergeyBiryukov3 years ago

  • Milestone changed from Future Release to 3.3

Closed #18336 as a duplicate.

comment:12 follow-up: azaozz3 years ago

  • Keywords needs-testing added
  • Milestone changed from 3.3 to Future Release
  • Type changed from defect (bug) to enhancement

This should be dealt with as part of is_ssl().

comment:13 in reply to: ↑ 12 evansolomon3 years ago

Replying to azaozz:

This should be dealt with as part of is_ssl().

The leading // is a valid URL syntax, so we should pass it to the client as is without any manipulation. Figuring out if it's an SSL page is unnecessary.

Here is a StackOverflow discussion on the same topic: http://stackoverflow.com/questions/550038/is-it-valid-to-replace-http-with-in-a-script-src-http

comment:14 TobiasBg2 years ago

  • Keywords 3.4-early added

I'd also like to see this patch in.
We would not have to modify any existing core code that uses is_ssl()}}, but it would allow users/developers to use protocol relative URLs in {{{wp_register_script() calls, if desired. No one would be forced to do it, but you would get the possibility. If you do it currently, the site URL gets prepended, gna.

If ^(\w+:)?// seems to risky, let's go with ^(https?:)?// as a starter.

georgestephanis2 years ago

Allowing styles and scripts to be referenced by http:// https:// and

comment:15 skithund2 years ago

  • Cc toni.viemero@… added

comment:16 follow-up: nacin2 years ago

  • Keywords needs-unit-tests commit added; needs-testing removed

Looks good. Would like some test coverage here if possible.

comment:17 in reply to: ↑ 16 evansolomon2 years ago

Replying to nacin:

Looks good. Would like some test coverage here if possible.

16560.diff works for me using r20310 for (a modified version of) the Optimizely plugin we use on WordPress.com.

Current version: https://gist.github.com/1275892/a8a037d263ba897e59e7f058a436696e892f93c9

Diff for test using the 16560.diff: https://gist.github.com/8c4ae177a258bbf345ac

Both work as expected.

comment:18 nacin22 months ago

  • Milestone changed from Future Release to 3.5

Would still be nice to see some basic unit tests. They could go in test_includes_wp-scripts.php and test_includes_wp-styles.php.

kurtpayne22 months ago

Unit tests

kurtpayne22 months ago

Asserting bad protocols (e.g. FTP) get prepended with current site's URL

comment:19 kurtpayne22 months ago

  • Cc kpayne@… added
  • Keywords needs-unit-tests removed

Replying to nacin:

Would still be nice to see some basic unit tests. They could go in test_includes_wp-scripts.php and test_includes_wp-styles.php.

Basic tests created. Anything else you can think of that should be asserted?

comment:20 kurtpayne22 months ago

Unit tests committed [UT749]

comment:21 nacin22 months ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In [21166]:

Allow protocol-relative URLs when registering/enqueueing scripts and styles.

props mimecine, TobiasBg, georgestephanis.
props kurtpayne for the test coverage.
fixes #16560.

Note: See TracTickets for help on using tickets.