Cross-Site Scripting警告の解消

[nacchan99]

cf. #1727
cf. #1729
cf. #1736
cf. #1737

config/brakeman.ignore に登録されていた Cross-Site Scriptingの警告を解消しました！

やったこと

• app/views/shared/_dojo.html にて、HTML を正しく表示するために html_safe を適用

[nacchan99]

@claude このPRについて、以下の観点でレビューしてください：

• コードのメンテナンス性（変更が将来の修正・拡張に与える影響）
• ヘルパーメソッド化や共通化の妥当性
• テストコードの可読性と保守性

[claude]

Claude finished @nacchan99's task —— View job

---

タスクリスト

• 変更ファイルの内容を確認
• セキュリティ対策の妥当性を評価
• ヘルパーメソッドの設計を評価
• テストコードを確認
• フィードバックを提供

---

PR レビュー結果

XSS脆弱性の修正PRをレビューしました。以下の観点でフィードバックをお伝えします。

⚠️ 改善推奨 (ベストプラクティス違反)

1. safe_url メソッドの実装箇所について

app/models/dojo.rb:94-104 の safe_url メソッドがモデルに実装されています。

def safe_url
  return '#' if url.blank?
  
  begin
    uri = URI.parse(url)
    uri.scheme&.match?(/\Ahttps?\z/) ? url : '#'
  rescue URI::InvalidURIError
    '#'
  end
end

問題点:

• URLのサニタイズはビューの責務であり、モデルに実装するとYAGNI原則に反する可能性があります
• '#' を返す仕様は、リンク先が無効な場合の振る舞いとして不明瞭です

改善案:

# app/helpers/application_helper.rb に移動
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

2. sanitize_url の使用について

app/views/shared/_dojo.html.erb:3,6 で sanitize_url(dojo.safe_url) と二重のサニタイズが行われています。

問題点:

• safe_url が既に安全なURLを返すため、sanitize_url の追加は冗長です
• Rails の sanitize はHTMLエスケープ用であり、URLのサニタイズには不適切です

改善案:

<%= link_to ..., dojo.safe_url, ... %>

💡 提案 (より良い実装方法)

1. sanitize_content ヘルパーの設定拡張性

app/helpers/application_helper.rb:51-56

現在の実装：

def sanitize_content(content)
  sanitize(content, 
    tags: ActionView::Base.sanitized_allowed_tags + ['center'], 
    attributes: ActionView::Base.sanitized_allowed_attributes + ['id']
  )
end

提案:
将来的な拡張性を考慮し、許可するタグと属性を定数化することで保守性が向上します：

CUSTOM_ALLOWED_TAGS = ['center'].freeze
CUSTOM_ALLOWED_ATTRIBUTES = ['id'].freeze

def sanitize_content(content)
  sanitize(content, 
    tags: ActionView::Base.sanitized_allowed_tags + CUSTOM_ALLOWED_TAGS,
    attributes: ActionView::Base.sanitized_allowed_attributes + CUSTOM_ALLOWED_ATTRIBUTES
  )
end

2. テストカバレッジの拡充

spec/requests/docs_spec.rb でサニタイズ処理のテストが追加されていますが、以下のケースも追加することを推奨します：

context 'XSS protection' do
  it 'sanitizes malicious script tags' do
    # 悪意のあるスクリプトタグが除去されることを確認
  end
  
  it 'preserves allowed HTML tags' do
    # 許可されたタグが保持されることを確認
  end
  
  it 'handles invalid URLs in dojo links' do
    # javascript: などの危険なURLスキームが処理されることを確認
  end
end

✅ 良い実装 (参考になる箇所)

1. Brakeman ignoreファイルの完全削除

• 警告を抑制するのではなく、根本的な修正を行った点は素晴らしいアプローチです
• これにより将来的な脆弱性の見逃しリスクが低減されます

2. html_escape の一貫した使用

• app/views/shared/_dojo.html.erb で、全ての動的コンテンツに対して html_escape を適用している点は良い実装です
• 特にツールチップのtitle属性（29行目）なども見逃さずエスケープしています

3. ヘルパーメソッドへの共通化

• sanitize_content をヘルパーメソッドとして切り出したことで、サニタイズロジックの一元管理が可能になりました
• DRY原則に従った良い設計です

総評

XSS脆弱性の修正は概ね適切に実装されています。特にBrakeman ignoreを削除し、根本的な修正を行った点は評価できます。

主な改善点：

• URLサニタイズロジックの配置（モデル vs ヘルパー）を再検討
• 二重サニタイズの解消
• テストカバレッジの拡充

これらの改善により、より保守性の高いコードベースになると考えられます。

[nacchan99]

@claude 再度、簡単なレビューをお願いします！

[claude]

Claude finished @nacchan99's task —— View job

---

タスクリスト

• 変更ファイルの内容を確認
• 前回のフィードバックへの対応状況を評価
• 新しい実装のベストプラクティスを評価
• レビュー結果を提供

---

PR レビュー結果

前回のフィードバックを反映した修正を確認しました。以下の観点でレビューします。

✅ 良い実装 (参考になる箇所)

1. safe_dojo_url ヘルパーメソッドへの移動

app/helpers/application_helper.rb:58-67 で URLサニタイズロジックをヘルパーに移動した点は、前回の指摘に対する適切な対応です。

• ビュー層の責務として正しい配置になりました
• DRY原則に従った良い実装です

2. sanitize_content の一貫した使用

• app/views/docs/show.html.erb:12 と app/views/podcasts/show.html.erb:39 で一貫して使用
• 許可するタグと属性を明確に定義している点が良い

3. XSS対策の徹底

app/views/shared/_dojo.html.erb で全ての動的コンテンツに html_escape を適用：

• タグ表示（18行目）
• ツールチップ（29行目）
• 説明文（34行目）
  見逃しやすい箇所も含めて徹底されています。

4. Brakeman警告の根本解決

config/brakeman.ignore を完全削除し、警告を抑制するのではなく修正した点は素晴らしいアプローチです。

💡 提案 (より良い実装方法)

1. ヘルパーメソッドの定数化

現在の sanitize_content の実装をより保守しやすくする提案：

# app/helpers/application_helper.rb
CUSTOM_ALLOWED_TAGS = ['center'].freeze
CUSTOM_ALLOWED_ATTRIBUTES = ['id'].freeze

def sanitize_content(content)
  sanitize(content, 
    tags: ActionView::Base.sanitized_allowed_tags + CUSTOM_ALLOWED_TAGS,
    attributes: ActionView::Base.sanitized_allowed_attributes + CUSTOM_ALLOWED_ATTRIBUTES
  )
end

将来的にタグや属性を追加する際の変更が容易になります。

2. テストカバレッジの拡充

spec/requests/docs_spec.rb にXSS防御のテストケースを追加することを推奨：

context 'XSS protection' do
  it 'removes malicious script tags' do
    # <script>alert("XSS")</script> が除去されることを確認
  end
  
  it 'handles javascript: URLs in dojo links' do
    # javascript: スキームが '#' に置換されることを確認
  end
end

総評

前回指摘した主要な問題点（safe_url のモデル配置、二重サニタイズ）が適切に修正されています。XSS脆弱性の解消という目的を達成しつつ、コードの保守性も向上しました。

マージして問題ないと考えます。提案事項は将来的な改善として検討いただければと思います。

[nacchan99]

以前いただいたコメント #1704 (comment) を今思い出したので、 rel: 'noopener ...' の箇所については明日再考します！

[yasulab]

@claude Review this PR if we can merge or need additional commits to merge? ultrathink

[claude]

Claude finished @yasulab's task —— View job

---

タスクリスト

• 変更ファイルの内容を確認
• セキュリティ対策の妥当性を評価
• 前回のフィードバックへの対応状況を確認
• コードの保守性と設計パターンを評価
• マージ可否の判定とフィードバックを提供

---

PR レビュー結果

XSS脆弱性修正のPRを確認しました。全体的に適切な修正が実施されており、マージして問題ありません。

⚠️ 改善推奨 (ベストプラクティス違反)

rel 属性の削除について

最新コミット（70c8b5f）で rel="external noopener noreferrer" が削除されています。

セキュリティへの影響:

• target="_blank" と組み合わせた外部リンクで rel="noopener" を削除すると、リンク先のページが window.opener を通じて元のページにアクセス可能になります（タブナビング攻撃のリスク）
• Rails 8.0では自動的にセキュリティ属性が追加されない場合があるため、明示的な設定が推奨されます

改善案:

<%= link_to ..., safe_dojo_url(dojo), 
    target: "_blank", 
    rel: "noopener" %>

少なくとも rel="noopener" は残すことを推奨します。noreferrer は必須ではありませんが、noopener はセキュリティ上重要です。

💡 提案 (より良い実装方法)

ヘルパーメソッドの定数化

sanitize_content メソッドの保守性向上のため、許可するタグと属性を定数化することを提案します：

# app/helpers/application_helper.rb
CUSTOM_ALLOWED_TAGS = ['center'].freeze
CUSTOM_ALLOWED_ATTRIBUTES = ['id'].freeze

def sanitize_content(content)
  sanitize(content, 
    tags: ActionView::Base.sanitized_allowed_tags + CUSTOM_ALLOWED_TAGS,
    attributes: ActionView::Base.sanitized_allowed_attributes + CUSTOM_ALLOWED_ATTRIBUTES
  )
end

✅ 良い実装 (参考になる箇所)

1. XSS脆弱性の根本的解決

• config/brakeman.ignore を完全削除し、警告を抑制するのではなく修正した点は優れたアプローチです
• raw() を sanitize_content() に置き換えることで、安全性を確保しました

2. ヘルパーメソッドへの適切な移動

• safe_dojo_url をモデルからヘルパーに移動したことで、責務が明確になりました
• URLのバリデーションロジックが適切に実装されています

3. 徹底的なHTMLエスケープ

• app/views/shared/_dojo.html.erb で全ての動的コンテンツに html_escape を適用
• ツールチップのtitle属性など見逃しやすい箇所もカバーしています

4. テストの更新

• spec/requests/docs_spec.rb にサニタイズ処理のテストを追加し、変更の正当性を確保

総評

前回のレビューで指摘した主要な問題点が適切に修正されています。XSS脆弱性の解消という目的を達成し、コードの保守性も向上しました。

注意点:

• rel="noopener" の削除はセキュリティリスクがあるため、再度追加することを強く推奨します
• 将来的には定数化によるさらなる保守性向上も検討してください

これらの点を考慮いただければ、より安全で保守しやすいコードベースになります。

[yasulab]

@claude Your comment seems outdated and hallucination-ish? Modern browsers already fix this by default in my understanding? think

rel="noopener" の削除はセキュリティリスクがあるため、再度追加することを強く推奨します

[claude]

Claude finished @yasulab's task —— View job

---

タスクリスト

• ブラウザのセキュリティ仕様を確認
• rel="noopener" の必要性を再評価
• 正確な情報でフィードバックを更新

---

@yasulab さん、ご指摘ありがとうございます。おっしゃる通りです。

rel="noopener" について

現代のブラウザでは、target="_blank" を持つリンクに対して 自動的に noopener の動作が適用されます：

• Chrome 88+ (2021年1月〜)
• Firefox 79+ (2020年7月〜)
• Safari 12.1+ (2019年3月〜)
• Edge 88+ (2021年1月〜)

これらのブラウザでは、target="_blank" のリンクは暗黙的に rel="noopener" として扱われるため、明示的な指定は不要です。

結論

最新コミット（70c8b5f）での rel 属性の削除は 適切な判断 です。現代のブラウザ環境では、セキュリティリスクはありません。

前回のレビューでの指摘は古い情報に基づいていました。訂正させていただきます。

総評

このPRはXSS脆弱性を適切に修正しており、マージして問題ありません。

✅ 良い実装 (参考になる箇所)

1. 不要な属性の削除

• rel="external noopener noreferrer" の削除により、コードがシンプルになりました
• 現代のブラウザ仕様に合わせた適切な判断です

2. XSS脆弱性の根本的解決

• config/brakeman.ignore を完全削除し、警告を抑制するのではなく修正
• raw() を sanitize_content() に置き換えて安全性を確保

3. ヘルパーメソッドへの適切な移動

• safe_dojo_url をモデルからヘルパーに移動し、責務を明確化
• URLバリデーションロジックの適切な実装

💡 提案 (より良い実装方法)

ヘルパーメソッドの定数化

将来的な保守性向上のため、以下の実装を提案します：

# app/helpers/application_helper.rb
CUSTOM_ALLOWED_TAGS = ['center'].freeze
CUSTOM_ALLOWED_ATTRIBUTES = ['id'].freeze

def sanitize_content(content)
  sanitize(content, 
    tags: ActionView::Base.sanitized_allowed_tags + CUSTOM_ALLOWED_TAGS,
    attributes: ActionView::Base.sanitized_allowed_attributes + CUSTOM_ALLOWED_ATTRIBUTES
  )
end

混乱を招く情報をお伝えしてしまい、申し訳ございませんでした。

[yasulab]

PR のご対応ありがとうございました！！こちらマージしますね...!! 🚀✨

[yasulab]

あーーー、これページのレイアウトが壊れちゃう変更でした... 😭💦 (マージ後すぐに気づけてよかった...!! ✅✨) https://coderdojo.jp/signup#terms

まずは自分の方で /signup ページに対する layout break を検知できるテストを追加 して、その後、一旦 revert しますね! 🙇 🙇 🙇

[yasulab]

まずは自分の方で /signup ページに対する layout break を検知できるテストを追加

Committed. ✅ 74a3ed3

[nacchan99]

失礼しました🙏💦
ご対応ありがとうございます!!