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

fix: Do Not Mutate Config for Redacted Retries #597

Merged
merged 9 commits into from
Jan 31, 2024
33 changes: 16 additions & 17 deletions src/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import {URL} from 'url';

import {pkg} from './util';
import extend from 'extend';

/**
* Support `instanceof` operator for `GaxiosError`s in different versions of this library.
Expand Down Expand Up @@ -81,9 +82,19 @@
) {
super(message);

// deep-copy config as we do not want to mutate
// the existing config for future retries/use
this.config = extend(true, {}, config);
if (this.response) {
this.response.config = extend(true, {}, this.response.config);
}

if (this.response) {
try {
this.response.data = translateData(config.responseType, response?.data);
this.response.data = translateData(
this.config.responseType,
this.response?.data
);
} catch {
// best effort - don't throw an error within an error
// we could set `this.response.config.responseType = 'unknown'`, but
Expand All @@ -98,36 +109,24 @@
}

if (config.errorRedactor) {
const errorRedactor = config.errorRedactor<T>;

// shallow-copy config for redaction as we do not want
// future requests to have redacted information
this.config = {...config};
if (this.response) {
// copy response's config, as it may be recursively redacted
this.response = {...this.response, config: {...this.response.config}};
}

const results = errorRedactor({config, response});
this.config = {...config, ...results.config};

if (this.response) {
this.response = {...this.response, ...results.response, config};
}
config.errorRedactor<T>({
config: this.config,
response: this.response,
});
}
}
}

export interface Headers {
[index: string]: any;

Check warning on line 121 in src/common.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
}
export type GaxiosPromise<T = any> = Promise<GaxiosResponse<T>>;

Check warning on line 123 in src/common.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type

export interface GaxiosXMLHttpRequest {
responseURL: string;
}

export interface GaxiosResponse<T = any> {

Check warning on line 129 in src/common.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
config: GaxiosOptions;
data: T;
status: number;
Expand All @@ -144,7 +143,7 @@
* Optional method to override making the actual HTTP request. Useful
* for writing tests.
*/
adapter?: <T = any>(

Check warning on line 146 in src/common.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
options: GaxiosOptions,
defaultAdapter: (options: GaxiosOptions) => GaxiosPromise<T>
) => GaxiosPromise<T>;
Expand All @@ -162,8 +161,8 @@
| 'TRACE'
| 'PATCH';
headers?: Headers;
data?: any;

Check warning on line 164 in src/common.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
body?: any;

Check warning on line 165 in src/common.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
/**
* The maximum size of the http response content in bytes allowed.
*/
Expand All @@ -173,13 +172,13 @@
*/
maxRedirects?: number;
follow?: number;
params?: any;

Check warning on line 175 in src/common.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
paramsSerializer?: (params: {[index: string]: string | number}) => string;
timeout?: number;
/**
* @deprecated ignored
*/
onUploadProgress?: (progressEvent: any) => void;

Check warning on line 181 in src/common.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
responseType?:
| 'arraybuffer'
| 'blob'
Expand All @@ -193,7 +192,7 @@
retry?: boolean;
// Should be instance of https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal
// interface. Left as 'any' due to incompatibility between spec and abort-controller.
signal?: any;

Check warning on line 195 in src/common.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
size?: number;
/**
* Implementation of `fetch` to use when making the API call. By default,
Expand Down Expand Up @@ -225,7 +224,7 @@
*
* @experimental
*/
export type RedactableGaxiosResponse<T = any> = Pick<

Check warning on line 227 in src/common.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
GaxiosResponse<T>,
'config' | 'data' | 'headers'
>;
Expand Down
9 changes: 8 additions & 1 deletion src/gaxios.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,10 @@ function loadProxy() {
if (proxy) {
HttpsProxyAgent = httpsProxyAgent;
}

return proxy;
}

loadProxy();

function skipProxy(url: string) {
Expand Down Expand Up @@ -186,7 +188,12 @@ export class Gaxios {
if (shouldRetry && config) {
err.config.retryConfig!.currentRetryAttempt =
config.retryConfig!.currentRetryAttempt;
return this._request<T>(err.config);

// The error's config could be redacted - therefore we only want to
// copy the retry state over to the existing config
opts.retryConfig = err.config?.retryConfig;

return this._request<T>(opts);
}
throw err;
}
Expand Down
9 changes: 8 additions & 1 deletion test/test.getch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -713,6 +713,7 @@ describe('🎏 data handling', () => {
body: 'grant_type=somesensitivedata&assertion=somesensitivedata',
};

// simulate JSON response
const responseHeaders = {
...config.headers,
'content-type': 'application/json',
Expand All @@ -725,16 +726,22 @@ describe('🎏 data handling', () => {
.reply(404, response, responseHeaders);

const instance = new Gaxios(JSON.parse(JSON.stringify(config)));
const requestConfig: GaxiosOptions = {
url: customURL.toString(),
method: 'POST',
};
const requestConfigCopy = JSON.parse(JSON.stringify({...requestConfig}));

try {
await instance.request({url: customURL.toString(), method: 'POST'});
await instance.request(requestConfig);

throw new Error('Expected a GaxiosError');
} catch (e) {
assert(e instanceof GaxiosError);

// config should not be mutated
assert.deepStrictEqual(instance.defaults, config);
assert.deepStrictEqual(requestConfig, requestConfigCopy);
assert.notStrictEqual(e.config, config);

// config redactions - headers
Expand Down
Loading