Project

General

Profile

Bug(バグ) #1687

パスワード再発行処理を行なおうとすると http://opauthmailaddress/ に飛ばされてしまう

Added by pnetan   almost 9 years ago. Updated almost 4 years ago.

Status:
Fixed(完了)
Priority:
High(高め)
Target version:
Start date:
2010-10-15
Due date:
% Done:

100%

3.6 で発生するか:
Yes
3.8 で発生するか:
Unknown (未調査)

Description

Overview (現象)

1. 「パスワードを忘れた方へ」リンクよりパスワード再発行の申請を行う
2. パスワード再発行メールに
http://sns.example.com//opAuthMailAddress/passwordRecoveryComplete/token/hogehoge/id/XX
というURLが記載されているので、このURLにアクセスする
3. 新しいパスワード入力して、決定ボタンを押下すると、
http://opauthmailaddress/passwordRecoveryComplete/token/hogehoge/id/XX
というURLに飛ばされてしまう

元の報告:
http://twitter.com/77web/statuses/27406372500

確認バージョン

  • 3.6beta6 再現した
  • 3.4 未確認

Causes (原因)

この問題は base_url に指定する URL の末尾に"/"を含めた場合に発生します。
2系では末尾に"/"が必須でしたが、3系はこれとは逆の仕様であると思われます。
したがって、この仕様について設定ファイル中に明記されていない事が問題であると思われます。
(2系では設定ファイル中にコメントアウトとして記述されています)

Way to fix (修正内容)

適切な対応としては、コメントに仕様を記述するよりも、
"/"が末尾に含まれる場合でも正常に動作する方が良いと思いますので、対応可能なように修正を行います。

OpenPNE.ymlがロードされるタイミング(opProjectConfiguration::setOpenPNEConfiguration())で、"/"が含まれていた場合に除去を行います。

この実装の場合、添付されているテストコードではsfConfigを直接書き換えているためエラーのままとなります。
また、op_base_url を書き換えた場合も"/"の除去は機能しません。

URL生成メソッドの opApplicationConfiguration::getAppRouting() 中に処理を記述することで対応は可能ですが、
この場合URL生成時に毎回処理を実行するため無駄な処理が走る事、他の箇所で参照されていた場合を考慮してこの実装は行いません。
(op_base_url を書き換えることは通常考えられない、この実装でも問題ないと考えます)

generateAppUrlTest.phps (2.84 KB) hiromi hishida, 2010-12-15 11:58


Related issues

Related to OpenPNE 3 - Backport(バックポート) #2076: パスワード再発行処理を行なおうとすると http://opauthmailaddress/ に飛ばされてしまう Fixed(完了) 2010-10-15 2011-06-24
Related to OpenPNE 3 - Bug(バグ) #1675: 設定ファイルを記述し忘れた場合に、招待メールに記載されたURLにアクセスできない Fixed(完了) 2010-10-14
Related to OpenPNE 3 - Backport(バックポート) #2681: パスワード再発行処理を行なおうとすると http://opauthmailaddress/ に飛ばされてしまう Invalid(無効) 2010-10-15

Associated revisions

Revision 53ddf52e (diff)
Added by Masato Nagasawa over 8 years ago

fixed remove when "/" was included end of the url in the "op_base_url" config(fixes #1687)

Revision 6f5cf83d (diff)
Added by Masato Nagasawa over 8 years ago

Revert "fixed remove when "/" was included end of the url in the "op_base_url" config(fixes #1687)"

This reverts commit 53ddf52e4bca623f388c4a9841a3129d45d1664e.

Revision eab44bc7 (diff)
Added by Masato Nagasawa over 8 years ago

fixed remove when "/" was included end of the url in the "op_base_url" config(fixes #1687)

History

#1 Updated by Kousuke Ebihara almost 9 years ago

  • Target version set to OpenPNE 3.7.0
  • 3.6 で発生するか set to Yes

#2 Updated by Masato Nagasawa almost 9 years ago

  • Status changed from New(新規) to Accepted(着手)
  • Assignee set to Masato Nagasawa

#3 Updated by Masato Nagasawa almost 9 years ago

  • Status changed from Accepted(着手) to New(新規)
  • Assignee deleted (Masato Nagasawa)

3.7.0-dev にて確認しましたが、再現できませんでした。
一度手放します。

#4 Updated by hiromi hishida over 8 years ago

元報告者です(http://twitter.com/77web)。
遅ればせながら原因と解決法がわかりました。
メールで届く再発行用URLがhttp://sns.openpne.jp//opAuthMailAddress/~となっているために、form action="//opAuthMailAddress/~"というHTMLが出力されて、ブラウザの仕様により(?)http://opauthmailaddress/~にPOSTしてしまうようです。
修正が必要になるのはopAuthMailAddressで送信するメールの本文を生成している部分(あるいはメールテンプレート)と思われます。

#5 Updated by hiromi hishida over 8 years ago

現象が再現するテストを書きましたので添付します。#1675のチケットと原因が同じだと思います。
手っ取り早い方法としては、メール送信のテンプレート側でapp_url_for()をabsolute=trueで使わずsfConfig::get('op_base_url')とabsolute=falseにしたapp_url_for()の返り値を繋げることで回避できます。

#6 Updated by Masato Nagasawa over 8 years ago

  • Project changed from OpenPNE 3 to opAuthMailAddressPlugin
  • Target version deleted (OpenPNE 3.7.0)
  • [QA]バグ通知済 set to No

#7 Updated by Kousuke Ebihara over 8 years ago

  • [QA]バグ通知済 changed from No to Yes

#8 Updated by Shingo Yamada over 8 years ago

  • Assignee set to Masato Nagasawa

#9 Updated by Masato Nagasawa over 8 years ago

  • Status changed from New(新規) to Accepted(着手)

添付されているテストコードを #1675 の修正適応済みの最新のmasterソースで確認しましたが、
現状でもエラーになっている状態なので、別に修正が必要なようです。

#10 Updated by Masato Nagasawa over 8 years ago

この問題は base_url に指定する URL の末尾に"/"を含めた場合に発生します。
2系では末尾に"/"が必須でしたが、3系はこれとは逆の仕様であると思われます。
したがって、この仕様について設定ファイル中に明記されていない事が問題であると思われます。
(2系では設定ファイル中にコメントアウトとして記述されています)

ですがより適切な対応としては、コメントに仕様を記述するよりも、
"/"が末尾に含まれる場合でも正常に動作する方が良いと思いますので、対応可能なように修正を行います。

#11 Updated by Masato Nagasawa over 8 years ago

  • Project changed from opAuthMailAddressPlugin to OpenPNE 3

#12 Updated by Masato Nagasawa over 8 years ago

  • Target version set to OpenPNE 3.7.0

#13 Updated by Masato Nagasawa over 8 years ago

  • Status changed from Accepted(着手) to Pending Review(レビュー待ち)
  • % Done changed from 0 to 50

更新履歴 53ddf52e4bca623f388c4a9841a3129d45d1664e で適用されました。

#14 Updated by Naoya Tozuka over 8 years ago

  • Status changed from Pending Review(レビュー待ち) to Rejected(差し戻し)
  1. base_url が config.yml で定義されていないケースが考慮されていない
  2. キャッシュから読み込んだconfigに対して毎回この処理を行っているが、できればキャッシュには変換済のものを格納したい
  3. 変数 $len が (長さ - 1) を表す数値に用いられているのが若干気持ち悪い(※$lenは変換後の長さとも言えるので見方の問題かも)
  4. '/' はシングルバイト文字なので mb_strlen(), mb_substr() ではなく strlen(), substr() でも可
  5. [mb_]strlen() や [mb_]substr() を使わずに
    if ('/' === substr($url, -1)) 
    {
      $config['base_url'] = substr($url, 0, -1);
    }
    のように書いてもよい
  6. 3.〜5. については、キャッシュされるのであれば正規表現を用いシンプルにしてはどうか

具体的には、

   protected function setOpenPNEConfiguration()
   {
     $opConfigCachePath = sfConfig::get('sf_cache_dir').DIRECTORY_SEPARATOR.'config'.DIRECTORY_SEPARATOR.'OpenPNE.yml.php';
     if (is_readable($opConfigCachePath))
     {
       $config = (array)include($opConfigCachePath);
     }
     else
     {
       $path = OPENPNE3_CONFIG_DIR.'/OpenPNE.yml';
       $config = sfYaml::load($path.'.sample');
       if (is_readable($path))
       {
         $config = array_merge($config, sfYaml::load($path));
       }

+      if (isset($config['base_url']))
+      {
+        $config['base_url'] = preg_replace('/\/$/', '', $config['base_url']);
+      }
+
       opToolkit::writeCacheFile($opConfigCachePath, '<?php return '.var_export($config, true).';');
     }
     $this->configureSessionStorage($config['session_storage']['name'], (array)$config['session_storage']['options']);
     unset($config['session_storage']);

-    $url = $config['base_url'];
-    $len = mb_strlen($url) - 1;
-    if (0 < $len && '/' == mb_substr($url, $len, 1))
-    {
-      $config['base_url'] = mb_substr($url, 0, $len);
-    }

     foreach ($config as $key => $value)
     {
       sfConfig::set('op_'.$key, $value);
     }
   }
のような対応が望ましいと思われます。
(但し、既にキャッシュされているものについては適用されないので本修正の反映後には symfony cc が必要です)

#15 Updated by Masato Nagasawa over 8 years ago

  • Status changed from Rejected(差し戻し) to Pending Review(レビュー待ち)

#16 Updated by Masato Nagasawa over 8 years ago

更新履歴 6f5cf83d865ea62d93004552951d4402677dd184 で適用されました。
更新履歴 eab44bc799b72239a0a1d08480067962f64d2864 で適用されました。

#17 Updated by Masato Nagasawa over 8 years ago

一度修正を取り消した上で改善案と同様の修正を行いました。

#18 Updated by Naoya Tozuka over 8 years ago

  • Status changed from Pending Review(レビュー待ち) to Pending Testing(テスト待ち)
  • % Done changed from 50 to 70

OKです。修正どうもありがとうございました。

#19 Updated by Fumie Toyooka almost 8 years ago

  • Status changed from Pending Testing(テスト待ち) to Fixed(完了)
  • % Done changed from 70 to 100

テストOKです

#20 Updated by kaoru n almost 4 years ago

  • 3.8 で発生するか set to Unknown (未調査)

Also available in: Atom PDF