Add checkServerIdentity to request config for http adapter#2498
Add checkServerIdentity to request config for http adapter#2498jasonsaayman merged 14 commits intoaxios:release/1.0.0-beta.1from Annihil:master
Conversation
|
can you add documentation and tests for this? |
|
All done 👍 |
yasuf
left a comment
yasuf
left a comment
There was a problem hiding this comment.
if we can avoid adding ssl-root-cas, that would make me more confident of this change, worst case scenario we add a util in axios instead of ssl-root-cas
| const msg = 'Certificate verification error: ' + | ||
| `The certificate of '${cert.subject.CN}' ` + | ||
| 'does not match our pinned fingerprint'; | ||
| return new Error(msg); |
There was a problem hiding this comment.
this should be throw new Error I believe?
There was a problem hiding this comment.
The correct way seems to be return according to NodeJS' doc
package.json
Outdated
| "minimist": "^1.2.0", | ||
| "mocha": "^5.2.0", | ||
| "sinon": "^4.5.0", | ||
| "ssl-root-cas": "^1.3.1", |
There was a problem hiding this comment.
is there a way to avoid adding this dependency?
There was a problem hiding this comment.
I figured out a way, it's gone now
test/unit/adapters/http.js
Outdated
| var server, proxy; | ||
| var path = require('path'); | ||
| require('ssl-root-cas') | ||
| .inject() |
There was a problem hiding this comment.
this method is deprecated in this dependency's documentation https://git.coolaj86.com/coolaj86/ssl-root-cas.js#inject, can we use something else?
There was a problem hiding this comment.
Good catch, ssl-root-cas module is not used anymore anyway
|
also can you add your change to the CHANGELOG.md under a new section, with a title: "Next release:" |
|
this rules. would love to see this merged |
* fix: remove byte order marker (UTF-8 BOM) when transform response * fix: remove BOM only utf-8 * test: utf-8 BOM * fix: incorrect param name Co-authored-by: Jay <[email protected]>
|
Can we please get this into master? |
* add checkServerIdentity to request config for http adapter * add checkServerIdentity unit test * add checkServerIdentity doc * add more unit tests for checkServerIdentity * remove ssl-root-cas dependency * add changes description to changelog * Fixing response with utf-8 BOM can not parse to json (axios#2419) * fix: remove byte order marker (UTF-8 BOM) when transform response * fix: remove BOM only utf-8 * test: utf-8 BOM * fix: incorrect param name Co-authored-by: Jay <[email protected]> * Update mergeConfig.js Co-authored-by: Yasu Flores <[email protected]> Co-authored-by: Cr <[email protected]> Co-authored-by: Jay <[email protected]>
8 participants
In order to achieve SSL certificate pinning such a shown here on the NodeJS doc, I added
checkServerIdentityto the request option for the nodejs http adapter 🙂