Inlined certificates #162
Merged
Inlined certificates #162
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
With the certificates file inlined, we no longer depend on require('fs'). This ‒ unfortunately ‒ makes Webpack compile this library to be used in a browser without hesitation. This runtime check should detect frontend integration.
|
|
It seems to me this library was never using the CA certificates because they were wrongly passed as "cert" instead of "ca". The reason it works despite this, is that Node.js actually bundles their own CA certificates, which were used instead.
|
I left 1 minor (nitpicky) comment. Thanks for thinking of that case. Tested this locally, and it works great! Thanks for this nice PR 👏 |
vernondegoede
approved these changes
Aug 18, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
cainstead ofcert1.What it looks like to me ‒ and I am not a security expert ‒ is that we are not using the certificates at this time. Instead, we fall back on the certificates included in Node.js.
With this PR, the certificates included in Node.js should be ignored in favour of the ones we provide.
Additionally, we are no longer loading the certificates file from the file system at runtime. This means we are no longer doing a
require('fs').To make our users not shoot themselves in the foot
There is a side-effect. In the current version, (mistakenly) integrating the library in a React app using create-react-app would cause this error:
It breaks, because
require('fs')doesn't provide the required API. However, this is the very line this PR removes…Without any extra checks, with this PR merged a developer can call
createMollieClientin a React app with no warnings/errors. (This is due to webpack providing a polyfill forhttpsandurl. It seems webpack 5 will cease this behaviour, but the current version of webpack still has it.)This PR includes an extra check, which detects polyfills for
https‒ and hopefully detects integrations in websites or apps that way.Thoughts
We could choose to rely on certificates included in Node.js (which we are doing now by accident) instead of providing our own list. This means giving up control and trusting Node.js to keep our users safe.
Alternatively, we could choose to only provide the certificate we need (i.e. USERTrust RSA Certification Authority).