Skip to content

Conversation

@mlocati
Copy link
Contributor

@mlocati mlocati commented Sep 26, 2019

Let's assume that:

  • when we have an @import marked as (optional), with a relative path starting with ../
  • the path does not actually exist

In that case, the library tries to look for the existence of that non existing path starting from the current directory.

This is an issue because:

  • the current directory shouldn't affect the CSS generation
  • in case of systems where the open_basedir is set, we may easily go out of the allowed path.

In this pull request, I start adding a test that shows this issue (that is, the TravisCI jobs should fail).

I'll add another commit once the TravisCI jobs complete.

@mlocati
Copy link
Contributor Author

mlocati commented Sep 26, 2019

Here's the failure that's fixed by 383db69

return true;
}
$path = str_replace(DIRECTORY_SEPARATOR, '/', $path);
if ($path[0] === '/') {
Copy link
Member

Choose a reason for hiding this comment

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

I suspect it would be more optimal to skip the str_replace and instead have:

if ( $path === DIRECTORY_SEPARATOR || $path[0] === '/' )

Below the preg_match would then be %^[a-z]:\\%i instead of %^[a-z]:/%i. Depending on whether supportingA:/ on Windows makes sense.

@Krinkle
Copy link
Member

Krinkle commented Mar 19, 2020

I'm currently uncomfortable merging this as-is as I'd like to better understand what ParseImport( $full_path, …) will or should do with URLs, and with absolute paths. It seems that right now for URLs at least, these are effectively disallowed due to the file_exists() check not working for them.

This change as-is would make that possible. There is #27 and #28, which would be best resolved first.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants