Jump to content

Community

Protecting the ACP Controllers


Daniel F
 Share

Recommended Posts

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!

 

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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() ) { ... }

Link to comment
Share on other sites

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.

 

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

  • 3 weeks later...
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.

Link to comment
Share on other sites

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

Link to comment
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.
Note: Your post will require moderator approval before it will be visible.

Guest
Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

Loading...
 Share

  • Recently Browsing   0 members

    No registered users viewing this page.

×
×
  • Create New...

Important Information

We use technologies, such as cookies, to customise content and advertising, to provide social media features and to analyse traffic to the site. We also share information about your use of our site with our trusted social media, advertising and analytics partners. See more about cookies and our Privacy Policy