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

Harden Headers #2721

Merged
merged 12 commits into from
May 3, 2021
Merged

Harden Headers #2721

merged 12 commits into from
May 3, 2021

Conversation

tankerkiller125
Copy link
Contributor

@tankerkiller125 tankerkiller125 commented Mar 20, 2021

**Partial Fix #353 **

Changes proposed in this pull request:

  • Adds the Referer Policy header (with config option)
  • Adds Xss Protection header
  • Adds Content Type header

Reviewers should focus on:

  • Does the config option for the Referer policy make sense? Or should we use some sort of constants?
  • We probably need to add some test for this but I'm unsure

Confirmed

  • Backend changes: tests are green (run composer test).

Additional PRs Required

Copy link
Sponsor Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

This definitely should have tests. We could add a header subfolder to the integration test directory?

Or alternatively, construct a fake middleware pipe with a fake handler, and use unit tests.

src/Admin/AdminServiceProvider.php Show resolved Hide resolved
src/Http/Middleware/ReferrerPolicyHeader.php Outdated Show resolved Hide resolved
src/Http/Middleware/XssProtectionHeader.php Outdated Show resolved Hide resolved
Copy link
Sponsor Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

Stylistic comment. Also would be good to see some basic integration tests (ping /, check for headers)

src/Http/Middleware/ReferrerPolicyHeader.php Outdated Show resolved Hide resolved
@tankerkiller125
Copy link
Contributor Author

This definitely should have tests. We could add a header subfolder to the integration test directory?

I'm thinking maybe a middleware subfolder, that way in the future we can test other middleware?

@tankerkiller125
Copy link
Contributor Author

As a note I'm not sure why the test are failing, it seems to be that it's hitting rate limits or something? Everything is working locally 100% just fine and all test are passing on my machine.

Copy link
Member

@davwheat davwheat left a comment

Choose a reason for hiding this comment

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

All looks good to me from a headers point of view!

src/Install/Steps/StoreConfig.php Show resolved Hide resolved
src/Install/Steps/StoreConfig.php Outdated Show resolved Hide resolved
@askvortsov1
Copy link
Sponsor Member

As a note I'm not sure why the test are failing, it seems to be that it's hitting rate limits or something?

FIgured it out. The setting method of test case runs BEFORE transactions are started, so those changes don't go into transactions. Opened a PR: flarum/testing#11

composer.json Outdated Show resolved Hide resolved
@askvortsov1
Copy link
Sponsor Member

askvortsov1 commented Apr 29, 2021

This should not be part of this PR.

Unfortunately it probably should, as tests break without it. There's a bunch of stuff that shouldn't though. From chat:

I think it might be easiest here to:

  • start a new branch from master
  • cherry-pick over relevant commits
  • run tests
  • force update current branch to this new branch

@tankerkiller125
Copy link
Contributor Author

I've re-based correctly this time, but it appears that test unrelated to this PR are failing again. I'm not entirely sure what's going on here. Maybe @askvortsov1 can take a look when he gets a chance?

@davwheat davwheat self-requested a review May 2, 2021 23:01
Copy link
Sponsor Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

Fixed up tests, moved those commits out to master to keep this PR clean. @davwheat did you self-request review because you had some concerns?

src/Install/Steps/StoreConfig.php Outdated Show resolved Hide resolved
@davwheat
Copy link
Member

davwheat commented May 3, 2021

@tankerkiller125 No, sorry, just wanted to clear my review as I hadn't tested it locally after the rebase and stuff.

No real changes have taken place, and Sasha has no issues with it, so it's good with me.

@tankerkiller125 tankerkiller125 merged commit 7eea247 into master May 3, 2021
@tankerkiller125 tankerkiller125 deleted the mk/353-harden-headers branch May 3, 2021 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hardening HTTP Headers
4 participants