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

[Proposal] By default set the X-Frame-Options HTTP header to SAMEORIGIN #1725

Closed
ianlandsman opened this Issue Jun 24, 2013 · 46 comments

Comments

Projects
None yet
@ianlandsman

ianlandsman commented Jun 24, 2013

This prevents adding a site sending the header from being included in an iframe which in turn prevents click hijacking. At this point iframes are pretty rare as a standard expected usage case and so defaulting to not allowing them seems to be a more secure way to go. It could always be enabled if needed in a particular case.

@bencorlett

This comment has been minimized.

Contributor

bencorlett commented Jun 24, 2013

Sounds good to me.

@rtablada

This comment has been minimized.

Contributor

rtablada commented Jun 24, 2013

This would be cool.

@rtablada

This comment has been minimized.

Contributor

rtablada commented Jun 24, 2013

Or have a config in app config that is like "allow_iframe"?

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Jun 24, 2013

I can gurantee you there won't be a config option for this :)

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Jun 24, 2013

I'm still researching the entire issue as a whole though

@ianlandsman

This comment has been minimized.

ianlandsman commented Jun 24, 2013

Yeah I'm not for a config option. It should just be default and override if needed. To be more clear, by setting sameorigin it can still be included in an iframe from the same domain so it's only limiting third party inclusion.

On Jun 24, 2013, at 12:28 AM, Taylor Otwell notifications@github.com wrote:

I'm still researching the entire issue as a whole though


Reply to this email directly or view it on GitHub.

@rtablada

This comment has been minimized.

Contributor

rtablada commented Jun 24, 2013

Yeah. I think it's good by default, I'm just thinking that it should be well documented where to go in the shell to modify that behavior. Whether a config, or you have to do something like Request::setHeader(...)? 

Sent from Mailbox for iPhone

On Mon, Jun 24, 2013 at 5:23 AM, Ian Landsman notifications@github.com
wrote:

Yeah I'm not for a config option. It should just be default and override if needed. To be more clear, by setting sameorigin it can still be included in an iframe from the same domain so it's only limiting third party inclusion.
On Jun 24, 2013, at 12:28 AM, Taylor Otwell notifications@github.com wrote:

I'm still researching the entire issue as a whole though

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub:
#1725 (comment)

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Nov 1, 2013

Done.

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Nov 1, 2013

If you're on 4.1 and don't want it add this to start file:

App::forgetMiddleware('Illuminate\Http\FrameGuard');
@Fractaliste

This comment has been minimized.

Fractaliste commented Nov 15, 2013

Is this functionality documented ? For instance if I want to use an other value.

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Nov 15, 2013

Not yet. It's 4.1 functionality. But it will be.


Sent from Mailbox for iPhone

On Fri, Nov 15, 2013 at 12:26 PM, Fractaliste notifications@github.com
wrote:

Is this functionality documented ?

Reply to this email directly or view it on GitHub:
#1725 (comment)

@BastianHofmann

This comment has been minimized.

Contributor

BastianHofmann commented Nov 19, 2013

Is there any way to remove the X-Frame-Options header for a single route? During an after filter the X-Frame-Options header is not yet set and therefore can't be removed.
As you can see, the header is set, after the application handles the request.

$response = $this->app->handle($request, $type, $catch);

$response->headers->set('X-Frame-Options', 'SAMEORIGIN');

Illuminate\Http\FrameGuard

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Nov 19, 2013

I can probably tweak it to check for the existence of the header. If it has already been set higher in the stack we shouldn't override. Good?

@BastianHofmann

This comment has been minimized.

Contributor

BastianHofmann commented Nov 19, 2013

Hmm... It wouldn't be possible to completely remove the header then, though? Just override it...

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Nov 19, 2013

There is an app forgetMiddleware function


Sent from Mailbox for iPhone

On Tue, Nov 19, 2013 at 11:54 AM, BastianHofmann notifications@github.com
wrote:

Hmm... It wouldn't be possible to completely remove the header then, though? Just override it...

Reply to this email directly or view it on GitHub:
#1725 (comment)

@BastianHofmann

This comment has been minimized.

Contributor

BastianHofmann commented Nov 19, 2013

Correct, but it can only be used before the app handles the request i.e. in the start file.
I'm looking to remove the header for a single route i.e. in a filter or simular.
As you said, checking if the header is already set, would be fairly easy, but that's not how x-frame-options works. To grant iframe access for everyone the header should not be present at all.

However for my usecase I found a work around which acctually makes my app more secure.

My Conclusion:
Check if the header has already been set in the FrameGuard. Change the FrameGuard whem a suitable solution is found.

BastianHofmann added a commit to BastianHofmann/framework that referenced this issue Nov 20, 2013

@Fractaliste

This comment has been minimized.

Fractaliste commented Nov 30, 2013

Why not to add an option into one of config files and check it in FrameGuard ?

@BastianHofmann

This comment has been minimized.

Contributor

BastianHofmann commented Nov 30, 2013

I can gurantee you there won't be a config option for this :)

That was discussed in this thread already.

@Rolice

This comment has been minimized.

Rolice commented Dec 15, 2013

After wasting a day to find the issue I had is here (litterally but finding the problem point in the code), this FrameGuard is not helping at all, since all Facebook app developers will have blank apps. FrameGuard is executed somewhere in the actual sending of response, where the user has no control. There is no option for overriding it, tried in App::after, but instead of overriding FrameGuard value, it overrides mine.

I suggest to remove it or make it configurable and mention it in the docs. Whole day searching in Google for hint and there were 0 useful topics and almost made me drop laravel for Facebook applications, where I find laravel very nice.

@chadwick37

This comment has been minimized.

chadwick37 commented Dec 18, 2013

I agree with Rolice. Wish I had these hours back that this issue stole from me.

@Guillaumez

This comment has been minimized.

Guillaumez commented Dec 19, 2013

Try $app->forgetMiddleware('Illuminate\Http\FrameGuard'); in the start.php file. It worked nice for me.

@Rolice

This comment has been minimized.

Rolice commented Dec 20, 2013

Thanks. Its a clean solution @Guillaumez I shared it in laravel forum topic I had opened before writing here: http://forums.laravel.io/viewtopic.php?pid=65620#p65620

It seems to work with forget middleware, ok this is solution, but we have to put it in somewhere in the docs, how I could relate as new user empty Facebook page with FrameGruard, especially with forgetMiddleware? Isn't it reversed logic, I expect to add/enable Middleware, not to forget it? I expect to write code, not to disable parts of it (talking for extra functionality which optimizes my code to run)? Its for me like:

<?php
ob_stop('ob_gzunziphandler'); // stops the output buffring and removes the "default" gzip compression
session_stop(); // disables the automatically started session
...

...which is not constructive logic, but kind of opposite. I am trying to look from another view point. It is cool to have many/all things loaded, but there is also performance cost. I am wondering how many points like that exist currently in the version I have? Do I need them all?

An idea is to move such optional executable functionality as an extension or something which is expected to be used only when needed (I don't instantiate FrameGuard, neither tell it what to do - it is self operating unit) or at least leave extra units disabled by default.

However, the most important thing is to document that and other "issues" like it - to make laravel more accessible and easier to use. I can try to describe it somewhere.

Correct me if I am wrong. I may also fork and apply solution to this issue after that.

@bencorlett

This comment has been minimized.

Contributor

bencorlett commented Dec 20, 2013

Frameguard is useful for 99% of cases, not useful for 1%. (Figure pulled out of my ass).

Because of this, it makes sense to suit the use of the majority.

Sent from my iPad
Please excuse my brevity

On 21 Dec 2013, at 8:30 am, Lyubomir Gardev notifications@github.com wrote:

Thanks. Its a clean solution.

It seems to work with forget middleware, ok this is solution, but we have to put it in somewhere in the docs, how I could relate as new user empty Facebook page with FrameGruard, especially with forgetMiddleware? Isn't it reversed logic, I expect to add/enable Middleware, not to forget it? I expect to write code, not to disable parts of it (talking for extra functionality which optimizes my code to run)? Its for me like:

@Rolice

This comment has been minimized.

Rolice commented Jan 17, 2014

Thanks for your advice, @bencorlett. Still, an article put somewhere in the docs to mention potential problems for cross-domain communication would be nice. I have to dig around that.

@shealan

This comment has been minimized.

shealan commented Jan 21, 2014

Is there no way to apply this override on a view by view basis? It flat out kills all Facebook page apps so has to be disabled to avoid blank pages. However, I assume it is useful generally so it would be good to keep it elsewhere.

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Jan 21, 2014

I might just pull this out. It seems to be nothing but trouble.

@ianlandsman

This comment has been minimized.

ianlandsman commented Jan 21, 2014

I still think this doesn't impact 99.9999% of sites and adds security for them with the tradeoff of people building Facebook apps have to set one option.

@Rolice

This comment has been minimized.

Rolice commented Jan 21, 2014

I dont know how many people affects, but surely those who develop cross-domain apps. It is useful, but the lack of control make it pain in the ass. If I can turn it off/on or set the value of DENY|SAMEORIGIN|ALLOW-FROM it will become shining diamond no matter if it is enabled by default (should be mentioned somewhere in the docs).

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Jan 21, 2014

I still wonder if this is better accomplished by an after filter:

App::after(function($route, $request, $response) {
    if ($request->is('whatever/*')) {
        $response->header('whatever', 'whatever');
    }
});
@Rolice

This comment has been minimized.

Rolice commented Jan 21, 2014

Well, for one is better that the developer has access to that code and can adapt it.

@Rolice

This comment has been minimized.

Rolice commented Jan 21, 2014

Tried it some time ago and it did not helped. In my case (then Laravel 4.0.x) FrameGuard got executed after that scope.

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Jan 21, 2014

Ah right, forgot about that. Yeah, this needs to be an after filter really.

@bencorlett

This comment has been minimized.

Contributor

bencorlett commented Jan 21, 2014

Totally agree T.
On 22 Jan 2014, at 7:31 am, Taylor Otwell notifications@github.com wrote:

Ah right, forgot about that. Yeah, this needs to be an after filter really.


Reply to this email directly or view it on GitHub.

@sudopeople

This comment has been minimized.

sudopeople commented Jan 25, 2014

@taylorotwell As a new user to Laravel, your after filter thought is pretty much how I expected to turn this off once I realized how it worked.

Originally though, I thought it would be slick if all the headers were present when I constructed my Response:

$response = Response::make('Laravel 4!');
$response->headers->unset('X-Frame-Options');//wishful thinking
return $response;

After Googling for a while I ended up here from a couple StackOverflow questions. It may be less than 1% but there's a handful of us looking for a proper solution. Importantly, I think most developers want FrameGuard on most of the time. In my case I just need to turn it off for a single controller method that acts like the Evernote Web Clipper - accessible from any page.

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Jan 28, 2014

Removing this by default in 4.2. Should be in an after filter - will leave FrameGuard class so people can add the middleware manually if they want.

@bencorlett

This comment has been minimized.

Contributor

bencorlett commented Jan 28, 2014

Manlove.
On 29 Jan 2014, at 9:39 am, Taylor Otwell notifications@github.com wrote:

Removing this by default in 4.2. Should be in an after filter - will leave FrameGuard class so people can add the middleware manually if they want.


Reply to this email directly or view it on GitHub.

@sercanvirlan

This comment has been minimized.

sercanvirlan commented Feb 18, 2014

this wasted me hours, if you want to build a facebook app you will come here, i wish you found this page early then me :)

@Rolice

This comment has been minimized.

Rolice commented Feb 19, 2014

I have added a topic in the forums and answered myself, so I hope it will help there. :)
It will be not automatically enabled and this problem will be solved in the new versions (4.2+).

@paulrose

This comment has been minimized.

Contributor

paulrose commented Mar 18, 2014

This is a major back step. Who decided that no one uses the iframe tag anymore? Has it been deprecated without the rest of the internet being informed?

@christoferd

This comment has been minimized.

christoferd commented Mar 28, 2014

This has wasted me a lot of time also.
Seriously, servers have cross domain protection.
Apache htaccess, etc. Why inject it deep in the core of a framework is beyond me. Where is all the help for this.
Laravel forum has no results for "Header Access-Control-Allow-Origin", weird.

@crynobone

This comment has been minimized.

Contributor

crynobone commented Mar 28, 2014

To all that feel this had wasted their time, how about getting involved with any proposal/suggestion on Github so before any changes happen your concern has been noted.

Just look at the the discussion above, it took @taylorotwell 4 months to finally make this changes, within those 4 months there no objection. Why? Because does who were involve (responding to it) doesn't have any objection.

@Rolice

This comment has been minimized.

Rolice commented Mar 28, 2014

This is the topic I have started: http://forumsarchive.laravel.io/viewtopic.php?pid=64869
Might help the others who run on the same problem. I have changed the title to make it easy to find on different phrases regarding the same thing.

@bagnz0r

This comment has been minimized.

bagnz0r commented Apr 23, 2014

Hey "modern web" advocates. Guess what? Facebook canvas apps are embedded within iframe.
Headers like this ARE NOT for framework to handle, but the web server. This is bad practice.

@bencorlett

This comment has been minimized.

Contributor

bencorlett commented Apr 23, 2014

Can this thread please just die.

Sent from my iPhone
Please excuse my brevity

On 23 Apr 2014, at 10:11 pm, bagnz0r notifications@github.com wrote:

Hey "modern web" advocates. Guess what? Facebook canvas apps are embedded within iframe.


Reply to this email directly or view it on GitHub.

@bagnz0r

This comment has been minimized.

bagnz0r commented Apr 23, 2014

Sounds good to me.

@ianlandsman

This comment has been minimized.

ianlandsman commented Apr 23, 2014

This is my all time favorite thread

@laravel laravel locked and limited conversation to collaborators Sep 26, 2014

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