As mentioned in the work ticket, node-fetch doesn't support conditional erroring on redirects. In this MR, we add an "after" middleware to check if the z.request experienced a redirect, and error out if the redirection ended up in a disallowed domain (currently only disallowing localhost). We need to check the resp.url as the req.url does not change on redirect.
Changes:
Install nock explicitly in core package
Add a throwForDisallowedHostnameAfterRedirect middleware and use it by default in the httpAfters middleware list (after logResponse)
Update prepare-response.js in order to surface the resp.redirected and resp.url properties for the new middleware. An alternative here could be to move the new middleware even higher on the list.
An alternative approach here could be to set redirect: 'manual' as an option and then perform manual parsing of the headers here.
The big upside to the alternative approach is that we could prevent the disallowed domain request from even happening, as opposed to just catching it like we do here. A potential downside is that it could potentially introduce breaking changes by not fully understanding how integrations are leveraging redirects (speaking for myself!) But ultimately, this change is just an attempt to help protect partners from end-user redirect abuse, which this this MR tackles.
As mentioned in the work ticket,
node-fetchdoesn't support conditional erroring on redirects. In this MR, we add an "after" middleware to check if thez.requestexperienced a redirect, and error out if the redirection ended up in a disallowed domain (currently only disallowing localhost). We need to check theresp.urlas thereq.urldoes not change on redirect.Changes:
nockexplicitly incorepackagethrowForDisallowedHostnameAfterRedirectmiddleware and use it by default in thehttpAftersmiddleware list (afterlogResponse)prepare-response.jsin order to surface theresp.redirectedandresp.urlproperties for the new middleware. An alternative here could be to move the new middleware even higher on the list.An alternative approach here could be to set
redirect: 'manual'as an option and then perform manual parsing of the headers here.The big upside to the alternative approach is that we could prevent the
disalloweddomain request from even happening, as opposed to just catching it like we do here. A potential downside is that it could potentially introduce breaking changes by not fully understanding how integrations are leveraging redirects (speaking for myself!) But ultimately, this change is just an attempt to help protect partners from end-user redirect abuse, which this this MR tackles.