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

[Technique] Améliorer la CSP #2626

Merged
merged 14 commits into from
Jun 14, 2024
Merged

[Technique] Améliorer la CSP #2626

merged 14 commits into from
Jun 14, 2024

Conversation

hmeneuvrier
Copy link
Collaborator

@hmeneuvrier hmeneuvrier commented Jun 3, 2024

Ticket

#2609

Description

Tentative d'avoir une CSP plus stricte
⚠️ Il reste des erreurs à l'utilisation de tinyMCE

💡 Je viens de penser qu'en ne générant pas de nonces pour les styles, on n'aurait peut-être moins d'erreurs sur tinyMCE... car je viens de réaliser que j'ai fini par externaliser tous les styles et que je n'utilise plus nonce="{{ app.request.attributes.get('csp_style_nonce') }}" ... à tester (et vérifier ensuite la CSP générée sur https://csp-evaluator.withgoogle.com/?csp=https://histologe.beta.gouv.fr pour voir si elle est meilleure... car il a l'air de ne pas trop tiquer sur l'utilisation de unsafe-inline sur les styles
EDIT : effectivement il ne reste que l'erreur dont je ne trouve pas la provenance :
image

⚠️ j'ai essayé d'ajouter 'strict-dynamic' dans script-src comme conseillé par CPS evaluator, mais ça empêche totalement l'utilisation de tinyMCE et tippy

Changements apportés

  • Génération d'une CSP via un listener src/EventListener/ContentSecurityPolicyListener.php et ajout de nonces à la volée, les différents paramètres étant définis dans config/app/csp.yaml (et ils sont plus stricts)
  • Externalisation de tous les scripts inline pouvant l'être
  • Externalisation de tous les styles inline pouvant l'être
  • Utilisation de nonces sur tous les styles et les scripts ne pouvant être externalisés (certains pourraient l'être en fait)
  • Utilisation de <image à la place de <use dans les svg
  • Passage à la dernière version de tinyMCE en CDN (et suppression de 87 fichiers de tinyMCE)

Pré-requis

Supprimer les variables SECURITY_CSP_HEADER_VALUE des fichiers d'environnement
npm run watch

Tests

⚠️ Les tests sont à faire console ouverte pour voir les erreurs en plus de l'aspect fonctionnel. Il y a toujours une erreur pour matomo en local, car je n'ai pas mis localhost dans la CSP

  • Tester l'aspect global de la plateforme (footer, page about etc.)
  • Tester le fonctionnement des apps VueJS (stats, formulaire, dashboard)
  • Tester les filtres sur la carto (et son affichage) et sur l'ancienne liste
  • Tester l'utilisation de tinyMCE (ajout de suivi, de visites etc.)
  • Tester l'utilisation de tippy (roll-over sur les images)
  • Tester l'acceptation / refus de signalement et d'affectation
  • Tester la suppression de la notice sur la page de suivi de signalement
  • Tester le changement de mail de partenaire et utilisateur (l'appel à checkMail)
  • Tester la suppression d'utilisateur et de partenaire
  • Regarder en fonction des modifications si je n'ai pas oublié des choses à tester :)
  • Faire des tests sur plusieurs navigateurs car require-trusted-types-for n'est pas pris en compte par tous

@hmeneuvrier hmeneuvrier force-pushed the feature/2609-technique-csp branch 3 times, most recently from cbf20af to a261510 Compare June 6, 2024 12:48
.env Outdated Show resolved Hide resolved
.env.sample Outdated Show resolved Hide resolved
@@ -0,0 +1,16 @@
parameters:
csp_parameters:
default-src: "'none'"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Donc la CSP est maintenant définie dans ce fichier. Par défaut tout est à none, et on accepte que ce dont on a besoin

parameters:
csp_parameters:
default-src: "'none'"
script-src: "'self' 'unsafe-inline' 'unsafe-eval' https://cdn.matomo.cloud/histologe.matomo.cloud/matomo.js https://cdn.jsdelivr.net/npm/leaflet@1.7.1/dist/ https://cdn.jsdelivr.net/npm/leaflet.markercluster@1.5.3/dist/ https://cdn.jsdelivr.net/npm/@popperjs/core@2.11.8/dist/ https://cdn.jsdelivr.net/npm/tippy.js@6/dist/ https://cdn.jsdelivr.net/npm/quill@2.0.2/dist/ https://cdn.jsdelivr.net/npm/tinymce@7.1.2/"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

on garde 'unsafe-unline' pour la rétrocompatibilité avec les anciens navigateurs, mais elle n'est pas utilisée pour les navigateurs qui acceptent les nonces (qui sont ajoutés dans le service), d'où les soucis avec tinyMCE
on garde 'unsafe-eval' pour le formulaire
On cible chacun des dossiers dont on a besoin plutôt qu'un général https://cdn.jsdelivr.net

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Le fait d'utiliser des nonces pour les script et les styles désactivent donc le unsafe-inline sur les navigateurs récents. D'où le besoin d'externaliser au maximum les scripts et les styles dans d'autres fichiers chargés


.fr-display-none {
display: none;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ajout de style pour virer des styles inline par ailleurs

@@ -0,0 +1,56 @@
const setBadge = (el) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

externalisation d'un script

@@ -62,6 +63,11 @@ services:
- '../src/Kernel.php'
- '../src/Tests/'

App\EventListener\ContentSecurityPolicyListener:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ajout du service

@@ -210,11 +210,6 @@ document?.querySelectorAll(".fr-pagination__link").forEach((e => {
}))))
}))
}));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tout le code sur les badges a été déplacé dans assets/controllers/search_filter_form.js

}
badge.remove();
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tout le code sur les badges a été déplacé dans assets/controllers/search_filter_form.js

$request = $event->getRequest();

$scriptNonce = bin2hex(random_bytes(16));
$styleNonce = bin2hex(random_bytes(16));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

création de nouveaux nonces à chaque requête

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

seulement pour les scripts

'form-action '.$cspParameters['form-action'].'; '.
'frame-ancestors '.$cspParameters['frame-ancestors'].'; '.
'media-src '.$cspParameters['media-src'].'; '.
'require-trusted-types-for '.$cspParameters['require-trusted-types-for'];
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

formation de la csp avec ce qui est dans le yaml en ajoutant les noncees

@@ -137,7 +137,7 @@
<label class="fr-label" for="signalement-edit-nde-conso-energie">
Consommation énergétique
</label>
<input class="fr-input fr-col-10" style="display: inline;" pattern="[0-9]*" inputmode="numeric" type="number" id="signalement-edit-nde-conso-energie" name="consommationEnergie" value="{{ signalementQualificationNDE.details.consommation_energie }}">
<input class="fr-input fr-col-10 fr-display-inline" pattern="[0-9]*" inputmode="numeric" type="number" id="signalement-edit-nde-conso-energie" name="consommationEnergie" value="{{ signalementQualificationNDE.details.consommation_energie }}">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

on utilise une classe css plutôt que du style inline

@@ -73,7 +73,7 @@
<div class="modal-upload-list-section" id="modal-upload-file-dynamic-content">
<div class="modal-upload-list"></div>
</div>
<div style="display:none">
<div class="fr-display-none">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

idem

@@ -54,7 +54,7 @@
{% if is_granted('ROLE_ADMIN') %}
<div class="fr-col-12 fr-col-md-6">
<label class="fr-label" for="bo-filters-territories"><strong>Filtrer par territoire</strong></label>
<select id="bo-filters-territories" class="fr-select" onchange="setBadge(this)">
<select id="bo-filters-territories" class="fr-select select-search-filter-form">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

on n'utilise plus de script inline, et on ajoute une classe qui permet au code du nouveau controller assets/controllers/search_filter_form.js de fonctionner

@@ -61,9 +61,9 @@
{% endblock %}
{% block javascripts %}
{{ encore_entry_script_tags('app') }}
<script>
<script nonce="{{ app.request.attributes.get('csp_script_nonce') }}">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

utilisation d'un nonce pour tippy

tippy('.part-infos-hover', {
content: (reference) => '<strong style="white-space: nowrap">' + reference.getAttribute('data-user') + '</strong>' + '<hr class="fr-pb-1v"><span style="white-space: nowrap">' + reference.getAttribute('data-mail') + '</span>',
content: (reference) => '<strong class="fr-ws-nowrap">' + reference.getAttribute('data-user') + '</strong>' + '<hr class="fr-pb-1v"><span class="fr-ws-nowrap">' + reference.getAttribute('data-mail') + '</span>',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

pas de style inline

templates/base.html.twig Outdated Show resolved Hide resolved
@@ -140,13 +144,62 @@
{% endif %}
{% if 'back_signalement_view' in app.request.get('_route') %}
<script src="https://cdn.jsdelivr.net/npm/@popperjs/core@2.11.8/dist/umd/popper.min.js"></script>
<script src="https://cdn.jsdelivr.net/npm/tippy.js@6/dist/tippy-bundle.umd.js"></script>
<script src="https://cdn.jsdelivr.net/npm/tippy.js@6/dist/tippy.umd.min.js"></script>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changement de source tippy pour être CSP compliant

templates/base.html.twig Outdated Show resolved Hide resolved
templates/base.html.twig Outdated Show resolved Hide resolved
tinymce.init({
selector: 'textarea.editor',
browser_spellcheck: true,
license_key: 'gpl',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ajout d'une licence suite à utilisation d'un cdn


</script> #}

<script nonce="{{ app.request.attributes.get('csp_script_nonce') }}">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ajout du nonce pour tinyMCE

@@ -79,7 +79,8 @@ Encore
})

.enableVueLoader(() => {}, {
version: 3
version: 3,
runtimeCompilerBuild: false
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Passage de toutes les apps VueJS en CSP compliant (c'était vraiment aussi simple que ça)

config/app/csp.yaml Outdated Show resolved Hide resolved
@hmeneuvrier
Copy link
Collaborator Author

hmeneuvrier commented Jun 7, 2024

Le passage de la CSP actuelle dans CSP Evaluator :
image

image

@hmeneuvrier hmeneuvrier changed the title [WIP] CSP #2609 [Technique] Améliorer la CSP Jun 7, 2024
@hmeneuvrier hmeneuvrier marked this pull request as ready for review June 10, 2024 08:19
Copy link
Collaborator

@sfinx13 sfinx13 left a comment

Choose a reason for hiding this comment

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

Lecture et test OK

@hmeneuvrier
Copy link
Collaborator Author

Pour garder une trace, @sfinx13 a trouvé la provenance de l'erreur

image

Il s'agit de l'extension navigateur Vue DevTools... donc pas de souci pour la mise en ligne

Copy link

sonarcloud bot commented Jun 14, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.6% Duplication on New Code

See analysis details on SonarCloud

Copy link
Collaborator

@emilschn emilschn left a comment

Choose a reason for hiding this comment

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

OK lecture et test

@emilschn emilschn merged commit 9570b18 into develop Jun 14, 2024
4 checks passed
@hmeneuvrier hmeneuvrier deleted the feature/2609-technique-csp branch June 27, 2024 19:52
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.

None yet

3 participants