CSS aggregation breaks file URL rewriting because it abuses it [#1961340]

Problem/Motivation

Two problems (intertwined):

1) file_create_url() result for the CSS file itself is abused to generate the URLs for files referenced by CSS files: Drupal 7 core's CSS aggregation calls file_create_url() (and thus allows hook_file_url_alter() to modify that URL) on the aggregate CSS file that's being built (e.g. http://site.com/subdir/sites/default/foobar.css). It then proceeds to strip the filename away, the result (http://site.com/subdir/sites/default/) of which it then uses to prepend to the referenced files. So e.g. if foobar.css references cat.png, it will concatenate http://site.com/subdir/sites/default and cat.png.
This breaks down as soon as each file gets a unique file URL, e.g. when it is rewritten to be served from a CDN with Far Future expiration headers. http://cdn.com/last-modified:2012-03-10/foobar.css works fine, but because of the above logic, the same URL will be forced upon each referenced file, which is of course very wrong.
2) file_create_url() is not applied to files referenced by CSS files: Drupal 7 core's CSS aggregation breaks modules that want to alter file URLs via hook_file_url_alter() because it does not route file URLs of files referenced by aggregated CSS files (e.g. background images, fonts).
This results in the inability of hook_file_url_alter() to process files referenced by a CSS file.

This breaks the CDN module, and forces many other modules (e.g. to use data URIs) to apply crazy hackery in the theme layer to parse HTML and then modify it.

The CDN module has been shipping with an override for Drupal core's CSS aggregation for over a year, but there are unsolvable compatibility problems with certain modules (e.g. http://drupal.org/project/css_emimage, see #1532178-10: Integrate with CSS Embedded Images module) and themes (e.g. http://drupal.org/project/omega, see #1790348: Far-Future mode: background images and font files referenced in CSS files incorrectly rewritten, but only in an Omega subtheme) because Drupal core forces us to use crazy theme layer hackery to achieve what we need to achieve.

Proposed resolution

Get the parent directory of the file, relative to the Drupal root:

			-	// Build the base URL of this CSS file: start with the full URL.
			-	$css_base_url = file_create_url($stylesheet['data']);
			-	// Move to the parent.
			-	$css_base_url = substr($css_base_url, 0, strrpos($css_base_url, '/'));
			-	// Simplify to a relative URL if the stylesheet URL starts with the
			-	// base URL of the website.
			-	if (substr($css_base_url, 0, strlen($GLOBALS['base_root'])) == $GLOBALS['base_root']) {
			-	$css_base_url = substr($css_base_url, strlen($GLOBALS['base_root']));
			-	}
			-
			+	// Get the parent directory of this file, relative to the Drupal root.
			+	$css_base_url = drupal_substr($stylesheet['data'], 0, strrpos($stylesheet['data'], '/'));

Route file URLs of files referenced by aggregated CSS files:

			-	return 'url(' . $path . ')';
			+ 	return 'url(' . file_create_url($path) . ')';

The resulting URL would now be absolute instead of relative (by default).

Remaining tasks

Patch needs to be re-tested.

User interface changes

The resulting URL would be absolute instead of relative (by default).

API changes

N/A

Comment	File	Size	Author
#32	css_aggregation_breaks_file_url_rewriting-1961340-D7-32.patch	1.44 KB	bradjones1
#32
#31	css_aggregation_breaks_file_url_rewriting-1961340-D7-31.patch	1.56 KB	mindcraft
#31
#26	css_aggregation_breaks_file_url_rewriting-1961340-26-D7.patch	1.66 KB	Wim Leers
#26
#17	css_ipZYI8mTpR6wbdRpFyWTf9ComDqzoOEcXneZw5qxiaY.css_.txt	8.37 KB	mrP
#10	css_aggregation_breaks_file_url_rewriting-1961340-10.patch	1.68 KB	Wim Leers
#10
#1	css_aggregation_breaks_file_url_rewriting-1961340-1.patch	1.68 KB	Wim Leers
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 4 April 2013 at 14:57

Status:

Active

» Needs review

File	Size
css_aggregation_breaks_file_url_rewriting-1961340-1.patch	1.68 KB

And… patch!

I've been wanting to file this patch for many, many months. At last :)

Comment #2

mrP CreditAttribution: mrP commented 9 April 2013 at 15:16

looks good to me. I just applied it against 7.23-dev and tested core css aggregation.

Comment #3

fubhy CreditAttribution: fubhy commented 9 April 2013 at 15:26

This is indeed required. It does not only solve some incompatibility issues in contrib but is simply the right way of solving this I guess. I am not sure if we need more discussion here so will leave it at NR but it's straightforward so would get +1 RTBC from me.

Comment #4

pjcdawkins CreditAttribution: pjcdawkins commented 9 April 2013 at 15:32

+        // Get the parent directory of this file, relative to the Drupal root.
+        $css_base_url = drupal_substr($stylesheet['data'], 0, strrpos($stylesheet['data'], '/'));

Tiny thing: surely drupal_substr() is not needed here, because you're only cutting the string based on strrpos() (Unicode isn't relevant because the only character that matters is '/'). Replace with substr().

Comment #5

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 9 April 2013 at 16:36

#4: I'll reroll to fix that. I'd like more feedback first though. At least one more "RTBC+1".

Comment #6

msonnabaum CreditAttribution: msonnabaum commented 11 April 2013 at 13:58

Version:

7.x-dev

» 8.x-dev

Yes, the approach here looks sound to me.

My only feedback is that I do think it should go in d8 first. I think it's quite likely that this will not get replaced in time.

Comment #7

Damien Tournoud CreditAttribution: Damien Tournoud commented 11 April 2013 at 14:06

So, we are transforming all relative URLs into absolute URLs? That doesn't sound fabulous for performance. Couldn't we fix the relative resolving code instead of removing it?

Also:

+        // Get the parent directory of this file, relative to the Drupal root.
+        $css_base_url = drupal_substr($stylesheet['data'], 0, strrpos($stylesheet['data'], '/'));

Don't mix drupal_* and non-drupal_* string functions. Never, ever. The original code was right.

Comment #8

Damien Tournoud CreditAttribution: Damien Tournoud commented 11 April 2013 at 14:07

Status:

Needs review

» Needs work

Comment #9

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 11 April 2013 at 14:42

(Thanks, I was going to change it to NW :))

So, we are transforming all relative URLs into absolute URLs? That doesn't sound fabulous for performance. Couldn't we fix the relative resolving code instead of removing it?

I'm not sure I understand. It is the original code that first builds an absolute URL for the CSS file (e.g. http://site.com/foo/bar/baz.css), then strips out the CSS file (http://site.com/foo/baz/) and appends referenced files to this URL — $base in _drupal_build_css_path() — (http://site.com/foo/baz/referenced_image.png). I.e. the result assumes that the URL to the referenced files is 99% the same. That's a poor assumption to make.
(As explained in the OP.)

The proposed patch doesn't make this assumption, and instead of having $base === 'http://site.com/foo/bar' (typically simplified to $base === '/foo/bar', which is still a URL, not a file path!), it just works with the file path ($base === 'foo/bar'). Yes, file_create_url() will need to be called for each referenced file, but this only happens when *building* a CSS aggregate, which is a rare operation. That is not a performance problem. It's a necessity. The current implementation is a bug.

If you mean that the *resulting* URL would now be absolute instead of relative (by default): yes, that's true. I don't see how that is a performance problem.

Comment #10

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 11 April 2013 at 14:46

Status:

Needs work

» Needs review

File	Size
css_aggregation_breaks_file_url_rewriting-1961340-10.patch	1.68 KB

Reroll that addresses #4 (bug) & #6 (D8, not D7). The latter half of #7 was a duplicate of #4.

Comment #11

11 April 2013 at 16:41

Status:

Needs review

» Needs work

The last submitted patch, css_aggregation_breaks_file_url_rewriting-1961340-10.patch, failed testing.

Comment #12

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 11 April 2013 at 17:19

Issue tags:

+Needs backport to D7

Drupal\locale\Tests\LocaleUpdateTest did not complete due to a fatal error. Appears to be unrelated.

Comment #13

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 12 April 2013 at 08:13

Status:

Needs work

» Needs review

#10: css_aggregation_breaks_file_url_rewriting-1961340-10.patch queued for re-testing.

Comment #14

Damien Tournoud CreditAttribution: Damien Tournoud commented 12 April 2013 at 12:07

Ok, that sounds good. How practical is it to build a test for this?

Comment #15

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 15 April 2013 at 09:12

\Drupal\system\Tests\File\UrlRewritingTest provides test coverage for hook_file_url_alter().

AFAICT, there shockingly is zero CSS aggregation test coverage. So: at the moment it is not practical at all. If there were such tests, it should be trivial to add test coverage for the particular changes that this patch is introducing. It also explains why the patch is green.

I don't recall writing any tests for Drupal that depend on the file system, so I'm not sure if there's special care needed to make that work.

Comment #16

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 14 May 2013 at 09:53

We're a month further now. Please review!

Comment #17

mrP CreditAttribution: mrP commented 15 May 2013 at 15:44

File	Size
css_ipZYI8mTpR6wbdRpFyWTf9ComDqzoOEcXneZw5qxiaY.css_.txt	8.37 KB

Tested against D8 as of http://drupalcode.org/project/drupal.git/commit/be9552daecd25adea9a68e16...

The file paths in the aggregated css are correct. Attached is one of the the aggregated css files from a default bartik install to demonstrate this.

+1 RTBC

Comment #18

benjifisher

he/him or they/them

English

Boston area

CreditAttribution: benjifisher commented 24 May 2013 at 00:10

#10: css_aggregation_breaks_file_url_rewriting-1961340-10.patch queued for re-testing.

Comment #19

dcam CreditAttribution: dcam commented 24 May 2013 at 02:48

Issue tags:

+Needs issue summary update

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

The summary may need to be updated with information from comments.

Comment #20

OldAccount CreditAttribution: OldAccount commented 25 May 2013 at 20:59

Issue tags:

-Needs issue summary update

Updated issue summary.

Comment #21

mradcliffe

he/him

English

CreditAttribution: mradcliffe commented 3 June 2013 at 01:58

I think this needs a re-roll now. The comment in #17 suggests that this is RTBC if it still passes tests.

Edit: I meant re-test, not re-roll.

Comment #22

mradcliffe

he/him

English

CreditAttribution: mradcliffe commented 3 June 2013 at 01:59

#10: css_aggregation_breaks_file_url_rewriting-1961340-10.patch queued for re-testing.

Comment #23

mikeytown2 CreditAttribution: mikeytown2 commented 3 June 2013 at 18:24

Status:

Needs review

» Reviewed & tested by the community

Passes tests.

Comment #24

Dries CreditAttribution: Dries commented 4 June 2013 at 01:12

Status:

Reviewed & tested by the community

» Fixed

Committed to 8.x. Thanks.

Comment #25

David_Rothstein CreditAttribution: David_Rothstein commented 4 June 2013 at 04:19

Version:	8.x-dev	» 7.x-dev
Status:	Fixed	» Patch (to be ported)

Comment #26

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 9 August 2013 at 08:52

Status:	Patch (to be ported)	» Needs review
Issue tags:	-Needs backport to D7

File	Size
css_aggregation_breaks_file_url_rewriting-1961340-26-D7.patch	1.66 KB

I manually hacked the D8 patch at #10 into a D7 patch. This should work :)

Comment #27

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 9 August 2013 at 08:54

Issue tags:

+CSS aggregation

Comment #28

das-peter CreditAttribution: das-peter commented 9 August 2013 at 09:44

Status:

Needs review

» Reviewed & tested by the community

The solution is the same as was committed to D8, the patch applies, tests are green -> I'd say RTBC.

Comment #29

mikeytown2 CreditAttribution: mikeytown2 commented 15 October 2013 at 21:01

Status:

Reviewed & tested by the community

» Needs work

Setting status to "needs work" as the CDN module has some issues with this logic
#1514182: IE @font-face CSS hack URL broken by CDN module's overridden CSS aggregation
And now AdvAgg after using similar code found in #26
#2112067: Paths embedded in CSS are unnecessarily encoded (and now decoded)

Comment #29.0

mikeytown2 CreditAttribution: mikeytown2 commented 15 October 2013 at 21:01

Issue summary:

View changes

Updated issue summary.

Comment #30

RaulMuroc CreditAttribution: RaulMuroc commented 28 February 2014 at 10:33

Patch in #26, when I apply like: git apply -v [patch] it returns it is corrupt in line 14.

I applied the changes manually.

Thank you.

Comment #31

mindcraft CreditAttribution: mindcraft commented 25 June 2014 at 04:39

File	Size
css_aggregation_breaks_file_url_rewriting-1961340-D7-31.patch	1.56 KB

Applied patch in #26 manually into Drupal version 7.23. Here attach the patch file.

Comment #32

bradjones1

English

CreditAttribution: bradjones1 commented 27 June 2014 at 02:21

Assigned:	Wim Leers	» Unassigned
Status:	Needs work	» Reviewed & tested by the community

File	Size
css_aggregation_breaks_file_url_rewriting-1961340-D7-32.patch	1.44 KB

The patch in #31 was a -p2, not a -p1, so here it is re-rolled. This is a straight backport of the D8 patch so I agree with this being RTBC; as for contrib modules having their own CSS aggregation quirks, that's not something that should hold up a patch to core IMHO.

Comment #33

mikeytown2 CreditAttribution: mikeytown2 commented 27 June 2014 at 16:27

The issue isn't 3rd party modules and aggregation quirks; It's browsers not being able to read encoded urls inside of CSS.

This

return 'url(' . file_create_url($path) . ')';

Should be this (from _advagg_build_css_path())

  $path = file_create_url($path);
  // Decoding path because some browsers can not handle encoded urls inside of CSS.
  $path = rawurldecode($path);
  return 'url(' . $path . ')';

Comment #34

8 July 2014 at 11:52

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch, 32: css_aggregation_breaks_file_url_rewriting-1961340-D7-32.patch, failed testing.

Comment #35

David_Rothstein CreditAttribution: David_Rothstein commented 10 July 2014 at 20:28

This is back to needs work thanks to a testbot fluke, but based on the above comment it sounds like it needs more discussion anyway.

Also:

If you mean that the *resulting* URL would now be absolute instead of relative (by default): yes, that's true. I don't see how that is a performance problem.

I think the issue isn't performance but rather security. Testing this patch on a site that supports both HTTP and HTTPS connections, you can easily run into a situation where the aggregated CSS file will have url(http://example.com...) in it (because it was originally generated on a non-SSL page), but then that aggregated CSS gets reused on the SSL page, leading the browser to give warnings about mixed secure/nonsecure content...

I haven't tested but I assume this affects Drupal 8 too?

Comment #36

mikeytown2 CreditAttribution: mikeytown2 commented 10 July 2014 at 20:57

Would we just need to add $GLOBALS['is_https'] to the $data variable before it gets hashed in https://api.drupal.org/api/drupal/includes!common.inc/function/drupal_build_css_cache/7">drupal_build_css_cache() for $filename or is it more complicated than that for http/https? For AdvAgg I include $GLOBALS['is_https'] and $GLOBALS['base_path'] and $GLOBALS['language']->language if locale is enabled.

Comment #37

SolomonGifford CreditAttribution: SolomonGifford commented 28 July 2014 at 12:44

Following up on David_Rothstein's comment, I think putting absolute urls in the css file is a bad idea for the SSL reason. Some high performance websites may choose to only serve some pages via https. This patch would totally break such sites (or force all css files to be loaded via https).

Additionally, sites that mirror content (or that generate content on a staging server) will no longer be true mirrors since the css will refer to the source domain and not the mirrored domain.

Comment #38

mikeytown2 CreditAttribution: mikeytown2 commented 24 October 2014 at 22:02

Heads up that this might have an issue with SVG that needs to be fixed as well #2362643: Drupal alters svg fill paths with base url -> broken svg

Comment #39

grendzy CreditAttribution: grendzy commented 13 November 2014 at 03:37

Version:

7.x-dev

» 8.0.x-dev

This is broken again (CSS has absolute URLs) in D8 due to #352951: Make JS & CSS Preprocessing Pluggable.

Comment #40

YesCT CreditAttribution: YesCT commented 13 November 2014 at 07:12

This might need a new separate issue (that can be "related" to this one).
function processFile() looks like it attempted to keep this fix.
Is it possible to add a test for this?

Comment #41

grendzy CreditAttribution: grendzy commented 14 November 2014 at 05:23

Version:

8.0.x-dev

» 7.x-dev

Ok, I created a new issue: #2375103: Regression: aggregated CSS should use server-relative URLs inside url() calls within the CSS file.

Comment #42

hass CreditAttribution: hass commented 14 November 2014 at 20:07

Version:

7.x-dev

» 8.0.x-dev

Comment #43

mikeytown2 CreditAttribution: mikeytown2 commented 14 November 2014 at 21:03

Comment #44

valthebald

Sofia

CreditAttribution: valthebald commented 25 September 2015 at 10:48

Issue tags:

+Barcelona2015

Evaluating during Friday sprint in Barcelona

Comment #45

jeroenmarinus

Antwerp

CreditAttribution: jeroenmarinus at Mia Interactive commented 25 September 2015 at 10:59

Version:

8.0.x-dev

» 7.x-dev

Seeing as this has it's own D8 issue now, tagging against D7 again. Not sure why it was set back to D8 in the first place...

Comment #46

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented 25 November 2015 at 15:59

I think it was pushed back to D8 because of the regressions mentioned above... but now there are a couple different issues that aim to fix those. Linking to those here.

Comment #47

3 August 2016 at 00:04

Dries committed 0756534 on 8.3.x

Issue #1961340 by Wim Leers, mrP: Fixed CSS aggregation breaks file URL...

Comment #48

3 August 2016 at 00:05

Dries committed 0756534 on 8.3.x

Issue #1961340 by Wim Leers, mrP: Fixed CSS aggregation breaks file URL...

Comment #49

27 January 2017 at 17:53

Dries committed 0756534 on 8.4.x

Issue #1961340 by Wim Leers, mrP: Fixed CSS aggregation breaks file URL...

Comment #50

27 January 2017 at 17:52

Dries committed 0756534 on 8.4.x

Issue #1961340 by Wim Leers, mrP: Fixed CSS aggregation breaks file URL...

CSS aggregation breaks file URL rewriting because it abuses it

Problem/Motivation

Proposed resolution

Remaining tasks

User interface changes

API changes

Related Issues

Comments

Related issues

Referenced by