Skip to content

Fixed filter failure on unescaped urls #123

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.

Sign up for GitHub

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

Open
juniwalk wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fixed filter failure on unescaped urls #123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 17 additions & 25 deletions WebLoader/Filter/CssUrlsFilter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public function setBasePath($basePath)
public function absolutizeUrl($url, $quote, $cssFile)
{
// is already absolute
if (preg_match('/^([a-z]+:\/)?\//', $url)) {
if (preg_match('/^([a-z]+:|\/)/i', $url)) {
Copy link
Collaborator

@fprochazka fprochazka Feb 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like a separate issue

Copy link
Author

@juniwalk juniwalk Feb 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just extension, I removed data uri filtering from the regexp so I filter it here.

Copy link
Collaborator

@fprochazka fprochazka Feb 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you think you could add some more tests for this method, so it's obviouse what you were tryting to solve?

Copy link
Author

@juniwalk juniwalk Feb 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly would you want? Paths from all urls are passed into this match, if it is https? or data uri, it is returned as is, if not, the path is then parsed further.

Copy link
Collaborator

@fprochazka fprochazka Feb 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like some unit tests for this method only.

Copy link
Author

@juniwalk juniwalk Feb 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added 2 assertions into testAbsolutizeAbsolutized, should be sufficient.

return $url;
}

Expand All @@ -71,7 +71,7 @@ public function absolutizeUrl($url, $quote, $cssFile)

$path = $this->cannonicalizePath($path);

return $quote === '"' ? addslashes($path) : $path;
return !$quote ? addslashes($path) : $path;
}

/**
Expand Down Expand Up @@ -107,30 +107,22 @@ public function cannonicalizePath($path)
*/
public function __invoke($code, \WebLoader\Compiler $loader, $file = null)
{
// thanks to kravco
$regexp = '~
(?<![a-z])
url\( ## url(
\s* ## optional whitespace
([\'"])? ## optional single/double quote
(?!data:) ## keep data URIs
( (?: (?:\\\\.)+ ## escape sequences
| [^\'"\\\\,()\s]+ ## safe characters
| (?(1) (?!\1)[\'"\\\\,() \t] ## allowed special characters
| ^ ## (none, if not quoted)
)
)* ## (greedy match)
)
(?(1)\1) ## optional single/double quote
\s* ## optional whitespace
\) ## )
~xs';

$self = $this;

return preg_replace_callback($regexp, function ($matches) use ($self, $file)
$regexp = '/
url\( ## url(
\s* ## optional space
([\'"])? ## optional quote
([^\)]+) ## anything up to ")" character
Copy link
Collaborator

@fprochazka fprochazka Feb 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO this can be simplified, but you've simplified it too much. It must match anything, including escaping characters, except the end of the quotation and ending bracked.

something like this should work url('img\'/layer2.jpg') and also this url("img/\"layer1.jpg")

Copy link
Author

@juniwalk juniwalk Feb 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well well well, this should pass in ok, I've added it into style.css (so it is tested) but looks like I've discovered another problem, url("img/\"layer1,jpg") gets fixed to url('img/\\\"layer1.jpg') which is wrong, right?
Problem is addslashes:

return $quote === '"' ? addslashes($path) : $path;

(?(1)\1) ## optional quote
\s* ## optional space
\) ## )
/ix';

return preg_replace_callback($regexp, function ($matches) use ($file)
{
return "url('" . $self->absolutizeUrl($matches[2], $matches[1], $file) . "')";
$path = trim($matches[2]); // Remove new lines, spaces, etc
$path = trim($path, '\'"'); // Remove quotes on each end (if any)

return "url('" . $this->absolutizeUrl($path, $matches[1], $file) . "')";
}, $code);
}

Expand Down
36 changes: 29 additions & 7 deletions tests/Filter/CssUrlsFilterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

namespace WebLoader\Test\Filter;

use WebLoader\Compiler;
use WebLoader\DefaultOutputNamingConvention;
use WebLoader\FileCollection;
use WebLoader\Filter\CssUrlsFilter;

class CssUrlsFilterTest extends \PHPUnit_Framework_TestCase
Expand All @@ -12,12 +15,16 @@ class CssUrlsFilterTest extends \PHPUnit_Framework_TestCase

protected function setUp()
{
$this->object = new CssUrlsFilter(__DIR__ . '/..', '/');
$this->filter = new CssUrlsFilter(__DIR__ . '/..', '/');

$files = new FileCollection(__DIR__ . '/../fixtures');
@mkdir($outputDir = __DIR__ . '/../temp/');
$this->compiler = new Compiler($files, new DefaultOutputNamingConvention(), $outputDir);
}

public function testCannonicalizePath()
{
$path = $this->object->cannonicalizePath('/prase/./dobytek/../ale/nic.jpg');
$path = $this->filter->cannonicalizePath('/prase/./dobytek/../ale/nic.jpg');
$this->assertEquals('/prase/ale/nic.jpg', $path);
}

Expand All @@ -26,10 +33,16 @@ public function testAbsolutizeAbsolutized()
$cssPath = __DIR__ . '/../fixtures/style.css';

$url = 'http://image.com/image.jpg';
$this->assertEquals($url, $this->object->absolutizeUrl($url, '\'', $cssPath));
$this->assertEquals($url, $this->filter->absolutizeUrl($url, '\'', $cssPath));

$abs = '/images/img.png';
$this->assertEquals($abs, $this->object->absolutizeUrl($abs, '\'', $cssPath));
$this->assertEquals($abs, $this->filter->absolutizeUrl($abs, '\'', $cssPath));

$abs = 'https://fonts.googleapis.com/css?family=Source+Sans+Pro:300,400,600,700,300italic,400italic,600italic';
$this->assertEquals($abs, $this->filter->absolutizeUrl($abs, '\'', $cssPath));

$abs = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg==';
$this->assertEquals($abs, $this->filter->absolutizeUrl($abs, '\'', $cssPath));
}

public function testAbsolutize()
Expand All @@ -38,20 +51,29 @@ public function testAbsolutize()

$this->assertEquals(
'/images/image.png',
$this->object->absolutizeUrl('./../images/image.png', '\'', $cssPath)
$this->filter->absolutizeUrl('./../images/image.png', '\'', $cssPath)
);

$this->assertEquals(
'/images/path/to/image.png',
$this->object->absolutizeUrl('./../images/path/./to/image.png', '\'', $cssPath)
$this->filter->absolutizeUrl('./../images/path/./to/image.png', '\'', $cssPath)
);
}

public function testAbsolutizeOutsideOfDocRoot()
{
$path = './../images/image.png';
$existingPath = __DIR__ . '/../../Compiler.php';
$this->assertEquals($path, $this->object->absolutizeUrl($path, '\'', $existingPath));
$this->assertEquals($path, $this->filter->absolutizeUrl($path, '\'', $existingPath));
}

public function testInvoke()
{
$cssPath = __DIR__ . '/../fixtures/style.css';
$code = file_get_contents($cssPath);

$css = $this->filter->__invoke($code, $this->compiler, $cssPath);

$this->assertSame(file_get_contents(__DIR__ . '/../fixtures/style.css.expected'), $css);
}
}
23 changes: 23 additions & 0 deletions tests/fixtures/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
@import url(https://fonts.googleapis.com/css?family=Source+Sans+Pro:300,400,600,700,300italic,400italic,600italic);

body {
background: url(img/boxed-bg.jpg);
}

.greedy-match {
background: url("img/\"layer1.jpg"), url('img\'/layer2.jpg');
}

.addslashes-if-unquoted {
background: url(img/'layer1.jpg);
}

.quote-mismatch {
background: url("img/layer2.jpg');
}

ul {
list-style-image: url(
data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg==
);
}
21 changes: 21 additions & 0 deletions tests/fixtures/style.css.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
@import url('https://fonts.googleapis.com/css?family=Source+Sans+Pro:300,400,600,700,300italic,400italic,600italic');

body {
background: url('/fixtures/img/boxed-bg.jpg');
}

.greedy-match {
background: url('/fixtures/img/\"layer1.jpg'), url('/fixtures/img\'/layer2.jpg');
}

.addslashes-if-unquoted {
background: url('/fixtures/img/\'layer1.jpg');
}

.quote-mismatch {
background: url('/fixtures/img/layer2.jpg');
}

ul {
list-style-image: url('data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg==');
}