Dynamic Render Path警告の解消（Book.exist?の安全化）#1737
Merged
Conversation
- Book.exist? メソッドで対象ディレクトリ内のファイル存在確認を行うよう修正 - 不要となった SotechshaPagesController / Sotechsha2PagesController を削除
Member
|
@claude Tell me detailed example stories of the Dynamic Render Path in this public Rails app?
|
Contributor
|
Claude finished @yasulab's task —— View job Dynamic Render Path脆弱性の詳細な解説このPRで解消されたDynamic Render Path脆弱性について、具体的な攻撃シナリオとともに説明します。 🚨 重大な問題 (セキュリティ、破壊的変更)修正前の脆弱性パターン1. 削除された脆弱なコントローラー# app/controllers/sotechsha_pages_controller.rb (削除済み)
class SotechshaPagesController < ApplicationController
def show
render "sotechsha_pages/#{params[:page]}" # ⚠️ 危険！
end
end2. 具体的な攻撃シナリオシナリオ1: ディレクトリトラバーサル攻撃 攻撃者が シナリオ2: 機密情報の露出 シナリオ3: 内部構造の探索 # 攻撃者が以下のようなパスを試行
/sotechsha/../../../../app/models/user
/sotechsha/../../../../app/controllers/admin_controller
/sotechsha/../../../../config/databaseエラーメッセージの違いから、存在するファイルと存在しないファイルを判別でき、アプリケーションの内部構造が露出する危険性があります。 3. Book.exist? の旧実装の問題# 修正前のBook.exist?
def self.exist?(title, page)
page.nil? ?
self.find(title).any? :
self.find(title).map(&:filename).include?(page + ".html") # ⚠️ 不完全なチェック
endこの実装では、 ✅ 良い実装 (参考になる箇所)修正後の安全な実装1. Book.exist? の改善版# app/models/book.rb:21-30
def self.exist?(title, page)
return false unless page.present?
view_paths = [
Rails.root.join("app/views/books/#{title}/#{page}.html.erb"),
Rails.root.join("app/views/#{title}/#{page}.html.erb")
]
view_paths.any? { |path| File.exist?(path) }
end安全性のポイント:
2. BooksController の安全な実装# app/controllers/books_controller.rb:16-21
def render_book_page(params)
book_title = params[:action].split('_').first
Book.exist?(book_title, params[:page]) ?
render("books/#{book_title}/#{params[:page]}") :
redirect_to("/#{book_title}", flash: { warning: '該当するページが見つかりませんでした 💦'} )
end攻撃シナリオに対する防御: 💡 提案 (より良い実装方法)さらなる安全性向上の提案
ALLOWED_PAGES = %w[0 1 2 3 4 5 6 gazou].freeze
def self.exist?(title, page)
return false unless ALLOWED_PAGES.include?(page)
# ... 既存のチェック
end
def self.exist?(title, page)
return false unless page.present?
# パスセパレータを含む場合は即座に拒否
return false if page.include?('/') || page.include?('\\')
# 既存のチェック...
end
# config/application.rb
config.content_security_policy do |policy|
policy.default_src :self
# その他のCSP設定
endまとめこのPRは、Brakemanが検出したDynamic Render Path脆弱性を適切に解消しています。修正により：
タスクリスト
|
Member
|
@nacchan99 良さそうです! デプロイ後、念のため本番環境でも影響がおきてないかどうか該当ページをそれぞれ確認しておいてモラ会えると助かります! 🙏 ✨ cc/ @rakuda-san-desu もし影響が出ていたらテストが抜け・漏れしているということなので、TDD で進めていただければ...!! (๑•̀ㅂ•́)و✨ |
Contributor
Author
|
ご確認ありがとうございます！ |
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
cf. #1727
cf. #1729
cf. #1736
config/brakeman.ignoreに登録されていたDynamic Render Pathの警告を解消しました！やったこと
Book.exist?メソッドで対象ディレクトリ内のファイル存在確認を行うよう修正SotechshaPagesController/Sotechsha2PagesControllerを削除.ignoreから削除