url-regex also matches fonts #89

Closed
futtta opened this Issue Mar 19, 2017 · 3 comments

Comments

1 participant
Owner

futtta commented Mar 19, 2017

@zytzagoo next up in our never-ending URL-regex story; the regex also matched font-url's which, if a CDN is set, are also moved to the CDN URL leading to unexpected behavior:

  • just had a tester who had fonts loaded from his own domain in his generated critical CSS but the full AO'ed CSS loaded them from the CDN
  • this could also lead to cross-domain-restrictions blocking fonts being loaded, which is why the font-replacement was behind a filter defaulting to false)

Now to make things harder; the regex is perfect for fixurl's (the font url's have to be fixed after all), but the "imaging"-logic should not apply to fonts. we could have whitelist of allowed extensions that can be cdn'ed (.jpg, .jpeg, .gif, .png, .svg, .bmp)?

What do you think?

Owner

futtta commented Mar 19, 2017

this could be a fix;

            } else if ((is_array($matches)) && (!empty($this->cdn_url))) {
                // change background image urls to cdn-url
                foreach($matches[1] as $count => $quotedurl) {
                    $url = trim($quotedurl," \t\n\r\0\x0B\"'");
                    // only cdn images, fonts are done later
                    if ( preg_match('#\.(jpe?g|png|gif|bmp)$#i',$url) ) {
			$cdn_url=$this->url_replace_cdn($url);
			$imgreplace[$matches[0][$count]] = str_replace($quotedurl,$cdn_url,$matches[0][$count]);
		    }
                }
            }
Owner

futtta commented Mar 19, 2017

committed a way better fix, allowing to remove that entire old block of font-cdn-replacement and removing some duplicate code; 65cd2d1

Owner

futtta commented Mar 19, 2017

confirmed OK by tester mentioned above, closing.

@futtta futtta closed this Mar 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment