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

Removed requirement to load JavaScript from js.braintreegateway.com #259

Conversation

lightswitch05
Copy link
Contributor

@lightswitch05 lightswitch05 commented Sep 11, 2018

Braintree was kind enough to supply a built version of their drop-in library in their node package via braintree/braintree-web-drop-in#233

By pulling this library in through a static build process, it removes the run-time dependency braintreegateway.com also reducing the attack surface of the vault

package.json Outdated
@@ -32,6 +32,7 @@
"@types/papaparse": "4.1.33",
"@types/webcrypto": "^0.0.28",
"angular2-template-loader": "^0.6.2",
"braintree-web-drop-in": "^1.13.0",
Copy link
Member

@kspearrin kspearrin Sep 11, 2018

Choose a reason for hiding this comment

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

Shouldn't this be a regular dependency and not a dev?

Copy link
Contributor Author

@lightswitch05 lightswitch05 Sep 11, 2018

Choose a reason for hiding this comment

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

I guess? The JS gets copied over during build time. I'm happy to update it if you want

Copy link
Member

@kspearrin kspearrin Sep 11, 2018

Choose a reason for hiding this comment

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

The qrious library you are modeling this after is in dependencies. please add it there too.

@kspearrin kspearrin merged commit 188ac50 into bitwarden:master Sep 11, 2018
@kspearrin
Copy link
Member

@kspearrin kspearrin commented Sep 11, 2018

I notice that the dropin is now unminified and 600kb in size. That's pretty large vs before. Any way we can get a min version?

@lightswitch05
Copy link
Contributor Author

@lightswitch05 lightswitch05 commented Sep 11, 2018

Seems reasonable, I asked here braintree/braintree-web-drop-in#233 (comment)

@lightswitch05
Copy link
Contributor Author

@lightswitch05 lightswitch05 commented Sep 12, 2018

After this change goes live, I believe the vault's CSP header could be updated:

Current:

content-security-policy: default-src 'self'; script-src 'self' 'sha256-ryoU+5+IUZTuUyTElqkrQGBJXr1brEv6r2CA62WUw8w=' https://js.stripe.com https://js.braintreegateway.com https://www.paypalobjects.com; style-src 'self' 'unsafe-inline' https://assets.braintreegateway.com https://*.paypal.com; img-src 'self' data: https://icons.bitwarden.net https://*.paypal.com https://www.paypalobjects.com https://q.stripe.com https://haveibeenpwned.com https://www.gravatar.com; child-src 'self' https://js.stripe.com https://assets.braintreegateway.com https://*.paypal.com https://*.duosecurity.com; frame-src 'self' https://js.stripe.com https://assets.braintreegateway.com https://*.paypal.com https://*.duosecurity.com; connect-src 'self' wss://notifications.bitwarden.com https://notifications.bitwarden.com https://cdn.bitwarden.com https://haveibeenpwned.com https://api.pwnedpasswords.com https://api.stripe.com https://www.paypal.com https://api.braintreegateway.com https://client-analytics.braintreegateway.com https://*.braintree-api.com https://www.google-analytics.com;
  • I believe https://js.braintreegateway.com can be removed from script-src. I'm not sure about the other braintree endpoints. I never see assets.braintreegateway.com or client-analytics.braintreegateway.com - but I also have not purchased anything

@lightswitch05
Copy link
Contributor Author

@lightswitch05 lightswitch05 commented Sep 13, 2018

That discussion with braintree about injecting iframes and scripts brought up some good points. I wonder how hard it would be to move all payment processing to a standalone page that is outside of the vault. Moving it out not only reduces the attack surface of the vault, but would also allow for a simpler and more robust CSP header

@lightswitch05
Copy link
Contributor Author

@lightswitch05 lightswitch05 commented Sep 13, 2018

A possible bonus to moving purchase outside of the vault: You don't need the user to re-type their password to unlock their vault if they clicked purchase from the browser extension. Right now, as a free user, if I click 'Premium Membership' from the browser extension, then I am brought to vault.bitwarden.com to sign in before it gives me the details to purchase. I don't know what analytics you have, but I'm sure there is a decent bounce percentage at that point. If the purchase step is outside of the vault, then you could pass the authorization key from the browser extension to the new purchase page, allowing the user to still identified in order to link the purchase with an account without needing them to re-enter their password. You wouldn't need the password because you don't need to decrypt the vault. Really, I'm not sure that authentication matters much at that point. Allow people to purchase premium for any user account, not sure why auth would be needed to purchase.

Unless there are some technical hurdles I'm not aware of, this seems like a win-win. Not only is security improved my removing 3rd party code, but its one less step before getting a user to the purchase page, theoretically improving conversion rates

@kspearrin
Copy link
Member

@kspearrin kspearrin commented Sep 15, 2018

There are future plans to create a "billing portal" that is separate from the web vault, however, for convenience reasons there will likely always be the ability to make purchases from directly within the web vault as well.

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.

None yet

2 participants