Conversation
Google/Yahoo のメール送信ガイドライン変更に対応するため、 RFC 8058 準拠のワンクリックメルマガ登録解除機能を実装しました。 主な変更点： - dtb_mailmaga_unsubscribe_token テーブルを追加（トークン管理） - SC_Helper_Mailmaga クラスを新規作成（トークン生成・検証） - SC_SendMail にカスタムヘッダー追加機能を実装 - メルマガ配信時に List-Unsubscribe と List-Unsubscribe-Post ヘッダーを追加 - ワンクリック登録解除ページを作成（/mailmaga/unsubscribe/） - PHPUnit テストを追加（21 tests, 69 assertions） セキュリティ： - 64文字のランダムトークン（256ビット相当） - トークン有効期限90日 - ヘッダーインジェクション対策 - 使用済みトークンの再利用防止 関連Issue: #819 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1316 +/- ##
==========================================
+ Coverage 55.08% 55.41% +0.33%
==========================================
Files 86 87 +1
Lines 11016 11184 +168
==========================================
+ Hits 6068 6198 +130
- Misses 4948 4986 +38
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:
|
- null合体演算子(??)を使用（isset() ? : から変更） - class定数をpublic constに変更（PSR準拠） - テストメソッド名をキャメルケースに変更（PSR-1準拠） Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This pull request implements RFC 8058-compliant one-click unsubscribe functionality for newsletter emails to meet Google and Yahoo's updated email sender guidelines. The implementation adds database infrastructure for token management, a new helper class for unsubscribe operations, custom email header support, an unsubscribe landing page, and comprehensive test coverage.
Changes:
- Introduces
dtb_mailmaga_unsubscribe_tokentable with PostgreSQL and MySQL schemas for secure token storage with 90-day expiration - Adds
SC_Helper_Mailmagahelper class implementing token generation, validation, and unsubscribe operations with cleanup functionality - Extends
SC_SendMailclass with custom header methods (addCustomHeader,clearCustomHeaders) including security protections against header injection and protected header overwrites - Integrates RFC 8058 headers (
List-Unsubscribe,List-Unsubscribe-Post) into newsletter sending workflow viaSC_Helper_Mail - Creates new unsubscribe page at
/mailmaga/unsubscribe/supporting both GET confirmation displays and POST one-click unsubscribe requests - Provides comprehensive PHPUnit test coverage (21 tests, 69 assertions) for both helper and mail sending functionality
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
html/install/sql/create_table_pgsql.sql |
Adds PostgreSQL schema for unsubscribe token table with indexes |
html/install/sql/create_table_mysqli.sql |
Adds MySQL schema for unsubscribe token table with indexes |
eccube_install.sh |
Registers sequence for token_id generation in PostgreSQL |
data/class/helper/SC_Helper_Mailmaga.php |
New helper class for token management and unsubscribe operations |
data/class/SC_SendMail.php |
Extends email class with custom header support and security validations |
data/class/helper/SC_Helper_Mail.php |
Integrates RFC 8058 headers into newsletter sending workflow |
data/class/pages/mailmaga/LC_Page_Mailmaga_Unsubscribe.php |
New page class handling unsubscribe requests per RFC 8058 |
html/mailmaga/unsubscribe/index.php |
Entry point for unsubscribe page |
data/Smarty/templates/default/mailmaga/unsubscribe.tpl |
User interface template for unsubscribe confirmation and completion |
tests/class/helper/SC_Helper_Mailmaga/SC_Helper_MailmagaTest.php |
Comprehensive unit tests for helper class (9 tests, 25 assertions) |
tests/class/SC_SendMailTest.php |
Extended tests for custom header functionality (12 tests, 44 assertions) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| // トークンを使用済みにマーク | ||
| SC_Helper_Mailmaga_Ex::markTokenAsUsed($token); |
There was a problem hiding this comment.
The unsubscribe process uses a transaction but doesn't handle the case where markTokenAsUsed returns false. If marking the token fails but the unsubscribe succeeds, the token could be reused. Consider checking the return value of markTokenAsUsed and throwing an exception if it fails to ensure atomicity.
| SC_Helper_Mailmaga_Ex::markTokenAsUsed($token); | |
| $tokenMarked = SC_Helper_Mailmaga_Ex::markTokenAsUsed($token); | |
| if (!$tokenMarked) { | |
| throw new Exception('トークンの無効化に失敗しました。'); | |
| } |
|
|
||
| $sqlval = []; | ||
| $sqlval['used_flag'] = 1; | ||
| $sqlval['used_date'] = 'CURRENT_TIMESTAMP'; |
There was a problem hiding this comment.
The 'used_date' field is set to the string 'CURRENT_TIMESTAMP' which may not work correctly in all database scenarios. This could result in the literal string being inserted instead of the actual timestamp value. Consider using a consistent approach with other timestamp fields in the codebase.
| $sqlval['used_date'] = 'CURRENT_TIMESTAMP'; | |
| $sqlval['used_date'] = date('Y-m-d H:i:s'); |
|
|
||
| $sqlval = []; | ||
| $sqlval['mailmaga_flg'] = 3; // 配信拒否 | ||
| $sqlval['update_date'] = 'CURRENT_TIMESTAMP'; |
There was a problem hiding this comment.
The 'update_date' field is set to the string 'CURRENT_TIMESTAMP' which may not be properly handled by the database layer. This could lead to inconsistent behavior across different database engines or configurations. Consider using a more reliable approach for setting timestamp values.
| $sqlval['update_date'] = 'CURRENT_TIMESTAMP'; | |
| $sqlval['update_date'] = date('Y-m-d H:i:s'); |
| <!--{else}--> | ||
| <p class="message">メールアドレス: <strong><!--{$tpl_email|h}--></strong></p> | ||
| <p>メールマガジンの登録を解除しますか?</p> | ||
| <form method="post" action="<!--{$smarty.server.REQUEST_URI|h}-->"> |
There was a problem hiding this comment.
Using REQUEST_URI in the form action poses a CSRF vulnerability risk and potential open redirect vulnerabilities. The form should POST to a known, safe endpoint rather than echoing back the current URI. Consider using a fixed action URL or implementing CSRF protection tokens if REQUEST_URI must be used.
| <form method="post" action="<!--{$smarty.server.REQUEST_URI|h}-->"> | |
| <form method="post"> |
| token varchar(64) NOT NULL, | ||
| email varchar(255) NOT NULL, | ||
| used_flag smallint NOT NULL DEFAULT 0, | ||
| used_date timestamp DEFAULT NULL, |
There was a problem hiding this comment.
The PostgreSQL schema uses 'timestamp DEFAULT NULL' for the used_date column, but PostgreSQL does not allow DEFAULT NULL on a timestamp column in this syntax. This should be 'timestamp' without the DEFAULT clause, or use 'timestamp DEFAULT NULL' only if the column is explicitly nullable. This could cause the table creation to fail on PostgreSQL.
| used_date timestamp DEFAULT NULL, | |
| used_date timestamp, |
| $sqlval['email'] = $email; | ||
| $sqlval['used_flag'] = 0; | ||
| $sqlval['expire_date'] = date('Y-m-d H:i:s', strtotime('+'.self::TOKEN_EXPIRE_DAYS.' days')); | ||
| $sqlval['create_date'] = 'CURRENT_TIMESTAMP'; |
There was a problem hiding this comment.
The 'create_date' field is set to the string 'CURRENT_TIMESTAMP' rather than being handled by the database. In some database scenarios, this literal string might be inserted instead of the current timestamp. Consider using the database's built-in handling or PHP's date function to ensure consistency across different database configurations.
| $sqlval['create_date'] = 'CURRENT_TIMESTAMP'; | |
| $sqlval['create_date'] = date('Y-m-d H:i:s'); |
| @@ -60,6 +62,7 @@ public function __construct() | |||
| $this->bcc = ''; | |||
| $this->replay_to = ''; | |||
There was a problem hiding this comment.
There's a typo in the property name. Line 63 sets 'replay_to' but the property declaration on line 46 is 'reply_to'. This inconsistency will cause 'replay_to' to be set as a new dynamic property instead of the intended 'reply_to' property, potentially breaking reply-to functionality.
|
|
||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
The token parameter is not validated for format or length before being passed to validateToken. While validateToken will fail gracefully for invalid tokens, adding basic input validation (e.g., checking that the token is exactly 64 hexadecimal characters) would prevent unnecessary database queries for obviously malformed tokens and provide better error messages.
| // トークン形式の簡易チェック（64 桁の 16 進数） | |
| if (!preg_match('/^[0-9a-fA-F]{64}$/', $token)) { | |
| $this->tpl_message = '無効なURLです。'; | |
| return; | |
| } |
|
|
||
| // テキストメール配信の場合 | ||
| if ($arrMail['mail_method'] == 2) { | ||
| $sendResut = $objMail->sendMail(); |
There was a problem hiding this comment.
The variable name '$sendResut' is misspelled - it should be '$sendResult'. This typo appears multiple times in the mail sending loop and could cause confusion during maintenance. While this doesn't break functionality (as the variable is used consistently), it reduces code quality.
| // トークンの取得 | ||
| $token = $_REQUEST['token'] ?? ''; |
There was a problem hiding this comment.
Using $_REQUEST to get the token combines GET, POST, and COOKIE data, which could potentially be exploited through cookie poisoning attacks. According to RFC 8058, the token should come from the URL query parameter for GET requests and POST body for POST requests. Use $_GET['token'] specifically to prevent potential security issues.
| // トークンの取得 | |
| $token = $_REQUEST['token'] ?? ''; | |
| // トークンの取得 (URL クエリパラメータから取得) | |
| $token = $_GET['token'] ?? ''; |
|
下位互換性を維持してそうですが、慎重に確認を進めます |
|
@nanasess https://www.coderabbit.ai/ja とか導入するとどうなんでしょうね。もっとちゃんとやらないと役に立たないかな |
|
@nobuhiko https://www.coderabbit.ai/ja は開発者数に対する課金なので難しそうですね。。。 |
|
@nanasess だけじゃ足りないですかね〜？ |
|
@nobuhiko OSS は無料なんですね。ちょっと交渉してみます！ |
|
@coderabbitai レビューお願いします |
|
変更内容を確認させていただきます：
詳細なレビューを実施いたします。 ✅ Actions performedReview triggered.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughウォークスルーRFC 8058準拠の一クリックメルマガ登録解除機能を実装します。トークン生成・検証、ページ処理、カスタムメールヘッダー注入、データベーステーブル作成、トークンクリーンアップバッチ、および関連テストを含みます。 変更内容
シーケンス図sequenceDiagram
actor User
participant Browser
participant Page as LC_Page_Mailmaga_Unsubscribe
participant Helper as SC_Helper_Mailmaga
participant DB as Database
User->>Browser: RFC 8058 List-Unsubscribe-Post<br/>POST /mailmaga/unsubscribe/index.php?token=xxx
Browser->>Page: POST request with token
Page->>Page: Validate token format (64 hex)
Page->>Helper: validateToken(token)
Helper->>DB: Query dtb_mailmaga_unsubscribe_token
DB-->>Helper: Token row data
Helper-->>Page: Token validation result
alt Valid & Unused & Not Expired
Page->>Page: processUnsubscribe()
Page->>Helper: unsubscribeMailmaga(customer_id)
Helper->>DB: UPDATE dtb_customer<br/>mailmaga_flg = 3
Page->>Helper: markTokenAsUsed(token)
Helper->>DB: UPDATE dtb_mailmaga_unsubscribe_token<br/>used_flg = 1, used_date = now()
DB-->>Helper: Update result
Page-->>Browser: 200 OK (empty body)
else Invalid/Expired/Used Token
Page-->>Browser: Error message
end
推定コードレビュー工数🎯 4 (Complex) | ⏱️ ~60 minutes レビュアー候補
詩
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@data/class/helper/SC_Helper_Mailmaga.php`:
- Around line 162-170: Implement a scheduled batch that calls
SC_Helper_Mailmaga::cleanupExpiredTokens() regularly: add a small CLI/cron entry
(e.g., a new script or console command) that boots the app context, invokes
SC_Helper_Mailmaga::cleanupExpiredTokens(), logs results and exits with proper
status, and register it in the system crontab or the app's scheduler so it runs
periodically; ensure the batch is idempotent and uses a simple lock (file/DB) to
avoid concurrent executions and include contextual logging referencing the
dtb_mailmaga_unsubscribe_token cleanup for observability.
In `@tests/class/helper/SC_Helper_Mailmaga/SC_Helper_MailmagaTest.php`:
- Around line 32-40: The test testGetUnsubscribeUrl is asserting the URL always
starts with 'https://' which fails when the test env's HTTPS_URL uses 'http://';
update the assertion to be environment-agnostic by checking the URL starts with
the configured base (use the same scheme as the HTTPS_URL constant) or assert it
starts with either 'http://' or 'https://', and keep the existing assertions for
'mailmaga/unsubscribe/index.php' and 'token=test-token-123'; modify the
testGetUnsubscribeUrl in SC_Helper_MailmagaTest to fetch the scheme/base from
HTTPS_URL and assert the generated URL begins with that value or one of the two
allowed schemes while still verifying path and token.
🧹 Nitpick comments (4)
data/class/SC_SendMail.php (1)
165-171: 保護ヘッダーのチェックは大文字小文字を区別している
in_array()による保護ヘッダーのチェックは大文字小文字を区別しますが、HTTPヘッダーは本来大文字小文字を区別しません。例えば、fromやFROMで保護を回避できる可能性があります。♻️ 大文字小文字を区別しない比較への修正案
// 重要なヘッダーの上書きを防止 $protectedHeaders = ['From', 'To', 'Subject', 'Cc', 'Bcc', 'Reply-To', 'Return-Path', 'Date', 'MIME-Version', 'Content-Type', 'Content-Transfer-Encoding']; - if (in_array($name, $protectedHeaders)) { + $protectedHeadersLower = array_map('strtolower', $protectedHeaders); + if (in_array(strtolower($name), $protectedHeadersLower, true)) { trigger_error('保護されたヘッダーは上書きできません: '.$name, E_USER_WARNING); return; }html/install/sql/create_table_pgsql.sql (1)
274-276: インデックス名の衝突リスクPostgreSQL ではインデックス名がスキーマスコープです。
idx_customer_id、idx_send_id、idx_expire_dateという汎用的な名前は、他のテーブルに同名のインデックスが追加された場合に衝突する可能性があります。♻️ テーブル名をプレフィックスとして追加する案
-CREATE INDEX idx_customer_id ON dtb_mailmaga_unsubscribe_token(customer_id); -CREATE INDEX idx_send_id ON dtb_mailmaga_unsubscribe_token(send_id); -CREATE INDEX idx_expire_date ON dtb_mailmaga_unsubscribe_token(expire_date); +CREATE INDEX dtb_mailmaga_unsubscribe_token_customer_id_key ON dtb_mailmaga_unsubscribe_token(customer_id); +CREATE INDEX dtb_mailmaga_unsubscribe_token_send_id_key ON dtb_mailmaga_unsubscribe_token(send_id); +CREATE INDEX dtb_mailmaga_unsubscribe_token_expire_date_key ON dtb_mailmaga_unsubscribe_token(expire_date);MySQL 版（Lines 273-275）も同様のパターンで命名することを推奨します。
data/class/helper/SC_Helper_Mail.php (1)
534-542: 送信失敗時のトークン生成についてトークンはメール送信前に生成されるため、送信に失敗した場合でもトークンがデータベースに残ります。これらの孤立トークンは有効期限（90日）後に自動クリーンアップされますが、送信失敗時のトークン生成を避けることで、データベースのノイズを減らせます。
ただし、現在の実装でも機能的には問題なく、パフォーマンスへの影響も軽微です。
♻️ 送信成功後にトークン生成する案（オプション）
トークン生成をメール送信成功後に移動する場合、ヘッダーを先に追加できないため、RFC 8058 準拠のためには現在の実装が適切です。この指摘は情報提供のみです。
data/class/pages/mailmaga/LC_Page_Mailmaga_Unsubscribe.php (1)
151-158: 例外メッセージのユーザー表示について確認してください。例外メッセージをそのままユーザーに表示していますが、内部エラーの詳細がユーザーに漏洩する可能性があります。
♻️ 修正案: ユーザー向けメッセージを一般化
} catch (Exception $e) { $objQuery->rollback(); $this->tpl_success = false; - $this->tpl_message = 'エラーが発生しました: '.$e->getMessage(); + $this->tpl_message = 'エラーが発生しました。しばらく時間をおいて再度お試しください。'; GC_Utils_Ex::gfPrintLog($e->getMessage()); }
…ailmaga-one-click-unsubscribe
EC-CUBE naming convention: PRIMARY KEY should be {table_name}_id
- token_id → mailmaga_unsubscribe_token_id
- Add dtb_mailmaga_unsubscribe_token to SQLite3 schema
- Update sequence name in eccube_install.sh
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Document EC-CUBE 2 conventions for: - PRIMARY KEY naming (table_name_id) - Sequence naming and registration - Multi-DBMS support (MySQL, PostgreSQL, SQLite3) - _Ex class auto-aliasing Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.claude/skills/ec-cube-primary-key.md:
- Around line 13-23: Add a blank line before the Markdown table and normalize
the pipe spacing and separator row so markdownlint is happy: ensure the header
row uses single spaces around pipes ("| テーブル名 | PRIMARY KEY |"), the separator
row uses only dashes and pipes ("|---|---|") with no extra spaces, and every
data row matches the same pipe/space pattern (e.g., "| dtb_customer |
customer_id |"). Update the rows shown in the diff (the header line, the
separator line, and all "| dtb_* | ... |" lines) to follow this consistent
formatting.
In `@data/class/helper/SC_Helper_Mailmaga.php`:
- Around line 92-97: The SQL uses CURRENT_TIMESTAMP causing timezone
inconsistencies; update validateToken and cleanupExpiredTokens to compute the
current timestamp in PHP (e.g., $now = date('Y-m-d H:i:s')) and replace any SQL
comparisons that use CURRENT_TIMESTAMP with a parameterized comparison (e.g.,
"expire_date > ?" or "expire_date <= ?") passing $now as the bound parameter
when querying the dtb_mailmaga_unsubscribe_token table; ensure used_flag checks
remain (used_flag = 0) and the same $now logic is applied in both validateToken
and cleanupExpiredTokens to keep comparisons consistent across DB engines.
This reverts commit b16549a.
| public function action() | ||
| { | ||
| // トークンの取得 | ||
| $token = $_REQUEST['token'] ?? ''; |
| send_id int NOT NULL, | ||
| token varchar(64) NOT NULL, | ||
| email varchar(255) NOT NULL, | ||
| used_flag smallint NOT NULL DEFAULT 0, |
There was a problem hiding this comment.
フラグは _flg で統一いるため、修正をお願いします
- $_REQUEST → $_GET に変更 (nanasess)
- used_flag → used_flg に命名規則統一 (nanasess)
- $sendResut typo → $sendResult に修正
- CURRENT_TIMESTAMP → date('Y-m-d H:i:s') でタイムゾーン統一
- markTokenAsUsed の戻り値チェックを追加
- replay_to typo → reply_to に修正
- form action の REQUEST_URI を削除
- テストの assertStringStartsWith を HTTPS_URL 定数に変更
- POST confirm 後に return 追加
- トークン形式の簡易バリデーション追加 (64桁HEX)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
html/install/sql/create_table_pgsql.sql (1)
274-276: インデックス名を既存の命名規則に合わせてください。現在のインデックス名（
idx_customer_id、idx_send_id、idx_expire_date）は、ファイル内の他のすべてのインデックスで使用されているdtb_<テーブル名>_<カラム名>_keyパターン（例:dtb_order_customer_id_key、dtb_send_customer_customer_id_key）と異なります。♻️ 推奨される修正
-CREATE INDEX idx_customer_id ON dtb_mailmaga_unsubscribe_token(customer_id); -CREATE INDEX idx_send_id ON dtb_mailmaga_unsubscribe_token(send_id); -CREATE INDEX idx_expire_date ON dtb_mailmaga_unsubscribe_token(expire_date); +CREATE INDEX dtb_mailmaga_unsubscribe_token_customer_id_key ON dtb_mailmaga_unsubscribe_token(customer_id); +CREATE INDEX dtb_mailmaga_unsubscribe_token_send_id_key ON dtb_mailmaga_unsubscribe_token(send_id); +CREATE INDEX dtb_mailmaga_unsubscribe_token_expire_date_key ON dtb_mailmaga_unsubscribe_token(expire_date);MySQL側の
create_table_mysqli.sqlでも同様にKEYの名前をdtb_mailmaga_unsubscribe_token_customer_id_keyなどの形式に統一することを検討してください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@html/install/sql/create_table_pgsql.sql` around lines 274 - 276, The index names for dtb_mailmaga_unsubscribe_token (currently idx_customer_id, idx_send_id, idx_expire_date) do not follow the repository naming convention; rename them to the pattern dtb_<table>_<column>_key (e.g., dtb_mailmaga_unsubscribe_token_customer_id_key, dtb_mailmaga_unsubscribe_token_send_id_key, dtb_mailmaga_unsubscribe_token_expire_date_key) in the CREATE INDEX statements and apply the same naming change for the corresponding KEY entries in create_table_mysqli.sql to keep MySQL and PostgreSQL consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data/class/SC_SendMail.php`:
- Around line 166-174: The header protection check in SC_SendMail (the block
comparing $name to $protectedHeaders) is case-sensitive and can be bypassed by
"from"/"subject" lowercase variants; normalize header names before comparison by
lowercasing (or uppercasing) both $name and the entries in $protectedHeaders (or
store $protectedHeaders as lowercased values) and then perform the in_array
check so the protected-header guard is case-insensitive; update the assignment
to $this->customHeaders[$name] accordingly (you may want to preserve the
original header casing when storing but use the normalized form only for the
membership test).
---
Nitpick comments:
In `@html/install/sql/create_table_pgsql.sql`:
- Around line 274-276: The index names for dtb_mailmaga_unsubscribe_token
(currently idx_customer_id, idx_send_id, idx_expire_date) do not follow the
repository naming convention; rename them to the pattern
dtb_<table>_<column>_key (e.g., dtb_mailmaga_unsubscribe_token_customer_id_key,
dtb_mailmaga_unsubscribe_token_send_id_key,
dtb_mailmaga_unsubscribe_token_expire_date_key) in the CREATE INDEX statements
and apply the same naming change for the corresponding KEY entries in
create_table_mysqli.sql to keep MySQL and PostgreSQL consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21d7f958-85bc-46fe-8676-c5d6f73dbbf6
📒 Files selected for processing (9)
data/Smarty/templates/default/mailmaga/unsubscribe.tpldata/class/SC_SendMail.phpdata/class/helper/SC_Helper_Mail.phpdata/class/helper/SC_Helper_Mailmaga.phpdata/class/pages/mailmaga/LC_Page_Mailmaga_Unsubscribe.phphtml/install/sql/create_table_mysqli.sqlhtml/install/sql/create_table_pgsql.sqlhtml/install/sql/create_table_sqlite3.sqltests/class/helper/SC_Helper_Mailmaga/SC_Helper_MailmagaTest.php
✅ Files skipped from review due to trivial changes (1)
- data/Smarty/templates/default/mailmaga/unsubscribe.tpl
🚧 Files skipped from review as they are similar to previous changes (5)
- html/install/sql/create_table_sqlite3.sql
- data/class/helper/SC_Helper_Mailmaga.php
- data/class/pages/mailmaga/LC_Page_Mailmaga_Unsubscribe.php
- tests/class/helper/SC_Helper_Mailmaga/SC_Helper_MailmagaTest.php
- html/install/sql/create_table_mysqli.sql
- 保護ヘッダー判定をケース非依存に修正 (SC_SendMail.php) - インデックス名を dtb_<table>_<column>_key 形式に統一 (MySQL/PostgreSQL/SQLite3) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
data/class/SC_SendMail.php (1)
324-327: カスタムヘッダーのクリアをsendMail()側で自動化するとより安全です。Line 324〜327 は保持状態をそのままマージするため、呼び出し側の
clearCustomHeaders()呼び忘れ時に次メールへ混入し得ます。送信直後の自動クリアを検討してください。改善案（送信後に自動クリア）
public function sendMail($isHtml = false) { $header = $isHtml ? $this->getHTMLHeader() : $this->getTEXTHeader(); $recip = $this->getRecip(); // メール送信 $result = $this->objMail->send($recip, $header, $this->body); + $this->clearCustomHeaders(); if (PEAR::isError($result)) { // XXX Windows 環境では SJIS でメッセージを受け取るようなので変換する。 $msg = mb_convert_encoding($result->getMessage(), CHAR_CODE, 'auto');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data/class/SC_SendMail.php` around lines 324 - 327, The loop merging $this->customHeaders into $arrHeader can leak headers if callers forget clearCustomHeaders(); update the sendMail() flow in class SC_SendMail to automatically call $this->clearCustomHeaders() (or reset $this->customHeaders = []) immediately after the merge/send completes (successfully or in finally/fallback) so each send starts with an empty customHeaders state; reference the merge loop where $arrHeader is populated from $this->customHeaders and the existing clearCustomHeaders()/sendMail() methods to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data/class/SC_SendMail.php`:
- Around line 150-174: The addCustomHeader method currently only checks for
CR/LF injection but allows empty or RFC-noncompliant header names; update
addCustomHeader to trim $name, ensure it's not empty, and validate it against
the RFC token characters (e.g. use a regex like
/^[A-Za-z0-9!#$%&'*+\-.^_`|~]+$/) before the protected header check, calling
trigger_error and returning on failure; keep the existing CR/LF and protected
header checks and then set $this->customHeaders[$name] = $value when validation
passes.
---
Nitpick comments:
In `@data/class/SC_SendMail.php`:
- Around line 324-327: The loop merging $this->customHeaders into $arrHeader can
leak headers if callers forget clearCustomHeaders(); update the sendMail() flow
in class SC_SendMail to automatically call $this->clearCustomHeaders() (or reset
$this->customHeaders = []) immediately after the merge/send completes
(successfully or in finally/fallback) so each send starts with an empty
customHeaders state; reference the merge loop where $arrHeader is populated from
$this->customHeaders and the existing clearCustomHeaders()/sendMail() methods to
locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc9f7add-70d6-4fb0-8ba5-c5ea136daf18
📒 Files selected for processing (4)
data/class/SC_SendMail.phphtml/install/sql/create_table_mysqli.sqlhtml/install/sql/create_table_pgsql.sqlhtml/install/sql/create_table_sqlite3.sql
✅ Files skipped from review due to trivial changes (1)
- html/install/sql/create_table_mysqli.sql
🚧 Files skipped from review as they are similar to previous changes (2)
- html/install/sql/create_table_pgsql.sql
- html/install/sql/create_table_sqlite3.sql
…scribe Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- CREATE TABLE dtb_mailmaga_unsubscribe_token を SQL ファイルから ec-cube2-migration 形式のマイグレーションに移行 - SC_SendMail::addCustomHeader にヘッダー名の RFC 7230 検証を追加 - トークンクリーンアップバッチ（SC_Batch_CleanupMailmagaToken）を追加 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
html/ は公開ディレクトリのため、CLIバッチスクリプトを置くべきではない。 バッチクラス(data/class/batch/)のみで十分。 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
テスト間でシーケンス値が蓄積し、PRIMARY KEY重複エラーが 発生していた問題を修正。 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ec-cube2-migration の Table API には varchar() メソッドが 存在しないため string() を使用する。 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@eccube_install.sh`:
- Line 146: シーケンス名が誤っており PostgreSQL の SERIAL 命名規則に合っていません：現在のシーケンス
`dtb_mailmaga_unsubscribe_token_mailmaga_unsubscribe_token_id_seq` を正しいシーケンス名
`dtb_mailmaga_unsubscribe_token_id_seq` に修正してください（該当箇所は eccube_install.sh 内の該当
CREATE/ALTER シーケンス やシーケンス参照部分、およびマイグレーションで使用している `serial()` が生成する `id`
カラムに関連する箇所を確認）。これによりシーケンス名が `{table_name}_{column_name}_seq` の形式と一致します。
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6cd84f68-4922-48aa-86a3-678f5a7ed428
📒 Files selected for processing (6)
data/class/SC_SendMail.phpdata/class/batch/SC_Batch_CleanupMailmagaToken.phpdata/migrations/Version20260401000001_CreateMailmagaUnsubscribeTokenTable.phpeccube_install.shhtml/install/sql/create_table_sqlite3.sqltests/class/helper/SC_Helper_Mailmaga/SC_Helper_MailmagaTest.php
✅ Files skipped from review due to trivial changes (3)
- data/migrations/Version20260401000001_CreateMailmagaUnsubscribeTokenTable.php
- html/install/sql/create_table_sqlite3.sql
- tests/class/helper/SC_Helper_Mailmaga/SC_Helper_MailmagaTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- data/class/SC_SendMail.php
setVal はシーケンスの drop/create を行うが、PostgreSQL では シーケンス名が切り詰められるため dropSequence が失敗する。 delete のみで十分なためsetValを削除。 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
migrationでテーブル・シーケンスが作られるため、 eccube_install.shのシーケンスリストに含めると重複エラーになる。 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…scribe dtb_login_attempt はmaster側でDDLからマイグレーションに移行済み(dbeb31a)のため、 master側（削除）を採用してコンフリクトを解消。 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
master に先行マージされた Version20260401000001_CreateLoginAttemptTable と バージョン番号が重複していたため、バージョンを 20260401000002 に変更。
Summary
Google/Yahoo のメール送信ガイドライン変更に対応するため、RFC 8058 準拠のワンクリックメルマガ登録解除機能を実装しました。
主な変更点
dtb_mailmaga_unsubscribe_tokenテーブルを追加（トークン管理）SC_Helper_Mailmagaを作成（トークン生成・検証・登録解除処理）addCustomHeader(),clearCustomHeaders()）List-UnsubscribeとList-Unsubscribe-Postヘッダーを自動追加/mailmaga/unsubscribe/を新規作成（GET/POST対応）RFC 8058 要件
List-Unsubscribeヘッダー: HTTPS URL を含むList-Unsubscribe-Postヘッダー:List-Unsubscribe=One-Click固定値セキュリティ対策
後方互換性
Test plan
関連Issue
Closes #819
🤖 Generated with Claude Code
Summary by CodeRabbit
新機能