Bug(バグ) #4012
完了メールアドレス設定から意図的に他のメンバーと同じメールアドレスを設定できる
100%
説明
Overview (概要)¶
再現手順:
- メンバーA でログインし、PCメールアドレス設定 (/member/config?category=pcAddress) を開く
- PCメールアドレスに
hoge@example.com
を入力して「送信」ボタンをクリックhoge@example.com
宛に確認メールが届くが、この時点ではメールに記載されている URL を開かない
- メンバーB でログインし、PCメールアドレス設定 (/member/config?category=pcAddress) を開く
- PCメールアドレスに
hoge@example.com
を入力して「送信」ボタンをクリック - メンバーA によって送られた「メールアドレス変更ページのお知らせ」のメールに記載されている URL を開く
- メンバーA のパスワードを入力して「送信」ボタンをクリック
- メンバーB によって送られた「メールアドレス変更ページのお知らせ」のメールに記載されている URL を開く
- メンバーB のパスワードを入力して「送信」ボタンをクリック
- メンバーA, メンバーB のPCメールアドレスが同じ
hoge@example.com
に設定された状態になる
Causes (原因)¶
lib/config/config/member_config.yml
では pc_address および mobile_address に対して IsUnique: true
が指定されており、実際に「PCメールアドレス設定」のフォームでは MemberConfigForm::isUnique()
によって重複チェックが行われている。
しかし、メールアドレス変更については、変更後のメールアドレス宛に /member/configComplete?token=***
のURLをメールで送った上で、確認が完了するまでは pc_address を変更しないという例外的な対応を取っている。そのため、MemberConfigForm のバリデーション時には重複しなかったが /member/configComplete の時点で重複するような場合には対処できていなかった。
Way to fix (修正内容)¶
opMemberActions::executeConfigComplete()
において、pc_address_pre または mobile_address_pre を pc_address または mobile_address に変更する直前にも重複チェックを行う。これにより、上記の再現手順でいう手順 8 の段階でエラーが発生するようになる。
なお、手順 4 の段階で pc_address_pre が重複するためこの時点でエラーを発生させるといった修正も考えられるが、これをエラーとしてしまうと悪意のあるメンバーが任意のメールアドレスを確認が未完了のまま放置することによって他のメンバーがそのメールアドレスを使用できない状態に出来てしまうため、pc_address_pre に対する重複チェックは行わない。
ファイル
Youichi Kimura さんが約8年前に更新
- ステータス を New(新規) から Pending Review(レビュー待ち) に変更
- 進捗率 を 0 から 50 に変更
下記の Pull Request にて修正しました (#3077, #4012 の修正を含んでいます)
https://github.com/openpne/OpenPNE3/pull/372
Youichi Kimura さんが約8年前に更新
手順 8 の member/configComplete で発生させるエラーについて、単に設定値が重複している旨のエラーを表示するだけでは意図が伝わりにくいように見えたため、下記のように変更後の設定値を表示する行を追加しています。
Youichi Kimura さんが約8年前に更新
- 関連している Backport(バックポート) #4017: メールアドレス設定から意図的に他のメンバーと同じメールアドレスを設定できる を追加
Youichi Kimura さんが約8年前に更新
- 関連している Backport(バックポート) #4018: メールアドレス設定から意図的に他のメンバーと同じメールアドレスを設定できる を追加
Shinichi Urabe さんがほぼ8年前に更新
- ステータス を Pending Review(レビュー待ち) から Rejected(差し戻し) に変更
https://github.com/openpne/OpenPNE3/pull/372#pullrequestreview-13092311 に直接コメントしてフィードバックしました
確認をお願いします
Youichi Kimura さんが約7年前に更新
#4012-7 のコメントを転記
lib/model/doctrine/MemberConfig.class.php:
@@ -117,6 +117,19 @@ public function getSetting()
return $config[$this->getName()];
}
+ public function checkUniqueness()
メソッド名からはユニーク性をチェックするように見えますが、内部の処理では設定値からユニーク判定が不要な場合も true と返されますので、メソッド名と処理に差異があるように感じます。
lib/model/doctrine/MemberConfig.class.php:
+ $duplicate = $this->getTable()->retrieveByNameAndValue($this->name, $this->value);
(既に重複しているデータがある前提となりますが) 取得したレコードが複数存在した場合で且つ、
opDoctrineQuery::fetchOne()
で取得したレコードがたまたま、ユーザ自身のレコードであった場合、重複判定が回避されます。
lib/action/opMemberAction.class.php:
@@ -309,6 +309,13 @@ public function executeConfigComplete(opWebRequest $request)
}
$config->setValue($pre->getValue());
+ if (!$config->checkUniqueness())
+ {
+ $this->getUser()->setFlash('error', 'The inputted value is already exist.');
エラーメッセージが親切であるのは良いことだと思いますが、該当メールアドレスがすでに SNS 上の誰かに紐付いて存在するという情報の暴露には繋がらないでしょうか。
変更しようとして受信したメールアドレスの持ち主自身での操作になると思いますので、難しいところですが。
Youichi Kimura さんが6年以上前に更新
#4012-9 の指摘について:
lib/model/doctrine/MemberConfig.class.php:
@@ -117,6 +117,19 @@ public function getSetting() return $config[$this->getName()]; } + public function checkUniqueness()
メソッド名からはユニーク性をチェックするように見えますが、内部の処理では設定値からユニーク判定が不要な場合も true と返されますので、メソッド名と処理に差異があるように感じます。
修正後のメソッド名として validateUniqueness
で考えています。
メソッド名以外は変更なしで、設定値にエラーが無いか重複チェックが不要な場合は true、設定値の重複によるエラーが発生した場合は false が返ります。
lib/model/doctrine/MemberConfig.class.php:
+ $duplicate = $this->getTable()->retrieveByNameAndValue($this->name, $this->value);
(既に重複しているデータがある前提となりますが) 取得したレコードが複数存在した場合で且つ、
opDoctrineQuery::fetchOne()
で取得したレコードがたまたま、ユーザ自身のレコードであった場合、重複判定が回避されます。
当チケットの修正前は他のメンバーと重複した pc_address, mobile_address が設定可能な状態であったため、この指摘について考慮が必要となります。
id | member_id | name | value |
---|---|---|---|
1001 | 1 | pc_address | sns@example.com |
1002 | 2 | pc_address | sns@example.com |
例えば sns@example.com
がPCメールアドレスとして重複登録されている場合、現在の修正通りに retrieveByNameAndValue メソッドで name = 'pc_address' AND value = 'sns@example.com'
(実際は name_value_hash
の値で比較される) に合致するレコードを 1 件だけ抽出すると、上記の表でいう id = 1001
または id = 1002
のどちらのレコードが返るか不定になります。
これを踏まえて「取得したレコードがたまたま、ユーザ自身のレコードであった場合、重複判定が回避されます」というのは、
member_id = 1
のメンバーがPCメールアドレスをsns@example.com
からsns@example.com
に変更sns@example.com
が他のメンバー (member_id = 2
) と重複して登録されている- 重複チェック時に
$duplicate
に取得されたレコードがid = 1001
であった
の条件が揃った場合になります。
あまり現実味の無い状況ではありますが、以下のように member_id の条件を fetchOne で絞り込むより前に移動することで対処することが可能です。
--- a/lib/model/doctrine/MemberConfig.class.php
+++ b/lib/model/doctrine/MemberConfig.class.php
@@ -125,9 +125,12 @@
return true;
}
- $duplicate = $this->getTable()->retrieveByNameAndValue($this->name, $this->value);
-
- return !$duplicate || $duplicate->member_id === $this->member_id;
+ $duplicate = $this->getTable()->createQuery('mc')
+ ->andWhere('mc.member_id <> ?', $this->member_id)
+ ->andWhere('mc.name_value_hash = ?', $this->getTable()->generateNameValueHash($this->name, $this->value))
+ ->fetchOne();
+
+ return !$duplicate;
}
public function generateRoleId(Member $member)
lib/action/opMemberAction.class.php:
@@ -309,6 +309,13 @@ public function executeConfigComplete(opWebRequest $request) } $config->setValue($pre->getValue()); + if (!$config->checkUniqueness()) + { + $this->getUser()->setFlash('error', 'The inputted value is already exist.');
エラーメッセージが親切であるのは良いことだと思いますが、該当メールアドレスがすでに SNS 上の誰かに紐付いて存在するという情報の暴露には繋がらないでしょうか。
変更しようとして受信したメールアドレスの持ち主自身での操作になると思いますので、難しいところですが。
この箇所については、変更後のメールアドレスの所有者自身でなければこのエラーメッセージに辿り着くことは無いため、現状の修正のままで問題は無いと考えています。
Rimpei Ogawa さんが約6年前に更新
- ステータス を Pending Review(レビュー待ち) から Rejected(差し戻し) に変更
Rimpei Ogawa さんがほぼ6年前に更新
- ステータス を Pending Review(レビュー待ち) から Pending Testing(テスト待ち) に変更
- 進捗率 を 50 から 70 に変更
Chiharu Nakajima さんがほぼ6年前に更新
- ステータス を Pending Testing(テスト待ち) から Rejected(差し戻し) に変更
- 進捗率 を 70 から 50 に変更
修正を取り込んだ状態でメールアドレス変更を行うと、メールアドレス変更時のパスワード入力画面で正しいパスワードを入力して「送信」を選択した場合、以下の500エラーが発生します(他のユーザと同じメールアドレスに変更しようとしているかどうかにかかわらず)。
500 | Internal Server Error | Doctrine_Record_UnknownPropertyException Unknown method MemberConfig::validateUniqueness stack trace at () in SF_ROOT_DIR/lib/vendor/symfony/lib/plugins/sfDoctrinePlugin/lib/vendor/doctrine/lib/Doctrine/Record.php line 2660 ... } } throw new Doctrine_Record_UnknownPropertyException(sprintf('Unknown method %s::%s', get_class($this), $method)); } /**
Chiharu Nakajima さんがほぼ6年前に更新
- ステータス を Rejected(差し戻し) から Pending Merge(マージ待ち) に変更
- 進捗率 を 50 から 80 に変更