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

Adding support for beforeRedirect config option #3852

Merged
merged 11 commits into from Mar 7, 2022
13 changes: 12 additions & 1 deletion README.md
Expand Up @@ -414,7 +414,18 @@ These are the available config options for making requests. Only the `url` is re

// `maxRedirects` defines the maximum number of redirects to follow in node.js.
// If set to 0, no redirects will be followed.
maxRedirects: 5, // default
maxRedirects: 21, // default

// `beforeRedirect` defines a function that will be called before redirect.
// Use this to adjust the request options upon redirecting,
// to inspect the latest response headers,
// or to cancel the request by throwing an error
// If maxRedirects is set to 0, `beforeRedirect` is not used.
beforeRedirect: (options, { headers }) => {
if (options.hostname === "example.com") {
jasonsaayman marked this conversation as resolved.
Show resolved Hide resolved
options.auth = "user:password";
}
};

// `socketPath` defines a UNIX Socket to be used in node.js.
// e.g. '/var/run/docker.sock' to send requests to the docker daemon.
Expand Down
2 changes: 1 addition & 1 deletion index.d.ts
@@ -1,5 +1,4 @@
// TypeScript Version: 3.0

export type AxiosRequestHeaders = Record<string, string | number | boolean>;

export type AxiosResponseHeaders = Record<string, string> & {
Expand Down Expand Up @@ -98,6 +97,7 @@ export interface AxiosRequestConfig<D = any> {
validateStatus?: ((status: number) => boolean) | null;
maxBodyLength?: number;
maxRedirects?: number;
beforeRedirect?: (options: Record<string, any>, responseDetails: {headers: Record<string, string>}) => void;
socketPath?: string | null;
httpAgent?: any;
httpsAgent?: any;
Expand Down
5 changes: 4 additions & 1 deletion lib/adapters/http.js
Expand Up @@ -227,6 +227,9 @@ module.exports = function httpAdapter(config) {
if (config.maxRedirects) {
options.maxRedirects = config.maxRedirects;
}
if (config.beforeRedirect) {
options.beforeRedirect = config.beforeRedirect;
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this break proxy support when redirection occurs?

The setProxy function, which is called before hitting this line, sets options.beforeRedirect to re-apply proxy configuration after a redirect. This line here happily stamps over it.

Either there should be something to harmonize the two features (such as registering a function that invokes both the original config beforeRedirect if needed, and the proxy re-configuration next) or the documentation should state that the config.beforeRedirect option is incompatible with proxy support (😿) and can introduce proxy redirect issues.

}
transport = isHttpsProxy ? httpsFollow : httpFollow;
}

Expand Down Expand Up @@ -326,7 +329,7 @@ module.exports = function httpAdapter(config) {

// Handle errors
req.on('error', function handleRequestError(err) {
if (req.aborted && err.code !== 'ERR_FR_TOO_MANY_REDIRECTS') return;
if (req.aborted && err.code !== 'ERR_FR_TOO_MANY_REDIRECTS') reject(err);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this request will never complete.

Copy link
Contributor Author

@zoran995 zoran995 Jan 14, 2022

Choose a reason for hiding this comment

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

just to expand more on this change, I haven't used enhanceError since this should be an error thrown from client application so we return it as is.
It's a bit strange that no one before run into the issue of stuck request

reject(enhanceError(err, config, null, req));
});

Expand Down
1 change: 1 addition & 0 deletions lib/core/mergeConfig.js
Expand Up @@ -80,6 +80,7 @@ module.exports = function mergeConfig(config1, config2) {
'decompress': defaultToConfig2,
'maxContentLength': defaultToConfig2,
'maxBodyLength': defaultToConfig2,
'beforeRedirect': defaultToConfig2,
'transport': defaultToConfig2,
'httpAgent': defaultToConfig2,
'httpsAgent': defaultToConfig2,
Expand Down
20 changes: 20 additions & 0 deletions test/unit/adapters/http.js
Expand Up @@ -241,6 +241,26 @@ describe('supports http with nodejs', function () {
});
});

it('should support beforeRedirect', function (done) {
server = http.createServer(function (req, res) {
res.setHeader('Location', '/foo');
res.statusCode = 302;
res.end();
}).listen(4444, function () {
axios.get('http://localhost:4444/', {
maxRedirects: 3,
beforeRedirect: function (options) {
if (options.path === '/foo') {
throw new Error('path not allowed');
}
}
}).catch(function (error) {
assert.equal(error.toJSON().message, 'path not allowed')
done();
});
});
});

it('should preserve the HTTP verb on redirect', function (done) {
server = http.createServer(function (req, res) {
if (req.method.toLowerCase() !== "head") {
Expand Down