Skip to content

Commit

Permalink
web-api: prevent apps.event.authorizations.list API from ever sending…
Browse files Browse the repository at this point in the history
… token in the body; fixes #1498 (#1737)
  • Loading branch information
filmaj authored Jan 29, 2024
1 parent f3dff4d commit 664db4e
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 11 deletions.
33 changes: 33 additions & 0 deletions packages/web-api/src/WebClient.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -1288,6 +1288,39 @@ describe('WebClient', function () {
});
});

describe('apps.event.authorizations.list API', function () {
it('should not send the token in the body if token is passed as a method argument', function () {
const client = new WebClient();
const scope = nock('https://slack.com')
.post(/api/)
.reply(200, (_uri, body) => {
assert.notInclude(body, token);
console.log('body is', body);
return { ok: true }
});
return client.apps.event.authorizations.list({
token,
})
.then(() => {
scope.done();
});
});
it('should not send the token in the body if token passed as client constructor', function () {
const client = new WebClient(token);
const scope = nock('https://slack.com')
.post(/api/)
.reply(200, (_uri, body) => {
assert.notInclude(body, token);
return { ok: true }
});
return client.apps.event.authorizations.list({
})
.then(() => {
scope.done();
});
});
});

describe('getAllFileUploads', () => {
const client = new WebClient(token);
it('adds a single file data to uploads with content supplied', async () => {
Expand Down
44 changes: 33 additions & 11 deletions packages/web-api/src/WebClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { TextDecoder } from 'util';
import isStream from 'is-stream';
import pQueue from 'p-queue';
import pRetry, { AbortError } from 'p-retry';
import axios, { AxiosInstance, AxiosResponse } from 'axios';
import axios, { AxiosHeaderValue, AxiosInstance, AxiosResponse } from 'axios';
// eslint-disable-next-line @typescript-eslint/naming-convention
import FormData from 'form-data';
import isElectron from 'is-electron';
Expand Down Expand Up @@ -48,7 +48,8 @@ import {
/*
* Helpers
*/

// Props on axios default headers object to ignore when retrieving full list of actual headers sent in any HTTP requests
const axiosHeaderPropsToIgnore = ['delete', 'common', 'get', 'put', 'head', 'post', 'link', 'patch', 'purge', 'unlink', 'options'];
const defaultFilename = 'Untitled';
const defaultPageSize = 200;
const noopPageReducer: PageReducer = () => undefined;
Expand Down Expand Up @@ -527,14 +528,14 @@ export class WebClient extends Methods {
/**
* Low-level function to make a single API request. handles queuing, retries, and http-level errors
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
private async makeRequest(url: string, body: any, headers: any = {}): Promise<AxiosResponse> {
private async makeRequest(
url: string,
body: Record<string, unknown>,
headers: Record<string, string> = {},
): Promise<AxiosResponse> {
// TODO: better input types - remove any
const task = () => this.requestQueue.add(async () => {
const requestURL = (url.startsWith('https' || 'http')) ? url : `${this.axios.getUri() + url}`;
this.logger.debug(`http request url: ${requestURL}`);
this.logger.debug(`http request body: ${JSON.stringify(redact(body))}`);
this.logger.debug(`http request headers: ${JSON.stringify(redact(headers))}`);

try {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Expand All @@ -547,6 +548,29 @@ export class WebClient extends Methods {
if (url.endsWith('admin.analytics.getFile')) {
config.responseType = 'arraybuffer';
}
// apps.event.authorizations.list will reject HTTP requests that send token in the body
// TODO: consider applying this change to all methods - though that will require thorough integration testing
if (url.endsWith('apps.event.authorizations.list')) {
// eslint-disable-next-line no-param-reassign
delete body.token;
}
this.logger.debug(`http request url: ${requestURL}`);
this.logger.debug(`http request body: ${JSON.stringify(redact(body))}`);
// compile all headers - some set by default under the hood by axios - that will be sent along
let allHeaders: Record<string, AxiosHeaderValue | undefined> = Object.keys(this.axios.defaults.headers)
.reduce((acc, cur) => {
if (!axiosHeaderPropsToIgnore.includes(cur)) {
acc[cur] = this.axios.defaults.headers[cur];
}
return acc;
}, {} as Record<string, AxiosHeaderValue | undefined>);

allHeaders = {
...this.axios.defaults.headers.common,
...allHeaders,
...headers,
};
this.logger.debug(`http request headers: ${JSON.stringify(redact(allHeaders))}`);
const response = await this.axios.post(url, body, config);
this.logger.debug('http response received');

Expand Down Expand Up @@ -883,8 +907,7 @@ export function buildThreadTsWarningMessage(method: string): string {
* @param body
* @returns
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
function redact(body: any): any {
function redact(body: Record<string, unknown>): Record<string, unknown> {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const flattened = Object.entries(body).map<[string, any] | []>(([key, value]) => {
// no value provided
Expand All @@ -909,8 +932,7 @@ function redact(body: any): any {
});

// return as object
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const initialValue: { [key: string]: any; } = {};
const initialValue: { [key: string]: unknown; } = {};
return flattened.reduce(
(accumulator, [key, value]) => {
if (key !== undefined && value !== undefined) {
Expand Down

0 comments on commit 664db4e

Please sign in to comment.