Fix Security (Medium): CSRFトークン・メールヘッダ・ファイルDL・XSSエスケープ#1346
Fix Security (Medium): CSRFトークン・メールヘッダ・ファイルDL・XSSエスケープ#1346
Conversation
- M-1: Use random_bytes() for CSRF token generation instead of rand()+uniqid() which is not cryptographically secure - M-6: Strip newlines from email subject before MIME encoding to prevent header injection - M-8: Add realpath validation to sfDownloadFile() to prevent path traversal and quote filename in Content-disposition header - M-9: Add |h escape filter to error message outputs in forgot/index.tpl, forgot/secret.tpl, admin/system/masterdata.tpl, admin/order/edit.tpl Refs #1340 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthrough複数テンプレートでエラーメッセージをHTMLエスケープ、メール件名の改行を正規化、ダウンロード時にrealpathでパストラ防止、セッショントークン生成を暗号学的に強化しました。 Changes
Sequence Diagram(s)（該当する大きな新機能や新しい複数コンポーネント間の制御フロー変更は含まれていないため、シーケンス図は生成していません。） Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1346 +/- ##
==========================================
- Coverage 54.72% 54.37% -0.35%
==========================================
Files 84 84
Lines 10815 10821 +6
==========================================
- Hits 5918 5884 -34
- Misses 4897 4937 +40
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
createTokenがsha1(40文字)からbin2hex(random_bytes(32))(64文字)に 変更されたため、テストの文字列長の期待値を更新 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@data/class/helper/SC_Helper_FileManager.php`:
- Around line 280-295: The directory-boundary check in
SC_Helper_FileManager::(download logic) is vulnerable: ensure
realpath(USER_REALDIR) is valid (not false) before using str_starts_with to
avoid TypeError, and enforce a trailing directory separator on the resolved user
dir so prefix matches cannot be spoofed (e.g. '/user_data' vs
'/user_data_backup'). Change the code to (1) call realpath(USER_REALDIR) and if
it returns false, log and abort; (2) normalize $userRealDir with
rtrim($userRealDir, DIRECTORY_SEPARATOR).DIRECTORY_SEPARATOR to guarantee a
final slash; (3) require $realpath !== false and check that
str_starts_with($realpath, $userRealDirWithSep) (or strpos($realpath,
$userRealDirWithSep) === 0) before allowing download; keep existing logging and
use $this->sfReadFile($realpath) for the response.
🧹 Nitpick comments (2)
data/Smarty/templates/admin/order/edit.tpl (1)
282-282: XSSエスケープの追加は適切です。
$errBirthに|hフィルタを追加する変更は正しく、エラーメッセージに含まれる可能性のあるHTML/JSインジェクションを防止します。ただし、このテンプレート内には他にも
|hなしで出力されている$arrErrの箇所が多数あります（例：Line 150, 192, 202, 211, 219, 229-231, 243-245, 304, 325, 332, 338, 341, 417, 422, 434, 448, 457, 466, 474, 481, 491, 578, 615, 625, 634, 644-646, 658-660, 674, 683, 693, 700, 706, 709, 717, 730-732, 755, 766, 789）。これらも同様にエスケープが必要です。As per coding guidelines, 「Check for XSS vulnerabilities: Ensure Smarty templates apply the|hfilter to escape HTML output」。data/class/helper/SC_Helper_FileManager.php (1)
291-292: Content-Disposition ヘッダのファイル名サニタイズを検討してください。
basename($realpath)で取得したファイル名をダブルクォートで囲む対応は良い改善ですが、ファイル名自体にダブルクォート"が含まれている場合、ヘッダの構造が崩れる可能性があります。♻️ ファイル名のサニタイズ案
$file_name = basename($realpath); - header('Content-disposition: attachment; filename="'.$file_name.'"'); - header('Content-type: application/octet-stream; name="'.$file_name.'"'); + $safe_file_name = str_replace('"', '_', $file_name); + header('Content-disposition: attachment; filename="'.$safe_file_name.'"'); + header('Content-type: application/octet-stream; name="'.$safe_file_name.'"');
- sfDownloadFileのディレクトリ境界チェック強化(末尾セパレータ付加) - Content-Dispositionファイル名のダブルクォートをサニタイズ - edit.tplの$arrErr出力に|hフィルタを追加(XSSエスケープ) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
data/Smarty/templates/admin/order/edit.tpl (1)
474-484:⚠️ Potential issue | 🟠 Major
$arrErr.totalと$arrErr.payment_totalに|hエスケープが漏れています。他の
$arrErr出力にはすべて|hフィルタが追加されていますが、合計とお支払い合計のエラー表示だけ未エスケープのままです。同じ XSS リスクがあるため、こちらも修正が必要です。🔒 修正案
- <span class="attention"><!--{$arrErr.total}--></span> + <span class="attention"><!--{$arrErr.total|h}--></span> <!--{$arrForm.total.value|default:0|n2s}--> 円 </td> </tr> <tr> <th colspan="5" class="column right">お支払い合計</th> <td class="right"> - <span class="attention"><!--{$arrErr.payment_total}--></span> + <span class="attention"><!--{$arrErr.payment_total|h}--></span>As per coding guidelines,
**/*.tpl: "Check for XSS vulnerabilities: Ensure Smarty templates apply the|hfilter to escape HTML output".
nanasess
left a comment
nanasess
left a comment
There was a problem hiding this comment.
レビュー結果
全体として適切なセキュリティ修正ですが、1点デグレードの懸念があります。
M-1: CSRFトークン生成の強化 ✅
sha1(uniqid(rand(), true)) → bin2hex(random_bytes(32)) の変更は適切です。
random_bytes()はCSPRNGを使用し、予測可能性の問題を解消- トークン長が40文字→64文字に変更されるが、セッションに保存・hidden inputで送受信のみでDBには保存されないため下位互換の問題なし
- テストの期待値も更新済み
M-6: メールヘッダインジェクション防止 ✅
処理順序の変更（エンコード前に改行除去）は正しいアプローチです。
- 修正前は
mb_encode_mimeheaderがRFC準拠で生成した折り返し改行まで破壊する副作用があった - 修正後は入力から改行を除去した上でエンコードするので、ヘッダインジェクションを防ぎつつMIMEエンコードの折り返しも保持される
M-8: ファイルダウンロードパス検証 ✅
realpath() + str_starts_with() + DIRECTORY_SEPARATOR 付加による多層防御は堅牢な実装です。
- シンボリックリンク・
..をrealpath()で解決してからチェック DIRECTORY_SEPARATOR付加で/user_data_backupのようなプレフィックス偽装を防止- サブディレクトリインストール（例:
https://example.com/shop/）でも、USER_REALDIRはHTML_REALDIR . USER_DIRで定義されるためrealpath()により正しく絶対パスが解決され、問題なく動作します
M-9: XSSエスケープ ⚠️
admin/order/edit.tpl, admin/system/masterdata.tpl, default/forgot/secret.tpl の |h 追加は適切です。
ただし default/forgot/index.tpl にデグレードの懸念があります。詳細は該当ファイルへのコメントを参照してください。
| <p class="attention"> | ||
| <!--{$arrErr.name01}--><!--{$arrErr.name02}--> | ||
| <!--{$errmsg}--> | ||
| <!--{$errmsg|h}--> |
There was a problem hiding this comment.
デグレードの懸念
LC_Page_Forgot.php:157 で $errmsg に <br/> を含むHTMLが設定されています:
$errmsg = 'ご入力のemailアドレスは現在仮登録中です。<br/>登録の際にお送りしたメールのURLにアクセスし、<br/>本会員登録をお願いします。';テンプレートで <!--{$errmsg|h}--> とすると、<br/> が <br/> にエスケープされ、改行が表示されずリテラル文字列として画面に表示されるデグレードが発生します。
対処案:
PHP側の <br/> を に変更し、テンプレート側で |h|nl2br とする:
// LC_Page_Forgot.php:157
$errmsg = "ご入力のemailアドレスは現在仮登録中です。
登録の際にお送りしたメールのURLにアクセスし、
本会員登録をお願いします。";<!--{$errmsg|h|nl2br}-->これにより、XSSエスケープを維持しつつ改行の表示も保たれると思われますが、下位互換を損うため BC Break となりそうです
- forgot/index.tpl: $errmsgの<br/>がエスケープされるデグレードを修正 PHP側で<br/>を\nに変更し、テンプレートで|h|nl2brとする - edit.tpl: $arrErr.totalと$arrErr.payment_totalに|hフィルタを追加 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
data/Smarty/templates/admin/order/edit.tpl (1)
776-779: 既存の未エスケープ出力（本 PR 外）— 後続対応を推奨このブロック（変更なし）の以下 3 箇所に
|hフィルターが適用されていません。今回の PR で対応された他のフィールドと同様のリスクがあります。- <th><!--{$arrForm.payment_type}-->情報</th> + <th><!--{$arrForm.payment_type|h}-->情報</th>-<!--{if $item.name != ""}--><!--{$item.name}-->：<!--{/if}--><!--{$item.value}--><br/> +<!--{if $item.name != ""}--><!--{$item.name|h}-->：<!--{/if}--><!--{$item.value|h}--><br/>管理者画面のみのため即時影響は限定的ですが、今回の XSS 対策の一環として合わせて修正することを推奨します。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data/Smarty/templates/admin/order/edit.tpl` around lines 776 - 779, This template outputs unescaped values in the payment info block; update the Smarty variables to use the HTML-escape filter. In the loop over arrForm.payment_info (the foreach with key=item), change occurrences of $item.name and $item.value to use the |h filter (e.g. {$item.name|h}, {$item.value|h}), and also apply |h to the header variable arrForm.payment_type ({$arrForm.payment_type|h}) so all three unescaped outputs in this block are escaped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@data/Smarty/templates/admin/order/edit.tpl`:
- Around line 776-779: This template outputs unescaped values in the payment
info block; update the Smarty variables to use the HTML-escape filter. In the
loop over arrForm.payment_info (the foreach with key=item), change occurrences
of $item.name and $item.value to use the |h filter (e.g. {$item.name|h},
{$item.value|h}), and also apply |h to the header variable arrForm.payment_type
({$arrForm.payment_type|h}) so all three unescaped outputs in this block are
escaped.
$arrForm.payment_type, $item.name, $item.value に|hを追加 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Summary
Medium severity のセキュリティ修正4件をまとめて対応。
M-1: CSRFトークン生成の強化
SC_Helper_Session::createToken()でsha1(uniqid(rand(), true))→bin2hex(random_bytes(32))に変更M-6: メールヘッダインジェクション防止
SC_SendMail::setSubject()で改行文字の除去をMIMEエンコード前に実施するよう順序を修正M-8: ファイルダウンロードのパス検証
SC_Helper_FileManager::sfDownloadFile()にrealpath()+str_starts_with()による USER_REALDIR 境界チェックを追加M-9: エラーメッセージのHTMLエスケープ
forgot/index.tpl,forgot/secret.tplの$errmsgに|hを追加admin/system/masterdata.tplの$errorMessageに|hを追加admin/order/edit.tplの$errBirthに|hを追加Test plan
Refs #1340
🤖 Generated with Claude Code
Summary by CodeRabbit