特定のURL / ドメインのリンクカードが文字化けするバグを修正#9778
Merged
Merged
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:
📝 WalkthroughWalkthroughリンク取得機能に安全なIP検証を追加し、Metadataモデルを新しいLinkFetcher::Fetcherを使用するように更新。LazyなURI評価を導入し、ナビゲーションリンクをメンター権限に基づいて条件分岐。リダイレクト処理とタイムアウト管理を含む包括的なリンク検証システムを構築。 Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant LinkCard as LinkCard::Card
participant Metadata
participant Fetcher as LinkFetcher::Fetcher
participant LinkChecker
participant SafeIpResolver
participant DNS as Resolv (DNS)
participant Network as Net::HTTP
Client->>LinkCard: request(url)
LinkCard->>Metadata: Metadata.new(url)
Metadata-->>LinkCard: metadata instance
LinkCard->>Metadata: fetch()
Metadata->>Metadata: uri (lazy evaluate)
Metadata->>Fetcher: Fetcher.fetch(`@url`)
Fetcher->>LinkChecker: valid_url?(url)
LinkChecker-->>Fetcher: validation result
alt URL Invalid
Fetcher-->>Metadata: nil
else URL Valid
Fetcher->>Fetcher: parse & normalize URI
Fetcher->>SafeIpResolver: resolve_ips(uri)
SafeIpResolver->>DNS: getaddresses(host)
DNS-->>SafeIpResolver: IP list
SafeIpResolver->>SafeIpResolver: check IP ranges
alt Any IP Blocked
SafeIpResolver-->>Fetcher: nil
else All IPs Safe
SafeIpResolver-->>Fetcher: safe IPs
Fetcher->>Fetcher: build_http(uri, ips)
Fetcher->>Network: GET request
Network-->>Fetcher: Net::HTTP response
alt Redirection
Fetcher->>Fetcher: build_redirect_url
Fetcher->>Fetcher: fetch(new_url, limit-1)
else Success
Fetcher-->>Metadata: Net::HTTPSuccess response
end
end
end
Metadata->>Metadata: parse(response)
Metadata-->>LinkCard: extracted metadata
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
mousu-a
force-pushed
the
bug/fix-link-card-encoding
branch
from
7668893 to
7c33520
Compare
mousu-a
changed the title
![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: 1
🤖 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/metadata.rb`:
- Around line 10-17: Metadata#fetch currently calls Net::HTTP#request_get
directly and can raise SSL/network exceptions; set sensible timeouts on the
Net::HTTP instance (open_timeout and read_timeout) and wrap the request in a
rescue that catches Net::OpenTimeout, Net::ReadTimeout, SocketError,
OpenSSL::SSL::SSLError (and optionally StandardError as a fallback), log the
error via the existing logger, and return nil instead of letting the exception
propagate; update the code around http = Net::HTTP.new(...), http.use_ssl = true
and response = http.request_get(`@uri.request_uri`) to apply these changes.
🪄 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: b28c683f-7157-41a5-85a1-a4984df4eff7
📒 Files selected for processing (1)
app/models/metadata.rb
🚀 Review AppURL: https://bootcamp-pr-9778-fvlfu45apq-an.a.run.app
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/system/markdown_test.rb (1)
131-132: アサーションをリンクカード要素に限定し、タイトル完全一致依存を緩めると堅牢です。Line 132 の
assert_textはページ全体を対象にするため、意図しない場所の同文言でも通る可能性があります。文字化け回帰の目的に合わせて、.a-link-card__title-textへのスコープ限定＋部分一致にすると安定します。差分案
- assert_selector '.a-link-card__title' - assert_text '角谷トーク2023 本編' + assert_selector '.a-link-card__title' + assert_selector '.a-link-card__title-text', text: /角谷トーク2023/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/system/markdown_test.rb` around lines 131 - 132, 現在のテストはページ全体を対象にする assert_text を使っており誤検出の原因になるため、`.a-link-card__title` 要素内の `.a-link-card__title-text` にスコープを限定して部分一致で検証するよう修正してください: 検出には既存の assert_selector '.a-link-card__title' を残し、続けて within('.a-link-card__title') または find('.a-link-card__title') ブロック内で `.a-link-card__title-text` を対象に assert_text '角谷トーク2023 本編' を部分一致（exact: false 相当のマッチ）で実行するように変更してください。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/system/markdown_test.rb`:
- Around line 131-132: 現在のテストはページ全体を対象にする assert_text
を使っており誤検出の原因になるため、`.a-link-card__title` 要素内の `.a-link-card__title-text`
にスコープを限定して部分一致で検証するよう修正してください: 検出には既存の assert_selector '.a-link-card__title'
を残し、続けて within('.a-link-card__title') または find('.a-link-card__title') ブロック内で
`.a-link-card__title-text` を対象に assert_text '角谷トーク2023 本編' を部分一致（exact: false
相当のマッチ）で実行するように変更してください。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 543dd44d-4049-44f9-8df3-916a84c9b1d7
📒 Files selected for processing (1)
test/system/markdown_test.rb
Contributor
Author
|
@komagata 現状やらなければいけないと考えている要件は以下の通りです。
Gemを導入するパターンでは、URLへのリクエストとエラーハンドリングはGemにやってもらえます。 自前で実装するパターンでは、URLへのリクエストとエラーハンドリングを自前で実装しなければいけなくなります。 以下の処理はどちらのパターンでも自前で実装する必要があります。
自分の意見としては、Gemを導入し、URLへのリクエストとエラーハンドリングを任せ、セキュリティチェックやエンコーディング設定などのリンクカード特有の設定に関しては自前で実装するのが良いと考えているのですがどうでしょうか？ |
Member
|
@mousu-a なるほどです。
これは具体的には何をどうチェックする感じですか？ |
Contributor
Author
|
@komagata |
Member
|
@mousu-a OSSなのでPRをいただくことには問題ないです。 |
Contributor
Author
|
@komagata
ユーザーが入力したリンクカードのURLが安全なものかどうかを以下の方法でチェックします。
|
Member
|
@mousu-a なるほどですね、説明ありがとうございます。 僕の印象としてはFaradayはおもに並列に複数のHTTPアクセスをするときに効率的に速くやるためのgemってイメージなので、ファーストチョイスとしてはNet::HTTPとかでいいのではないか、FaradayはちょっとToo muchなのではないかな〜という感想です。 |
mousu-a
force-pushed
the
bug/fix-link-card-encoding
branch
from
2d6e277 to
29f235b
Compare
mousu-a
commented
Apr 16, 2026
| end | ||
| http.open_timeout = DEFAULT_TIMEOUT | ||
| http.read_timeout = DEFAULT_TIMEOUT | ||
| http.response_body_encoding = true |
Contributor
Author
There was a problem hiding this comment.
http.response_body_encoding = trueとすることで、encodingをHTMLから推測してくれるようになります。
https://github.com/ruby/ruby/blob/master/lib/net/http.rb#L2450
https://github.com/ruby/ruby/blob/master/lib/net/http/response.rb#L372
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/models/link_fetcher/safety_validator.rb (1)
59-65:valid_http_uri?の戻り値が不整合ですLine 62 で
nilを返していますが、メソッド名の?サフィックスは boolean を返すことを示唆しています。Line 64 はtrue/falseを返すため、一貫性がありません。呼び出し元の
valid?ではunlessで falsy チェックしているため動作はしますが、明示的にfalseを返す方が意図が明確です。♻️ 修正案
def valid_http_uri?(uri) port = uri.port || uri.inferred_port - return nil unless [80, 443].include?(port) + return false unless [80, 443].include?(port) - uri.scheme&.downcase&.in?(%w[http https]) + uri.scheme&.downcase&.in?(%w[http https]) || false end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/link_fetcher/safety_validator.rb` around lines 59 - 65, The method valid_http_uri? returns nil for unsupported ports but returns boolean elsewhere, causing inconsistency; change it to return false instead of nil for non-allowed ports so the method always returns a boolean (true/false). Update the guard in valid_http_uri? (references: method name valid_http_uri?, the port check using uri.port || uri.inferred_port and the in?(%w[http https]) scheme check) to return false when the port is not in [80, 443] while keeping the rest of the logic intact.app/models/link_fetcher/fetcher.rb (1)
62-69: URI ライブラリの使用に不整合がありますLine 28 では
Addressable::URI.parseを使用していますが、Line 66 では標準ライブラリのURI.joinを使用しています。コード全体でAddressable::URIを一貫して使用しているため、統一性のためAddressable::URI.joinの使用を検討してください。♻️ 修正案
def build_redirect_url(url, response) location = response['location'] return nil if location.blank? - URI.join(url, location).to_s - rescue URI::InvalidURIError + Addressable::URI.join(url, location).to_s + rescue Addressable::URI::InvalidURIError nil end🤖 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 62 - 69, The build_redirect_url method uses the stdlib URI.join but elsewhere Addressable::URI.parse is used; change this to use Addressable::URI.join for consistency (use the same Addressable::URI namespace when combining the base url and response['location']), ensure the result is converted to a string as before, and update the rescue to catch Addressable::URI::InvalidURIError (or the appropriate Addressable exception) instead of URI::InvalidURIError to correctly handle join errors in build_redirect_url.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@db/fixtures/reports.yml`:
- Around line 509-580: Add a test that exercises the
SafetyValidator::BLOCKED_IP_RANGES using the URLs from the report91 fixture:
load report91's URLs and for each blocked-IP URL call the link-card generation
path (e.g., the method used to build/fetch cards such as LinkCard.build or
LinkCard.fetch) and assert the result is rejected (returns nil or a blocked
response) rather than producing a card; reference
SafetyValidator::BLOCKED_IP_RANGES and report91 in the test so the blocked
address list is actually exercised.
---
Nitpick comments:
In `@app/models/link_fetcher/fetcher.rb`:
- Around line 62-69: The build_redirect_url method uses the stdlib URI.join but
elsewhere Addressable::URI.parse is used; change this to use
Addressable::URI.join for consistency (use the same Addressable::URI namespace
when combining the base url and response['location']), ensure the result is
converted to a string as before, and update the rescue to catch
Addressable::URI::InvalidURIError (or the appropriate Addressable exception)
instead of URI::InvalidURIError to correctly handle join errors in
build_redirect_url.
In `@app/models/link_fetcher/safety_validator.rb`:
- Around line 59-65: The method valid_http_uri? returns nil for unsupported
ports but returns boolean elsewhere, causing inconsistency; change it to return
false instead of nil for non-allowed ports so the method always returns a
boolean (true/false). Update the guard in valid_http_uri? (references: method
name valid_http_uri?, the port check using uri.port || uri.inferred_port and the
in?(%w[http https]) scheme check) to return false when the port is not in [80,
443] while keeping the rest of the logic intact.
🪄 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: e570afd0-ba41-4864-86db-7eb1b796ba24
📒 Files selected for processing (5)
app/models/link_fetcher/fetcher.rbapp/models/link_fetcher/safety_validator.rbapp/models/metadata.rbdb/fixtures/reports.ymltest/system/markdown_test.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- test/system/markdown_test.rb
mousu-a
force-pushed
the
bug/fix-link-card-encoding
branch
from
de8dd73 to
fa9936b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
app/models/link_fetcher/safety_validator.rb (1)
45-49:uri.schemeが nil の場合の NoMethodError
valid_http_uri?はuri.scheme.downcaseを直接呼んでいますが、schemeがnil（例:Addressable::URI.parse('example.com')）の場合NoMethodErrorになります。Metadata経由では事前にLinkChecker::Checker.valid_url?が呼ばれるため現状は実質ガードされていますが、SafetyValidator単体の堅牢性としてはuri.scheme&.downcase&.in?(...)もしくは先にscheme存在確認する方が安全です。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/link_fetcher/safety_validator.rb` around lines 45 - 49, valid_http_uri? calls uri.scheme.downcase which raises NoMethodError when scheme is nil; update the method (in SafetyValidator#valid_http_uri?) to guard the scheme before downcasing — e.g. check presence with uri.scheme && uri.scheme.downcase.in?(%w[http https]) or use safe navigation uri.scheme&.downcase&.in?(...), and keep the port check (port = uri.port || uri.inferred_port) unchanged so the method safely returns false for nil schemes.test/models/link_fetcher/fetcher_test.rb (2)
45-51:build_redirect_urlテストは外部URL不要
build_redirect_urlは内部でURI.joinするだけで実ネットワークを叩かないため、httpbin.orgを例示しなくても任意のダミーURL（例:https://example.com/redirect/1）で十分です。外部サービス名がテストに現れると誤読を招きやすいので、ダミードメインにする方が意図が明確です。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/models/link_fetcher/fetcher_test.rb` around lines 45 - 51, The test for Fetcher.build_redirect_url uses an external domain; change the test input URL (in the test '.build_redirect_url' block) from 'https://httpbin.org/redirect/1' to a dummy domain such as 'https://example.com/redirect/1' and update the expected result accordingly (i.e., 'https://example.com/get') so the test still validates URI.join behavior without referencing an external service.
7-33: VCRで外部DNSはスタブされない点に注意VCRはHTTPリクエストを録画/再生しますが、
Resolv.getaddresses（SafetyValidator.valid?内）およびLinkChecker::Checker.valid_domain?のDNS解決はスタブされません。そのため、bootcamp.fjord.jpやbootcamp.fjo.jpの解決結果に依存してテストが不安定になる恐れがあります（オフライン環境・CIのDNS遮断時に失敗、将来ドメインの状態変化で失敗など）。可能であれば
Resolv.stub :getaddresses, ...でDNS部分もスタブするとより再現性のあるテストになります。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/models/link_fetcher/fetcher_test.rb` around lines 7 - 33, Tests rely on live DNS resolution via Resolv.getaddresses (called inside SafetyValidator.valid? and LinkChecker::Checker.valid_domain?), which makes them flaky; update the tests to stub DNS by wrapping the VCR blocks (or the entire test) with a Resolv.stub :getaddresses returning deterministic IPs (or raising for invalid/blocked domains) so SafetyValidator.valid? and LinkChecker::Checker.valid_domain? get controlled responses; ensure the stub is scoped to each test case so valid_url returns expected addresses and invalid/blocked cases return nil/raise as needed and is restored after the test.test/models/link_fetcher/safety_validator_test.rb (2)
42-47:to_range.firstによる IPv6 範囲の扱い
IPAddr#to_range.firstは範囲先頭のIPを返しますが、::/128や0.0.0.0/32のような /32 や /128 の単独アドレスは問題ない一方、to_rangeは一部環境/バージョンのIPAddrで意図通りに列挙できない場合があります。各レンジの代表IPとしてip_range.to_s.split('/').firstでネットワークアドレスを直接使う方がシンプルで意図明確です。動作に問題なければ現状維持でも構いません。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/models/link_fetcher/safety_validator_test.rb` around lines 42 - 47, The test uses ip_range.to_range.first which can behave inconsistently for singleton IPv4/IPv6 ranges; change the representative IP selection in the test for SafetyValidator::BLOCKED_IP_RANGES to use the network address string parsed from the CIDR (e.g. derive the first address via ip_range.to_s.split('/').first) and pass that into SafetyValidator.safe_ip? — update the test loop that builds blocked_ip_addr and keep the rest of the test (iterating BLOCKED_IP_RANGES and asserting assert_not SafetyValidator.safe_ip?(...)) unchanged.
7-25: 実DNS解決に依存するテスト
'https://bootcamp.fjord.jp/'を使うvalid?/valid_http_uri?のテストは、valid?内のResolv.getaddresses(uri.host)が実DNSを引きます。CIのネットワーク状況やドメインのDNS状態に依存するため、Resolv.stub(:getaddresses, ['93.184.216.34']) { ... }のようにスタブするか、IP直指定（http://8.8.8.8/）にしておくと安定します。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/models/link_fetcher/safety_validator_test.rb` around lines 7 - 25, The tests call SafetyValidator.valid? and SafetyValidator.valid_http_uri? using a real host which triggers Resolv.getaddresses and makes tests flaky; update the tests to avoid real DNS by stubbing Resolv.getaddresses (e.g. Resolv.getaddresses(uri.host) => ['93.184.216.34']) around the assertions or replace the URI with a literal IP (e.g. http://8.8.8.8/) so SafetyValidator.valid? and SafetyValidator.valid_http_uri? no longer perform real DNS lookups during the test.
🤖 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/safety_validator.rb`:
- Around line 38-43: The current validity check in
LinkFetcher::SafetyValidator#valid? can be bypassed by DNS rebind at request
time because LinkFetcher::Fetcher#build_http calls Net::HTTP.new with the
hostname again; to fix, ensure you perform DNS resolution once, validate the
resolved IP(s) with safe_ip? and then have build_http use the validated IP
address (call Net::HTTP.new(resolved_ip, port)) while preserving the original
hostname for the HTTP Host header and TLS SNI (set Host header and configure TLS
SNI to the original uri.host) so the connection uses the checked IP but presents
the original host name; update the code paths that call build_http to accept and
pass the resolved IP and keep the safety check in SafetyValidator#valid? as the
single source of truth.
---
Nitpick comments:
In `@app/models/link_fetcher/safety_validator.rb`:
- Around line 45-49: valid_http_uri? calls uri.scheme.downcase which raises
NoMethodError when scheme is nil; update the method (in
SafetyValidator#valid_http_uri?) to guard the scheme before downcasing — e.g.
check presence with uri.scheme && uri.scheme.downcase.in?(%w[http https]) or use
safe navigation uri.scheme&.downcase&.in?(...), and keep the port check (port =
uri.port || uri.inferred_port) unchanged so the method safely returns false for
nil schemes.
In `@test/models/link_fetcher/fetcher_test.rb`:
- Around line 45-51: The test for Fetcher.build_redirect_url uses an external
domain; change the test input URL (in the test '.build_redirect_url' block) from
'https://httpbin.org/redirect/1' to a dummy domain such as
'https://example.com/redirect/1' and update the expected result accordingly
(i.e., 'https://example.com/get') so the test still validates URI.join behavior
without referencing an external service.
- Around line 7-33: Tests rely on live DNS resolution via Resolv.getaddresses
(called inside SafetyValidator.valid? and LinkChecker::Checker.valid_domain?),
which makes them flaky; update the tests to stub DNS by wrapping the VCR blocks
(or the entire test) with a Resolv.stub :getaddresses returning deterministic
IPs (or raising for invalid/blocked domains) so SafetyValidator.valid? and
LinkChecker::Checker.valid_domain? get controlled responses; ensure the stub is
scoped to each test case so valid_url returns expected addresses and
invalid/blocked cases return nil/raise as needed and is restored after the test.
In `@test/models/link_fetcher/safety_validator_test.rb`:
- Around line 42-47: The test uses ip_range.to_range.first which can behave
inconsistently for singleton IPv4/IPv6 ranges; change the representative IP
selection in the test for SafetyValidator::BLOCKED_IP_RANGES to use the network
address string parsed from the CIDR (e.g. derive the first address via
ip_range.to_s.split('/').first) and pass that into SafetyValidator.safe_ip? —
update the test loop that builds blocked_ip_addr and keep the rest of the test
(iterating BLOCKED_IP_RANGES and asserting assert_not
SafetyValidator.safe_ip?(...)) unchanged.
- Around line 7-25: The tests call SafetyValidator.valid? and
SafetyValidator.valid_http_uri? using a real host which triggers
Resolv.getaddresses and makes tests flaky; update the tests to avoid real DNS by
stubbing Resolv.getaddresses (e.g. Resolv.getaddresses(uri.host) =>
['93.184.216.34']) around the assertions or replace the URI with a literal IP
(e.g. http://8.8.8.8/) so SafetyValidator.valid? and
SafetyValidator.valid_http_uri? no longer perform real DNS lookups during the
test.
🪄 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: 98eba5ce-0068-441e-b6e0-04b730276d4d
📒 Files selected for processing (8)
app/models/link_card/card.rbapp/models/link_fetcher/fetcher.rbapp/models/link_fetcher/safety_validator.rbapp/models/metadata.rbdb/fixtures/reports.ymltest/cassettes/link_fetcher/fetcher/fetch/success.ymltest/models/link_fetcher/fetcher_test.rbtest/models/link_fetcher/safety_validator_test.rb
💤 Files with no reviewable changes (1)
- app/models/link_card/card.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- db/fixtures/reports.yml
- app/models/link_fetcher/fetcher.rb
Comment on lines
+38
to
+43
| def valid?(uri) | ||
| return false unless valid_http_uri?(uri) | ||
|
|
||
| ips = Resolv.getaddresses(uri.host) | ||
| ips.all? { |ip| safe_ip?(IPAddr.new(ip)) } | ||
| end |
Contributor
There was a problem hiding this comment.
DNSリバインディング（TOCTOU）リスクについて補足
valid? では Resolv.getaddresses(uri.host) で解決したIPを検証していますが、実際のリクエストは LinkFetcher::Fetcher#build_http 内の Net::HTTP.new(uri.host, ...) がホスト名から再度DNS解決を行うため、検証時と接続時で異なるIPに解決される（DNSリバインディング）可能性があります。攻撃者が制御するDNSを使うと、1回目は公開IP、2回目はプライベートIPを返すことでブロックをすり抜けられます。
短TTL攻撃への完全対策が本PRのスコープ外であれば現状維持で問題ありません。対策するなら、解決したIPをそのまま Net::HTTP.new(ip, port) に渡し、Host ヘッダと SNI にオリジナルのホスト名を設定する実装が定石です。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/models/link_fetcher/safety_validator.rb` around lines 38 - 43, The
current validity check in LinkFetcher::SafetyValidator#valid? can be bypassed by
DNS rebind at request time because LinkFetcher::Fetcher#build_http calls
Net::HTTP.new with the hostname again; to fix, ensure you perform DNS resolution
once, validate the resolved IP(s) with safe_ip? and then have build_http use the
validated IP address (call Net::HTTP.new(resolved_ip, port)) while preserving
the original hostname for the HTTP Host header and TLS SNI (set Host header and
configure TLS SNI to the original uri.host) so the connection uses the checked
IP but presents the original host name; update the code paths that call
build_http to accept and pass the resolved IP and keep the safety check in
SafetyValidator#valid? as the single source of truth.
Contributor
Author
There was a problem hiding this comment.
Host ヘッダと SNIにホスト名を入れるやり方だと自前でやることが増えてしまって大変なので、
http.ipaddr=にチェックしたIPを入れるのではどうですか？
Contributor
There was a problem hiding this comment.
@mousu-a、素晴らしいアイデアです！http.ipaddr= の活用は非常にシンプルで実用的な解決策です。
Net::HTTP#ipaddr= はセッション開始前であれば接続先のIPアドレスを固定でき、@address（ホスト名）はそのまま残るため、TLS SNI と Host ヘッダは自動的に元のホスト名を使い続けます。手動設定は不要です。
ただし、この方法を実現するには SafetyValidator.valid? が現在 true/false しか返さない点が課題になります。build_http に渡す検証済みIPが必要なので、以下のような設計変更が考えられます。
# safety_validator.rb に resolved_ips を返すメソッドを追加
module_function
def resolve_safe_ips(uri)
return nil unless valid_http_uri?(uri)
ips = Resolv.getaddresses(uri.host)
return nil if ips.empty?
return nil unless ips.all? { |ip| safe_ip?(IPAddr.new(ip)) }
ips
end# fetcher.rb の build_http を修正
def build_http(uri)
safe_ips = SafetyValidator.resolve_safe_ips(uri)
return nil unless safe_ips # バリデーション失敗時は nil
http = Net::HTTP.new(uri.host, uri.port)
http.ipaddr = safe_ips.first # 検証済みIPに固定 → 再DNS解決なし
http.use_ssl = uri.scheme == 'https'
http.verify_mode = OpenSSL::SSL::VERIFY_PEER
http.open_timeout = DEFAULT_TIMEOUT
http.read_timeout = DEFAULT_TIMEOUT
http
endこれにより：
- DNS解決は
SafetyValidator内で1回のみ http.ipaddr=によってTCP接続先が検証済みIPに固定される- ホスト名は
@addressに残るため Host ヘッダ・SNI は自動的に正しく設定される - DNSリバインディング攻撃を防止できる
既存の valid? メソッドはそのまま残しつつ、resolve_safe_ips を追加する形であれば後方互換性も維持できます。ご検討ください！
🧠 Learnings used
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: 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: 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/models/graduation_notifier.rb:23-28
Timestamp: 2025-09-12T21:16:47.639Z
Learning: Rails upgrade PRにおいて、configuration systemの変更やwebhook設定の改善提案も、アップグレードに直接関連しない場合は別PRで対応すべきである。PRのスコープ維持が重要。
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:52-56
Timestamp: 2025-09-12T21:17:45.295Z
Learning: Rails upgrade PRにおいて、configuration systemの改善提案（shared configuration fallback等）も、アップグレードに直接関連しない場合は別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.
Contributor
Author
There was a problem hiding this comment.
その場合SafetyValidatorではなくなると思うんですけど、どういう命名にしたらいいと思いますか？
Contributor
There was a problem hiding this comment.
@mousu-a、ご指摘のとおり、IPを返すメソッドが増えると「Validator」という名前と責務がずれてくる感覚はありますね。いくつか候補を挙げます。
案1: SafetyValidator のまま維持する
resolve_safe_ips も「安全なIPかどうかを検証した上で返す」処理なので、バリデーションの一形態と捉えると名前の一貫性は保てます。最もシンプルで変更コストが低い選択肢です。
案2: LinkFetcher::SafeResolver
DNS解決 + 安全性チェック = 安全な解決（SafeResolve）
「解決する（Resolve）」と「安全（Safe）」を組み合わせた名前で、IPを返す責務も自然に表現できます。valid? も「解決前の事前チェック」として残せます。
案3: LinkFetcher::SsrfGuard
目的（SSRF対策）を直接名前に出す方法です。guard という言葉は「検証してブロック/許可する」ニュアンスを含むので、valid? も resolve_safe_ips も同じ傘の下に収まります。
個人的なおすすめ
責務の中心が「SSRFを防ぐための安全なDNS解決」であれば SafeResolver が一番しっくりくると思います。valid? はそのまま残して、resolve_safe_ips を追加する形でも命名と実態が合います。
module LinkFetcher
module SafeResolver # ← リネーム
def valid?(uri) ... end # 既存のバリデーション
def resolve_safe_ips(uri) ... end # 新規追加
end
endいずれにせよ、既存の valid? が外部から呼ばれている箇所（Fetcher 等）のリネームも必要になるので、その点だけご注意ください。
🧠 Learnings used
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: 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: su-su-su-su
Repo: fjordllc/bootcamp PR: 0
File: :0-0
Timestamp: 2025-07-26T15:24:14.799Z
Learning: fjordllc/bootcampプロジェクトでは、Rubyの述語メソッドには?を付ける命名規則（例: can_be_checker?）を徹底している。
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:52-56
Timestamp: 2025-09-12T21:17:45.295Z
Learning: Rails upgrade PRにおいて、configuration systemの改善提案（shared configuration fallback等）も、アップグレードに直接関連しない場合は別PRで対応すべきである。PRのスコープ維持が重要。
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: sekito1107
Repo: fjordllc/bootcamp PR: 9081
File: app/models/transcoder/client.rb:103-107
Timestamp: 2025-08-27T03:06:37.725Z
Learning: In Ruby on Rails ActiveStorage code, when using safe navigation operators (&.) for validation, maintain consistency by using the validated variable in subsequent operations rather than re-dereferencing the same chain directly.
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: config/initializers/sorcery.rb:0-0
Timestamp: 2025-09-11T16:02:24.782Z
Learning: Rails アップグレードPRにおいて、JWT設定の改善では複数のソース（secrets, credentials, ENV）を優先順位付きで検索し、本番環境では未設定時に起動を停止させる実装が推奨される。開発・テスト環境でのみsecret_key_baseフォールバックを許可する構成が安全である。
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.
Contributor
Author
There was a problem hiding this comment.
SafeResolverですが、危険なIPでも安全に解決して返してくれる感じがないですか？
安全が保証されたIPを返す 安全が保証できなければraiseする / nilを返す クラス / モジュールを作ろうとしているんですが、ちょっと命名に迷っています。
Contributor
There was a problem hiding this comment.
@mousu-a、おっしゃるとおりで、SafeResolverは「危険なIPを安全に処理して解決してくれる」という読み方もできますね。ご指摘はもっともです。
「安全が保証されたIPだけを返す」という意図を明確にする候補をいくつか挙げます：
案1: SafeIpResolver
「安全なIP（Safe IP）を解決して返す（Resolver）」
Safe が IP に直接かかるため、「安全が保証されたIPを返す」という意味が一番伝わりやすいと思います。
案2: VerifiedIpResolver
「検証済みIP（Verified IP）を解決して返す（Resolver）」
Verified は「検証を通過した」というニュアンスがあり、「安全かどうかチェックしてから返す」という処理の流れも表現できます。
案3: SsrfGuard
「SSRF攻撃を防ぐ（Guard）」
目的（SSRF対策）を直接名前に出す方法です。valid? もIPを返すメソッドも「SSRFを防ぐ手段」として一つの傘に収まります。ただしGuard単体だとIPを返すという挙動が少し伝わりにくいかもしれません。
個人的なおすすめ
「安全が保証されたIPを返す、できなければnil/raise」という責務を最も明確に表現するなら SafeIpResolver が一番しっくりくると思います。Safe → Ip → Resolver の語順で「安全なIPを解決して返すもの」と素直に読めます。
🐰 ✨
🧠 Learnings used
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: 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/models/graduation_notifier.rb:23-28
Timestamp: 2025-09-12T21:16:47.639Z
Learning: Rails upgrade PRにおいて、configuration systemの変更やwebhook設定の改善提案も、アップグレードに直接関連しない場合は別PRで対応すべきである。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: 9101
File: app/notifiers/discord_notifier.rb:52-56
Timestamp: 2025-09-12T21:17:45.295Z
Learning: Rails upgrade PRにおいて、configuration systemの改善提案（shared configuration fallback等）も、アップグレードに直接関連しない場合は別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.
Contributor
Author
|
@komagata そこで質問なのですが、特定のURLでリンクカードが文字化けする問題のPR(本PR)と、fetch周りの処理を改善するPR(別PR)に分けるべきでしょうか？ 個人的には、コミットの境目もハッキリ分かれていますし(コミット履歴)、このままだとレビューも大変になりそうなので分けた方が良いと考えています。 |
mousu-a
force-pushed
the
bug/fix-link-card-encoding
branch
from
fa9936b to
5f14672
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/views/application/_global_nav.slim (1)
25-31:⚠️ Potential issue | 🟠 Major日報リンク遷移先の条件が一貫していません
Line 25 と Line 29 の条件分岐が異なるため、以下の不整合が発生しています：
- Line 29:
admin_or_mentor_login?で未チェック件数バッジを表示
- 結果：管理者も未チェック件数が見える
- Line 25:
mentor_login?でリンク先を分岐
- 結果：管理者は
reports_path（全件）に遷移管理者には未チェック件数バッジが表示されるにもかかわらず、クリックすると全件表示になる挙動となり、UXが不整合です。
修正方法: Line 25 を
admin_or_mentor_login? ? reports_unchecked_index_path : reports_pathに揃えるか、Line 29 をmentor_login?に揃える必要があります（app/helpers/page_tabs/reports_helper.rbline 7 もadmin_or_mentor_login?を使用しており、こちらに合わせるのが妥当です）。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/application/_global_nav.slim` around lines 25 - 31, The reports link uses inconsistent login checks: change the condition that sets reports_link (variable reports_link) to use the same predicate as the badge so admin_or_mentor_login? ? reports_unchecked_index_path : reports_path (instead of mentor_login?), ensuring the link destination, badge rendering (Cache.unchecked_report_count), and helpers (e.g., page_tabs/reports_helper.rb behavior) are consistent with admin_or_mentor_login? and leave current_link(...) unchanged.
🤖 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/safe_ip_resolver.rb`:
- Around line 48-52: The log currently prints the full URI (in the rescue
handlers for InvalidUriError and UnsafeIpError), which may leak sensitive
userinfo or query tokens; update the two rescue blocks (the handlers for
InvalidUriError => e and UnsafeIpError => e in SafeIpResolver/SafeResolver) to
log only non-sensitive components (e.g., uri.scheme, uri.host and optional port)
and avoid interpolating the full uri string; also defensively handle cases where
uri may be nil or not a parsed URI (use safe access or to_s fallback for
scheme/host) so logging never exposes query/userinfo.
- Around line 25-34: Change the special-purpose IP ranges list in
safe_ip_resolver.rb to be more fail-closed: replace IPAddr.new('0.0.0.0/32')
with IPAddr.new('0.0.0.0/8') and add the common non-global/reserved ranges
(IPv4: 127.0.0.0/8, 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, 169.254.0.0/16,
100.64.0.0/10, 192.0.2.0/24, 198.51.100.0/24, 203.0.113.0/24, 224.0.0.0/4,
240.0.0.0/4) and IPv6 equivalents (::1/128, ::/128 handling, fe80::/10,
fc00::/7, ff00::/8, 2001:db8::/32, ::ffff:0:0/96) into the same array (the
constant with IPAddr.new entries) so the resolver (the list used by the safe IP
check in safe_ip_resolver.rb) explicitly rejects multicast, reserved,
documentation, link-local, loopback, unique-local and other non-global
addresses.
- Around line 56-60: The validation in valid_http_uri? should be fail-closed for
missing scheme/host: explicitly check that uri.scheme and uri.host are present
(non-nil) before calling uri.scheme.downcase or proceeding to DNS resolution,
and return false immediately if either is missing; keep the existing port check
(uri.port || uri.inferred_port) and scheme validation but perform them only
after the presence checks. Update valid_http_uri? (and any places that call
Resolv.getaddresses with uri.host) so you never call methods on nil (e.g., avoid
uri.scheme.downcase when scheme is nil and avoid passing nil to
Resolv.getaddresses).
- Around line 44-47: resolve_ips currently treats an empty DNS result as safe
because all_ips_safe?([]) returns true; update the logic in the method that
calls Resolv.getaddresses (the resolve_ips method using
Resolv.getaddresses(uri.host)) to explicitly reject empty results by raising
UnsafeIpError when ips.empty? is true, so that resolve_ips never returns an
empty array (matching callers like fetcher.rb which only check for nil).
---
Outside diff comments:
In `@app/views/application/_global_nav.slim`:
- Around line 25-31: The reports link uses inconsistent login checks: change the
condition that sets reports_link (variable reports_link) to use the same
predicate as the badge so admin_or_mentor_login? ? reports_unchecked_index_path
: reports_path (instead of mentor_login?), ensuring the link destination, badge
rendering (Cache.unchecked_report_count), and helpers (e.g.,
page_tabs/reports_helper.rb behavior) are consistent with admin_or_mentor_login?
and leave current_link(...) unchanged.
🪄 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: b2814a75-5cfd-4607-b57d-2b63131cc403
📒 Files selected for processing (10)
app/models/link_card/card.rbapp/models/link_fetcher/fetcher.rbapp/models/link_fetcher/safe_ip_resolver.rbapp/models/metadata.rbapp/views/application/_global_nav.slimdb/fixtures/reports.ymltest/cassettes/link_fetcher/fetcher/fetch/success.ymltest/models/link_fetcher/fetcher_test.rbtest/models/link_fetcher/safe_ip_resolver_test.rbtest/system/markdown_test.rb
💤 Files with no reviewable changes (1)
- app/models/link_card/card.rb
✅ Files skipped from review due to trivial changes (1)
- test/models/link_fetcher/fetcher_test.rb
🚧 Files skipped from review as they are similar to previous changes (4)
- app/models/metadata.rb
- test/system/markdown_test.rb
- db/fixtures/reports.yml
- app/models/link_fetcher/fetcher.rb
Comment on lines
+56
to
+60
| def valid_http_uri?(uri) | ||
| port = uri.port || uri.inferred_port | ||
|
|
||
| port.in?([80, 443]) && uri.scheme.downcase.in?(%w[http https]) | ||
| end |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "safe_ip_resolver.rb" -type fRepository: fjordllc/bootcamp
Length of output: 106
🏁 Script executed:
cd app && find . -name "safe_ip_resolver.rb" -type f | head -20Repository: fjordllc/bootcamp
Length of output: 102
🏁 Script executed:
fd -t f "safe_ip_resolver" 2>/dev/null || find . -name "*safe_ip_resolver*" -type f 2>/dev/null | head -20Repository: fjordllc/bootcamp
Length of output: 154
🏁 Script executed:
rg -l "safe_ip_resolver" --type rbRepository: fjordllc/bootcamp
Length of output: 87
🏁 Script executed:
rg "valid_http_uri" -A 20 -B 5 --type rbRepository: fjordllc/bootcamp
Length of output: 87
🏁 Script executed:
cat -n app/models/link_fetcher/safe_ip_resolver.rbRepository: fjordllc/bootcamp
Length of output: 2408
🏁 Script executed:
cat -n test/models/link_fetcher/safe_ip_resolver_test.rbRepository: fjordllc/bootcamp
Length of output: 3184
scheme/host 欠落URIを fail-closed にしてください。
uri.scheme.downcase は scheme がないURLで NoMethodError になり、現在の rescue 対象外です。同様に、uri.host が nil の場合、Resolv.getaddresses(nil) へ進むことになります。これらの無効な状態を検証時にブロックし、false を返してください。
🛡️ 修正例
def valid_http_uri?(uri)
+ return false if uri.blank?
+
+ scheme = uri.scheme&.downcase
+ return false if scheme.blank? || uri.host.blank?
+
port = uri.port || uri.inferred_port
- port.in?([80, 443]) && uri.scheme.downcase.in?(%w[http https])
+ port.in?([80, 443]) && scheme.in?(%w[http https])
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/models/link_fetcher/safe_ip_resolver.rb` around lines 56 - 60, The
validation in valid_http_uri? should be fail-closed for missing scheme/host:
explicitly check that uri.scheme and uri.host are present (non-nil) before
calling uri.scheme.downcase or proceeding to DNS resolution, and return false
immediately if either is missing; keep the existing port check (uri.port ||
uri.inferred_port) and scheme validation but perform them only after the
presence checks. Update valid_http_uri? (and any places that call
Resolv.getaddresses with uri.host) so you never call methods on nil (e.g., avoid
uri.scheme.downcase when scheme is nil and avoid passing nil to
Resolv.getaddresses).
Contributor
Author
There was a problem hiding this comment.
#9778 (comment)
このコメントと同様で、リンクの正当性チェックはこのモジュールの責務ではないので、その点に関して過度に防衛的なコードにする必要性はないと考えています。
Contributor
There was a problem hiding this comment.
@mousu-a、ご説明ありがとうございます。URIの正当性チェックは上流の LinkFetcher::Fetcher の責務であり、SafeIpResolver はIPの安全性解決のみを担うというコントラクト設計ですね。了解しました。この指摘は取り下げます。
🧠 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: 9778
File: app/models/link_fetcher/safe_ip_resolver.rb:25-34
Timestamp: 2026-04-21T03:37:11.383Z
Learning: In fjordllc/bootcamp PR `#9778`, `LinkFetcher::SafeIpResolver::BLOCKED_IP_RANGES` intentionally excludes documentation/TEST-NET ranges (192.0.2.0/24, 198.51.100.0/24, 203.0.113.0/24), benchmark ranges (198.18.0.0/15), multicast (224.0.0.0/4), and reserved (240.0.0.0/4) from the blocklist. The module's scope is strictly SSRF safety (blocking access to genuinely reachable internal/sensitive infrastructure), not link card validity. These ranges are excluded because they are not real SSRF vectors in practice (they don't route to internal services, and TCP connections to them are rejected at the OS/network layer).
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: 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: 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/models/graduation_notifier.rb:23-28
Timestamp: 2025-09-12T21:16:47.639Z
Learning: Rails upgrade PRにおいて、configuration systemの変更やwebhook設定の改善提案も、アップグレードに直接関連しない場合は別PRで対応すべきである。PRのスコープ維持が重要。
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:52-56
Timestamp: 2025-09-12T21:17:45.295Z
Learning: Rails upgrade PRにおいて、configuration systemの改善提案（shared configuration fallback等）も、アップグレードに直接関連しない場合は別PRで対応すべきである。PRのスコープ維持が重要。
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/models/product.rb:59-61
Timestamp: 2025-09-11T14:47:53.256Z
Learning: Rails アップグレードPRでは、アップグレードに直接関連しない性能改善や機能追加の提案は避けるべき。PRのスコープを維持することが重要で、そのような改善提案は別PRで対応すべきである。
Learnt from: Miya096jp
Repo: fjordllc/bootcamp PR: 9102
File: app/controllers/mentor/buzzes_controller.rb:0-0
Timestamp: 2025-09-04T01:39:22.261Z
Learning: fjordllc/bootcamp プロジェクトでは、更新処理の成功時の通知メッセージについて、I18n を使用せずに日本語文字列を直接記述する方針で一貫性を保っている。
Learnt from: jun-kondo
Repo: fjordllc/bootcamp PR: 0
File: :0-0
Timestamp: 2025-07-23T20:42:19.974Z
Learning: fjordllc/bootcampプロジェクトでは、hタグ（見出し）の文言は日本語でハードコーディングする方針が確立されており、I18n対応は行わない。例：app/views/welcome/logo.html.slimでh2、h3タグが日本語でハードコーディングされている。
Learnt from: ryufuta
Repo: fjordllc/bootcamp PR: 8992
File: app/models/correct_answer_notifier.rb:23-23
Timestamp: 2025-07-30T07:26:36.599Z
Learning: PRのスコープを維持することが重要であり、バグ修正PRにおいて、修正対象以外の機能（例：ルーティングテスト）の追加提案は適切ではない。そのような改善が必要な場合は別PRで対応すべきである。
Learnt from: hirokiej
Repo: fjordllc/bootcamp PR: 8740
File: app/helpers/reports_helper.rb:56-64
Timestamp: 2025-06-29T03:44:15.179Z
Learning: このプロジェクト（fjordllc/bootcamp）では、ja.ymlファイルで一部の単語や文章のみI18n対応されているが、多くのテキストは日本語でハードコーディングされており、完全な国際化対応は行われていない。新しい機能でもI18n対応は不要と判断される。
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.
5f14672 to
7082fa7
Compare
Contributor
Author
|
@komagata |
mousu-a
force-pushed
the
bug/fix-link-card-encoding
branch
from
4cdc678 to
0fc3793
Compare
Contributor
Author
Member
|
@mousu-a コメント見逃しておりました。大変申し訳ありません。 |
Contributor
Author
ありがとうございます！ |
Member
|
📝 ステージングで確認。 
文字化けはしてないけど動いてないっぽい？ |
Contributor
Author
|
@komagata |
Member
|
@mousu-a 僕の方でも修正対応しております〜 |
Contributor
Author
|
@komagata 調査結果なども書きましたので少しでも助けになればと思います。 |
Member
|
@mousu-a すみません、急ぎだったのでこちらの方で修正してしまっておりました。 |
Member
|
📝 本番環境で動作確認しました。 |
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
概要
一部URL / ドメインのリンクカードが文字化けする問題を修正しました。
変更確認方法
bug/fix-link-card-encodingをローカルに取り込み、チェックアウトbin/devでサーバーを起動Screenshot
変更前
変更後
Summary by CodeRabbit
リリースノート
New Features
Bug Fixes
Refactor
Tests