Protecting the ACP Controllers

[Daniel F]

We have removed the session id from the ACP urls, meaning that you have to make sure to use CSRF protections in all your methods which change a state!
We also advice to use \IPS\Request::i()->confirmedDelete() in all your methods where you're deleting data!

 

One of the few examples from the MP reviews is this code:

    protected function approve()
    {
        if ( !\IPS\Request::i()->id )
        {
            \IPS\Output::i()->error( 'node_error', '2myApp', 404, '' );
        }

        $a = \IPS\myApp\Item::load( \IPS\Request::i()->id );

        $a->open		= 1;
        $a->save();

        /* Log History */
        \IPS\myApp\History::addEntry( 'foo', x, \IPS\Member::loggedIn()->name );
        \IPS\Output::i()->redirect( \IPS\Http\Url::internal( '.' ));
    }

 

Once an administrator with a valid ACP session calls the url, he would automatically approve the advert, meaning that any member could post an encoded URL (or use one of the other methods which I'm not going to mention here ) to lead the admin into the trap!

 

To prevent that this happens, you have to utilise the CSRF key and to check in your method if the key is valid, before anything else is done!

 

[DawPi]

It should be enough?

 

/**
 * Run conversion
 */
class _convert extends \IPS\Dispatcher\Controller
{
	/**
	 * @brief	Has been CSRF-protected
	 */
	public static $csrfProtected = TRUE;

 

[Ryan Ashbrook]

3 minutes ago, DawPi said:

It should be enough?

 

/**
 * Run conversion
 */
class _convert extends \IPS\Dispatcher\Controller
{
	/**
	 * @brief	Has been CSRF-protected
	 */
	public static $csrfProtected = TRUE;

 

You need to ensure you are actively using the CSRF Key and checking it where appropriate, just like you would do on the front-end.

Once done, then yes, you just add that property and it will whitelist the controller.

[DawPi]

Just now, Ryan Ashbrook said:

and checking it where appropriate

You mean this in all methods?

 

	/**
	 * Remove Converted Data
	 *
	 * @return	void
	 */
	protected function emptyData()
	{
		\IPS\Session::i()->csrfCheck();

 

[Ryan Ashbrook]

Just now, DawPi said:

You mean this in all methods?

 

	/**
	 * Remove Converted Data
	 *
	 * @return	void
	 */
	protected function emptyData()
	{
		\IPS\Session::i()->csrfCheck();

 

Any methods that alter the state of something. So, saving settings (which happens already if you are using \IPS\Helpers\Form), deleting, editing, etc. etc.

So, for instance, on a delete button you would do:

array(
	'title'		=> 'delete',
	'icon'		=> 'trash',
	'link'		=> \IPS\Http\Url::internal( "app=myapp&module=mymodule&controller=mycontroller&do=delete" )->csrf(),
	'data'		=> array( 'delete' => '' )
)

Then your delete method would do:

public function delete()
{
	\IPS\Request::i()->confirmedDelete();

	// ... etc ...
}

For any other method, you would do:

\IPS\Http\Url::internal( "app=myapp&module=mymodule&controller=mycontroller&do=somethingThatChangesSomething" )->csrf()

Then:

public function somethingThatChangesSomething()
{
	\IPS\Session::i()->csrfCheck();

	// ... etc ...
}

Most of the framework will largely handle this, however you should be mindful. A good example is flagging a member as a spammer in the admin members controller. This does not use a a form, or anything that would automatically apply a CSRF check, however it's an action that alters the state of the site (ban the user, hide their content, etc.). Therefore, it needs a CSRF check to make sure it was an action the administrator actually wanted to do.

[InvisionHQ]

Unfortunately, it's the same thing every time.

Here, too, we are forced to operate without a minimum of documentation.

So we proceed by trial and error and we're also wasting your time.

My first file released as an update for 4.5 was clearly rejected and with a lot of probability inspired Daniel to do this post.

This is frustrating not so much because of the rejection but because every time you make a mistake, this rule comes to mind:

Quote

Rejection

If your resource has been unfortunately rejected, we will respond with the reasons for rejection via a ticket. If your resource is rejected twice because of significant issues, there will be a period of 6 weeks before any changes will be reviewed further. This time will allow you to review your submission and correct any issues with it. Should further reviews be rejected we may not be able to accept your submission at all. Submissions with generally low code quality may also be rejected.

Turning something that's supposed to be a pleasure into Russian roulette.

I understand everything, improve the third party code, provide a more qualified mp, but at the same time I hope there is a minimum of flexibility at this stage.

I say this without wanting to be polemical. Because the alternative is to delay the apps update as much as possible and wait for others to make mistakes so that we have other posts like this one that better explain what to do and what not to do.

[onlyME]

I got -200 error when uploading file if using \IPS\Session::i()->csrfCheck()

public function foo()
{
  \IPS\Session::i()->csrfCheck();
  $form = new \IPS\Helpers\Form;
  $form->add( new \IPS\Helpers\Form\Upload(....) );
  ....
}

 

[Mark]

48 minutes ago, onlyME said:

I got -200 error when uploading file if using \IPS\Session::i()->csrfCheck()

public function foo()
{
  \IPS\Session::i()->csrfCheck();
  $form = new \IPS\Helpers\Form;
  $form->add( new \IPS\Helpers\Form\Upload(....) );
  ....
}

 

The form helper already provides CSRF-protection if every change of state is contained within the if ( $values = $form->values() ) { ... }

[Stuart Silvester]

4 hours ago, InvisionHQ said:

Unfortunately, it's the same thing every time.

Here, too, we are forced to operate without a minimum of documentation.

So we proceed by trial and error and we're also wasting your time.

To be fair, this was detailed in May, CSRF protection works exactly as it does on the front-end, there are not any functional changes to how you implement it.

 

[InvisionHQ]

I don't want to insist on that, even if the posts following mine prove otherwise.

Only trying one upgrade I find changes on:

• _createMember -> add $postBeforeRegister, $form
• public static $csrfProtected = TRUE on admin modules
• function contentImage add parameter $ignorePermissions = FALSE
• (today I learn) \IPS\Session::i()->csrfCheck() instead check on request ID
• (now I learn) that the form helper already provides CSRF-protection if every change of state is contained within the if ( $values = $form->values() ) { ... }

I assume from the non-answer that there will be no elasticity in this stage.

Then my next upload is an ALL IN.

[Stuart Silvester]

1 hour ago, InvisionHQ said:

I don't want to insist on that, even if the posts following mine prove otherwise.

Only trying one upgrade I find changes on:

• _createMember -> add $postBeforeRegister, $form
• public static $csrfProtected = TRUE on admin modules
• function contentImage add parameter $ignorePermissions = FALSE
• (today I learn) \IPS\Session::i()->csrfCheck() instead check on request ID
• (now I learn) that the form helper already provides CSRF-protection if every change of state is contained within the if ( $values = $form->values() ) { ... }

I assume from the non-answer that there will be no elasticity in this stage.

Then my next upload is an ALL IN.

We haven't had to enforce it yet, we're still human and realize there's a lot of things that have changed for 4.5. We just want to make sure that we're not constantly rejecting things for the same reasons.

[The Old Man]

On 7/28/2020 at 7:45 AM, InvisionHQ said:

there will be a period of 6 weeks before any changes will be reviewed further.

Sweet Jebus. That's off putting for inexperienced new developers.

[Midnight Modding]

On 7/28/2020 at 4:01 AM, Mark said:

The form helper already provides CSRF-protection if every change of state is contained within the if ( $values = $form->values() ) { ... }

I think confirmedDelete() does too, right? I added in the csrf check a lot of places last year, only to then go through IPB files and figure out it is actually already done most places I change a state in.

[Mark]

1 minute ago, Midnight Modding said:

I think confirmedDelete() does too, right? I added in the csrf check a lot of places last year, only to then go through IPB files and figure out it is actually already done most places I change a state in.

Correct