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(frontend): security XSS / image tag #4030

Merged
merged 25 commits into from
Dec 10, 2021
Merged

fix(frontend): security XSS / image tag #4030

merged 25 commits into from
Dec 10, 2021

Conversation

maxgfr
Copy link
Member

@maxgfr maxgfr commented Nov 19, 2021 •
edited by carolineBda
Loading

fix #4028
fix #3560

Note : une fois mergé, il faudra vérifier que maintenant la page est valide :
https://validator.w3.org/nu/?doc=https%3A%2F%2Fcode-du-travail-numerique-master.dev.fabrique.social.gouv.fr%2Fmodeles-de-courriers%2Frupture-du-contrat-en-periode-dessai-a-linitiative-du-salarie
Si c'est bon, on peut passer à OK la RGAA 8.2 P10

@github-actions github-actions bot temporarily deployed to code-du-travail-numerique-maxgfr-image-affich-431pjn November 19, 2021 16:23 Inactive
@github-actions github-actions bot temporarily deployed to code-du-travail-numerique-maxgfr-image-affich-431pjn November 19, 2021 18:12 Inactive
m-maillot
m-maillot requested changes Nov 22, 2021
View reviewed changes
Copy link
Contributor

@m-maillot m-maillot left a comment

Choose a reason for hiding this comment

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

Je pense que ça vient de l'assainissement des données, du coup on a bien l'image 👍
Mais par contre on a un autre bug sur la première partie :
image

PS: Top le passage en TSX 👍

@maxgfr
Copy link
Member Author

maxgfr commented Nov 22, 2021

Je pense que ça vient de l'assainissement des données, du coup on a bien l'image 👍 Mais par contre on a un autre bug sur la première partie : image

PS: Top le passage en TSX 👍

Oui, merci 😄

@maxgfr maxgfr requested a review from m-maillot November 22, 2021 16:21
@github-actions github-actions bot temporarily deployed to code-du-travail-numerique-maxgfr-image-affich-431pjn November 22, 2021 16:25 Inactive
carolineBda
carolineBda reviewed Nov 23, 2021
View reviewed changes
Copy link
Contributor

@carolineBda carolineBda left a comment

Choose a reason for hiding this comment

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

A la fin on utilise quand meme dangerouslySetInnerHTML ?

Je vois plusieurs exemple avec html-react-parser, c'était pas mieux ?

packages/code-du-travail-frontend/__tests__/__snapshots__/fiche-ministere-travail.test.js.snap
@@ -2329,7 +2329,9 @@ exports[`<FicheMT /> should render 1`] = `
<div
class="c54 c55"
>
<div>
<div
class=""
Copy link
Contributor

Choose a reason for hiding this comment

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

c'est bizarre que ça rajoute une class vide

Copy link
Member Author

Choose a reason for hiding this comment

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

Oui, c'est vrai, bonne question

Copy link
Member Author

Choose a reason for hiding this comment

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

En vrai, c'est dû au parsing fait par le xss, qui essaye de retirer le plus de choses externes. Mais en soit, relou si ça génère des <div class="" .. dans la prod

Copy link
Member Author

Choose a reason for hiding this comment

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

Dans la prod, il me semble que ça ne génère pas cela, donc pour moi c'est ok j'ai l'impression

Copy link
Contributor

Choose a reason for hiding this comment

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

c'est juste qu'avant on l'ajoutait pas et maintenant oui

@maxgfr
Copy link
Member Author

maxgfr commented Nov 23, 2021 •
edited by carolineBda
Loading

@carolineBda

A la fin on utilise quand meme dangerouslySetInnerHTML ?

Oui, mais on passe des propriétés qui ont été au préalable parse

Je vois plusieurs exemple avec html-react-parser, c'était pas mieux ?

Sur la doc de html-react-parser :

Is this XSS safe?

No, this library is not XSS (cross-site scripting) safe. See #94.

Does invalid HTML get sanitized?

No, this library does not sanitize HTML. See #124, #125, and #141.

Are <script> tags parsed?

Although <script> tags and their contents are rendered on the server-side, they're not evaluated on the client-side. See #98.

Ok 👍

@maxgfr
Copy link
Member Author

maxgfr commented Nov 24, 2021 •
edited
Loading

Balises HTML utilisée sur :

code-du-travail

fiche-ministere-travail

fiche-service-public

glossaire

modeles-de-courrier

contributions avec le composant Answer et Contributions

dossiers avec le composant Answer

informations avec le composant Answer et Accordion

integrations-js avec le composant Accordion

@github-actions github-actions bot temporarily deployed to code-du-travail-numerique-maxgfr-image-affich-431pjn November 25, 2021 15:33 Inactive
@github-actions github-actions bot temporarily deployed to code-du-travail-numerique-maxgfr-image-affich-431pjn November 25, 2021 16:06 Inactive
@github-actions github-actions bot temporarily deployed to code-du-travail-numerique-maxgfr-image-affich-431pjn November 25, 2021 18:41 Inactive
@github-actions github-actions bot temporarily deployed to code-du-travail-numerique-maxgfr-image-affich-431pjn November 25, 2021 18:54 Inactive
@github-actions github-actions bot temporarily deployed to code-du-travail-numerique-maxgfr-image-affich-431pjn November 26, 2021 10:15 Inactive
@github-actions github-actions bot temporarily deployed to code-du-travail-numerique-maxgfr-image-affich-431pjn December 1, 2021 17:28 Inactive
@github-actions github-actions bot temporarily deployed to code-du-travail-numerique-maxgfr-image-affich-431pjn December 1, 2021 17:47 Inactive
@github-actions github-actions bot temporarily deployed to code-du-travail-numerique-maxgfr-image-affich-431pjn December 2, 2021 15:49 Inactive
@maxgfr
Copy link
Member Author

maxgfr commented Dec 2, 2021

Les class="" ne semble pas venir du code traité pas la balise Html. J'essaye de récupérer l'information de ces div, et je n'y arrive pas. Je ne comprends pas pq les snapshot réagissent comme cela. (cc: @carolineBda @m-maillot)

@github-actions github-actions bot temporarily deployed to code-du-travail-numerique-maxgfr-image-affich-431pjn December 2, 2021 16:26 Inactive
@carolineBda
Copy link
Contributor

ok pour moi. Dommage pour les class vides mais c'est pas primordial du tout. @m-maillot tu peux review la PR du coup

@github-actions
Copy link

github-actions bot commented Dec 9, 2021

🎉 Deployment for commit fe8f3f3 :

Ingresses
Docker images
  • 📦 docker pull ghcr.io/socialgouv/cdtn/code-du-travail-api:sha-fe8f3f3a08fe825ae9cdc3a078a902a6de7695ca
  • 📦 docker pull ghcr.io/socialgouv/cdtn/code-du-travail-frontend:sha-fe8f3f3a08fe825ae9cdc3a078a902a6de7695ca
  • 📦 docker pull ghcr.io/socialgouv/docker/wait-for-http:6.56.1
Debug

@maxgfr maxgfr merged commit 28c122d into master Dec 10, 2021
@maxgfr maxgfr deleted the maxgfr/image-affichage branch December 10, 2021 08:44
rmelisson pushed a commit that referenced this pull request Jan 25, 2022
@maxgfr @m-maillot @rmelisson
fix(frontend): security XSS / image tag (#4030)
e230524 
Co-authored-by: Martial Maillot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carolineBda carolineBda carolineBda approved these changes

@m-maillot m-maillot m-maillot approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Security] Html parsing Problème d'affichage fiche MT "Activité partielle – chômage partiel"
3 participants
@maxgfr @carolineBda @m-maillot