From faf751bca3c040f2641dd6a805571cd2b5c8cdda Mon Sep 17 00:00:00 2001 From: nacchan Date: Wed, 13 Aug 2025 12:04:24 +0900 Subject: [PATCH 1/6] =?UTF-8?q?security:=20=5Fdojo.html.erb=20=E5=86=85?= =?UTF-8?q?=E3=81=AE=20dojo.url=20=E3=82=92=E5=AE=89=E5=85=A8=E5=8C=96?= =?UTF-8?q?=E3=81=97=E3=81=A6=20XSS=20=E3=82=92=E9=98=B2=E6=AD=A2?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/models/dojo.rb | 13 +++++++++++++ app/views/shared/_dojo.html.erb | 4 ++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/app/models/dojo.rb b/app/models/dojo.rb index 2e7a853ac..ca3894c89 100644 --- a/app/models/dojo.rb +++ b/app/models/dojo.rb @@ -89,6 +89,19 @@ def active? def active_at?(date) created_at <= date && (inactivated_at.nil? || inactivated_at > date) end + + # URLの安全性をチェックして、安全なURLを返す + def safe_url + return '#' if url.blank? + + # URI.parseがエラーになるか、HTTPスキームでない場合は'#'を返す + begin + uri = URI.parse(url) + uri.scheme&.match?(/\Ahttps?\z/) ? url : '#' + rescue URI::InvalidURIError + '#' + end + end # 再活性化メソッド def reactivate! diff --git a/app/views/shared/_dojo.html.erb b/app/views/shared/_dojo.html.erb index 227f87723..07e1c9490 100644 --- a/app/views/shared/_dojo.html.erb +++ b/app/views/shared/_dojo.html.erb @@ -1,9 +1,9 @@
  • - <%= link_to lazy_image_tag(dojo.logo, alt: "CoderDojo #{dojo.name}", class: 'dojo-picture'), dojo.url, + <%= link_to lazy_image_tag(dojo.logo, alt: "CoderDojo #{dojo.name}", class: 'dojo-picture'), dojo.safe_url, target: "_blank", rel: "external noopener" %> - <%= link_to "#{dojo.name} (#{dojo.prefecture.name})", dojo.url, target: "_blank", rel: "external noopener" %> + <%= link_to "#{dojo.name} (#{dojo.prefecture.name})", dojo.safe_url, target: "_blank", rel: "external noopener" %> <% if not dojo.counter == 1 %> Date: Thu, 14 Aug 2025 11:22:10 +0900 Subject: [PATCH 2/6] =?UTF-8?q?fix:=20dojo=E3=83=93=E3=83=A5=E3=83=BC?= =?UTF-8?q?=E3=81=AEXSS=E8=84=86=E5=BC=B1=E6=80=A7=E3=82=92=E8=A7=A3?= =?UTF-8?q?=E6=B6=88?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - html_escape() でユーザー入力データを適切にエスケープ - sanitize_url() でリンク先URLを安全化 - link_to に noreferrer を追加し Reverse Tabnabbing 対策 --- app/views/shared/_dojo.html.erb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/app/views/shared/_dojo.html.erb b/app/views/shared/_dojo.html.erb index 07e1c9490..2cc383f41 100644 --- a/app/views/shared/_dojo.html.erb +++ b/app/views/shared/_dojo.html.erb @@ -1,9 +1,9 @@ -
  • +
  • - <%= link_to lazy_image_tag(dojo.logo, alt: "CoderDojo #{dojo.name}", class: 'dojo-picture'), dojo.safe_url, - target: "_blank", rel: "external noopener" %> + <%= link_to lazy_image_tag(dojo.logo, alt: html_escape("CoderDojo #{dojo.name}"), class: 'dojo-picture'), sanitize_url(dojo.safe_url), + target: "_blank", rel: "external noopener noreferrer" %> - <%= link_to "#{dojo.name} (#{dojo.prefecture.name})", dojo.safe_url, target: "_blank", rel: "external noopener" %> + <%= link_to html_escape("#{dojo.name} (#{dojo.prefecture.name})"), sanitize_url(dojo.safe_url), target: "_blank", rel: "external noopener noreferrer" %> <% if not dojo.counter == 1 %> <% dojo.tags.first(5).each do |tag| %> -
  • <%= tag %>
    • +
  • <%= html_escape(tag) %>
    • <% end %> <% if dojo.tags.length > 5 %> @@ -26,12 +26,12 @@
    ' - title="<%= dojo.tags[5..].join(', ') %>">...
    • + title="<%= html_escape(dojo.tags[5..].join(', ')) %>">...
    • <% end %>

    - <%= dojo.description %> + <%= html_escape(dojo.description) %> <% if dojo.is_private %> <%= link_to 'Private', doc_path('private-dojo'), class: 'dojo-private' %> <% end %> From c6e2346f6edfe45dd848ea3d868e6d04887d34d4 Mon Sep 17 00:00:00 2001 From: nacchan Date: Thu, 14 Aug 2025 15:39:10 +0900 Subject: [PATCH 3/6] =?UTF-8?q?security:=20raw()=20=E3=82=92=20sanitize=5F?= =?UTF-8?q?content()=20=E3=81=AB=E7=BD=AE=E3=81=8D=E6=8F=9B=E3=81=88?= =?UTF-8?q?=E3=81=A6=20XSS=20=E8=84=86=E5=BC=B1=E6=80=A7=E3=82=92=E4=BF=AE?= =?UTF-8?q?=E6=AD=A3?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - docs/show.html.erb と podcasts/show.html.erb で raw() を sanitize_content() ヘルパーに変更 - ApplicationHelper に sanitize_content() メソッドを追加し、HTML サニタイズ処理を共通化 - Rails デフォルトに加えて 'center' タグと 'id' 属性を許可してコンテンツを適切に表示 - docs_spec.rb のテスト期待値にも同じサニタイズ処理を適用 --- app/helpers/application_helper.rb | 7 +++++++ app/views/docs/show.html.erb | 4 +--- app/views/podcasts/show.html.erb | 2 +- spec/requests/docs_spec.rb | 3 ++- 4 files changed, 11 insertions(+), 5 deletions(-) diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 8bb33b872..181694323 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -48,6 +48,13 @@ def page_lang(lang) lang.empty? ? 'ja' : lang end + def sanitize_content(content) + sanitize(content, + tags: ActionView::Base.sanitized_allowed_tags + ['center'], + attributes: ActionView::Base.sanitized_allowed_attributes + ['id'] + ) + end + # 'inline_' プレフィックスがついたflashメッセージをビュー内で表示するヘルパー # inline_alert → alert, inline_warning → warning のように変換してBootstrapのCSSクラスを適用 def render_inline_flash_messages diff --git a/app/views/docs/show.html.erb b/app/views/docs/show.html.erb index f3f01be1b..076dc8f8a 100644 --- a/app/views/docs/show.html.erb +++ b/app/views/docs/show.html.erb @@ -9,7 +9,7 @@

    - <%= raw @content %> + <%= sanitize_content(@content) %>
    <% if request.path.start_with? '/docs' %> @@ -22,5 +22,3 @@ <%= render 'shared/social_buttons' %>
    - - diff --git a/app/views/podcasts/show.html.erb b/app/views/podcasts/show.html.erb index 172df8bba..2a8ec4a22 100644 --- a/app/views/podcasts/show.html.erb +++ b/app/views/podcasts/show.html.erb @@ -36,7 +36,7 @@ - <%= raw Rinku.auto_link(@content) %> + <%= sanitize_content(Rinku.auto_link(@content)) %> diff --git a/spec/requests/docs_spec.rb b/spec/requests/docs_spec.rb index ab689cb29..a17e748ca 100644 --- a/spec/requests/docs_spec.rb +++ b/spec/requests/docs_spec.rb @@ -14,6 +14,7 @@ get doc_path(param) doc = Document.new(param) expected = Kramdown::Document.new(doc.content, input: 'GFM').to_html + expected = ApplicationController.helpers.sanitize_content(expected) expect(response.body).to include(expected.strip) end @@ -23,4 +24,4 @@ expect(response.status).to eq 302 end end -end \ No newline at end of file +end From 440fac1cf4c505f3380f9e9fca61fb7eee22be20 Mon Sep 17 00:00:00 2001 From: nacchan Date: Thu, 14 Aug 2025 15:44:41 +0900 Subject: [PATCH 4/6] =?UTF-8?q?chore:=20XSS=20=E8=84=86=E5=BC=B1=E6=80=A7?= =?UTF-8?q?=E4=BF=AE=E6=AD=A3=E3=81=AB=E4=BC=B4=E3=81=84=20Brakeman=20igno?= =?UTF-8?q?re=20=E3=82=92=E5=85=A8=E5=89=8A=E9=99=A4?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- config/brakeman.ignore | 181 ----------------------------------------- 1 file changed, 181 deletions(-) delete mode 100644 config/brakeman.ignore diff --git a/config/brakeman.ignore b/config/brakeman.ignore deleted file mode 100644 index 186b08239..000000000 --- a/config/brakeman.ignore +++ /dev/null @@ -1,181 +0,0 @@ -{ - "ignored_warnings": [ - { - "warning_type": "Cross-Site Scripting", - "warning_code": 4, - "fingerprint": "8ba988098c444755698e4e65d38a94f4095948c1a9bc6220c7e2a4636c3c04d7", - "check_name": "LinkToHref", - "message": "Potentially unsafe model attribute in `link_to` href", - "file": "app/views/shared/_dojo.html.erb", - "line": 6, - "link": "https://brakemanscanner.org/docs/warning_types/link_to_href", - "code": "link_to(\"#{Dojo.new.name} (#{Dojo.new.prefecture.name})\", Dojo.new.url, :target => \"_blank\", :rel => \"external noopener\")", - "render_path": [ - { - "type": "controller", - "class": "HomeController", - "method": "show", - "line": 7, - "file": "app/controllers/home_controller.rb", - "rendered": { - "name": "home/show", - "file": "app/views/home/show.html.erb" - } - }, - { - "type": "template", - "name": "home/show", - "line": 161, - "file": "app/views/home/show.html.erb", - "rendered": { - "name": "shared/_dojos", - "file": "app/views/shared/_dojos.html.erb" - } - }, - { - "type": "template", - "name": "shared/_dojos", - "line": 2, - "file": "app/views/shared/_dojos.html.erb", - "rendered": { - "name": "shared/_dojo", - "file": "app/views/shared/_dojo.html.erb" - } - } - ], - "location": { - "type": "template", - "template": "shared/_dojo" - }, - "user_input": "Dojo.new.url", - "confidence": "Weak", - "cwe_id": [ - 79 - ], - "note": "" - }, - { - "warning_type": "Cross-Site Scripting", - "warning_code": 2, - "fingerprint": "b22a549fb14a7e6b3a9c34991ffcacd354dc768d74d50a8f6901e23c3ea19538", - "check_name": "CrossSiteScripting", - "message": "Unescaped model attribute", - "file": "app/views/podcasts/show.html.erb", - "line": 39, - "link": "https://brakemanscanner.org/docs/warning_types/cross_site_scripting", - "code": "Rinku.auto_link(Kramdown::Document.new(self.convert_shownote(Podcast.find_by(:id => params[:id]).content), :input => \"GFM\").to_html)", - "render_path": [ - { - "type": "controller", - "class": "PodcastsController", - "method": "show", - "line": 31, - "file": "app/controllers/podcasts_controller.rb", - "rendered": { - "name": "podcasts/show", - "file": "app/views/podcasts/show.html.erb" - } - } - ], - "location": { - "type": "template", - "template": "podcasts/show" - }, - "user_input": "Podcast.find_by(:id => params[:id]).content", - "confidence": "Weak", - "cwe_id": [ - 79 - ], - "note": "" - }, - { - "warning_type": "Cross-Site Scripting", - "warning_code": 4, - "fingerprint": "b29f98f4da690ffb7c663390c21db3a71174dae17d06234deab9d6655af6babe", - "check_name": "LinkToHref", - "message": "Potentially unsafe model attribute in `link_to` href", - "file": "app/views/shared/_dojo.html.erb", - "line": 3, - "link": "https://brakemanscanner.org/docs/warning_types/link_to_href", - "code": "link_to(lazy_image_tag(Dojo.new.logo, :alt => (\"CoderDojo #{Dojo.new.name}\"), :class => \"dojo-picture\"), Dojo.new.url, :target => \"_blank\", :rel => \"external noopener\")", - "render_path": [ - { - "type": "controller", - "class": "HomeController", - "method": "show", - "line": 7, - "file": "app/controllers/home_controller.rb", - "rendered": { - "name": "home/show", - "file": "app/views/home/show.html.erb" - } - }, - { - "type": "template", - "name": "home/show", - "line": 161, - "file": "app/views/home/show.html.erb", - "rendered": { - "name": "shared/_dojos", - "file": "app/views/shared/_dojos.html.erb" - } - }, - { - "type": "template", - "name": "shared/_dojos", - "line": 2, - "file": "app/views/shared/_dojos.html.erb", - "rendered": { - "name": "shared/_dojo", - "file": "app/views/shared/_dojo.html.erb" - } - } - ], - "location": { - "type": "template", - "template": "shared/_dojo" - }, - "user_input": "Dojo.new.url", - "confidence": "Weak", - "cwe_id": [ - 79 - ], - "note": "" - }, - { - "warning_type": "Cross-Site Scripting", - "warning_code": 2, - "fingerprint": "e4187193a881ef4e98b77f205c86fcafbef3d65d9269bba30e8612f6a59273ed", - "check_name": "CrossSiteScripting", - "message": "Unescaped model attribute", - "file": "app/views/docs/show.html.erb", - "line": 12, - "link": "https://brakemanscanner.org/docs/warning_types/cross_site_scripting", - "code": "Kramdown::Document.new(Document.new(params[:id]).content, :input => \"GFM\").to_html", - "render_path": [ - { - "type": "controller", - "class": "DocsController", - "method": "show", - "line": 42, - "file": "app/controllers/docs_controller.rb", - "rendered": { - "name": "docs/show", - "file": "app/views/docs/show.html.erb" - } - } - ], - "location": { - "type": "template", - "template": "docs/show" - }, - "user_input": "Document.new(params[:id]).content", - "confidence": "Weak", - "cwe_id": [ - 79 - ], - "note": "" - } - ], - "brakeman_version": "7.1.0" -} From e780ba914c8f656147b0729c9af7875e794bf952 Mon Sep 17 00:00:00 2001 From: nacchan Date: Thu, 14 Aug 2025 16:14:55 +0900 Subject: [PATCH 5/6] =?UTF-8?q?refactor:=20safe=5Furl=E3=83=A1=E3=82=BD?= =?UTF-8?q?=E3=83=83=E3=83=89=E3=82=92=E3=83=A2=E3=83=87=E3=83=AB=E3=81=8B?= =?UTF-8?q?=E3=82=89=E3=83=98=E3=83=AB=E3=83=91=E3=83=BC=E3=81=AB=E7=A7=BB?= =?UTF-8?q?=E5=8B=95?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Dojoモデルのsafe_urlメソッドをApplicationHelperのsafe_dojo_urlに移動 - app/views/shared/_dojo.html.erbでsafe_dojo_url()ヘルパーを使用 --- app/helpers/application_helper.rb | 11 +++++++++++ app/models/dojo.rb | 13 ------------- app/views/shared/_dojo.html.erb | 4 ++-- 3 files changed, 13 insertions(+), 15 deletions(-) diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 181694323..98b8b49c8 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -55,6 +55,17 @@ def sanitize_content(content) ) end + def safe_dojo_url(dojo) + return '#' if dojo.url.blank? + + begin + uri = URI.parse(dojo.url) + uri.scheme&.match?(/\Ahttps?\z/) ? dojo.url : '#' + rescue URI::InvalidURIError + '#' + end + end + # 'inline_' プレフィックスがついたflashメッセージをビュー内で表示するヘルパー # inline_alert → alert, inline_warning → warning のように変換してBootstrapのCSSクラスを適用 def render_inline_flash_messages diff --git a/app/models/dojo.rb b/app/models/dojo.rb index ca3894c89..25399bf1b 100644 --- a/app/models/dojo.rb +++ b/app/models/dojo.rb @@ -90,19 +90,6 @@ def active_at?(date) created_at <= date && (inactivated_at.nil? || inactivated_at > date) end - # URLの安全性をチェックして、安全なURLを返す - def safe_url - return '#' if url.blank? - - # URI.parseがエラーになるか、HTTPスキームでない場合は'#'を返す - begin - uri = URI.parse(url) - uri.scheme&.match?(/\Ahttps?\z/) ? url : '#' - rescue URI::InvalidURIError - '#' - end - end - # 再活性化メソッド def reactivate! if inactivated_at.present? diff --git a/app/views/shared/_dojo.html.erb b/app/views/shared/_dojo.html.erb index 2cc383f41..d5bd601da 100644 --- a/app/views/shared/_dojo.html.erb +++ b/app/views/shared/_dojo.html.erb @@ -1,9 +1,9 @@
  • - <%= link_to lazy_image_tag(dojo.logo, alt: html_escape("CoderDojo #{dojo.name}"), class: 'dojo-picture'), sanitize_url(dojo.safe_url), + <%= link_to lazy_image_tag(dojo.logo, alt: html_escape("CoderDojo #{dojo.name}"), class: 'dojo-picture'), safe_dojo_url(dojo), target: "_blank", rel: "external noopener noreferrer" %> - <%= link_to html_escape("#{dojo.name} (#{dojo.prefecture.name})"), sanitize_url(dojo.safe_url), target: "_blank", rel: "external noopener noreferrer" %> + <%= link_to html_escape("#{dojo.name} (#{dojo.prefecture.name})"), safe_dojo_url(dojo), target: "_blank", rel: "external noopener noreferrer" %> <% if not dojo.counter == 1 %> Date: Fri, 15 Aug 2025 09:34:01 +0900 Subject: [PATCH 6/6] =?UTF-8?q?fix:=20rel=E5=B1=9E=E6=80=A7=E3=82=92?= =?UTF-8?q?=E5=89=8A=E9=99=A4?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - link_to の rel="external noopener noreferrer" を削除 - Brakemanで警告なし、RSpecテストも全て通過済み --- app/views/shared/_dojo.html.erb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/shared/_dojo.html.erb b/app/views/shared/_dojo.html.erb index d5bd601da..cc4b8f8fe 100644 --- a/app/views/shared/_dojo.html.erb +++ b/app/views/shared/_dojo.html.erb @@ -1,9 +1,9 @@
  • <%= link_to lazy_image_tag(dojo.logo, alt: html_escape("CoderDojo #{dojo.name}"), class: 'dojo-picture'), safe_dojo_url(dojo), - target: "_blank", rel: "external noopener noreferrer" %> + target: "_blank" %> - <%= link_to html_escape("#{dojo.name} (#{dojo.prefecture.name})"), safe_dojo_url(dojo), target: "_blank", rel: "external noopener noreferrer" %> + <%= link_to html_escape("#{dojo.name} (#{dojo.prefecture.name})"), safe_dojo_url(dojo), target: "_blank" %> <% if not dojo.counter == 1 %>