Skip to content

fix: CSSTidy ImportantComments not handled properly#359

Merged
ezyang merged 4 commits intoezyang:masterfrom
Wolfrank1149:fixCommentCssTidy
Jan 22, 2023
Merged

fix: CSSTidy ImportantComments not handled properly#359
ezyang merged 4 commits intoezyang:masterfrom
Wolfrank1149:fixCommentCssTidy

Conversation

@Wolfrank1149
Copy link
Copy Markdown
Contributor

@Wolfrank1149 Wolfrank1149 commented Jan 11, 2023 •
edited
Loading

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.

@Wolfrank1149 Wolfrank1149 marked this pull request as draft January 11, 2023 18:29
@Wolfrank1149
fix: CSSTidy ImportantComments not handled properly
a9431ac 
Signed-off-by: Francis Lévesque <wolfrank2164@gmail.com>
@Wolfrank1149 Wolfrank1149 marked this pull request as ready for review January 11, 2023 19:19
ezyang
ezyang requested changes Jan 12, 2023
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

Wolfrank1149 commented Jan 12, 2023

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
Copy Markdown
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
Copy Markdown
Contributor Author

Wolfrank1149 commented Jan 16, 2023 •
edited
Loading

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.

ezyang
ezyang approved these changes Jan 16, 2023
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
@semantic-release-bot
chore(release): 4.17.0 [skip ci]
bbc513d 
# [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)
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Nov 17, 2023

🎉 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

Reviewers

@ezyang ezyang ezyang approved these changes

Assignees

No one assigned

Labels

released

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Wolfrank1149 @ezyang