リンクカード セキュリティチェックを追加#9959
Open
mousu-a wants to merge 21 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughLinkFetcherモジュール群を追加し、SSRF対策の安全なIP解決とリダイレクト対応を伴うHTTPフェッチを実装。Metadataはこれを利用するように変更され、LinkCardは事前のURL妥当性チェックを削除。フロントは角括弧のエンコーディングを復元してからURL解析するよう修正、テストとフィクスチャを追加／更新。 ChangesLinkFetcher + Metadata + LinkCard 一貫改修
sequenceDiagram
participant Client
participant Metadata
participant LinkFetcher as LinkFetcher::Fetcher
participant SafeIpResolver as LinkFetcher::SafeIpResolver
participant DNS
participant HTTP as RemoteHTTP
Client->>Metadata: fetch(url)
Metadata->>LinkFetcher: fetch(url)
LinkFetcher->>SafeIpResolver: resolve_ips(uri)
SafeIpResolver->>DNS: getaddresses(hostname)
DNS-->>SafeIpResolver: [ips]
SafeIpResolver->>SafeIpResolver: filter blocked ranges
SafeIpResolver-->>LinkFetcher: safe_ips or nil
alt Safe IPs
LinkFetcher->>LinkFetcher: build_http(uri, ips)
LinkFetcher->>HTTP: GET request (host pinned to IP)
HTTP-->>LinkFetcher: response
alt Redirect
LinkFetcher->>LinkFetcher: build_redirect_url(url, response)
LinkFetcher->>LinkFetcher: fetch(redirect_url, limit-1)
else Success
LinkFetcher-->>Metadata: Net::HTTPSuccess
end
else Unsafe IP
LinkFetcher-->>Metadata: nil
end
Metadata-->>Client: response or nil
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/models/link_fetcher/fetcher.rb`:
- Around line 44-49: The rescue for TooManyRedirects in LinkFetcher should not
log the full user-provided URL; change the Rails.logger.info("[LinkFetcher] Too
many redirects: #{url}") to a sanitized message that omits sensitive
parts—either a fixed message like Rails.logger.info("[LinkFetcher] Too many
redirects") or log only the hostname using a safe extraction (e.g.,
URI.parse(url).host wrapped to handle parse errors) so query/auth info is never
written; update the TooManyRedirects rescue in fetcher.rb accordingly and ensure
other rescue blocks (e.g., the FETCH_ERRORS rescue) follow the same pattern if
they log URLs.
In `@app/models/link_fetcher/safe_ip_resolver.rb`:
- Around line 25-34: Add the missing broadcast address to the blocked ranges:
update the BLOCKED_IP_RANGES constant in link_fetcher/safe_ip_resolver.rb to
include IPAddr.new('255.255.255.255/32') (alongside the existing entries like
IPAddr.new('0.0.0.0/8'), IPAddr.new('::/128'), IPAddr.new('100.64.0.0/10'), and
IPAddr.new('::ffff:0:0/96')), and adjust/ensure the safe_ip? spec/regression
tests assert that '255.255.255.255' is treated as unsafe so the new test case
passes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 29fb0a54-7366-4fc1-97f6-2151b119d88b
📒 Files selected for processing (8)
app/models/link_card/card.rbapp/models/link_fetcher/fetcher.rbapp/models/link_fetcher/safe_ip_resolver.rbapp/models/metadata.rbdb/fixtures/reports.ymltest/cassettes/link_fetcher/fetcher/fetch/success.ymltest/models/link_fetcher/fetcher_test.rbtest/models/link_fetcher/safe_ip_resolver_test.rb
💤 Files with no reviewable changes (1)
- app/models/link_card/card.rb
🚀 Review AppURL: https://bootcamp-pr-9959-fvlfu45apq-an.a.run.app
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/models/link_fetcher/fetcher.rb`:
- Around line 30-31: When calling SafeIpResolver.resolve_ips from
LinkFetcher::Fetcher, wrap the call in a rescue that catches the
SafeIpResolver-specific exceptions (unsafe/non-http/not-found variants) and
return nil to absorb unsafe URLs instead of letting the exception bubble; do the
same for the other resolve_ips invocation used later in this class, optionally
logging at debug level but ensuring the method returns nil on those errors so
LinkFetcher::Fetcher keeps the URI-validation responsibility and fails safely.
- Around line 25-26: The rescue NameError occurs because the local variable uri
isn't set when TooManyRedirects is raised in fetch; to fix, ensure uri is
initialized before any potential raise by parsing the incoming url early (e.g.,
set uri = URI.parse(url) at the top of fetch) or, alternatively, change the
rescue usage so it references a safe fallback (parse url inside the rescue or
use a fixed message) instead of reading uri.host; update the fetch method and
its rescue block to rely on that initialized uri (or the safe fallback) so
uri.host access won't trigger NameError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 72ee2300-3182-4499-bf7a-c12bb5297067
📒 Files selected for processing (2)
app/models/link_fetcher/fetcher.rbapp/models/link_fetcher/safe_ip_resolver.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- app/models/link_fetcher/safe_ip_resolver.rb
Comment on lines
+30
to
+31
| safe_ips = SafeIpResolver.resolve_ips(uri) | ||
| return nil if safe_ips.nil? |
Contributor
There was a problem hiding this comment.
SafeIpResolver.resolve_ips の安全性例外が未捕捉で外に漏れる可能性があります
resolve_ips は unsafe/non-http/not-found 系を例外で返す設計ですが、現状の rescue 範囲に含まれていません。危険 URL を「弾く」目的なら、ここで nil フォールバックに吸収したほうが安定です。
修正例
rescue TooManyRedirects => e
Rails.logger.info("[LinkFetcher] #{e.class}: #{uri.host}")
nil
+ rescue SafeIpResolver::NonHttpUriError,
+ SafeIpResolver::AddressNotFoundError,
+ SafeIpResolver::UnsafeIpError => e
+ Rails.logger.info("[LinkFetcher] #{e.class}: host=#{uri.host}")
+ nil
rescue *FETCH_ERRORS => e
Rails.logger.warn("[LinkFetcher] #{e.class}: #{e.message}: host=#{uri.host}")
nilBased on learnings: SafeIpResolver は IP 安全性の責務に限定し、URI/リンク妥当性は上流（LinkFetcher::Fetcher）で扱う契約です。
Also applies to: 47-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/models/link_fetcher/fetcher.rb` around lines 30 - 31, When calling
SafeIpResolver.resolve_ips from LinkFetcher::Fetcher, wrap the call in a rescue
that catches the SafeIpResolver-specific exceptions (unsafe/non-http/not-found
variants) and return nil to absorb unsafe URLs instead of letting the exception
bubble; do the same for the other resolve_ips invocation used later in this
class, optionally logging at debug level but ensuring the method returns nil on
those errors so LinkFetcher::Fetcher keeps the URI-validation responsibility and
fails safely.
Contributor
Author
There was a problem hiding this comment.
resolve_ipsはraiseした場合nilが返ると思います。
IPを解決する際のエラーはSafeIpResolverでハンドリングして欲しいのでこのままでいきたいと考えています。
Contributor
There was a problem hiding this comment.
@mousu-a ご確認ありがとうございます。AI サマリーで resolve_ips の実装を確認したところ、おっしゃる通り NonHttpUriError・AddressNotFoundError・UnsafeIpError はすべて resolve_ips 内部で rescue されて nil を返す設計になっていますね。私のレビューコメントは実装を誤読したものでした。失礼いたしました。
現在の設計（SafeIpResolver が IP 解決に関するすべてのエラーを内部でハンドリングし、Fetcher は safe_ips.nil? チェックのみ行う）は、責務の分離として理にかなっています。このままで問題ありません。
✏️ Learnings added
Learnt from: mousu-a
Repo: fjordllc/bootcamp PR: 9959
File: app/models/link_fetcher/fetcher.rb:30-31
Timestamp: 2026-04-30T12:15:39.527Z
Learning: In fjordllc/bootcamp `app/models/link_fetcher/safe_ip_resolver.rb`, the `resolve_ips` module function internally rescues all its own error classes (`NonHttpUriError`, `AddressNotFoundError`, `UnsafeIpError`) and returns `nil` on any failure (logging via Rails.logger). Callers such as `LinkFetcher::Fetcher` do NOT need to rescue these exceptions — a simple `return nil if safe_ips.nil?` check is sufficient. Error handling for IP resolution is fully owned by `SafeIpResolver`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: mousu-a
Repo: fjordllc/bootcamp PR: 9778
File: app/models/link_fetcher/safe_ip_resolver.rb:48-52
Timestamp: 2026-04-21T03:25:37.345Z
Learning: In fjordllc/bootcamp, for link-fetcher safety resolver components like `LinkFetcher::SafeIpResolver`, treat the input contract as: callers must provide a valid, already-parsed URI object. Do not add defensive `nil`/blank checks inside the resolver (e.g., `&.`) unless you also change the upstream contract/validation flow. The resolver’s responsibility is IP safety resolution only; URI/link validity is expected to be enforced upstream (e.g., in `LinkFetcher::Fetcher`).
Learnt from: mousu-a
Repo: fjordllc/bootcamp PR: 9959
File: app/models/link_fetcher/safe_ip_resolver.rb:25-47
Timestamp: 2026-04-30T03:55:13.961Z
Learning: In fjordllc/bootcamp `app/models/link_fetcher/safe_ip_resolver.rb`, `BLOCKED_IP_RANGES` takes a conservative "block anything not needed for legitimate global HTTP" approach. In addition to loopback, private, link-local, and CGNAT ranges, it also blocks: broadcast (255.255.255.255/32), documentation/TEST-NET ranges (192.0.2.0/24, 198.51.100.0/24, 203.0.113.0/24), benchmark range (198.18.0.0/15), IETF protocol assignments (192.0.0.0/24), multicast (224.0.0.0/4, ff00::/8), reserved (240.0.0.0/4), IPv6 discard prefix (100::/64), IPv6 documentation (2001:db8::/32), and IPv4-mapped IPv6 (::ffff:0:0/96). This policy was finalized in PR `#9959` (previously those documentation/benchmark/multicast ranges were intentionally excluded in PR `#9778`, but that decision was revised).
Learnt from: zecky1120
Repo: fjordllc/bootcamp PR: 9944
File: app/models/article.rb:0-0
Timestamp: 2026-04-26T13:25:16.020Z
Learning: 外部API呼び出しを含むクラスメソッド（例: Article.agent_summary）のような責務は、モデル側で完結することを優先する。成功時は { "summary": ... }、失敗時は { "error": ... } のようにレスポンス用ハッシュを返し、例外処理はモデル側で RubyLLM::Error / Faraday::Error を rescue して吸収する（コントローラー側で同種の rescue を追加しない）。また、入力のバリデーションとして body.blank? の早期リターンを行い、body.to_s.slice(0, 2000) で送信内容を2000文字にトランケートして外部APIへの負荷を軽減する。コントローラーは render json: 呼び出し元メソッド(params[:body]) のように1行で返す構成を基本とする。
Learnt from: zecky1120
Repo: fjordllc/bootcamp PR: 9944
File: app/controllers/articles_controller.rb:65-87
Timestamp: 2026-04-26T12:50:51.738Z
Learning: fjordllc/bootcamp の `Article.agent_summary` メソッドパターン（`app/models/article.rb`）: 外部API呼び出しを含むクラスメソッドでは、成功時は `{ summary: ... }`、失敗時は `{ error: ... }` のハッシュを返し、コントローラー側では `render json: Article.agent_summary(params[:body])` の1行で済む設計が好まれる。モデル側で `rescue RubyLLM::Error, Faraday::Error` をキャッチし、コントローラーに rescue を書かないスタイル。
Learnt from: Miya096jp
Repo: fjordllc/bootcamp PR: 9102
File: app/models/buzz.rb:6-6
Timestamp: 2025-08-30T00:15:39.732Z
Learning: 管理者限定のフォーム入力の場合、SSRFリスクは大幅に軽減されるため、スキームのホワイトリスト（http/httpsのみ）とNokogiri::HTML.parse()の正しい使用法による最小限の修正で十分とする。
Learnt from: thmz337
Repo: fjordllc/bootcamp PR: 8590
File: app/models/movie.rb:22-24
Timestamp: 2025-07-12T09:56:13.360Z
Learning: Rails scopeでnilやfalseを返すと、NoMethodErrorは発生せず、代わりにModel.allが返される。これによりメソッドチェインは継続するが、全レコードが読み込まれるため意図しない動作となる可能性がある。
Learnt from: masyuko0222
Repo: fjordllc/bootcamp PR: 8934
File: app/controllers/test_feature_toggle_controller.rb:4-6
Timestamp: 2025-07-15T04:26:24.899Z
Learning: テスト用の一時的なコードで、すぐにリバートされる予定の場合、nil安全性などの防御的プログラミングの実装は不要とされることがある。
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:131-135
Timestamp: 2025-09-12T21:18:00.834Z
Learning: Rails アップグレードPRにおいて、product_review_not_completed メソッドの webhook URL 設定の shared フォールバック追加も、設定システム全体の変更として別PRで対応すべきである。
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:19-23
Timestamp: 2025-09-12T21:17:26.029Z
Learning: Rails アップグレードPRにおいて、config_for(:secrets) を使った webhook URL 設定の shared セクション未参照問題は、設定システム全体の変更として別PRで対応すべきである。
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9957
File: app/models/pjord.rb:0-0
Timestamp: 2026-04-28T19:17:25.989Z
Learning: fjordllc/bootcamp の Pjord モデル（`app/models/pjord.rb`）では、LLM出力から内部前置き文を除去するために多層防御を採用している。①システムプロンプトで内部手順説明の出力を禁止、②`PjordResponse` スキーマで構造化出力の `body` フィールドのみを抽出（`extract_public_response_body` が String / `to_h` 両パスを処理）、③正規表現 `/\A\s*(?:検索結果を踏まえて、)?(?:日報へのコメント|回答|コメント)を作成します。\s*/` による前置き除去を最終フォールバックとする。
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/models/graduation_notifier.rb:23-28
Timestamp: 2025-09-12T21:16:47.639Z
Learning: Rails upgrade PRにおいて、configuration systemの変更やwebhook設定の改善提案も、アップグレードに直接関連しない場合は別PRで対応すべきである。PRのスコープ維持が重要。
Learnt from: mousu-a
Repo: fjordllc/bootcamp PR: 8566
File: app/helpers/pair_work_helper.rb:31-33
Timestamp: 2026-01-15T03:20:24.643Z
Learning: In this Ruby project, prefer Numeric#hours (plural) for adding time durations. Use the plural hours method when converting integers to durations (e.g., hour_count.hours). Apply this consistently across the codebase to ensure uniform time calculations and Rails-style duration handling.
Learnt from: kutimiti1234
Repo: fjordllc/bootcamp PR: 9526
File: app/models/regular_event.rb:0-0
Timestamp: 2026-01-19T14:07:11.947Z
Learning: In Rails apps, avoid placing display/filtering logic in models. Keep business/domain logic in models, and handle display-related filtering in controllers. For RegularEvent filtering based on params[:target] (e.g., 'not_finished' or 'all'), implement the logic in the controller (e.g., RegularEventsController) and keep models free of presentation concerns.
mousu-a
force-pushed
the
bug/fix-link-card-fetch-security-check
branch
2 times, most recently
from
6e494d5 to
a2195a9
Compare
Contributor
Author
Member
あれ、そうなんですか？どういう意味かな？ |
Contributor
Author
紛らわしくてすいません！ |
Member
|
@mousu-a なるほどですね。僕にレビュー依頼で大丈夫です。 |
Member
|
@mousu-a conflictの解消をお願いできればありがたいです〜 |
mousu-a
force-pushed
the
bug/fix-link-card-fetch-security-check
branch
2 times, most recently
from
87c2134 to
142da38
Compare
Contributor
Author
|
@komagata |
Contributor
Author
|
@komagata |
エラーハンドリングを追加 リダイレクト時のフォールバック処理を追加
危険なIPが弾かれているか
余計な空行を削除
mousu-a
force-pushed
the
bug/fix-link-card-fetch-security-check
branch
2 times, most recently
from
57cebbf to
9aa8d6c
Compare
今まではfetchモジュールのコードが、リンクがvalidなものである前提のコードになっていたため 合わせてuriを遅延初期化するようにした この時点ではURLがvalidであることが保証されていないため
SafetyValidatorをSafeIpResolverに命名変更 合わせてテストも変更
hostを出すにはパースしなければならず、そこまでしたいほどではないと判断した
mousu-a
force-pushed
the
bug/fix-link-card-fetch-security-check
branch
from
9aa8d6c to
e4aa671
Compare
Contributor
Author
|
@komagata |
Member
|
@mousu-a これらを自前実装するのはtoo muchなのでssrf_filter gemとかを使ったほうがいいかもです。 |
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.
Issue
issueではないですが下記PRが途中で肥大化したのでチェリーピックし、PRを分割しました。
概要
リンクカード機能にリダイレクト時のフォールバック処理、URLのセキュリティチェックを追加しました。
変更確認方法
bug/fix-link-card-fetch-security-checkをローカルに取り込むbin/rails db:seedを実行bin/devでサーバーを起動Screenshot
変更前
変更後
Summary by CodeRabbit
Flakewatch Report
Review App