chore: sécuriser le rendu des URLs externes et supprimer l’ignore Brakeman XSS#508
chore: sécuriser le rendu des URLs externes et supprimer l’ignore Brakeman XSS#508
Conversation
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: app/models/link.rb
Did you find this useful? React with a 👍 or 👎 |
yann120
force-pushed
the
chore/fix_xss_vulnerability
branch
from
ed0816b to
ce3a5e2
Compare
| <td class="fr-cell--center"><%= link_icon(:article, t('.view_name', name: site.name_with_fallback), url_for([site, site.audit]), title: t('.view_name', name: site.name_with_fallback), line: true, side: :left, sr_only: true) %></td> | ||
| <td><%= link_to site.url_without_scheme_and_www.truncate(35, separator: /\W/), site.url, target: :_blank, title: site.url_without_scheme_and_www.length > 35 ? site.url_without_scheme_and_www : nil %></td> | ||
| <% display_url = site.url_without_scheme_and_www %> | ||
| <% truncated_url = display_url.truncate(35, separator: /\W/) %> |
There was a problem hiding this comment.
Est‑ce qu’on veut mettre ce 35 dans une constante globale ?
Je ne sais pas si on réutiliserait cela pour ne pas avoir d’URL trop longues à afficher.
There was a problem hiding this comment.
On l'utilise à plusieurs endroit donc oui !
| end | ||
|
|
||
| context "when a stored site URL is unsafe" do | ||
| let(:unsafe_url) { "ftp://example.com" } |
There was a problem hiding this comment.
Ici, je voulais injecter du JavaScript, mais c'est impossible.
Alors, une page FTP pour le test contourne le problème, même si, en effet, ce n'est pas une injection XSS.
There was a problem hiding this comment.
Tu testes 3 fois cette fonctionnalité déjà. Ici on a pas à tester le sites controller, tu as changé le url validator, c'est audit model que tu veux tester ou url validator (qui d'ailleurs n'a pas à être ce qu'il est et serait un très bon sujet de scouting (🙄)).
On peut en parler en direct, ça sera plus simple.
| return if href.blank? | ||
|
|
||
| uri = parse(href) | ||
| uri.to_s if uri.host.present? && uri.scheme&.match?(/\Ahttps?\z/) |
There was a problem hiding this comment.
| uri.to_s if uri.host.present? && uri.scheme&.match?(/\Ahttps?\z/) | |
| uri.to_s if uri.host.present? && uri.scheme&.in?(%w[http https]) |
| end | ||
|
|
||
| context "when a stored site URL is unsafe" do | ||
| let(:unsafe_url) { "ftp://example.com" } |
There was a problem hiding this comment.
Tu testes 3 fois cette fonctionnalité déjà. Ici on a pas à tester le sites controller, tu as changé le url validator, c'est audit model que tu veux tester ou url validator (qui d'ailleurs n'a pas à être ce qu'il est et serait un très bon sujet de scouting (🙄)).
On peut en parler en direct, ça sera plus simple.
| <td class="fr-cell--center"><%= link_icon(:article, t('.view_name', name: site.name_with_fallback), url_for([site, site.audit]), title: t('.view_name', name: site.name_with_fallback), line: true, side: :left, sr_only: true) %></td> | ||
| <td><%= link_to site.url_without_scheme_and_www.truncate(35, separator: /\W/), site.url, target: :_blank, title: site.url_without_scheme_and_www.length > 35 ? site.url_without_scheme_and_www : nil %></td> | ||
| <% display_url = site.url_without_scheme_and_www %> | ||
| <% truncated_url = display_url.truncate(35, separator: /\W/) %> |
There was a problem hiding this comment.
On l'utilise à plusieurs endroit donc oui !
yann120
force-pushed
the
chore/fix_xss_vulnerability
branch
from
7d861a6 to
1451d20
Compare
2 participants
Cette PR corrige l’alerte XSS Brakeman sur le rendu de
site.url.Elle:
Linkhtml_safeinutile dans le message de redirectionVérification:
bundle exec brakeman -qne remonte plus de warning XSS