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.

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

Jump to bottom

Fix(web-twig): Remove raw filter that cause a XSS issue #742

Merged
merged 5 commits into from
Apr 3, 2023
Merged

Fix(web-twig): Remove raw filter that cause a XSS issue #742

merged 5 commits into from
Apr 3, 2023

Conversation

literat
Copy link
Collaborator

@literat literat commented Mar 22, 2023

No description provided.

@literat literat requested a review from kdosiodjinud March 22, 2023 22:02
@github-actions github-actions bot added the bug Something isn't working label Mar 22, 2023
@literat literat marked this pull request as draft March 22, 2023 22:02
@netlify
Copy link

netlify bot commented Mar 22, 2023 •
edited
Loading

✅ Deploy Preview for spirit-design-system-storybook canceled.

Name Link
🔨 Latest commit 0e6d244
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system-storybook/deploys/642a8922307ebc000885f95f

@netlify
Copy link

netlify bot commented Mar 22, 2023 •
edited
Loading

✅ Deploy Preview for spirit-design-system-demo canceled.

Name Link
🔨 Latest commit 0e6d244
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system-demo/deploys/642a8922d03663000736ac05

@literat literat force-pushed the fix/web-twig-raw-issue branch 3 times, most recently from 489fdee to 897a720 Compare March 23, 2023 19:46
@literat literat marked this pull request as ready for review March 23, 2023 20:42
@literat literat requested a review from dlouhak March 23, 2023 20:57
kdosiodjinud
kdosiodjinud reviewed Mar 24, 2023
View reviewed changes
Copy link

@kdosiodjinud kdosiodjinud left a comment

Choose a reason for hiding this comment

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

Please, specify better doc (rule out magical) and answer my question about onClick :) thank you

packages/web-twig/src/Resources/components/Button/Button.twig Show resolved Hide resolved
packages/web-twig/src/Resources/components/CheckboxField/README.md Outdated Show resolved Hide resolved
@literat
Copy link
Collaborator Author

literat commented Mar 24, 2023

@kdosiodjinud I have added a branch which you can test in your application https://github.com/lmc-eu/spirit-web-twig-bundle/tree/fix/web-twig-raw-issue

@literat literat force-pushed the fix/web-twig-raw-issue branch from 340d2f7 to ae65213 Compare March 31, 2023 08:34
vmilersky
vmilersky requested changes Mar 31, 2023
View reviewed changes
packages/web-twig/src/Resources/components/ButtonLink/ButtonLink.twig Outdated Show resolved Hide resolved
packages/web-twig/src/Resources/components/Link/Link.twig Outdated Show resolved Hide resolved
packages/web-twig/src/Resources/components/Modal/Modal.twig Outdated Show resolved Hide resolved
packages/web-twig/src/Twig/PropsExtension.php Outdated Show resolved Hide resolved
packages/web-twig/src/Resources/components/TextFieldBase/TextFieldBase.twig Outdated Show resolved Hide resolved
packages/web-twig/src/Resources/components/Accordion/Accordion.twig Show resolved Hide resolved
packages/web-twig/src/Resources/components/TextFieldBase/README.md Outdated Show resolved Hide resolved
packages/web-twig/src/Resources/components/Button/Button.twig Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Mar 31, 2023 •
edited
Loading

Coverage Status

Coverage: 96.209% (+0.04%) from 96.172% when pulling 0e6d244 on fix/web-twig-raw-issue into bdecd53 on main.

@literat literat force-pushed the fix/web-twig-raw-issue branch from 51881b5 to afb1b3d Compare March 31, 2023 15:51
literat added 3 commits April 3, 2023 10:06
@literat
Fix(web-twig): Remove raw filter that cause a XSS issue
dfe9691
@literat
Feat(web-twig): Introduce UNSAFE props for Field components
36339eb 
  * UNSAFE_label - unescaped label with `raw` filter
  * UNSAFE_helperText - unescaped helper text with `raw` filter
  * UNSAFE_message - unescaped validation message with `raw` filter

refs #DS-645
@literat
Feat(web-twig): Allow event handling attributes via transferring props
e0a1771
literat added 2 commits April 3, 2023 10:06
@literat
Feat(web-twig): Deprecate onClick prop on every component
52ea655 
  * use native `onclick` attribute instead
  * deprecation will be removed in the next major version

refs #DS-645
@literat
Refactor(web-twig): Pass title prop as native attribute through rest
0e6d244 
props
@literat literat force-pushed the fix/web-twig-raw-issue branch from afb1b3d to 0e6d244 Compare April 3, 2023 08:06
@literat literat merged commit 80ac214 into main Apr 3, 2023
@literat literat deleted the fix/web-twig-raw-issue branch April 3, 2023 12:01
literat added a commit that referenced this pull request May 5, 2023
@literat
Fix(web-twig): Custom escaping of HTML attributes
6c7be61 
  * while removing double quotes in #742 we let Twig to escape our
    attributes automagically
  * this lead us to bug where double quotes where missing in HTML output
    for `placeholder` attribute and thus from multiple words of
placeholder only first was displayed
  * we are moving back and returning double quotes to concatenation of
    attributes while also adding the escape functionality
  * because of custom fabrication of the HTML tags and using double
    quotes and Twig's automated escaping we must escape values by
ourselves
  * @see: https://twig.symfony.com/doc/3.x/filters/escape.html ->
    Caution
  * `{{ var|escape(strategy)|raw }} {# won't be double-escaped #}`
  * @see: #742

refs #DS-760
@literat literat mentioned this pull request May 5, 2023
Merged
literat added a commit that referenced this pull request May 9, 2023
@literat
Fix(web-twig): Custom escaping of HTML attributes
fd830fe 
  * while removing double quotes in #742 we let Twig to escape our
    attributes automagically
  * this lead us to bug where double quotes where missing in HTML output
    for `placeholder` attribute and thus from multiple words of
placeholder only first was displayed
  * we are moving back and returning double quotes to concatenation of
    attributes while also adding the escape functionality
  * because of custom fabrication of the HTML tags and using double
    quotes and Twig's automated escaping we must escape values by
ourselves
  * @see: https://twig.symfony.com/doc/3.x/filters/escape.html ->
    Caution
  * `{{ var|escape(strategy)|raw }} {# won't be double-escaped #}`
  * @see: #742

refs #DS-760
literat added a commit that referenced this pull request May 16, 2023
@literat
Fix(web-twig): Custom escaping of HTML attributes
7c355a7 
  * while removing double quotes in #742 we let Twig to escape our
    attributes automagically
  * this lead us to bug where double quotes where missing in HTML output
    for `placeholder` attribute and thus from multiple words of
placeholder only first was displayed
  * we are moving back and returning double quotes to concatenation of
    attributes while also adding the escape functionality
  * because of custom fabrication of the HTML tags and using double
    quotes and Twig's automated escaping we must escape values by
ourselves
  * @see: https://twig.symfony.com/doc/3.x/filters/escape.html ->
    Caution
  * `{{ var|escape(strategy)|raw }} {# won't be double-escaped #}`
  * @see: #742

refs #DS-760
literat added a commit that referenced this pull request May 16, 2023
@literat
Fix(web-twig): Custom escaping of HTML attributes
5d18658 
  * while removing double quotes in #742 we let Twig to escape our
    attributes automagically
  * this lead us to bug where double quotes where missing in HTML output
    for `placeholder` attribute and thus from multiple words of
placeholder only first was displayed
  * we are moving back and returning double quotes to concatenation of
    attributes while also adding the escape functionality
  * because of custom fabrication of the HTML tags and using double
    quotes and Twig's automated escaping we must escape values by
ourselves
  * @see: https://twig.symfony.com/doc/3.x/filters/escape.html ->
    Caution
  * `{{ var|escape(strategy)|raw }} {# won't be double-escaped #}`
  * @see: #742

refs #DS-760
literat added a commit that referenced this pull request May 18, 2023
@literat
Fix(web-twig): Custom escaping of HTML attributes
c827c70 
  * while removing double quotes in #742 we let Twig to escape our
    attributes automagically
  * this lead us to bug where double quotes where missing in HTML output
    for `placeholder` attribute and thus from multiple words of
placeholder only first was displayed
  * we are moving back and returning double quotes to concatenation of
    attributes while also adding the escape functionality
  * because of custom fabrication of the HTML tags and using double
    quotes and Twig's automated escaping we must escape values by
ourselves
  * @see: https://twig.symfony.com/doc/3.x/filters/escape.html ->
    Caution
  * `{{ var|escape(strategy)|raw }} {# won't be double-escaped #}`
  * @see: #742

refs #DS-760
literat added a commit that referenced this pull request May 18, 2023
@literat
Fix(web-twig): Custom escaping of HTML attributes
42176b2 
  * while removing double quotes in #742 we let Twig to escape our
    attributes automagically
  * this lead us to bug where double quotes where missing in HTML output
    for `placeholder` attribute and thus from multiple words of
placeholder only first was displayed
  * we are moving back and returning double quotes to concatenation of
    attributes while also adding the escape functionality
  * because of custom fabrication of the HTML tags and using double
    quotes and Twig's automated escaping we must escape values by
ourselves
  * @see: https://twig.symfony.com/doc/3.x/filters/escape.html ->
    Caution
  * `{{ var|escape(strategy)|raw }} {# won't be double-escaped #}`
  * @see: #742

refs #DS-760
literat added a commit to lmc-eu/spirit-web-twig-bundle that referenced this pull request May 18, 2023
@literat
Fix(web-twig): Custom escaping of HTML attributes
6c00891 
  * while removing double quotes in #742 we let Twig to escape our
    attributes automagically
  * this lead us to bug where double quotes where missing in HTML output
    for `placeholder` attribute and thus from multiple words of
placeholder only first was displayed
  * we are moving back and returning double quotes to concatenation of
    attributes while also adding the escape functionality
  * because of custom fabrication of the HTML tags and using double
    quotes and Twig's automated escaping we must escape values by
ourselves
  * @see: https://twig.symfony.com/doc/3.x/filters/escape.html ->
    Caution
  * `{{ var|escape(strategy)|raw }} {# won't be double-escaped #}`
  * @see: lmc-eu/spirit-design-system#742

refs #DS-760
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kdosiodjinud kdosiodjinud kdosiodjinud left review comments

@vmilersky vmilersky vmilersky approved these changes

@adamkudrna adamkudrna Awaiting requested review from adamkudrna adamkudrna is a code owner

@crishpeen crishpeen Awaiting requested review from crishpeen crishpeen is a code owner

@tomassychra tomassychra Awaiting requested review from tomassychra

@dlouhak dlouhak Awaiting requested review from dlouhak

Assignees

@literat literat

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@literat @coveralls @kdosiodjinud @vmilersky @dlouhak