-
Notifications
You must be signed in to change notification settings - Fork 99
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
SC_Response::sendRedirect() transactionid= を画一的に追加せず、一定条件での継承のみとする #922
Comments
導入当初はガラケーなど、cookie を使用できない環境への配慮もあったのですが、現代では不要ですね。 以下の脆弱性対応のため、GET でも CSRF トークンのチェックをしているページがあります。 |
謎な適用もありますね。本来は必要な処理を POST に変更してチェックするのが良さそうですが、応急対応として本質的な要否に関わらず mode で割り切って判定している感じですかね。 いずれにしても、本メソッドが絡むリダイレクトに関しましては、transactionid= を追加する必要は無いと想定していますが、良さそうでしょうか? |
transactionid が指定されている場合、そちらを優先する。
とりあえず値を継承するパターンを実装してみました。 需要があるか不明なので、さくっと削除してしまう方が良いのか、意見をいただけますと幸いです。 |
@seasoftjapan |
@nanasess
その対応も不要で、リダイレクト前にあった transactionid も切り捨てて良い感じでしょうか?
|
@seasoftjapan ec-cube2/data/class/pages/LC_Page.php Lines 649 to 655 in a7236b6
リダイレクトの際に mode が付与される場合は transactionid が必要ですが、それ以外は切り捨ててしまって問題ないと思っています |
そもそもリダイレクトを通らないケースが大半だと思いますし、アクセスログの観点ではリダイレクト前も記録されますから、「管理画面」「mode なし」「transactionid あり」というリクエストを発生させる箇所があったら、そっちを改められないかの観点も確認した方が良さそうですね。 ちょっと嫌な予感がするのが、リダイレクト前後で mode なし → あり に変わるパターンがあると、厄介ですね…。 もう一つ気づきました。先日の master...seasoftjapan:eccube-2_13:seasoft-922 は、PRG パターンを考慮できていませんでした。 |
おっしゃるとおりですね
よろしくお願いいたします🙇♂️ |
情報ありがとうございます。 |
備忘録を兼ねてまとめます。 #922 (comment) から抜粋:
PRG パターン「メルマガ管理>テンプレート設定」画面 (/admin/mail/template_input.php) で登録する際、以下の流れが発生する。
フロント同様に transactionid を破棄してしまうと、完了画面を表示できない。 完了画面を PRG で表示するために transactionid を維持するのはバカバカしいけど、応急対応だから仕方がない。考慮して実装する。
$_REQUEST で実装していたので、結果 PRG でも動く。(というか、上のコメント時、PRP パターンが念頭にあったと思うが、本件メソッドが扱うのは (307 ではなく) 302 リダイレクトなので、考慮不要だった。) よって、これをベースに、フロント画面や、管理画面で mode が無い場合、transactionid を維持しないよう分岐すれば良さそう (不要に transactionid が含まれるのを避けられる)。 GRG パターン (って用語があるか知らんけど)以下のような mode ありの GET 同士のリダイレクトでも考慮が必要なはず。
しかし、こういった使い方が実在するか不明。 master...seasoftjapan:eccube-2_13:seasoft-922 で動作するので、新たに難が見つからなければ維持する予定。 |
SC_Response::sendRedirect() transactionid= を画一的に追加せず、一定条件での継承のみとする #922
不要ならば削除したい。少なくとも全てのリダイレクトに適用する必要は無いと思うので、必要な箇所のみに適用したい。
背景・参考
The text was updated successfully, but these errors were encountered: