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 CSP to the payments-server #1923

Closed
jaredhirsch opened this issue Jul 24, 2019 · 4 comments · Fixed by #2053
Closed

Add CSP to the payments-server #1923

jaredhirsch opened this issue Jul 24, 2019 · 4 comments · Fixed by #2053
Assignees

Comments

@jaredhirsch
Copy link
Member

jaredhirsch commented Jul 24, 2019

Seems a little involved, spinning off as a separate sub-issue from #1128.

We might be able to reuse the CSP-related middleware from the fxa-content-server package.

@jaredhirsch jaredhirsch self-assigned this Jul 24, 2019
@LZoog LZoog self-assigned this Jul 26, 2019
@jaredhirsch jaredhirsch removed their assignment Jul 26, 2019
@jaredhirsch
Copy link
Member Author

I found some docs on the CSP config required for the Stripe JS to work: https://stripe.com/docs/security#content-security-policy

LZoog added a commit that referenced this issue Aug 13, 2019
Remove payments/content consolidated CSP, fix Prettier-ified files, create separate middleware for payments

Fix prettier things

Add Stripe CSP things to payments server

Add more config vars into proper directives

Add stripe checkout API

Remove unneeded directive

Add tests to csp.test.js

Remove unneeded test no-CSP lines

Remove isCspRequired check in payment server (not content server)

csp.enabled true by default

Update CSP violations to report to content server

Template literal return for getOrigin
LZoog added a commit that referenced this issue Aug 13, 2019
Remove payments/content consolidated CSP, fix Prettier-ified files, create separate middleware for payments

Fix prettier things

Add Stripe CSP things to payments server

Add more config vars into proper directives

Add stripe checkout API

Remove unneeded directive

Add tests to csp.test.js

Remove unneeded test no-CSP lines

Remove isCspRequired check in payment server (not content server)

csp.enabled true by default

Update CSP violations to report to content server

Template literal return for getOrigin

Update Stripe url doc
LZoog added a commit that referenced this issue Aug 15, 2019
Remove payments/content consolidated CSP, fix Prettier-ified files, create separate middleware for payments

Fix prettier things

Add Stripe CSP things to payments server

Add more config vars into proper directives

Add stripe checkout API

Remove unneeded directive

Add tests to csp.test.js

Remove unneeded test no-CSP lines

Remove isCspRequired check in payment server (not content server)

csp.enabled true by default

Update CSP violations to report to content server

Template literal return for getOrigin

Update Stripe url doc

Address PR things
LZoog added a commit that referenced this issue Aug 15, 2019
Remove payments/content consolidated CSP, fix Prettier-ified files, create separate middleware for payments

Fix prettier things

Add Stripe CSP things to payments server

Add more config vars into proper directives

Add stripe checkout API

Remove unneeded directive

Add tests to csp.test.js

Remove unneeded test no-CSP lines

Remove isCspRequired check in payment server (not content server)

csp.enabled true by default

Update CSP violations to report to content server

Template literal return for getOrigin

Update Stripe url doc

Address PR things

Add DATA back to payments CSP
LZoog added a commit that referenced this issue Aug 16, 2019
Remove payments/content consolidated CSP, fix Prettier-ified files, create separate middleware for payments

Fix prettier things

Add Stripe CSP things to payments server

Add more config vars into proper directives

Add stripe checkout API

Remove unneeded directive

Add tests to csp.test.js

Remove unneeded test no-CSP lines

Remove isCspRequired check in payment server (not content server)

csp.enabled true by default

Update CSP violations to report to content server

Template literal return for getOrigin

Update Stripe url doc

Address PR things

Add DATA back to payments CSP

Change variable name and only add in development
LZoog pushed a commit that referenced this issue Aug 16, 2019
chore(fxa-payments-server): fixes #1923 - add CSP to the payments server
clouserw added a commit that referenced this issue Aug 16, 2019
@jvehent
Copy link

jvehent commented Sep 27, 2019

The CSP is not super useful at the moment because it allows unsafe styles.
content-security-policy: connect-src 'self' https://api.accounts.firefox.com https://oauth.accounts.firefox.com https://profile.accounts.firefox.com https://api.stripe.com; default-src 'self'; font-src 'self'; frame-src https://js.stripe.com https://hooks.stripe.com; img-src 'self' data: https://secure.gravatar.com https://firefoxusercontent.com; media-src 'none'; object-src 'none'; report-uri https://accounts.firefox.com/_/csp-violation; script-src 'self' https://js.stripe.com; style-src 'self' 'unsafe-inline'

Could we get that removed?

@jvehent jvehent reopened this Sep 27, 2019
@lmorchard
Copy link
Contributor

lmorchard commented Sep 27, 2019

The CSP is not super useful at the moment because it allows unsafe styles.
Could we get that removed?

If I recall, we need unsafe-inline for style-src because React injects inline styles. In particular, for the third-party Stripe elements over which we don't have much control otherwise (because they're managed by Stripe's JS). Might need to dig to verify this, but I think that's the reason. So, we could remove it but not have any styling for the Stripe elements.

@clouserw
Copy link
Member

Sorry, I didn't see this Issue reopened before I filed #2649 . I'm going to close this one, since the patch is landed/live already (and Issues are tied to milestones). Please discuss in #2649

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 a pull request may close this issue.

5 participants