Skip to content
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.

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

fix: CSSTidy ImportantComments not handled properly #359

Merged
merged 4 commits into from Jan 22, 2023

Conversation

Wolfrank1149
Copy link
Contributor

@Wolfrank1149 Wolfrank1149 commented Jan 11, 2023

Fix for issue #357

Added a check if $decls is an array before the foreach and not keeping the value if it's not.

Also added a unit test for that case.

Signed-off-by: Francis Lévesque <wolfrank2164@gmail.com>
@Wolfrank1149 Wolfrank1149 marked this pull request as ready for review January 11, 2023 19:19
Copy link
Owner

@ezyang ezyang left a comment

Choose a reason for hiding this comment

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

You are bypassing validation of style entirely, therefore introducing a security vulnerability here. You must still validate style.

@Wolfrank1149
Copy link
Contributor Author

The validation is still done, I only skip it for the important comments. Do want me to also check them? If yes, do I validate them like a style or do you have a specifict validation for comments?

@ezyang
Copy link
Owner

ezyang commented Jan 14, 2023

Hmm, ok, so basically it sounds like what you are saying is the decls are string, they are comments? I think I would still feel more comfortable if we limit the valid contents of the comment. Can we allow only !important for your use case?

@Wolfrank1149
Copy link
Contributor Author

Wolfrank1149 commented Jan 16, 2023

Here the out put from print_r($this-_tidy->css) in ExtractStyleBlocks.php when I run the unit test I added.

(
    [!] => ! Important 
! Important2 
    [41] => Array
        (
            [div] => Array
                (
                    [text-align] => right
                )

        )
)

I also decided to simply remove the comments since they have no impact on the code. It's safer and easier to manage.

Copy link
Owner

@ezyang ezyang left a comment

Choose a reason for hiding this comment

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

thank you much appreciated

@ezyang
Copy link
Owner

ezyang commented Jan 17, 2023

Woops our ci is broken

@ezyang ezyang merged commit 78a9b4d into ezyang:master Jan 22, 2023
github-actions bot pushed a commit that referenced this pull request Nov 17, 2023
# [4.17.0](v4.16.0...v4.17.0) (2023-11-17)

### Bug Fixes

* CSSTidy ImportantComments not handled properly ([#359](#359)) ([78a9b4d](78a9b4d))
* fix CI ([#361](#361)) ([9ec687c](9ec687c))
* Invalid scheme check in Attr.TargetBlank ([#363](#363)) ([0176ef4](0176ef4))
* semantic release ([#339](#339)) ([d82f3d9](d82f3d9))
* semantic release ([#341](#341)) ([e55fead](e55fead)), closes [#339](#339)
* Support for locales using decimal separators other than . (dot) ([#372](#372)) ([43f49ac](43f49ac))

### Features

* Add support for all text-decoration properties ([#360](#360)) ([2d775c0](2d775c0))
* Allows commas to be included in tel URI ([#389](#389)) ([ec92490](ec92490)), closes [#388](#388)

### Reverts

* Revert "fix: semantic release (#339)" (#340) ([3e83215](3e83215)), closes [#339](#339) [#340](#340)
Copy link

🎉 This PR is included in version 4.17.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants