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

USH-015 — Missing X-XSS-Protection HTTP Header #1610

Closed
2 tasks
willdoran opened this issue Mar 6, 2017 · 17 comments · May be fixed by ushahidi/platform-client#1262
Closed
2 tasks

USH-015 — Missing X-XSS-Protection HTTP Header #1610

willdoran opened this issue Mar 6, 2017 · 17 comments · May be fixed by ushahidi/platform-client#1262
Labels
Cleanup/Done? Codebase: API Indicates issue work will be in API P1 - Immediate Theme: Release Bundling Issues that affect the outputs of the release creation process Theme: SaaS Theme: Security

Comments

@willdoran
Copy link
Contributor

willdoran commented Mar 6, 2017

Expected behaviour

  • Add the HTTP header in the response as: X-XSS-Protection: 1; mode=block

Actual behaviour

  • The X-XSS-Protection HTTP Header is not set by the webserver. This header enables the Cross-Site Scripting (XSS) filter built into most recent web browser.

Steps to reproduce the behaviour/error

Acceptance criteria:

  • Add documentation for OSS deployers
  • The server response should have a X-XSS-Protection header setup as: X-XSS-Protection: 1; mode=block

Aha! Link: https://ushahiditeam.aha.io/features/PROD-26

@rjmackay
Copy link
Contributor

rjmackay commented Jun 7, 2017

This should be a simple fix.

For client: add this header via nginx config
For cloud ui: add the header via middleware

@rowasc
Copy link
Contributor

rowasc commented Jun 14, 2018

@willdoran is this in the correct column? Are we blocked?

@willdoran
Copy link
Contributor Author

@rowasc I'm not 100% sure, I don't think it should be in In Development, I'm moving it to ready for spec. I believe the solution for this is to change our nginx config and add some docs for OSS users.

@rjmackay
Copy link
Contributor

@rowasc @willdoran I don't think this bears any further speccing.
Are we happy to approve this as-is?

@rowasc
Copy link
Contributor

rowasc commented Jun 25, 2018

I think so. Not much to spec here.
I'd say APPROVED with 2 as is

@rjmackay
Copy link
Contributor

rjmackay commented Jul 6, 2018

I've added this to platform-cloud-ansible. It's only deployed to v3-qa right now. We can deploy to other environments by running
ansible-playbook -i hosts/ENV cloud-interface.yml platform-client.yml --start-at-task="copy nginx config file"

This PR adds the header to the example config files
ushahidi/platform-client#1197

@rjmackay
Copy link
Contributor

rjmackay commented Jul 6, 2018

@tuxpiper can you help me out with platform-release? Is one of these htaccess or nginx templates used as for the client? https://github.com/ushahidi/platform-release/tree/master/dist

@tuxpiper
Copy link
Member

tuxpiper commented Jul 6, 2018

@rjmackay

Yes, these are used in the docker images that can be built from the repo:

https://github.com/ushahidi/platform-release/blob/master/run.sh#L134
https://github.com/ushahidi/platform-release/blob/master/run.sh#L163

And they are bundled in the downloadable tarball, for deployers to take as reference while setting up their systems.

@tuxpiper
Copy link
Member

Re-opening to apply on the CloudFront deployments

@tuxpiper
Copy link
Member

Implementing this properly in the AWS stack would mean having different CDN distributions for index and assets.

A blocker for that is modifying the client's build routine so that index.html can reference the assets to a different domain. (i.e. "assets.ushahidi.io/app.XXXXX.js" instead of just "app.XXXXX.js"

@rjmackay
Copy link
Contributor

@tuxpiper thats probably sensible to do that anyway but I'm not clear why it's required for this ticket?

@rjmackay
Copy link
Contributor

@tuxpiper this ushahidi/platform-client#1258 gives you a way to pass in a base URL. We'll need to test and verify if all the assets still work as expected though.

@tuxpiper
Copy link
Member

@tuxpiper thats probably sensible to do that anyway but I'm not clear why it's required for this ticket?

Yep sorry I didn't document, I found out about this too much towards the end of my day.

The injection of headers in Cloudfront is done with lambda@edge (l@e), which carries a cost per execution. So while things would work by applying the l@e function to the whole distribution, we would be paying extra for unnecessarily invoking the function for purely static assets.

I tried working around that by having different cache behaviors in a single Cloudfront distribution (one for the "/index.html" path , configured to invoke l@e, another for the rest), but was sabotaged by the fact that any URL that doesn't match an asset file must be internally redirected to /index.html for angular routing .

Then I remembered we had #1322 open, and back then we though it was a good thing to do anyway, to separate purely static assets to their own domain. I was hoping it would be an easy thing to configure and you did it :) Thanks!

@rjmackay
Copy link
Contributor

Yup. The only caveat is that SVGs are a pain. Because they're XML most browsers refuse to load them cross-origin. So we can either inline the SVGs or load them from the same origin (ie. quakemap.ushahidi.io not assets.ushahidi.io)

@tuxpiper
Copy link
Member

Trying a different approach with ushahidi/platform-client#1262

@rowasc rowasc added Codebase: API Indicates issue work will be in API Theme: Security labels Jun 9, 2019
@tuxpiper tuxpiper added Theme: SaaS Theme: Release Bundling Issues that affect the outputs of the release creation process P0 - Urgent P1 - Immediate and removed P0 - Urgent labels Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup/Done? Codebase: API Indicates issue work will be in API P1 - Immediate Theme: Release Bundling Issues that affect the outputs of the release creation process Theme: SaaS Theme: Security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants