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 XSS when using setFormatValue($callable) #6255

Open
wants to merge 2 commits into
base: 4.x
Choose a base branch
from

Conversation

allan-simon
Copy link

@allan-simon allan-simon commented Apr 11, 2024

instead if people do need it we force them do to

->setStripTag(true) before

Example to reproduce create two entities Comment and Author in the Comment crud controller do the following

        yield AssociationField::new('author', 'Author of the comment')
            ->formatValue(function ($value, Author $author) {
                 return $author->getName();
            });

if the author's name contains <script>alert("coucou")</script> it will inject a XSS in the admin

instead with this PR you will be required to do

        yield AssociationField::new('author', 'Author of the comment')
           ->stripTag(false)
            ->formatValue(function ($value, Author $author) {
                 return $author->getName();
            });

if you want to intentionnaly disable it

…ing twig default escape by wrapping the string in a "Markup" object that is whitelisted

instead if people do need it we force them do to

->setStripTag(true) before
@allan-simon allan-simon changed the title 4.x Fix XSS when using setFormatValue($callable) Apr 11, 2024
…ing twig default escape by wrapping the string in a "Markup" object that is whitelisted

instead if people do need it we force them do to

->setStripTag(true) before
@allan-simon
Copy link
Author

here I really think it's a bug , because TextField::setFormatValue is safe by default , while associationField::setFormatValue is not

@allan-simon
Copy link
Author

@javiereguiluz do you agree with me here (so that I know if it's worth it to fix the PR )

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

Successfully merging this pull request may close these issues.

1 participant