feat: improve HTTP redirects behavior #262
Conversation
This commit modifies the Node core so that it will include "safe" headers when performing a cross-site redirect where both the original and redirected hosts are within IBM's "cloud.ibm.com" domain. Signed-off-by: Norbert Biczo <[email protected]>
Signed-off-by: Norbert Biczo <[email protected]>
Signed-off-by: Norbert Biczo <[email protected]>
padamstx
left a comment
padamstx
left a comment
There was a problem hiding this comment.
Looks good.
Only suggestion is to add a couple tests
| <!-- Remove items that do not apply. For completed items, change [ ] to [x]. --> | ||
|
|
||
| - [ ] `npm test` passes (tip: `npm run lint-fix` can correct most style issues) | ||
| - [ ] `npm test` passes (tip: `npm run lint:fix` can correct most style issues) |
lib/request-wrapper.ts
Outdated
|
|
||
| // Returns true iff safe headers should be copied to a redirected request. | ||
| function shouldCopySafeHeadersOnRedirect(fromHost: string, toHost: string): boolean { | ||
| return fromHost.endsWith('.cloud.ibm.com') && toHost.endsWith('.cloud.ibm.com'); |
There was a problem hiding this comment.
This logic should work as long as we are also supporting the scenario where a redirection to the same host also results in the safe headers being copied, which I assume is the case since you're not completely overriding axios' redirection behavior (presumably) :)
There was a problem hiding this comment.
Those scenarios are covered by the unit tests now and yes, we are not overriding the whole redirection behavior, it's just a sort of post-processing in the chain, before sending the next, redirected request.
test/unit/redirect.test.js
Outdated
| }); | ||
|
|
||
| it('should include safe headers within cloud.ibm.com domain', async () => { | ||
| const url1 = 'http://region1.cloud.ibm.com'; |
There was a problem hiding this comment.
let's use https in these URLs :)
There was a problem hiding this comment.
Agreed! I also change the protocols in my Python core PR, because I used HTTP there too.
| @@ -0,0 +1,211 @@ | |||
| /** | |||
There was a problem hiding this comment.
need to add tests for redirects to the same host:
- both are region1.cloud.ibm.com
- both are region2.notcloud.ibm.com
Signed-off-by: Norbert Biczo <[email protected]>
Signed-off-by: Norbert Biczo <[email protected]> chore: small improvements Signed-off-by: Norbert Biczo <[email protected]> chore: fix tests Signed-off-by: Norbert Biczo <[email protected]> chore: fix unit tests Signed-off-by: Norbert Biczo <[email protected]> chore: lint fix Signed-off-by: Norbert Biczo <[email protected]>
Signed-off-by: Norbert Biczo <[email protected]>
Signed-off-by: Norbert Biczo <[email protected]>
This commit modifies the Node core so that it will include "safe" headers when performing a cross-site redirect where both the original and redirected hosts are within IBM's "cloud.ibm.com" domain.
Checklist
npm testpasses (tip:npm run lint-fixcan correct most style issues)