-
Notifications
You must be signed in to change notification settings - Fork 71
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
アクセス可能なリンクがリンク切れと判定されないようにする #4046
アクセス可能なリンクがリンク切れと判定されないようにする #4046
Conversation
c41257e
to
13d0825
Compare
e1be455
to
7148865
Compare
@kaisumi |
7148865
to
9753544
Compare
「証明書を認証できない」と表現していた箇所について、「証明書の検証に失敗する」のような表現の方が適切であると思ったため、そのように修正しました。 |
9753544
to
4e10fa5
Compare
気づいたことがあったので、以下のとおり対応しました。
|
4e10fa5
to
4ec23db
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 完全一致: `$ apt-cache search ^vim$` | ||
- 前方一致: `$ apt-cache search ^vim` | ||
TEXT | ||
test '#extract_links from a practice' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
細かい話で恐縮ですが、ここはpage
で35行目がpractice
かと思います。
- 完全一致: `$ apt-cache search ^vim$` | ||
- 前方一致: `$ apt-cache search ^vim` | ||
user: komagata | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
published_atの記載がないと正しく表示されないケースがあるかもしれません。
[末尾が閉じ括弧の URL へのリンク](https://ja.wikipedia.org/wiki/マジックナンバー_(プログラム)) | ||
[SSLサーバー証明書の検証に失敗する host へのリンク](https://www.tablesgenerator.com/markdown_tables) | ||
[日本語を含む URL へのリンク](https://ja.wikipedia.org/wiki/あ) | ||
user: komagata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
published_atの記載がないと正しく表示されないケースがあるかもしれません。
- A. [正規表現](https://ja.wikipedia.org/wiki/%E6%AD%A3%E8%A6%8F%E8%A1%A8%E7%8F%BE) を使う。 | ||
- 完全一致: `$ apt-cache search ^vim$` | ||
- 前方一致: `$ apt-cache search ^vim` | ||
user: komagata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
published_atの記載がないと正しく表示されないケースがあるかもしれません。(他のデータでpublished_atがないものがあるのは気になりますね)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
「正しく表示されないケース」があることに気づいていませんでした。
他の page のフィクスチャも含めて published_at
を設定するように対応しようと思いますー
ただ参考までに、「正しく表示されないケース」とはどのようなケースか教えてもらえますでしょうか?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
質問ありがとうございます!説明足らずで恐縮です。
最近私が関わったPRで、WIP機能をつける際にpublished_atを追加しており、一覧表示や個別表示の際にpublished_atを表示するようにしていました。WIPの場合はその部分の表示を'執筆中'としていたのでWIPではない記事にpublished_atが抜けているとエラーが発生します。practiseでは見つけられませんでしたが、pagesでも同様の処置がされているようでした。(そして私はdb/fixturesにpublished_atを設定しないまま町田さんにデザインを依頼し、ご指摘をいただきましたorz)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
なるほど、理解しました!丁寧なご説明ありがとうございますー🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
一部コメントに対するコードが見辛くなっていたので修正しました 🙇
links = LinkChecker::Extractor.extract_all_links(documents) | ||
checker = LinkChecker::Checker.new(links) | ||
|
||
ChatNotifier.message(checker.summary_of_broken_links, username: 'リンクチェッカー', webhook_url: ENV['DISCORD_BUG_WEBHOOK_URL']) | ||
|
||
render plain: checker.summary_of_broken_links |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
※任意です
同じモジュールを2行連続で呼び出しているのが気になりまして。。。
以下になるようにCheckerをモジュールにしてしまえば可読性とパフォーマンスの両面で利点があると思いました。(メソッド名はあくまで例です)
documents = Page.all + Practice.all
summary_of_broken_links = LinkChecker::Checker.check_links_in_documents(documents)
ChatNotifier.message(summary_of_broken_links, username: 'リンクチェッカー', webhook_url: ENV['DISCORD_BUG_WEBHOOK_URL'])
render plain: summary_of_broken_links
前提:Extractorはcheck_links_in_documents内で呼び出し、@broken_links、@is_checkedは引数で渡すようなことを考えていました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kaisumi
たしかに、Checker
をモジュールにした方が可読性があがって良いですね!
同様に、 Extractor
もモジュールにしようと思いますー💪
以下、一つ相談があります。
Extractorはcheck_links_in_documents内で呼び出し
こちらについては、たしかにコントローラーから呼び出しやすくなって良いと思ったものの、Checker
の中から Extractor
を呼び出すようにすると両者が密結合する形になり、互いに影響を受けやすくテストのしやすさも減るようになるため、それぞれ分けて呼び出すようにしたいと考えています。
次のようなイメージです。
documents = Page.all + Practice.all
links = LinkChecker::Extractor.extract_links(documents)
summary_of_broken_links = LinkChecker::Checker.check_broken_links(links)
ChatNotifier.message(summary_of_broken_links, username: 'リンクチェッカー', webhook_url: ENV['DISCORD_BUG_WEBHOOK_URL'])
render plain: summary_of_broken_links
もしこのイメージでよさそうでしたら、このような方向で修正してみます。どうでしょうか?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
密結合する形になり、互いに影響を受けやすくテストのしやすさも減るようになる
なるほど!扱いやすくしておく工夫なのですね。勉強になります 😄
ご提案のイメージは読みやすくて良いと思います 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kaisumi
早速のご返信ありがとうございますー!それではこの方向で修正してみますー
app/models/link_checker/extractor.rb
Outdated
def initialize(document) | ||
@document = document | ||
end | ||
|
||
def extract_links | ||
@document.body.scan(MARKDOWN_LINK_REGEXP).map do |title, url_or_path| | ||
title = title.strip | ||
url_or_path = url_or_path.strip | ||
url_or_path = "https://bootcamp.fjord.jp#{url_or_path}" if url_or_path.match?(%r{^/}) | ||
|
||
links.select { |link| URI::DEFAULT_PARSER.make_regexp.match(link.url) } | ||
Link.new(title, url_or_path, @document.title, "https://bootcamp.fjord.jp#{@document.path}") | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
※任意です
こちらも、インスタンス化しなくても良さそうに思いました。
d236c2d
to
9049940
Compare
2c230c9
to
7766721
Compare
host が www.tablesgenerator.com のサーバーへ Net::HTTP.get_response でアクセスすると、次の例外が発生した。 OpenSSL::SSL::SSLError: SSL_connect returned=1 errno=0 state=error: certificate verify failed (unable to get local issuer certificate) 調査した結果、この host のサーバーに中間証明書が設定されていないことが原因で証明書を検証できていないようだった。 そのためこの host の URL は証明書を検証せずにレスポンスを確認することとした。
f54bfa7
to
192e5b6
Compare
192e5b6
to
09e20c9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kaisumi
いただいたコメントをもとに、修正してみました!
再度のレビューをお願いいたしますー🙏
※ コメントいただいたこと以外に新たに気づいて修正したことがあります。コードにコメントする形で説明しています。
end | ||
links | ||
texts = ['リンク切れがありました。'] | ||
texts << broken_links.sort.map(&:to_s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link クラスの sort 処理(並び順の判定ロジック)と文字列表現( #to_s
)を Link クラス自身に持たせ、Checker クラスはそのメソッドを呼び出すようにしました
links_with_valid_url = links.select { |link| valid_url?(link.url) && !denied_host?(link.url) } | ||
links_with_response = check_response(links_with_valid_url) | ||
broken_links = links_with_response.select { |link| !link.response || link.response > 403 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checker#check
の中で以下の3つの処理が行われており、メソッドの責務が大きいと思ったため、それぞれ分けて行うようにしました。
- 「url が有効」 かつ「DENY_HOST に含まれない」 link(レスポンス確認対象の link)の判定
- link の url のレスポンスの確認
- リンク切れしている link の選別
end | ||
|
||
def <=>(other) | ||
(source_url <=> other.source_url).nonzero? || url <=> other.url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check.summary のテストを常に成功させるため、 link の並び順を決める(比較する)ときに source_url だけでなく url も比較できるようにしました
※ 修正前は source_url のみで比較していたため、source_url が同じ link 同士の並び順が不安定でした
test '.check_response' do | ||
VCR.use_cassette 'link_checker/checker/check_response' do | ||
expected = [ | ||
LinkChecker::Link.new( | ||
'HDDが分かる', | ||
'http://homepage2.nifty.com/kamurai/HDD.htm', | ||
'PC性能の見方を知る', | ||
"https://bootcamp.fjord.jp/practices/#{practices(:practice3).id}", | ||
false | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
修正前は、次のように expected と actrual どちらも同じインスタンスを比較していました(簡略化しています)
actual = Checker.check_response([@link_hdd, @link_not_exist])
@link_hdd.response = false
@link_not_exist.response = 404
expected = [@link_hdd, @link_not_exist]
assert_equal expected, actual
この場合、最後に上書きされた response が actual のインスタンスにも反映され(つまり expected と actrual で常に等しくなり)、テストが意味を為していないことに気づきました。
そのため expected と actrual でインスタンスを別々に用意することにしました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀 ❗ 気が付かずにすみませんでした 😞 ご対応ありがとうございます!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
修正お疲れ様でした!良さそうです 😄
丁寧にご説明いただきありがとうございました。
そしてとても勉強になりました 👍
@kaisumi ご確認、ありがとうございますー! |
@komagata こちら、お時間のあるときにレビューをお願いいたします🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
確認しました、OKですー🙆♂️
@AudioStakes 面倒なIssueをありがとうございます〜! |
目的
以下の Issue の対応
Refs: #3185
リンク切れ通知のキャプチャ
変更前
次の3つのアクセス可能なURL へのリンクも「リンク切れ」として通知されていた。
※ Discord の通知テキストでは リンクの URL とタイトルの区切り文字(
|
)が URL の一部として解釈され、%7C
とエンコードされて通知されている変更後
上記3つのアクセス可能なURL が「リンク切れ」として通知されなくなった。
※
|
が URL の一部として解釈されないようにした(この修正は手軽に行えたため対応したものの、通知の見た目の修正は本来の Issue の目的のスコープ外であり、 #4020 で改めて対応することになっている)やったこと
URL の host がリンク切れ判定の対象外の host かどうかを期待どおりに判別できるようにした
修正前はいくつかの host (
www.amazon.co.jp
など)をリンク切れ判定の対象外として設定していたものの、 host の判別が期待どおりに動いていなかった。対応として、URL のパースに gem の
Addressable
を使用することで、host を期待どおり判別できるようにした。※ 詳細は「URL のパースとエンコードに gem の Addressable を使うように変更した理由」に記載
正規表現で末尾が
)
の URL に完全一致できるようにした修正前は、次の URL の末尾の
)
が抜け落ちた状態で処理されていた。https://ja.wikipedia.org/wiki/マジックナンバー_(プログラム)
その結果、アクセスしても 404 が返ってくるためリンク切れと判定されていた。
対応として、
URI::DEFAULT_PARSER.make_regexp
( URI.regexpと同値。 RuboCop のLint/UriRegexp
の修正を受け、前者を使用。)の正規表現を使用して、末尾の)
も含めて完全一致できるようにした。SSL サーバー証明書の検証に失敗する host は検証せずにアクセスできるようにした
修正前は、次の URL に
Net::HTTP.get_response
でアクセスすると SSL サーバー証明書の検証に失敗するという例外が発生し、「リンク切れ」と判定されていた。https://www.tablesgenerator.com/markdown_tables
この URL のサイト(Tables Generator)はブラウザからアクセス可能であるため、「リンク切れ」と判定されないように対応することとした。SSL サーバー証明書の検証に失敗する原因は、サーバーから中間証明書を取得できないことと判断した(そのように判断した理由は「Tables Generator の URL の SSL サーバー証明書の検証に失敗する原因は中間証明書を取得できないためと判断した理由」に記載)。
対応として、URL の host がこのサイトの場合は証明書の検証に失敗してもアクセスできるように(すなわちリンク切れ判定できるように)した。
他の対応手段として「リンク切れ判定の対象外とする」という対応も検討したものの、ブートキャンプでは Tables Generator へのリンク数が多いため、実際にリンク切れ判定できた方が良いと考えた。
また、SSL 証明書を検証できなくてもアクセスできるように対応する際、
Net::HTTP.get_response
よりもOpenURI.open_uri
の方がそのオプションの設定を手軽にできたため後者を使うようにした。リンク切れの通知先を「通知チャンネル」から「バグ、誤字脱字報告、機能要望🙇」に変更できるようにした
新たな環境変数
DISCORD_BUG_WEBHOOK_URL
を設定に加え、その変数に「バグ、誤字脱字報告、機能要望🙇」チャンネルの Webhook URL を設定することで、リンク切れの通知先が「バグ、誤字脱字報告、機能要望🙇」となるようにした。動作確認手順
※ 「自分用の Discord チャンネルの Webhook URL を用意できる場合」と「できない場合」の2通りに分けました。対応可能な方で動作確認をお願いします。
共通すること
$ bundle install
と$ rails db:seed
を実行自分用の Discord チャンネルの Webhook URL を用意できる場合
自分用の Discord チャンネルの Webhook URL を取得
次のように、環境変数
DISCORD_BUG_WEBHOOK_URL
に Webhook URL を設定した状態でrails console
を起動してコードを実行※ production モードの場合でしか Discord への通知を実行できないように実装されているため、 development モードで通知するためにあえて rails console でコードを実行しています
Discord の通知の内容に「共通すること」に記載の URL が含まれていないことを確認
自分用の Discord チャンネルの Webhook URL を用意できない場合
$ rails s
を実行してサーバーを起動URL のパースとエンコードに gem の Addressable を使うように変更した理由
Addressable
は、 README の Description に記載のとおり、 Ruby の標準ライブラリURI
の代わりとして扱えるように実装されており、RFC 3986 に準拠するように実装されている。これにより、文字列を URI にパースするときに URI として認識できない文字(非 ASCII 文字)の適切なエンコードを事前に行う必要がなくなり、より手軽に URI をパースできるようになるため(パースした後、適切にエンコードされた URI を生成できるという利点もある)。※ RFC 3986 は現在の URI の仕様もしくはそれに近い仕様。HTTP のリソースと仕様書 によると「インターネット標準」という状態。
※ README の Description には ‘Addressable closely conforms to RFC 3986, RFC 3987, and RFC 6570 (level 4).’ と記載されているため、完全に RFC 3986 の仕様どおりというわけではなく、対応できていないこともあるのかもしれない
経緯
事の発端としては、修正前の次のコードで 「URL の host がリンク切れ判定対象外の host かどうか」を判別できていないため、期待どおりに判別できるようにするという修正をしていた
bootcamp/app/models/link_checker/checker.rb
Lines 34 to 37 in 837c48d
host を判別できていない原因は、
URI.encode_www_form_component(link.url)
で URL の文字列全体をエンコードしていることにより、/
(https://www...
の//
)なども不要にエンコードされてしまい、uri.host
がnil
を返していたこと(すなわちすべての URL がDENY_LIST.include?(uri.host)
でfalse
を返していた)この対応として、URI::UNSAFEの正規表現に該当する文字列のみをエンコードするようにした結果、期待どおりに host が返されるようになった
この対応で OK と判断したものの、その後に同様のエンコード処理が
app/models/link_checker/client.rb
にも実装されていることに気づいたbootcamp/app/models/link_checker/client.rb
Lines 22 to 30 in 4d0eaae
両者のエンコード対象文字列の正規表現は次のようになっている
[^-_.!~*'()a-zA-Z\d;\/?:@&=+$,\[\]]
encode_ja
[-_.!~*'()a-zA-Z0-9;/?:@&=+$,%#]
以外「
#
をエンコード対象に含むか否か」などの違いがあるものの、目的はどちらも同じと捉え、どちらか一方にロジックを合わせた方が良いと考えた。調べてみると# は URL のフラグメント識別子として認識される文字列 であるためエンコードしない方が良さそうであり、
encode_ja
のロジックに合わせた方が良いと考えた。しかし、その判断が正しいのかどうか判断がつかなかった(たとえば、フラグメント識別子ではない
#
はエンコードした方が良いのかどうか?などの疑問が残った)。他の手段を調べた結果 gem の Addressable を見つけ、「エンコードしていない文字列を URI としてパース」や「パースした後にエンコードした URI を生成」ということができ、現在の URI の仕様に近い実装がされていることやスターの数の多さから信頼できる gem であると判断し、Addressable を使うこととした。
Tables Generator の URL の SSL サーバー証明書の検証に失敗する原因は中間証明書を取得できないためと判断した理由
修正前は、Tables Generator の URL. https://www.tablesgenerator.com/markdown_tables に
Net::HTTP.get_response
でアクセスすると次のような例外が生じ、「リンク切れ」と判定されていた例外が発生する原因は、エラーメッセージにあるとおり、証明書の検証に失敗すること
Tables Generator の SSL 証明書の階層構造を Google Chrome で確認すると、次の通りになっている
※ SSL 証明書の解説は サーバー証明書/中間CA証明書/ルート証明書の違いとは? の記事がわかりやすかったです
$ openssl s_client
コマンドでこのサイトの証明書チェインを確認すると、次のようにtablesgenerator.com の証明書以降の証明書チェインが続かない(つまり中間証明書を取得できない)という結果になる※
$ openssl s_client
コマンドの解説は opensslコマンドで証明書情報を確認したい の解説がわかりやすかったですこの結果から、Tables Generator のサイトから中間証明書を取得できないため検証に失敗していると判断した。
なお、中間証明書を取得できる場合は、次のような結果になる