Skip to content

Commit

Permalink
fix(RequestHandler): only reset tokens for authenticated 401s (#7508)
Browse files Browse the repository at this point in the history
  • Loading branch information
vladfrangu committed Mar 6, 2022
1 parent c12d61a commit b9ff7b0
Show file tree
Hide file tree
Showing 6 changed files with 50 additions and 25 deletions.
4 changes: 2 additions & 2 deletions packages/rest/__tests__/REST.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ test('Request and Response Events', async () => {
method: 'get',
path: '/request',
route: '/request',
data: { files: undefined, body: undefined },
data: { files: undefined, body: undefined, auth: true },
retries: 0,
}) as APIRequest,
);
Expand All @@ -254,7 +254,7 @@ test('Request and Response Events', async () => {
method: 'get',
path: '/request',
route: '/request',
data: { files: undefined, body: undefined },
data: { files: undefined, body: undefined, auth: true },
retries: 0,
}) as APIRequest,
expect.objectContaining({ status: 200, statusText: 'OK' }) as Response,
Expand Down
10 changes: 10 additions & 0 deletions packages/rest/__tests__/RequestHandler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -357,9 +357,19 @@ test('Bad Request', async () => {
});

test('Unauthorized', async () => {
const setTokenSpy = jest.spyOn(invalidAuthApi.requestManager, 'setToken');

// Ensure authless requests don't reset the token
const promiseWithoutTokenClear = invalidAuthApi.get('/unauthorized', { auth: false });
await expect(promiseWithoutTokenClear).rejects.toThrowError('401: Unauthorized');
await expect(promiseWithoutTokenClear).rejects.toBeInstanceOf(DiscordAPIError);
expect(setTokenSpy).not.toHaveBeenCalled();

// Ensure authed requests do reset the token
const promise = invalidAuthApi.get('/unauthorized');
await expect(promise).rejects.toThrowError('401: Unauthorized');
await expect(promise).rejects.toBeInstanceOf(DiscordAPIError);
expect(setTokenSpy).toHaveBeenCalledTimes(1);
});

test('Reject on RateLimit', async () => {
Expand Down
11 changes: 9 additions & 2 deletions packages/rest/src/lib/REST.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
import { EventEmitter } from 'node:events';
import { CDN } from './CDN';
import { InternalRequest, RequestData, RequestManager, RequestMethod, RouteLike } from './RequestManager';
import {
HandlerRequestData,
InternalRequest,
RequestData,
RequestManager,
RequestMethod,
RouteLike,
} from './RequestManager';
import { DefaultRestOptions, RESTEvents } from './utils/constants';
import type { AgentOptions } from 'node:https';
import type { RequestInit, Response } from 'node-fetch';
Expand Down Expand Up @@ -160,7 +167,7 @@ export interface APIRequest {
/**
* The data that was used to form the body of this request
*/
data: Pick<InternalRequest, 'files' | 'body'>;
data: HandlerRequestData;
/**
* The number of times this request has been attempted
*/
Expand Down
8 changes: 7 additions & 1 deletion packages/rest/src/lib/RequestManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,8 @@ export interface InternalRequest extends RequestData {
fullRoute: RouteLike;
}

export type HandlerRequestData = Pick<InternalRequest, 'files' | 'body' | 'auth'>;

/**
* Parsed route data for an endpoint
*
Expand Down Expand Up @@ -293,7 +295,11 @@ export class RequestManager extends EventEmitter {
const { url, fetchOptions } = this.resolveRequest(request);

// Queue the request
return handler.queueRequest(routeId, url, fetchOptions, { body: request.body, files: request.files });
return handler.queueRequest(routeId, url, fetchOptions, {
body: request.body,
files: request.files,
auth: request.auth !== false,
});
}

/**
Expand Down
7 changes: 4 additions & 3 deletions packages/rest/src/lib/handlers/IHandler.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import type { RequestInit } from 'node-fetch';
import type { InternalRequest, RouteData } from '../RequestManager';
import type { HandlerRequestData, RouteData } from '../RequestManager';

export interface IHandler {
queueRequest: (
routeId: RouteData,
url: string,
options: RequestInit,
bodyData: Pick<InternalRequest, 'files' | 'body'>,
requestData: HandlerRequestData,
) => Promise<unknown>;
readonly inactive: boolean;
// eslint-disable-next-line @typescript-eslint/method-signature-style -- This is meant to be a getter returning a bool
get inactive(): boolean;
readonly id: string;
}
35 changes: 18 additions & 17 deletions packages/rest/src/lib/handlers/SequentialHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,11 @@ import fetch, { RequestInit, Response } from 'node-fetch';
import { DiscordAPIError, DiscordErrorData, OAuthErrorData } from '../errors/DiscordAPIError';
import { HTTPError } from '../errors/HTTPError';
import { RateLimitError } from '../errors/RateLimitError';
import type { InternalRequest, RequestManager, RouteData } from '../RequestManager';
import type { HandlerRequestData, RequestManager, RouteData } from '../RequestManager';
import { RESTEvents } from '../utils/constants';
import { hasSublimit, parseResponse } from '../utils/utils';
import type { RateLimitData } from '../REST';
import type { IHandler } from './IHandler';

/* Invalid request limiting is done on a per-IP basis, not a per-token basis.
* The best we can do is track invalid counts process-wide (on the theory that
Expand All @@ -26,7 +27,7 @@ const enum QueueType {
/**
* The structure used to handle requests for a given bucket
*/
export class SequentialHandler {
export class SequentialHandler implements IHandler {
/**
* The unique id of the handler
*/
Expand Down Expand Up @@ -162,26 +163,26 @@ export class SequentialHandler {
* @param routeId The generalized api route with literal ids for major parameters
* @param url The url to do the request on
* @param options All the information needed to make a request
* @param bodyData The data that was used to form the body, passed to any errors generated and for determining whether to sublimit
* @param requestData Extra data from the user's request needed for errors and additional processing
*/
public async queueRequest(
routeId: RouteData,
url: string,
options: RequestInit,
bodyData: Pick<InternalRequest, 'files' | 'body'>,
requestData: HandlerRequestData,
): Promise<unknown> {
let queue = this.#asyncQueue;
let queueType = QueueType.Standard;
// Separate sublimited requests when already sublimited
if (this.#sublimitedQueue && hasSublimit(routeId.bucketRoute, bodyData.body, options.method)) {
if (this.#sublimitedQueue && hasSublimit(routeId.bucketRoute, requestData.body, options.method)) {
queue = this.#sublimitedQueue!;
queueType = QueueType.Sublimit;
}
// Wait for any previous requests to be completed before this one is run
await queue.wait();
// This set handles retroactively sublimiting requests
if (queueType === QueueType.Standard) {
if (this.#sublimitedQueue && hasSublimit(routeId.bucketRoute, bodyData.body, options.method)) {
if (this.#sublimitedQueue && hasSublimit(routeId.bucketRoute, requestData.body, options.method)) {
/**
* Remove the request from the standard queue, it should never be possible to get here while processing the
* sublimit queue so there is no need to worry about shifting the wrong request
Expand All @@ -197,7 +198,7 @@ export class SequentialHandler {
}
try {
// Make the request, and return the results
return await this.runRequest(routeId, url, options, bodyData);
return await this.runRequest(routeId, url, options, requestData);
} finally {
// Allow the next request to fire
queue.shift();
Expand All @@ -218,14 +219,14 @@ export class SequentialHandler {
* @param routeId The generalized api route with literal ids for major parameters
* @param url The fully resolved url to make the request to
* @param options The node-fetch options needed to make the request
* @param bodyData The data that was used to form the body, passed to any errors generated
* @param requestData Extra data from the user's request needed for errors and additional processing
* @param retries The number of retries this request has already attempted (recursion)
*/
private async runRequest(
routeId: RouteData,
url: string,
options: RequestInit,
bodyData: Pick<InternalRequest, 'files' | 'body'>,
requestData: HandlerRequestData,
retries = 0,
): Promise<unknown> {
/*
Expand Down Expand Up @@ -292,7 +293,7 @@ export class SequentialHandler {
path: routeId.original,
route: routeId.bucketRoute,
options,
data: bodyData,
data: requestData,
retries,
});
}
Expand All @@ -309,7 +310,7 @@ export class SequentialHandler {
} catch (error: unknown) {
// Retry the specified number of times for possible timed out requests
if (error instanceof Error && error.name === 'AbortError' && retries !== this.manager.options.retries) {
return await this.runRequest(routeId, url, options, bodyData, ++retries);
return await this.runRequest(routeId, url, options, requestData, ++retries);
}

throw error;
Expand All @@ -325,7 +326,7 @@ export class SequentialHandler {
path: routeId.original,
route: routeId.bucketRoute,
options,
data: bodyData,
data: requestData,
retries,
},
res.clone(),
Expand Down Expand Up @@ -466,25 +467,25 @@ export class SequentialHandler {
}
}
// Since this is not a server side issue, the next request should pass, so we don't bump the retries counter
return this.runRequest(routeId, url, options, bodyData, retries);
return this.runRequest(routeId, url, options, requestData, retries);
} else if (res.status >= 500 && res.status < 600) {
// Retry the specified number of times for possible server side issues
if (retries !== this.manager.options.retries) {
return this.runRequest(routeId, url, options, bodyData, ++retries);
return this.runRequest(routeId, url, options, requestData, ++retries);
}
// We are out of retries, throw an error
throw new HTTPError(res.statusText, res.constructor.name, res.status, method, url, bodyData);
throw new HTTPError(res.statusText, res.constructor.name, res.status, method, url, requestData);
} else {
// Handle possible malformed requests
if (res.status >= 400 && res.status < 500) {
// If we receive this status code, it means the token we had is no longer valid.
if (res.status === 401) {
if (res.status === 401 && requestData.auth) {
this.manager.setToken(null!);
}
// The request will not succeed for some reason, parse the error returned from the api
const data = (await parseResponse(res)) as DiscordErrorData | OAuthErrorData;
// throw the API error
throw new DiscordAPIError(data, 'code' in data ? data.code : data.error, res.status, method, url, bodyData);
throw new DiscordAPIError(data, 'code' in data ? data.code : data.error, res.status, method, url, requestData);
}
return null;
}
Expand Down

0 comments on commit b9ff7b0

Please sign in to comment.