Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ADMIN_DIR のチェック強化 #426

Merged
merged 2 commits into from Feb 16, 2021
Merged

Conversation

nanasess
Copy link
Contributor

  • ADMIN_DIR 以外から LC_Page_Admin 配下へリクエストされた場合は拒否する
  • E2Eテストは別途作成します

@nobuhiko
Copy link
Contributor

プラグインが影響でないですかね?

@nanasess
Copy link
Contributor Author

@nobuhiko うーん、そもそも ADMIN_DIR 以外からリクエストを受信している場合は、認証をスルーしている可能性が高いので危険な状態な可能性があると思うんですが、いかがでしょう?

もしくは、以下のような感じの方が安全ですかね

         if (stripos($_SERVER['REQUEST_URI'], rtrim(ROOT_URLPATH.ADMIN_DIR, '/')) === false) {
             // ADMIN_DIR 以外からのリクエストは認証を要求する
             SC_Utils_Ex::sfIsSuccess(new SC_Session_Ex());
         }

@nobuhiko
Copy link
Contributor

@nanasess LC_Page_Admin 配下は常に認証を求められてるもんだと思ってましたが、本家はしてないのか・・ ADMIN_DIR に関わらず認証はしたほうがいいんじゃないかと思ったり。

@nanasess
Copy link
Contributor Author

@nobuhiko ADMIN_DIR/require_base.php で認証かけているので、通常はすべて認証かかってるんです。
過去に LC_Page_Admin を継承せずに管理画面のページを作成して、認証をスルーしてしまう事故が発生していたのでこうなってるんですが😥

@nobuhiko
Copy link
Contributor

@nobuhiko うーん、そもそも ADMIN_DIR 以外からリクエストを受信している場合は、認証をスルーしている可能性が高いので危険な状態な可能性があると思うんですが、いかがでしょう?

もしくは、以下のような感じの方が安全ですかね

         if (stripos($_SERVER['REQUEST_URI'], rtrim(ROOT_URLPATH.ADMIN_DIR, '/')) === false) {
             // ADMIN_DIR 以外からのリクエストは認証を要求する
             SC_Utils_Ex::sfIsSuccess(new SC_Session_Ex());
         }

こっちのほうがいいかなーと思います

@nanasess
Copy link
Contributor Author

@nobuhiko ありがとうございます。修正しました

@nobuhiko
Copy link
Contributor

@nanasess ありがとうございます!

@okazy
Copy link
Contributor

okazy commented Feb 16, 2021

Windowsのテストは他のプルリクで対応済みのため取り込みます。
対応ありがとうございました。

@okazy okazy merged commit 3695516 into EC-CUBE:master Feb 16, 2021
@nanasess nanasess deleted the improve-admin branch October 3, 2022 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants