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

Add security headers to the default response #110

Merged
merged 11 commits into from Sep 19, 2018
Merged

Add security headers to the default response #110

merged 11 commits into from Sep 19, 2018

Conversation

@xh3n1
Copy link
Member

@xh3n1 xh3n1 commented Aug 28, 2018

Added security headers such as:

'X-Content-Type-Options' =>'nosniff',
'Content-Security-Policy' => "default-src 'none'",
 'X-Frame-Options' => 'DENY'

Fix #111

@xh3n1 xh3n1 force-pushed the set-header branch 2 times, most recently from 0727e1e to f05824c Aug 28, 2018
@xh3n1 xh3n1 requested a review from samtuke Aug 28, 2018
*
* @return Response
*/
public function createResponse(ViewHandler $handler, View $view, Request $request, $format)
Copy link
Collaborator

@oliverklee oliverklee Aug 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we cover this with an integration test?

Loading

Copy link
Collaborator

@oliverklee oliverklee Aug 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can have the string type hinting for the parameter.

Loading

composer.json Outdated
@@ -128,6 +128,9 @@
"messages": {
"Symfony\\Component\\HttpKernel\\Exception\\BadRequestHttpException": true
}
},
"service": {
"view_handler": "my.secure_view_handler"

Copy link
Collaborator

@oliverklee oliverklee Aug 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please let's drop this empty line.

Loading

@xh3n1
Copy link
Member Author

@xh3n1 xh3n1 commented Aug 30, 2018

@oliverklee Thank you for the review 😄
I made the changes, and actually wrote a system test for the SecuredViewHandler. Also fixed #112

Loading

@xh3n1 xh3n1 changed the title Add security headers to the default response Add security headers to the default response and run the system tests on Travis Aug 30, 2018
.travis.yml Outdated
- >
echo;
echo "Running the system tests";
vendor/bin/phpunit tests/System/;
Copy link
Collaborator

@oliverklee oliverklee Sep 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xh3n1 Thanks for the change.

I'd like to stick to the "one change/topic per PR" policy. Could you please move this to a separate PR? This will keep the history (and git blame) clean and allow for clean reverts if needed. Thanks!

Loading

Copy link
Member Author

@xh3n1 xh3n1 Sep 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oliverklee Thanks, I added it as a separate PR.

Loading

@xh3n1 xh3n1 changed the title Add security headers to the default response and run the system tests on Travis Add security headers to the default response Sep 10, 2018
Copy link
Collaborator

@oliverklee oliverklee left a comment

Looks generally good with minor things to polish.

Could you also please rebase from master and add a changelog entry? Thanks! 🍪

Loading

my.secure_view_handler:
parent: fos_rest.view_handler.default
calls:
- ['registerHandler', [ 'json', ["@my.secure_handler", 'createResponse'] ] ]
Copy link
Collaborator

@oliverklee oliverklee Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please let's unify this to always use single quotes unless the double quotes are required here.

Loading

{
$view->setHeaders([
'X-Content-Type-Options' => 'nosniff',
'Content-Security-Policy' => "default-src 'none'",
Copy link
Collaborator

@oliverklee oliverklee Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it also work to switch the single and double quotes here?

Loading

*/
class SecuredViewHandler
{

Copy link
Collaborator

@oliverklee oliverklee Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please let's drop the blank line here.

Loading


/**
* This class is used to add headers to the default response.
* @author Xheni Myrtaj <xheni@phplist.com> .
Copy link
Collaborator

@oliverklee oliverklee Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's polish this: :-)

Could you please add a blank line above the @author line and drop the period at the end of the @author line?

Loading

*/
public function createResponse(ViewHandler $handler, View $view, Request $request, string $format)
{
$view->setHeaders([
Copy link
Collaborator

@oliverklee oliverklee Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please let's put the [ and ] on separate lines. This will avoid formatting changes when using the PhpStorm autoformat or the php-cs-fixer.

Loading

@@ -0,0 +1,70 @@
<?php
declare(strict_types=1);
namespace PhpList\RestBundle\Tests\System\Controller;
Copy link
Collaborator

@oliverklee oliverklee Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To keep things consistent, please let's have a blank line between the declare line and the namespace section.

Loading

* Test for security headers
* @author Xheni Myrtaj <xheni@phplist.com>
*
*
Copy link
Collaborator

@oliverklee oliverklee Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please let's remove the blank lines.

Loading


/**
* Test for security headers
* @author Xheni Myrtaj <xheni@phplist.com>
Copy link
Collaborator

@oliverklee oliverklee Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency, please let's have a blank line above the @author line.

Loading

*/
class SecuredViewHandlerTest extends TestCase
{

Copy link
Collaborator

@oliverklee oliverklee Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please let's drop the blank line. (In general, there should be no blank line below a { or above a }.)

Loading

$expectedHeaders = [
'X-Content-Type-Options' => 'nosniff',
'Content-Security-Policy' => "default-src 'none'",
'X-Frame-Options' => 'DENY'
Copy link
Collaborator

@oliverklee oliverklee Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please let's have a comma after the last array element.

Loading

xh3n1 added 7 commits Sep 12, 2018
Signed-off-by: Xheni Myrtaj <myrtajxheni@gmail.com>
Signed-off-by: Xheni Myrtaj <myrtajxheni@gmail.com>
Signed-off-by: Xheni Myrtaj <myrtajxheni@gmail.com>
Signed-off-by: Xheni Myrtaj <myrtajxheni@gmail.com>
Signed-off-by: Xheni Myrtaj <myrtajxheni@gmail.com>
Signed-off-by: Xheni Myrtaj <myrtajxheni@gmail.com>
Signed-off-by: Xheni Myrtaj <myrtajxheni@gmail.com>
@xh3n1
Copy link
Member Author

@xh3n1 xh3n1 commented Sep 12, 2018

Thanks for the review @oliverklee I applied the requested coding style and rebased to master. I hope that I didn't miss anything. Just a question about changelog.md, under which release should I add the changes?

Loading

Signed-off-by: Xheni Myrtaj <myrtajxheni@gmail.com>
@oliverklee
Copy link
Collaborator

@oliverklee oliverklee commented Sep 12, 2018

Just a question about changelog.md, under which release should I add the changes?

Under "x.y.z (next release)", please.

Loading

*
* @return Response
*/
public function createResponse(ViewHandler $handler, View $view, Request $request, string $format)
Copy link
Collaborator

@oliverklee oliverklee Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it work to have Response as a return type declaration?

Loading

Copy link
Member Author

@xh3n1 xh3n1 Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it works.

Loading

@xh3n1
Copy link
Member Author

@xh3n1 xh3n1 commented Sep 12, 2018

ok thanks @oliverklee , I will create another PR for that.

Loading

Signed-off-by: Xheni Myrtaj <myrtajxheni@gmail.com>
@oliverklee
Copy link
Collaborator

@oliverklee oliverklee commented Sep 12, 2018

I will create another PR for that.

Optimally, the changelog entry for a PR should be part of the same PR.

Loading

src/ViewHandler/SecuredViewHandler.php Outdated Show resolved Hide resolved
Loading
Signed-off-by: Xheni Myrtaj <myrtajxheni@gmail.com>
Copy link
Collaborator

@oliverklee oliverklee left a comment

Beautiful!

Loading

@oliverklee oliverklee merged commit c8e3636 into master Sep 19, 2018
3 checks passed
Loading
@oliverklee oliverklee deleted the set-header branch Sep 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants