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

Remove default serialization of all view vars. #569

Merged
merged 1 commit into from Dec 6, 2017

Conversation

Projects
None yet
8 participants
@ADmad
Copy link
Member

ADmad commented Dec 4, 2017

This is an unsafe default and a comment warning is not enough.

Remove default serialization of all view vars.
This is an unsafe default and a comment warning is not enough.
@dereuromark

This comment has been minimized.

Copy link
Member

dereuromark commented Dec 4, 2017

As alternative, how about putting it into a protected method and then using it commented out?
This way you can activate with a single line commenting in.

@markstory

This comment has been minimized.

Copy link
Member

markstory commented Dec 5, 2017

@chinpei215

This comment has been minimized.

Copy link
Member

chinpei215 commented Dec 5, 2017

I would suggest to stop loading RequestHandlerComponent by default. Even if we remove the problematic code, Cake3 applications would still leak sensitive information, as Bake generates $this->set('_serialize', ['users']); or something.

Also, I think it would be better to announce this issue officially, as almost all Cake3 applications would be affected by this issue. Though I have already announce this issue to people in #japanese channel on our Slack, I am not sure what is the best way to secure their applications. For now I have recommended to remove $this->loadCompnent('RequestHandler'); from their AppController.

@ADmad

This comment has been minimized.

Copy link
Member

ADmad commented Dec 5, 2017

@dereuromark We would still be providing a way to use the framework in an unsafe way so best to remove it.

@chinpei215 I am not in favor removing RequestHandlerComponent from being loaded by default. I am pretty sure it's used in most apps. While bake might be generating $this->set('_serialize', ['users']); is pretty evident that if you serialize $users all user data is gonna end up in the output unless you restrict the fields.

@inoas

This comment has been minimized.

Copy link
Contributor

inoas commented Dec 5, 2017

Also by default sensitive fields such as passwords are hidden by bakes defaults AFAIR.
But if it must be we can outcomment the $this->set('_serialize', ['users']); in bake instead of the RequestHandler?

@chinpei215

This comment has been minimized.

Copy link
Member

chinpei215 commented Dec 5, 2017

While bake might be generating $this->set('_serialize', ['users']); is pretty evident that if you serialize $users all user data is gonna end up in the output unless you restrict the fields.

I disagree. I think any beginners would not touch the code as they wouldn't understand what it means.

@markstory

This comment has been minimized.

Copy link
Member

markstory commented Dec 5, 2017

Removing RequestHandlerComponent by default will make building API's require a bit more work. I think making APIs easy to build should be something we enable as it is a requirement for new applications that use client side frameworks.

@chinpei215

This comment has been minimized.

Copy link
Member

chinpei215 commented Dec 6, 2017

Seriously? I don't think adding one line $this->loadComponent('RequestHandler') is a hard work.
Imagine if you have a users' profile page on your website. The page doesn't show you any sensitive information, such as email, family_name, given_name , address, and so on. It shows people nickname only. However, if you access the page with Accept: application/json header, you will see those sensitive information. It is a very bad behavior.
When we consider security issue, it's best to be on the safe side. When we compare 'adding a single $this->loadComponent('RequestHandler') (otherwise you cannot get json response)' with 'removing _serialize from your baked code (otherwise you will expose any sensitive information), the latter wins.
If you still have not changed your opinion, let's ask Mozilla Secure Open Source for their opinions. I think they overlooked this issue. But if they knew this issue, they would have marked this as critical. Because any websites made with Cake3 are possible to expose any sensitive information if the developers don't have enough knowledge about RequestHandler. I have never used it in 2.x. So if I made a website with Cake3, my website would have become vulnerable.

@inoas

This comment has been minimized.

Copy link
Contributor

inoas commented Dec 6, 2017

What about shipping a prefixed Api/ApiAppController in cakephp/app with RequestHandler and auto serialization and xml/json response only enabled?

@chinpei215

This comment has been minimized.

Copy link
Member

chinpei215 commented Dec 6, 2017

I wouldn't oppose it.

@markstory

This comment has been minimized.

Copy link
Member

markstory commented Dec 6, 2017

@chinpei215 Those are fair points, and you're right adding one component load is not hard.

@markstory markstory merged commit 6ca4561 into 3.next Dec 6, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@dereuromark dereuromark deleted the ADmad-patch-1 branch Dec 6, 2017

@lorenzo

This comment has been minimized.

Copy link
Member

lorenzo commented Dec 6, 2017

Maybe we should have a different component (or controller method) to enable automatic json views?

@chinpei215

This comment has been minimized.

Copy link
Member

chinpei215 commented Dec 6, 2017

It could be an another approach.

@saeideng

This comment has been minimized.

Copy link
Member

saeideng commented Dec 6, 2017

how happen if set $this->set('_serialize', false); here ?

@rchavik

This comment has been minimized.

Copy link
Member

rchavik commented Dec 8, 2017

Related #501

@markstory

This comment has been minimized.

Copy link
Member

markstory commented Dec 8, 2017

Backported to master in 84ec633

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