Skip to content

Brakeman を CI で実行する設定 #1727

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

Merged
merged 5 commits into from
Aug 7, 2025
Merged

Brakeman を CI で実行する設定 #1727

merged 5 commits into from
Aug 7, 2025

Conversation

nacchan99
Copy link
Contributor

@nacchan99 nacchan99 commented Aug 6, 2025

CI上で Brakeman を実行する仕組みを導入しました!

まずは Brakeman の導入と安定稼働を第一の目的 として、現時点で検出されている警告はすべて .ignore に登録することで、CIがパスする状態にしています。

⚙️ 設定内容(GitHub Actions)

CI上では下記のコマンドでBrakemanを実行しています。

bundle exec brakeman --ignore-config config/brakeman.ignore --exit-on-warn --quiet

これにより、.ignore に記載されていないLevel 2以上の警告が1件でもあれば、CIが失敗します。(今後発生する新たなセキュリティリスクはCI上でブロックされる状態です!◎)

🔧 対応内容

  • config/brakeman.ignore を追加し、既存の警告を登録
  • CIで .ignore を参照してBrakemanを実行するよう設定
  • 1件だけ、pokemon_download.html.erb に対するBrakemanエラーが発生したため、該当箇所を修正 → a08c447

📌 今後の方針

@nacchan99
Copy link
Contributor Author

nacchan99 commented Aug 7, 2025

現時点で検出されているセキュリティ問題(メモ)

🔴 エラー(Parse Error)

app/views/pokemons/show.html.erb:16

  • 問題: ERBテンプレートの構文エラー
  • 原因: 16行目に不正な<文字があり、パースできない
  • 対策: テンプレートの構文を修正する

⚠️ セキュリティ警告

1. Command Injection(コマンドインジェクション)× 2件

lib/statistics/aggregation.rb:163
lib/upcoming_events/aggregation.rb:89

問題のコード⬇︎

curl -X POST -H 'Content-type: application/json' --data '{"text":"#{msg}"}' #{slack_hook_url} -o /dev/null -w "slack: %{http_code}"

リスク: msgやslack_hook_urlに悪意のあるコマンドが含まれると、システムコマンドが実行される可能性
対策例⬇︎

# Net::HTTPを使用する安全な方法
require 'net/http'
require 'json'

uri = URI(slack_hook_url)
payload = { text: msg }.to_json
Net::HTTP.post(uri, payload, 'Content-Type' => 'application/json')

2. Dynamic Render Path(動的レンダーパス)× 2件

app/controllers/sotechsha2_pages_controller.rb:5
app/controllers/sotechsha_pages_controller.rb:5

問題のコード⬇︎

render(action => "sotechsha2_pages/#{params[:page]}")

リスク: params[:page]を直接使うと、意図しないファイルがレンダーされる可能性
対策例⬇︎

# 許可するページをホワイトリストで制限
allowed_pages = %w[about contact privacy terms]
page = params[:page]
if allowed_pages.include?(page)
  render action: "sotechsha2_pages/#{page}"
else
  redirect_to root_path
end

3. Cross-Site Scripting(XSS)× 4件

3-1. Kramdown出力(2件)
app/views/docs/show.html.erb:12
app/views/podcasts/show.html.erb:39

問題: MarkdownをHTMLに変換した結果がエスケープされずに出力
対策例⬇︎

<%# raw の代わりに sanitize を使用 %>
<%= sanitize(Kramdown::Document.new(content, input: "GFM").to_html) %>

3-2. link_toのhref(2件)
app/views/shared/_dojo.html.erb:3, 6

問題: Dojo.new.urlが信頼できないURLの可能性
対策例⬇︎

# Dojoモデルでバリデーション追加
class Dojo < ApplicationRecord
  validates :url, format: { with: URI.regexp(%w[http https]) }
end

# またはビューで検証
<% if valid_url?(dojo.url) %>
  <%= link_to(..., dojo.url, ...) %>
<% end %>

@nacchan99
Copy link
Contributor Author

@claude Brakemanの初期設定として --exit-on-warn を有効にし、警告レベル2以上でCIを失敗させるようにしています。
この運用は一般的でしょうか?

また、プロジェクトに合わせて今後調整も検討しています。
よくある方針やおすすめの運用があれば教えてください。

Copy link
Contributor

claude bot commented Aug 7, 2025

Claude finished @nacchan99's task —— View job


Brakemanの運用方針について

Todo:

  • Brakeman設定ファイルを確認
  • ベストプラクティスについて回答を作成

💡 提案

--exit-on-warn の運用について

現在の設定(--exit-on-warn でレベル2以上の警告でCI失敗)は段階的導入として妥当です。ただし、プロジェクトの状況に応じた調整が推奨されます。

よくある運用パターン

1. 段階的導入(現在の方針)

# 初期導入時
bundle exec brakeman --exit-on-warn --quiet
  • メリット:既存の警告を config/brakeman.ignore で一時的に無視し、新規の問題を防ぐ
  • デメリット:技術的負債が蓄積する可能性

2. 警告レベル別の運用

# 高リスクのみでCI失敗
bundle exec brakeman --exit-on-error

# または特定レベル以上で失敗
bundle exec brakeman --exit-on-warn --warning-level 3
  • レベル1-2: 低〜中リスク(情報として記録)
  • レベル3-5: 高リスク(CI失敗)

3. カテゴリ別の除外

# 特定の警告タイプを除外
bundle exec brakeman --skip-checks CommandInjection,DynamicRenderPath
  • 対応困難な警告タイプを一時的に除外
  • 段階的に対応範囲を広げる

おすすめの段階的アプローチ

Phase 1: 導入期(現在)

bundle exec brakeman --exit-on-warn --quiet
  • 既存警告を brakeman.ignore に登録
  • 新規の問題のみブロック

Phase 2: 改善期(3-6ヶ月後)

bundle exec brakeman --exit-on-warn --warning-level 3
  • 高リスク警告(レベル3以上)のみでCI失敗
  • 中リスク警告は警告として表示

Phase 3: 成熟期(6-12ヶ月後)

bundle exec brakeman --exit-on-warn --no-ignore
  • ignore ファイルを段階的に削減
  • すべての警告に対応

追加の推奨設定

# より詳細なレポート出力(デバッグ時)
bundle exec brakeman --format json --output brakeman-report.json

# PR コメントへの自動投稿(GitHub Actions)
- name: Upload Brakeman Report
  if: failure()
  uses: actions/upload-artifact@v3
  with:
    name: brakeman-report
    path: brakeman-report.json

brakeman.ignore の管理

# config/brakeman.ignore に理由を記載
{
  "ignored_warnings": [
    {
      "fingerprint": "xxx",
      "note": "2025-08-07: Slackへの通知処理。将来的にNet::HTTPに置き換え予定 (#1728)"
    }
  ]
}

各警告に対して:

  • 無視する理由を明記
  • 対応予定のIssue番号を記載
  • 定期的にレビュー(四半期ごとなど)
✅ 良い実装
  • CI導入時に --quiet オプションを使用(ノイズ削減)
  • workflow_dispatch で手動実行も可能
  • Ruby バージョンを .ruby-version から読み込み(一貫性確保)

現在検出されている警告(Command Injection、XSS等)は確かにセキュリティリスクが高いため、まずは brakeman.ignore に登録して新規の問題を防ぎつつ、別Issueで計画的に対応していく方針は適切だと思います!

@nacchan99 nacchan99 changed the title [WIP] Brakeman を CI で実行する設定 Brakeman を CI で実行する設定 Aug 7, 2025
@nacchan99
Copy link
Contributor Author

ご確認いただいたので、マージします🚀

@nacchan99 nacchan99 merged commit cd40f76 into main Aug 7, 2025
5 checks passed
@nacchan99 nacchan99 deleted the add-brakeman-ci branch August 7, 2025 05:19
@nacchan99
Copy link
Contributor Author

config/brakeman.ignore の公開リスクについて、AIにも確認してみました🧠💬

回答内容を踏まえ、重大なセキュリティリスクには該当しないと判断したため、こちらのPRはマージ済みです🙆‍♀️

以下、AIの回答全文です!

🔍 brakeman.ignore を公開するリスク
✅ 基本的には問題なし
brakeman.ignore の公開自体は直接的なセキュリティリスクにはなりません。
理由:
脆弱性の詳細な悪用方法は含まれていない
ソースコードが既に公開されているので、脆弱性は調査すれば分かる
多くのOSSプロジェクトで同様のファイルが公開されている
⚠️ 注意すべき点

  1. 攻撃者への情報提供
    既知の脆弱性箇所を明示してしまう
    どの警告を無視しているかが一目で分かる
    攻撃の優先順位を決める手がかりになる可能性
  2. 特に注意が必要な項目
    Command Injection(2件)やDynamic Render Path(2件)は影響が大きいため、攻撃者が狙いやすい可能性があります。

🛡️ 推奨対策

  1. 計画的な修正スケジュール
  2. 高リスク項目の優先対応
    Command Injection → 最優先
    Dynamic Render Path → 高優先
    XSS → 中優先
  3. 定期的な見直し
    四半期ごとに無視リストを見直し
    修正したものは削除
    📊 総合判断
    現時点では公開していても問題ありませんが、放置は推奨しません。
    リスクを最小化するためには、計画的にセキュリティ問題を修正していくことが重要です。
    まずはCIが通るようになったので、次は高リスクの Command Injection から段階的に修正していくのが良いアプローチです!

.ignore に登録された警告は、優先度をつけて段階的に対応していきます🙌 → .ignore に登録された Brakeman 警告を解消する #1729

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.

1 participant