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

yii2-app-advanced - unnesessary meta tags in passwordResetToken html email #3358

Closed
SDKiller opened this issue May 5, 2014 · 8 comments
Closed
Assignees
Milestone

Comments

@SDKiller
Copy link
Contributor

SDKiller commented May 5, 2014

When email with password reset token is sent - csrf meta tags are included in head section of email html message:

<meta name="csrf-param" ...
<meta name="csrf-token" ...

That makes no sense since they are not used to validate anything - password reset link can be used standalone by simple copying it to browser address bar.

@samdark samdark added this to the 2.0 GA milestone May 5, 2014
@SDKiller
Copy link
Contributor Author

SDKiller commented May 5, 2014

I managed to fix this by adding

Yii::$app->getRequest()->enableCsrfValidation = false;

to \advanced\common\mail\passwordResetToken.php

But I am not sure - maybe it should be done on the level of yii\web\View ?

@lynicidn
Copy link
Contributor

lynicidn commented May 7, 2014

csrf for not Get requests. Reset link is Get

@SDKiller
Copy link
Contributor Author

SDKiller commented May 7, 2014

@lynicidn
I'm afraid you did not catch the point I was talking about.

The issue is that when rendering email html layout, yii\web\View adds csrf meta tags automatically.
It is possibly because enableCsrfValidation remains in request from the password reset form.
The same is with emails from contact form.
The only way to fix this right now is to explicitly declare

Yii::$app->getRequest()->enableCsrfValidation = false;

in email html layout.

@samdark
Copy link
Member

samdark commented May 7, 2014

Yes, it does that. Will fix it but since it doesn't cause any trouble it's low priority.

@klimov-paul
Copy link
Member

The valid fix for this will be removal direct coupling between yii'web\View and yii\web\Request at
https://github.com/yiisoft/yii2/blob/master/framework/web/View.php#L464

Request can add CSRF meta tags via View::registerMetaTag().
In order to prevent unnecessay instantiating of the 'view' component, we can use a 'beginPage' event, which can be added to the view config automatically at Request initialization.

@cebe
Copy link
Member

cebe commented Jun 6, 2014

👍

@klimov-paul
Copy link
Member

Crap, it seems I can not avoid instantiating of the 'view' component: there is no way to append event handler via config in safe way.
Following code works fine:

public function init()
    {
        parent::init();
        if ($this->enableCsrfValidation) {
            $handler = function ($event) {
                /* @var \yii\base\Event $event */
                $view = $event->sender;
                if ($view instanceof View) {
                    $view->registerMetaTag(['name' => 'csrf-param', 'content' => $this->csrfParam], __CLASS__ . '::csrfParam');
                    $view->registerMetaTag(['name' => 'csrf-token', 'content' => $this->getCsrfToken()], __CLASS__ . '::csrfToken');
                }
            };
            if (Yii::$app->has('view', true)) {
                Yii::$app->getView()->on('beginPage', $handler);
            } else {
                $components = Yii::$app->getComponents();
                $view = $components['view'];
                $view['on beginPage'] = $handler;
                Yii::$app->set('view', $view);
            }
        }
    }

However if there is already a key 'on beginPage' in the 'view' config, there is no proper way to keep it.
Besides code becomes too heavy.

@klimov-paul
Copy link
Member

Yet another option will be just placing following code at the top of the layout file:

<?php
$request = Yii::$app->request;
if ($request->enableCsrfValidation) {
    $this->registerMetaTag(['name' => 'csrf-param', 'content' => $request->csrfParam]);
    $this->registerMetaTag(['name' => 'csrf-token', 'content' => $request->getCsrfToken()]);
}

But in this case it would not be automatic and developer will be responsible for it, so BC will be broken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment