プロジェクト

全般

プロフィール

バグ #1233

完了

Twitterのメッセージに含まれる <>& がHTML実体参照のままになっている

cob odo さんがほぼ6年前に追加. ほぼ5年前に更新.

ステータス:
終了
優先度:
通常
担当者:
対象バージョン:
プラグイン名:
twitter
クラッシュする:
いいえ

説明

表題の通り。

twitter_api_keysとdisplay_requirements以外のサードパーティプラグインが無い状態のconfで、master/developともに再現しました。


ファイル


関連するチケット

関連している 提案 #1241: Scoreの初期値をModelが提供できるようにするtoshi_aの判断待ち操作

toshi_a 初音 さんがほぼ6年前に更新

mikutterのCKの凍結解除をお願いするためにフォールバック用のコンシューマキーをすべて手放したため完全にTwitterができません。
誰か実際に再現するかやってみてほしいです。

Izumi Tsutsui さんがほぼ6年前に更新

いつもの NetBSD/i386 8.0_RC1 + pkgsrc mikutter 3.7.0 + 2ee74d6c#1229 の修正の環境で
以下のように再現しますね。
https://twitter.com/tsutsuii/status/995119161520013312


Izumi Tsutsui さんがほぼ6年前に更新

#1227c95d16b2 が原因なのかなあ、ということで
差分を見て野生の勘で以下のパッチを当てると直るっぽいです。
どこかの alias 修正漏れ?

diff --git a/core/plugin/twitter/model/message.rb b/core/plugin/twitter/model/message.rb
index 4f58e235..ed2061fb 100644
--- a/core/plugin/twitter/model/message.rb
+++ b/core/plugin/twitter/model/message.rb
@@ -660,6 +660,7 @@ class Plugin::Twitter::Message < Diva::Model
   # 本文を人間に読みやすい文字列に変換する
   def to_show
     @to_show ||= body.gsub(DESCRIPTION_UNESCAPE_REGEXP, &DESCRIPTION_UNESCAPE_RULE).freeze end
+    alias_method :description, :to_show

   # このMessageのパーマリンクを取得する
   # ==== Return

toshi_a 初音 さんがほぼ6年前に更新

#1233-3 だと、 #1227 の問題が再発しないでしょうか。
Plugin::Twitter::Message は、文字列置換を行うと文字列の長さが変わってしまうためEntityの位置がずれてしまいます。したがって c95d16b2 では、実体参照の解除をScoreで行うようにしています。

Izumi Tsutsui さんがほぼ6年前に更新

#1227 の本質をわかっていなかったりしますが、
認証のPINを入れたあとに「追加しますか」のウインドウが出なかった
ということがあって、それと同じ現象になるということでしょうか。

とりあえず twitter_api_keys を入れた状態で
Twitter world 削除→ mikutter 終了 → mikutter 再起動 → Twitterアカウントworld追加
を1回やったところでは正常に Twitter World 追加されましたが、
再現するしないの問題でもなさそうですね。

いずれにせよ #1227 では意図して
「description の挙動としてはエスケープ文字列を置き換えずに渡す」
ということであれば、 to_show を使うべきところで description が使われてしまっている
とかなんでしょうか。(深く考えずに書いてます)

cob odo さんがほぼ6年前に更新


下から順に、

> https://mstdn.kanagu.info

> mstdn.kanagu.info

> mstdn

と入力したものです。

要するに、Entityが存在し、Entityの処理のために(Entityより前の部分を)TextNoteにする場合には、unescapeが効いたTextNoteが生成されますが、Entityが無い場合にはそういったことは起こらず、unescape処理が行われないために、こうなってしまうのだと思います。
恐らくこの場合のTextNoteはscore_by_scoreのelse節で生成されているものなのではないでしょうか?

Izumi Tsutsui さんがほぼ6年前に更新

いろいろ試した結果のあまり役に立たないメモですが

source:core/plugin/twitter/twitter.rbtext_note に以下の notice を入れてみて、

diff --git a/core/plugin/twitter/twitter.rb b/core/plugin/twitter/twitter.rb
index 1cb0208d..938e19c5 100644
--- a/core/plugin/twitter/twitter.rb
+++ b/core/plugin/twitter/twitter.rb
@@ -492,6 +492,7 @@ Plugin.create(:twitter) do
   # Plugin::Twitter::Message#descriptionの結果が実体参照をエスケープすると
   # Entityのインデックスがずれるので、このメソッドで行う。
   def text_note(description:)
+    notice description.gsub(Plugin::Twitter::Message::DESCRIPTION_UNESCAPE_REGEXP, &Plugin::Twitter::Message::DESCRIPTION_UNESCAPE_RULE).freeze
     Diva::Model(:score_text).new(description: description.gsub(Plugin::Twitter::Message::DESCRIPTION_UNESCAPE_REGEXP, &Plugin::Twitter::Message::DESCRIPTION_UNESCAPE_RULE))
   end

#1233#note-3 のツイートを show_tweet プラグイン
https://github.com/osak/show_tweet
で受信してみると、一応以下のように text_note は複数回呼ばれているようです。

warning: /usr/pkg/lib/ruby/2.4.0/rubygems/deprecate.rb:62:in `block (2 levels) in deprecate': NOTE: Skin.get is deprecated; use get_path instead. It will be removed on or after 2018-01-01.
Skin.get called from /home/tsutsui/.mikutter-test/plugin/show_tweet/show_tweet.rb:17.
notice: {MIKUTTER_DIR}/core/plugin/twitter/twitter.rb:495:in `text_note': <>&
&lt;&gt;&amp;
という mikutter twitter プラグインのエスケープ問題再発問題
notice: {MIKUTTER_DIR}/core/plugin/twitter/twitter.rb:495:in `text_note': <>&
&lt;&gt;&amp;
という mikutter twitter プラグインのエスケープ問題再発問題
notice: {MIKUTTER_DIR}/core/plugin/twitter/twitter.rb:495:in `text_note': <>&
&lt;&gt;&amp;
という mikutter twitter プラグインのエスケープ問題再発問題
notice: {MIKUTTER_DIR}/core/plugin/twitter/twitter.rb:495:in `text_note': <>&
&lt;&gt;&amp;
という mikutter twitter プラグインのエスケープ問題再発問題
notice: {MIKUTTER_DIR}/core/plugin/twitter/twitter.rb:495:in `text_note': <>&
&lt;&gt;&amp;
という mikutter twitter プラグインのエスケープ問題再発問題
notice: {MIKUTTER_DIR}/core/plugin/twitter/twitter.rb:495:in `text_note': <>&
&lt;&gt;&amp;
という mikutter twitter プラグインのエスケープ問題再発問題
notice: {MIKUTTER_DIR}/core/plugin/twitter/twitter.rb:495:in `text_note': <>&
&lt;&gt;&amp;
という mikutter twitter プラグインのエスケープ問題再発問題
notice: {MIKUTTER_DIR}/core/plugin/twitter/twitter.rb:495:in `text_note': <>&
&lt;&gt;&amp;
という mikutter twitter プラグインのエスケープ問題再発問題
notice: {MIKUTTER_DIR}/core/plugin/twitter/twitter.rb:495:in `text_note': <>&
&lt;&gt;&amp;
という mikutter twitter プラグインのエスケープ問題再発問題
notice: {MIKUTTER_DIR}/core/plugin/twitter/twitter.rb:495:in `text_note': <>&
&lt;&gt;&amp;
という mikutter twitter プラグインのエスケープ問題再発問題
notice: {MIKUTTER_DIR}/core/plugin/twitter/twitter.rb:495:in `text_note': <>&
&lt;&gt;&amp;
という mikutter twitter プラグインのエスケープ問題再発問題
notice: {MIKUTTER_DIR}/core/plugin/twitter/twitter.rb:495:in `text_note': <>&
&lt;&gt;&amp;
という mikutter twitter プラグインのエスケープ問題再発問題
notice: {MIKUTTER_DIR}/core/plugin/twitter/twitter.rb:495:in `text_note': <>&
&lt;&gt;&amp;
という mikutter twitter プラグインのエスケープ問題再発問題
notice: {MIKUTTER_DIR}/core/plugin/twitter/twitter.rb:495:in `text_note': <>&
&lt;&gt;&amp;
という mikutter twitter プラグインのエスケープ問題再発問題
notice: {MIKUTTER_DIR}/core/plugin/twitter/twitter.rb:495:in `text_note': <>&
&lt;&gt;&amp;
という mikutter twitter プラグインのエスケープ問題再発問題
notice: {MIKUTTER_DIR}/core/plugin/twitter/twitter.rb:495:in `text_note': <>&
&lt;&gt;&amp;
という mikutter twitter プラグインのエスケープ問題再発問題
notice: {MIKUTTER_DIR}/core/plugin/twitter/twitter.rb:495:in `text_note': <>&
&lt;&gt;&amp;
という mikutter twitter プラグインのエスケープ問題再発問題
notice: {MIKUTTER_DIR}/core/plugin/twitter/twitter.rb:495:in `text_note': <>&
&lt;&gt;&amp;
という mikutter twitter プラグインのエスケープ問題再発問題
notice: {MIKUTTER_DIR}/core/plugin/streaming/perma_streamer.rb:33:in `block in mainloop': PermaStreamer exit

ただ、どれがTL描画に使われているのかというのは把握できていません。

cob odo さんがほぼ6年前に更新

  • 関連している 提案 #1241: Scoreの初期値をModelが提供できるようにする を追加

toshi_a 初音 さんがほぼ6年前に更新

  • ステータス分類待ち から 実装待ち に変更

なるほど、確かに単一のText Noteだと、Scoreによる置き換えが発生しませんね。
#1227 を実装する時、実体参照解除機能をもたせたModelを検討したのですが、大げさになる為止めたということがありました。しかしよく考えれば、TextNoteを使うということは一切文字列を置換しないということを表明していることになるのですから、内容を変えているならTextNoteを使うべきではないですね。

cob odo さんがほぼ6年前に更新

逆に、TextNoteを使わないということは他のプラグインによるScore置き換えを一切拒否するということになるので、それはそれで面白くないなーと思いました。

cob odo さんがほぼ6年前に更新

サードパーティプラグインでのScore置き換えを許容し、かつ #1241 で提案した方法を除いて、Unescape済みTextNoteを採用させるには、先頭にdescriptionが空のNoteを置けばいいはずだ、ということで、例えば添付したパッチのような実装が考えられます。

toshi_a 初音 さんがほぼ6年前に更新

#1233-10

これは思ってました。注記したときは勝手に再帰展開されるしこれでいけると思ってたんですが、再帰展開されるのは Diva::Model(:score_text) だけなので、確かに別の方法を考えないとですね。

#1233-11 のパッチはたしかに動きそうですが(Twitterの規約に従い、mikutter以外のCKを手放してしまってまたTwitterできなくなったため未確認です)、他にエレガントな方法があればそちらを採用したいですね。

他の解決策

実体参照を解除したdescriptionを持っているText Noteを仮に、UnescapedTextNoteと呼ぶことにします。

scoreルールがscoreを再帰的に問い合わせた結果を使う

私の当初の考えをブラッシュアップすると、Text Noteの代わりにUnescapedTextNoteを返すのではなく、UnescapedTextNoteを引数にscoreを得て、その内容を返すようにすれば全てのModelに適用できるScoreルールは適用されるようになります。
ただ、twitter_tweetではなくUnescapedTextNoteが対象となるため、twitter_tweetのみを対象にしたScoreルールが適用されません。

愚直な解決策としては、score_ofのオプション引数として、親となるModelとして別のものを渡せるようにすることです。しかしあまりにも解決方法が場当たり的すぎて、これも他に良い方法があれば避けたいところです。

TextNoteの要件を拡張

現在、score_ofがNoteを再帰展開するルールは、次のようなものです。

note.class.slug == :score_text

これが例えば次のようになっていれば、score_ofがUnescapedTextNoteを受け取った時に、Text Noteと同じように再帰展開が試みられます。

should_expand?(note)

もともと、Model slugを使うのはダックタイピングを阻害していて嫌だと思っていたので、副作用も少ないこの方法は私の中では今のところ有望です。

全てのNoteが再帰的に展開されるようにする

実はチケットにもしておらず、mikutter 3.8で実現しようと思っていた機能です。現在のScoreの仕組みでは、ハイパーリンクがNoteを持つことができないので、プレンーテキストにしかリンクを貼れないという問題があります。

具体的には、絵文字が含まれるURLは、twemojiプラグインで絵文字を展開できない(又は、twemojiプラグインのせいでURLが分断され、ハイパーリンクにならない)という問題があります。現状実装されているWorld系プラグインでこれが問題になることはまずないようですが。

この機能を実現するとしたら、全ての未知のNoteは必ず再帰展開を試みられることになるため、UnescapedTextNoteは特に工夫をしなくても #1233-10 の問題が発生しなくなります。

先延ばしにした理由は単純に解決できない問題があるからです。本筋から逸れるので程々にしますが、実装によってはScoreが単なるModelの配列ではなくなったり、そのせいでMiraclePainterがキャッシュしづらくなるという問題が起こると予想しています。

ncaq エヌユル さんがほぼ6年前に更新

twitter.rbの295行のfilter_score_filterを弄ってdeadbeefのtext noteを返すようにしてみたのですが全く何も反映されませんでした…
プリントデバッグしてみたら呼び出されてはいるようなのですが
何に使われてるんでしょうこの処理…
ここのscore見てみるとちゃんとunescapeされてるんですよね

cob odo さんがほぼ6年前に更新

  • クラッシュするいいえ から はい に変更

ncaq net さんは書きました:

twitter.rbの295行のfilter_score_filterを弄ってdeadbeefのtext noteを返すようにしてみたのですが全く何も反映されませんでした…
プリントデバッグしてみたら呼び出されてはいるようなのですが
何に使われてるんでしょうこの処理…
ここのscore見てみるとちゃんとunescapeされてるんですよね

TextNoteのみで構成されるscoreは https://dev.mikutter.hachune.net/projects/mikutter/repository/revisions/acbbb1e2b5dd77f490da3b41ae8e41a92a60f846/entry/core/plugin/twitter/twitter.rb#L298 の条件で落ちます。
そこを書き換えても、 https://reference.mikutter.hachune.net/basis/2018/04/24/score.html の「Score選定アルゴリズム」により却下される場合があります。

score_filterがつくるscoreはあくまで提案で、複数提案されるscoreのどれが選ばれるかによって最終的な結果が変わるので、実行時にコードパスとして通っているからといって、必ずしもそれが結果に影響するわけではないんですね。

cob odo さんがほぼ6年前に更新

  • クラッシュするはい から いいえ に変更

Izumi Tsutsui さんが5年以上前に更新

master に対して #1233-111233-dummynote.patch を適用してみたところ、回避策としては問題ないようです。

toshi_a 初音 さんは書きました:

#1233-11 のパッチはたしかに動きそうですが(Twitterの規約に従い、mikutter以外のCKを手放してしまってまたTwitterできなくなったため未確認です)、他にエレガントな方法があればそちらを採用したいですね。

#1233-12 の本質対策に時間がかかるならば上記を先に適用しておくという選択肢もありかと思いますが
弊害はあるでしょうか。

ぼちぼち指摘が出ているようです。
https://twitter.com/ray45422/status/1020669432446660608

mikutterでhtmlでエスケープしないと表示できない文字がエスケープされたまま表示されてる

cob odo さんが5年以上前に更新

弊害はあるでしょうか。

ない(つもり)です。

あるとすれば3.8向けのMiraclePainterリニューアル作業への影響がある、といった感じなんだと思います。

toshi_a 初音 さんが5年以上前に更新

#1233-12 の本質対策に時間がかかるならば上記を先に適用しておくという選択肢もありかと思いますが
弊害はあるでしょうか。

#1233-11 の次善策は一見うまく動いていますが、このハックが同様の問題を抱えるサードパーティプラグインで横行すると、場合によっては将来のmikutterの実装が(ハックとの互換性を考えて)複雑なものになってしまうことを危惧していました。なので TextNoteの要件を拡張 が簡単に出来なかった場合に #1233-11 の適用をしようと考えていました。

(実はこの検証のために、OSC展示やテストのためにAkkiesoftから貰っていたTwitter APIキーを有効にしたところ標準プラグインのバグと思われるものを見つけたと思ったらPortalの問題だった、といったyak shavingをやっているところで、そうこうしているうちにTwitter APIに関する新しい発表があったりして少々焦っています)

cob odo さんが5年以上前に更新

別の問題を追っていたら、図らずもコレに対する修正ができたので、パッチをここに貼ってみます。
このパッチでは、以下の2つの修正をします。

  • Plugin::Twitter::Message#descriptionがHTML実体参照を解除したものを返すようにする
  • Plugin[:twitter].score_by_entityPlugin::Twitter::Message#descriptionではなくPlugin::Twitter::Message#bodyを使うようにする

この結果、以下の効果が得られます。

  • 本文表示時にentityが無い場合にも実体参照が解除された状態になる(entityがある場合は #1227 で対処された方のコードを通るため、実体参照は解除されている)
  • 「本文をコピー」時に実体参照が解除された状態になる
  • extractプラグインによる:popup_notify通知の本文パートで実体参照が解除された状態になる

cob odo さんが5年以上前に更新

:popup_notifyが使用するnotify-sendはむしろHTML実体参照でエスケープしなきゃいけないんですね。

http://www.galago-project.org/specs/notification/0.9/x161.html

さっきのパッチを使用するなら、送る直前に改めてエスケープするコードを追加する必要がありますね……

cob odo さんが5年以上前に更新

ちょっといくつか見てみましたが、メッセージモデルのdescriptionがHTML実体参照エスケープされているものを返すサードパーティプラグインは見当たりませんでした。

  • Worldon: APIからHTMLで取得→descriptionはunescapeして返す
  • Slack: APIから専用の記法で記述されたテキストで取得→Entity機構でユーザ名などを加工して表示するが、descriptionはそのまま。
  • Haiku: APIからはてな記法で記述されたテキストで取得→そのまま返す
  • RSS https://github.com/yukkurisinai/mikutter_rss : リポジトリ消失?
  • IRC https://bitbucket.org/Phenomer/mikutter-irc : Divaより前の時代のプラグインですが、IRCなのでおそらくHTML実体参照は無い。

とすると、<>&などが含まれたメッセージの通知はTwitter以外ではそもそもマトモに動いてなかったんでしょうか? Linuxデスクトップを使っていないのでわかりませんが……
動いていないとしたら別チケットを立てるべきですね。

cob odo さんが5年以上前に更新

DirectMessageで落ちたのでパッチに対策を入れました。

cob odo さんがほぼ5年前に更新

ブランチをつくりました

cob odo さんがほぼ5年前に更新

  • ステータス実装待ち から レビュー待ち に変更
  • 担当者Izumi Tsutsui にセット
  • 対象バージョン3.7 から 3.8 に変更
  • プラグイン名twitter にセット
  • ブランチtopic/1233-html-entities-in-twitter-description にセット

レビューお願いします。

toshi_a 初音 さんがほぼ5年前に更新

修正方針はこれで良いと思います

Izumi Tsutsui さんがほぼ5年前に更新

  • ステータスレビュー待ち から マージ待ち に変更
  • 対象バージョン を削除 (3.8)

topic ブランチで検証しようとすると totori.dip.jp の gem でハマるという罠があるので
とりあえずで ffab1813 の差分を pkgsrc の mikutter にオレオレパッチで当ててテストしました。

show_tweet プラグインで
https://twitter.com/tsutsuii/status/995119161520013312
を表示させてみて、意図通り <>& が表示されました。
なので問題ないと思います。

Izumi Tsutsui さんがほぼ5年前に更新

  • 対象バージョン3.8 にセット

Izumi Tsutsui さんがほぼ5年前に更新

リロードしてもプルダウンメニューの選択が変わらない
(ので意図せず設定が戻ることがある)のは
Redmine の問題なのか Firefox の問題なのか……(´・ω・`)

cob odo さんがほぼ5年前に更新

  • ステータスマージ待ち から 終了 に変更

merge して push しました。

他の形式にエクスポート: Atom PDF