Skip to content

feat: Add security headers #58

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

Merged
merged 3 commits into from
Nov 14, 2018
Merged

feat: Add security headers #58

merged 3 commits into from
Nov 14, 2018

Conversation

jrconlin
Copy link
Member

Issue #19

@jrconlin jrconlin requested review from pjenvey and rfk November 14, 2018 03:37
fn cspreport(
req: &HttpRequest<session::WsChannelSessionState>,
) -> Box<Future<Item = HttpResponse, Error = Error>> {
use std::str;
Copy link
Member

Choose a reason for hiding this comment

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

nit: move this use to the top

Copy link
Member Author

Choose a reason for hiding this comment

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

Heh, I was honestly wondering if I should put all the SecOps calls into their own file so that we could drop them in place in the future.

I think if I get to that point, though, it's probably worth just packaging them up properly.

@@ -94,6 +98,7 @@ fn channel_route(req: &HttpRequest<session::WsChannelSessionState>) -> Result<Ht
"remote_ip" => &meta_info.remote
);

// Cannot apply headers here.
Copy link
Member

Choose a reason for hiding this comment

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

ws::start returns an HttpResponse so you probably could? -- assuming you'd even want to. I don't think the foxsec headers are terribly important there but probably wouldn't hurt the ws handshake either.

If you're fine w/ injecting those headers into every response, actix_web's DefaultHeaders middleware provides an easier way (this fn add_headers works for me but if applicable, the middleware might be a nice refactor for later)

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking into the code, I don't think I can, or at least, not without some weird surgery. The problem is that ws::start generates the HttpResponse and then posts it independently. So I can't append the headers before it's sent out.

I didn't think about the DefaultHeader middleware, and yeah, these are fairly universal. I'll do the refactor later since I want to address a few things about that. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

#59

@jrconlin jrconlin merged commit 8a70fff into master Nov 14, 2018
@pjenvey pjenvey deleted the feat/19 branch November 14, 2018 22:15
@pjenvey pjenvey restored the feat/19 branch November 14, 2018 22:15
@jrconlin jrconlin deleted the feat/19 branch December 18, 2018 17:07
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.

2 participants