Bug(バグ) #2356
管理画面でプロフィール項目の識別名に全角文字や角括弧が使えてしまう
100%
Description
概要¶
管理画面でプロフィール項目を作成、あるいは編集するときに、識別名に全角文字(※)が使える。全角文字が使えること自体は識別名がそのような仕様であれば問題ないが、それは想定した仕様ではないはずであり、識別名に全角文字を含めてプロフィール項目を作ってしまうと、携帯版でプロフィール編集が成功しなくなる(バリデータに弾かれる)という問題が起こる(PC版では弾かれない、ある文字列の場合はPC版も弾かれる:後述)。
※ここでは本来想定外の文字を「全角文字」と称することにする。実際はこれに全角でなくても弾くべき文字(例えば半角カナ等)が含まれるはずである。
「識別名に全角文字が含まれている場合に携帯版で、ある文字列の場合にPC版で、プロフィール編集ができなくなる」という問題と捉えることも可能かもしれないが、そもそも識別名に全角文字が使えることは想定していないため、「プロフィール項目の識別名に全角文字が使えてしまう」という問題として捉える。
この問題は #839 (3.5.1 向け) ですでに扱われているはずだが、 #839 は修正が不完全なまま完了とされている。このバックポートチケット #900 (3.4.4 向け) では、より適切な修正が行われており、 3.5 以降でこの問題が残ったままになってしまっている。
携帯版で弾かれる理由¶
識別名に全角文字が使われているとき、PC版ではプロフィール編集が問題なく行えるようだが、携帯版ではバリデータに弾かれてしまう。「正しくありません。(Invalid.)」という文言しか表示されないため何が原因で弾かれているのかすぐには分からないが、これは「識別名に使われている全角文字が、内部では UTF-8 エンコーディングで扱われるのに対し、ブラウザ側では Shift_JIS エンコーディングとして扱われるため、その差異により識別名のキーが不一致を起こす」ということによるものである。
キーの不一致を起こしてしまうことが問題ではなく、識別名に全角文字が使えてしまうことを問題の本質と捉えるべきだろう。
PC版で弾かれるケース¶
PC版では、(偶然ではあるが)内部でのエンコーディングもブラウザ側でも UTF-8 エンコーディングとしてキー値が扱われるため、例えば識別名が「あいうえお」であってもプロフィールの編集は問題なく行える。この場合、 input 要素の name 属性値は "profile[あいうえお][value]" のようになる(プロフィール項目の種類によって若干異なるが)。
識別名に「abc"def」を使った場合は、 htmlspecialchars() 相当の処理でエスケープ(文字参照への変換)が行われるため name 属性値は "profile[abc"def]" となる。
ここで問題となるのは識別名に「aaa[bbb」や「xxx]yyy」のように角括弧を含んでいる場合で、角括弧はエスケープの対象ではないため name 属性値が "profile[xxx]yyy][value]" となってしまう。これでは内部とブラウザ側で、識別子のキーが一致するものの、角括弧によって表現されている配列の構造が崩れてしまうためキーの取得ができず、「正しくありません。」とバリデータに弾かれてしまう。これは原理的に明らかだが、PC版と携帯版共通の問題である。
修正方針¶
#900 での検討内容をもとに、識別名に対して同様のバリデートを行うようにする。
#900 ではバリデートは行われているが、入力可能な文字の説明や、エラーメッセージが適切ではないため、メッセージの追加も併せて行うことにする。
修正前と修正後の動作¶
======== 修正前 ==== バリデートでエラーとなる(登録できない) 1. 【適切(*1)】 "op_preset_hoge" 先頭が op_preset_ で始まる名前を付けようとする(3系独自のバリデート:正しくありません) 2. 【適切】 "" 空文字列(必須項目です) 3. 【仕様による(*2)】 " " 半角スペースのみ(trim が有効:必須項目です) 4. 【仕様による(*2)】 " " 全角スペースのみ(opValidatorString の trim が有効:必須項目です) ==== バリデートでエラーとならない(登録できる) 5. 【適切】 "foo" 妥当な文字列 6. 【仕様による(*2)】 " bar " 前後に半角スペースを含む妥当な文字列(trim が有効) 7. 【仕様による(*2)】 " buzz " 前後に全角スペースを含む妥当な文字列(opValidatorString の trim が有効) 8. 【不適切】 "あいうえお" 全角文字を含む文字列 9. 【不適切】 "abc]def" 角括弧(開き、閉じの少なくとも一方)を含む文字列 10. 【仕様による(*3)】 "hoge#fuga=piyo?" いろいろな記号を含む文字列 11. 【仕様による(*3)】 "12345" 半角数字のみの文字列 12. 【仕様による(*3)】 "____" アンダースコアのみの文字列 13. 【仕様による(*3)】 "123abd" 数字から始まる妥当そうな文字列 ======== 修正後 ==== 識別名に関する注釈を追加 (*0) 「アンダースコアと半角英数字のみが使えます。識別名にはアルファベットが含まれていなければなりません。」 ==== バリデートでエラーとなる(登録できない) 1. 【適切(*1)】 "op_preset_hoge" 先頭が op_preset_ で始まる名前を付けようとする(3系独自のバリデート:適切なエラーメッセージ) 2. 【適切】 "" 空文字列(必須項目です) x. 【適切】上記 3, 4, 6, 7, 8, 9, 10, 11, 12 (今回の修正:形式が不正) ==== バリデートでエラーとならない(登録できる) 5. 【適切】 "foo" 妥当な文字列 13. 【適切】 "123abd" 数字から始まる妥当そうな文字列 14. 【適切】 妥当な文字列はこの他、アンダースコアと半角英数字のみからなり、少なくともアルファベットを1文字含む文字列
変更点について:
- (*0) 識別名に対する注釈
- 識別名の入力フォームのそばに、識別名として受け付けられる文字列についての説明を加えた。
- (*1) op_preset_ で始まる名前の場合
- 修正前もエラーとなっていたが、修正前には適切なエラーメッセージが表示されていなかったので修正した。
- (*2) 入力文字列の前後に空白がある場合
- trim は行わないままバリデートすることにした。つまり、修正前は受け付けられていた 6 や 7 のケースを両方ともエラーとなるようにした。
- (*3) 識別名に使える文字列と、識別名の制約について
- #900 での検討内容を妥当だと判断したため、 10, 11, 12 のケースはエラーとなるようにした。
- 13 のケース(数字から始まる識別名)は一般的には受け付けない印象があるが、プロフィール項目の識別名としては受け付けても特に問題がなく、 2 系時代でも受け付けていたため、 3.4 系の修正と合わせ、これは許容することにした。 12 のケース(アンダースコアのみ)を弾いたのも 3.4 系と合わせるためであり、 3.6 系以降で 12 のケースを弾いた特別な理由はない( 12 のケースを弾かないという選択肢もあり得た)。
(*2) について、 trim を行う必要があるかどうかについてだが、 sfValidatorString の trim (一般的な空白類のみ)にしても opValidatorString の trim (全角スペースを含む空白類)にしても、識別名において trim を行う必要はないと判断した。そもそも trim は、空白類の入力も受け付けるテキスト入力エリアにおいて、その入力値の前後に付く連続した空白類を取り除くという意図が強いように思う。もともと空白類の入力を受け付けないテキスト入力エリアでは入力値が trim される必要はないと判断した。
対象バージョン¶
master(3.7), 3.6.x で修正が必要。
3.4.x では #900 の修正が行われているため対象外だが、メッセージの追加(上記 *0 と *1 の内容)についてはバックポート対応が可能ではある。
Related issues
Associated revisions
(fixes #2356) add validation for the profile identification name on backend.
(fixes #2356) set error message for a case that inputted "op_prefix_" as a prefix.
History
#1 Updated by Minoru Takai about 13 years ago
- Target version set to OpenPNE 3.7.0
#2 Updated by Minoru Takai about 13 years ago
この問題は #839 の内容です。
#839 (3.5.1 向け) とそのBP #900 (3.4.x 向け) がありましたが、 #839 は修正が不十分なまま完了となっています。言い換えれば、 3.4.x 向けのみ、より適切な修正が施されています。
これが影響してこのチケットでの問題が生じています。 #839 はすでにクローズしているため、このチケットで #839 の修正内容を引き継ぐこととします。
このチケット作成時点の内容を残しておきます。
プロフィール項目を管理画面で作成するときに識別名に全角文字が使え、その場合に携帯版でプロフィール編集が失敗する
概要¶
現時点の OpenPNE-3.6 以降で、
- プロフィール項目追加画面(/pc_backend.php/profile/edit)にアクセスする
- 識別名に「全角文字を含めた文字列」を入力する
- プロフィール項目を作成する
- mobile_frontend ログイン後、プロフィール編集ページ(/member/edit/profile)にアクセスする
- プロフィール編集ページで「確定」ボタンを押す
すると、「正しくありません。」というエラーメッセージが表示され、プロフィール編集が成功しない。
識別名が半角英字列ではない場合について、
- pc_backend では、プロフィール作成時点でバリデータに弾かれない
- pc_frontend では、プロフィール編集時点でバリデータに弾かれない
- mobile_frontend では、PC版と異なり、プロフィール編集時点でバリデータに弾かれる
という動作となっている。また、識別名に使える文字列が何なのかについて pc_backend では分からない。
修正すべき内容¶
- プロフィール項目の識別名に全角文字を含めた場合に、 pc_backend と pc_frontend ではエラーにならず、 mobile_frontend だけエラーになる不自然な動作を解消する。
mobile_frontend でもバリデータに弾かれないようにするのか、あるいは識別名に使える文字を制限するのか、このどちらかが考えられるが、 OpenPNE-3.4 以前は pc_backend でプロフィール項目を作成する時点で「全角文字が含まれている」という場合や「数字から始まる」という場合にはバリデータに弾かれるようになっている。
恐らく次の内容の修正を行うことになる。
- pc_backend でプロフィール項目を作成する時点で、識別名に使える文字が何かを明示し、それに反する文字列はバリデータで弾くようにする。
対象バージョン¶
master(3.7), 3.6.x で修正が必要。
3.4.x では pc_backend でバリデータに弾かれるため、携帯版でプロフィール編集が失敗する問題は起こらない。しかしながら、「識別名に使える文字が何か」を明示する修正を行うとしたら 3.4.x にもバックポートは可能なはずである。
#3 Updated by Minoru Takai about 13 years ago
- Status changed from New(新規) to Accepted(着手)
せっかくなので #900 の内容をそのまま取り込むよりも好ましい改善を行います。
2 系の仕様¶
- webapp/lib/OpenPNE/Validator.php
357- case 'regexp': 358- if (isset($rule['regexp']) && !preg_match($rule['regexp'], $reqval)) { 359- if (isset($rule['type_error'])) { 360- $error = $rule['type_error']; 361- } else { 362- $error = "{$rule['caption']}を正しく入力してください"; 363- }
- ここで弾かれており、その制約は次の正規表現によるものでした
識別名を弾いた時点の $rule['regexp'] の値: string '/^[a-z0-9_]+$/i' (length=15)
そして、その後に別のバリデートも行われています。
- webapp/modules/admin/do/update_c_profile.php
- webapp/modules/admin/do/insert_c_profile.php
15- function execute($requests) 16- { 17- if (db_admin_c_profile_name_exists($requests['name'])) { 18- admin_client_redirect('insert_c_profile', 'その識別名は既に登録されています'); 19- } 20- if (is_numeric($requests['name'])) { 21- admin_client_redirect('insert_c_profile', '識別名は数値のみには設定できません'); 22- }
3.4 系の仕様(3.4.4 以降)¶
- lib/form/doctrine/ProfileForm.class.php : 56, 59(=118), 64 行目の 3 箇所
55- $this->validatorSchema->setPostValidator( 56- new sfValidatorDoctrineUnique(array('model' => 'Profile', 'column' => array('name')), array('invalid' => 'Already exist.')) 57- ); 58- 59- $this->mergePostValidator(new sfValidatorCallback(array('callback' => array('ProfileForm', 'validateName')))); 60- $this->setValidator('default_public_flag', new sfValidatorChoice(array('choices' => array_keys(Doctrine::getTable('Profile')->getPublicFlags())))); 61- $this->setValidator('value_min', new sfValidatorPass()); 62- $this->setValidator('value_max', new sfValidatorPass()); 63- $this->setValidator('value_type', new sfValidatorString(array('required' => false, 'empty_value' => 'string'))); 64- $this->setValidator('name', new sfValidatorRegex(array('pattern' => '/^\w*[a-z]\w*$/i'))); 65- : 116- static public function validateName($validator, $values) 117- { 118- if (0 === strpos($values['name'], 'op_preset_')) 119- { 120- throw new sfValidatorError($validator, 'invalid'); 121- }
3.6 系以降の修正前の仕様¶
上記 3.4 系の 64 行目の部分が以下のようになっている。
$this->setValidator('name', new opValidatorString(array('required' => true, 'trim' => true)));
これによるバリデーションは行われている。
2 系と 3.4 系の違い¶
2 系:
- (A1) 正規表現 /^[a-z0-9_]+$/i にマッチする
- (A2) 既存の識別名ではない
- (A3) is_numeric($value) が偽である
3.4 系:
- (B1) 既存の識別名ではない
- (B2) op_preset_ で始まらない( /^op_preset_/ にマッチしない、と同値)
- (B3) 正規表現 /^\w*[a-z]\w*$/i にマッチする
この条件を全て満たす文字列が許容されています。両者の比較において、「既存の識別名ではない」ことのチェックには差異はありません。また 3 系独自の op_preset_ で始まらないことは以下では触れません。
誤解がないように正規表現について示しておきますが、 /¥w/ と /[_a-zA-Z0-9]/ と /[_a-z0-9]/i はそれぞれ同値である前提とします。(余談ですが、preg 系ではなく mb_ereg 系の関数を用いる場合、\w や \d のようなエスケープシーケンスが全角文字まで含む場合があります。これらは通常であれば半角文字のみを指すものとして考えてよいですが、「常に /\w/ が /[_a-zA-Z0-9]/ と一致する」と思い込むのは危険かもしれません。付け加えれば /[0-9]/ が /0|1|2|3|4|5|6|7|8|9/ と一致することも前提条件があることを忘れてはなりません。)
- 2 系は、 /^¥w+$/ にマッチし、 is_numeric($value) が偽である、という文字列が許容されます。
- 3.4 系は、 /^¥w+$/ にマッチし、 アルファベットを含む、という文字列が許容されます。
後半の内容が異なっていますが、この差異は #900 で検討されたものであり、このコメントを書いている時点でもその検討内容は妥当だと思っています。
ところで、2系の「 is_numeric() が偽となるかどうか」をチェックしているのは意図がよく分かりません。これでは 1eA が通るのに 1e1 が弾かれ、 0xfg が通るのに 0xff が通らない状態になっています。ここでは「 ctype_digit() が偽となるかどうか」をチェックするべきでしょう。この違いのことは以下では触れません(「数字のみではない」ことをチェックしていると考えておきます)。
さて、ここまでの確認を経て、
- 2 系は、「 /^¥w+$/ がマッチし、数字のみではない」(アンダースコアのみは許容される)
- 3.4 系は、「 /^¥w+$/ がマッチし、アルファベットが含まれる」(アンダースコアのみは許容されない)
という制約であることが分かりました。
3.6 系以降での修正¶
管理画面の「識別名についての注釈」について、 2 系は「※半角英数 と _ のみ(数値のみも不可)」という文言があります。しかし 3 系にはありません。
3.6 系以降では、個人的にアンダースコアのみは許容しなくてよいと思うので、バリデーションは 3.4 系と同一のものでよいと考えます。そして、管理画面に注釈をつける改善も施そうと思います。
#4 Updated by Minoru Takai about 13 years ago
- Assignee set to Minoru Takai
#5 Updated by Minoru Takai about 13 years ago
- Status changed from Accepted(着手) to Pending Review(レビュー待ち)
- % Done changed from 0 to 50
更新履歴 db7c968fab6e646914d40caf64acdb05850b26db で適用されました。
#6 Updated by Minoru Takai about 13 years ago
更新履歴 01d4f29766ac6ea709c51178004b644d8dc5f9ba で適用されました。
などとコミットログに書いていますが、 "op_prefix_" は "op_preset_" の誤りです。
#7 Updated by Minoru Takai about 13 years ago
- Subject changed from プロフィール項目を管理画面で作成するときに識別名に全角文字が使え、その場合に携帯版でプロフィール編集が失敗する to 管理画面でプロフィール項目の識別名に全角文字や角括弧が使えてしまう
- Description updated (diff)
チケットの Description を書き換えました。
#8 Updated by Shingo Yamada about 13 years ago
- Priority changed from Normal(通常) to High(高め)
#9 Updated by Shingo Yamada about 13 years ago
- 360対象 set to RC1
#10 Updated by Yuma Sakata about 13 years ago
- 360対象 deleted (
RC1)
#11 Updated by Yuma Sakata about 13 years ago
- 360対象 set to RC1
#12 Updated by Kousuke Ebihara about 13 years ago
- Status changed from Pending Review(レビュー待ち) to Pending Testing(テスト待ち)
- % Done changed from 50 to 70
#13 Updated by Minoru Takai about 13 years ago
- Description updated (diff)
#14 Updated by Shingo Yamada about 13 years ago
- 360対象 changed from RC1 to 3.6.0
#15 Updated by Kousuke Ebihara about 13 years ago
- 360対象 changed from 3.6.0 to RC2
#16 Updated by Fumie Toyooka about 13 years ago
- Status changed from Pending Testing(テスト待ち) to Fixed(完了)
- % Done changed from 70 to 100
#17 Updated by Fumie Toyooka about 13 years ago
- Status changed from Fixed(完了) to Pending Testing(テスト待ち)
- % Done changed from 100 to 70
#18 Updated by Fumie Toyooka almost 13 years ago
- Status changed from Pending Testing(テスト待ち) to Fixed(完了)
- % Done changed from 70 to 100
テストOKです。
#19 Updated by kaoru n about 9 years ago
- 3.8 で発生するか set to Unknown (未調査)