SC_Response::sendRedirect() transactionid= を画一的に追加せず、一定条件での継承のみとする

[seasoftjapan]

不要ならば削除したい。少なくとも全てのリダイレクトに適用する必要は無いと思うので、必要な箇所のみに適用したい。

背景・参考

• URLのバリエーションが多数発生する。
  • Google Search Console で重複コンテンツとして検出されるサイトを散見する。
  • ブラウザーの履歴が汚れる。
  • アクセスログが汚れる。
• 原点は 3a7af72 のよう。
• そもそも、リダイレクトで transactionid を付与するのは、無意味かむしろ些かアンセキュアな気もする。transactionid を受け取ったリクエストに関して、値を継承してリダイレクトするなどが妥当なような。

[nanasess]

導入当初はガラケーなど、cookie を使用できない環境への配慮もあったのですが、現代では不要ですね。

以下の脆弱性対応のため、GET でも CSRF トークンのチェックをしているページがあります。
https://www.ec-cube.net/info/weakness/weakness.php?id=64

[seasoftjapan]

以下の脆弱性対応のため、GET でも CSRF トークンのチェックをしているページがあります。 https://www.ec-cube.net/info/weakness/weakness.php?id=64

謎な適用もありますね。本来は必要な処理を POST に変更してチェックするのが良さそうですが、応急対応として本質的な要否に関わらず mode で割り切って判定している感じですかね。

いずれにしても、本メソッドが絡むリダイレクトに関しましては、transactionid= を追加する必要は無いと想定していますが、良さそうでしょうか?

[seasoftjapan]

transactionid を受け取ったリクエストに関して、値を継承してリダイレクトするなどが妥当なような。

とりあえず値を継承するパターンを実装してみました。
master...seasoftjapan:eccube-2_13:seasoft-922

需要があるか不明なので、さくっと削除してしまう方が良いのか、意見をいただけますと幸いです。

[nanasess]

@seasoftjapan
本質的には SC_Response::sendRedirect() で transactionid を発行する必要は無さそうです。
一部、GET で CSRF チェックをしている箇所のみ利用できれば問題ないと思います

[seasoftjapan]

@nanasess
master...seasoftjapan:eccube-2_13:seasoft-922 にて、transactionid の新たな付与は行わず、transactionid を含む URL を別の URL へリダイレクトするときのみ transactionid を継承する (リダイレクト前に transactionid が無ければ transactionid は付与しない) 仕様としました。

/aaa → /bbb?x=1
/aaa?transactionid=foo → /bbb?x=1&transactionid=foo

その対応も不要で、リダイレクト前にあった transactionid も切り捨てて良い感じでしょうか?

/aaa?transactionid=foo → /bbb?x=1

[nanasess]

@seasoftjapan
クエリストリングに mode が含まれる場合は、GET でも CSRF のチェックをしています。

ec-cube2/data/class/pages/LC_Page.php

Lines 649 to 655 in a7236b6

	$mode = $this->getMode();
	if ($_SERVER['REQUEST_METHOD'] == 'POST' || !SC_Utils::isBlank($mode)) {
	if (!SC_Helper_Session_Ex::isValidToken(false)) {
	SC_Utils_Ex::sfDispError(INVALID_MOVE_ERRORR);
	SC_Response_Ex::actionExit();
	}
	}

リダイレクトの際に mode が付与される場合は transactionid が必要ですが、それ以外は切り捨ててしまって問題ないと思っています

[seasoftjapan]

そもそもリダイレクトを通らないケースが大半だと思いますし、アクセスログの観点ではリダイレクト前も記録されますから、「管理画面」「mode なし」「transactionid あり」というリクエストを発生させる箇所があったら、そっちを改められないかの観点も確認した方が良さそうですね。

ちょっと嫌な予感がするのが、リダイレクト前後で mode なし → あり に変わるパターンがあると、厄介ですね…。
無いよなフツー・・・と、無いことを祈りつつ、確認しようと思います。

もう一つ気づきました。先日の master...seasoftjapan:eccube-2_13:seasoft-922 は、PRG パターンを考慮できていませんでした。
管理画面はリダイレクト先に mode がある場合は transactionid を維持 (新たな付与はしない)。フロントはサクッと削除。これで再検討しようと思います。

[nanasess]

@seasoftjapan

そもそもリダイレクトを通らないケースが大半だと思いますし、アクセスログの観点ではリダイレクト前も記録されますから、「管理画面」「mode なし」「transactionid あり」というリクエストを発生させる箇所があったら、そっちを改められないかの観点も確認した方が良さそうですね。

おっしゃるとおりですね

ちょっと嫌な予感がするのが、リダイレクト前後で mode なし → あり に変わるパターンがあると、厄介ですね…。
無いよなフツー・・・と、無いことを祈りつつ、確認しようと思います。

sendRedirect で grep して調べた感じは無さそうでした。

管理画面はリダイレクト先に mode がある場合は transactionid を維持 (新たな付与はしない)。フロントはサクッと削除。これで再検討しようと思います。

よろしくお願いいたします🙇‍♂️

[seasoftjapan]

ちょっと嫌な予感がするのが、リダイレクト前後で mode なし → あり に変わるパターンがあると、厄介ですね…。
無いよなフツー・・・と、無いことを祈りつつ、確認しようと思います。

sendRedirect で grep して調べた感じは無さそうでした。

情報ありがとうございます。
SC_Response::sendRedirectFromUrlPath() SC_Response::reload() SC_Display::reload() についても確認しましたが大丈夫そうでした。

[seasoftjapan]

備忘録を兼ねてまとめます。

#922 (comment) から抜粋:

@seasoftjapan クエリストリングに mode が含まれる場合は、GET でも CSRF のチェックをしています。
リダイレクトの際に mode が付与される場合は transactionid が必要ですが、それ以外は切り捨ててしまって問題ないと思っています

PRG パターン

「メルマガ管理＞テンプレート設定」画面 (/admin/mail/template_input.php) で登録する際、以下の流れが発生する。

POST /admin/mail/template_input.php? (302 Found)
GET /admin/mail/template_input.php?mode=complete

フロント同様に transactionid を破棄してしまうと、完了画面を表示できない。

完了画面を PRG で表示するために transactionid を維持するのはバカバカしいけど、応急対応だから仕方がない。考慮して実装する。

もう一つ気づきました。先日の master...seasoftjapan:eccube-2_13:seasoft-922 は、PRG パターンを考慮できていませんでした。

$_REQUEST で実装していたので、結果 PRG でも動く。(というか、上のコメント時、PRP パターンが念頭にあったと思うが、本件メソッドが扱うのは (307 ではなく) 302 リダイレクトなので、考慮不要だった。)

よって、これをベースに、フロント画面や、管理画面で mode が無い場合、transactionid を維持しないよう分岐すれば良さそう (不要に transactionid が含まれるのを避けられる)。

GRG パターン (って用語があるか知らんけど)

以下のような mode ありの GET 同士のリダイレクトでも考慮が必要なはず。

GET /admin/hogehoge.php?mode=foo (302 Found)
GET /admin/hogehoge.php?mode=bar

しかし、こういった使い方が実在するか不明。

master...seasoftjapan:eccube-2_13:seasoft-922 で動作するので、新たに難が見つからなければ維持する予定。