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

socket-mode(fix): redact ephemeral tokens and secrets from debug logs #1832

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 89 additions & 0 deletions packages/socket-mode/src/SocketModeClient.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,95 @@ describe('SocketModeClient', () => {
assert.equal(passedEnvelopeId, envelopeId);
});
});

describe('redact', () => {
let spies: sinon.SinonSpy;

beforeEach(() => {
spies = sinon.spy();
});

afterEach(() => {
sinon.reset();
});

it('should remove tokens and secrets from incoming messages', async () => {
const logger = new ConsoleLogger();
logger.debug = spies;
const client = new SocketModeClient({
appToken: 'xapp-example-001',
logger,
});

const input = {
type: 'hello',
payload: {
example: '12',
token: 'xoxb-example-001',
event: {
bot_access_token: 'xwfp-redaction-001',
},
values: {
secret: 'abcdef',
},
inputs: [
{ id: 'example', mock: 'testing' },
{ id: 'mocking', mock: 'testure' },
],
},
};
const expected = {
type: 'hello',
payload: {
example: '12',
token: '[[REDACTED]]',
event: {
bot_access_token: '[[REDACTED]]',
},
values: {
secret: '[[REDACTED]]',
},
inputs: [
{ id: 'example', mock: 'testing' },
{ id: 'mocking', mock: 'testure' },
],
},
};

client.emit('message', JSON.stringify(input));
assert(spies.called);
assert(spies.calledWith(`Received a message on the WebSocket: ${JSON.stringify(expected)}`));
});

it('should respond with undefined when attempting to redact undefined', async () => {
const logger = new ConsoleLogger();
logger.debug = spies;
const client = new SocketModeClient({
appToken: 'xapp-example-001',
logger,
});

const input = undefined;

client.emit('message', JSON.stringify(input));
assert(spies.called);
assert(spies.calledWith('Received a message on the WebSocket: undefined'));
});

it('should print the incoming data if parsing errors happen', async () => {
const logger = new ConsoleLogger();
logger.debug = spies;
logger.error = spies;
const client = new SocketModeClient({
appToken: 'xapp-example-001',
logger,
});

client.emit('message', '{"number":');
assert(spies.called);
assert(spies.calledWith('Received a message on the WebSocket: {"number":'));
});
});
});
});

Expand Down
43 changes: 37 additions & 6 deletions packages/socket-mode/src/SocketModeClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -284,8 +284,6 @@
this.logger.debug('Unexpected binary message received, ignoring.');
return;
}
const payload = data.toString();
this.logger.debug(`Received a message on the WebSocket: ${payload}`);

// Parse message into slack event
let event: {
Expand All @@ -299,10 +297,13 @@
accepts_response_payload?: boolean; // type: events_api, slash_commands, interactive
};

const payload = data?.toString();
try {
event = JSON.parse(payload);
this.logger.debug(`Received a message on the WebSocket: ${JSON.stringify(SocketModeClient.redact(event))}`);
} catch (parseError) {
// Prevent application from crashing on a bad message, but log an error to bring attention
this.logger.debug(`Received a message on the WebSocket: ${payload}`);
this.logger.debug(
`Unable to parse an incoming WebSocket message (will ignore): ${parseError}, ${payload}`,
);
Expand All @@ -325,7 +326,7 @@
// Define Ack, a helper method for acknowledging events incoming from Slack
const ack = async (response: Record<string, unknown>): Promise<void> => {
if (this.logger.getLevel() === LogLevel.DEBUG) {
this.logger.debug(`Calling ack() - type: ${event.type}, envelope_id: ${event.envelope_id}, data: ${JSON.stringify(response)}`);
this.logger.debug(`Calling ack() - type: ${event.type}, envelope_id: ${event.envelope_id}, data: ${JSON.stringify(SocketModeClient.redact(response))}`);

Check warning on line 329 in packages/socket-mode/src/SocketModeClient.ts

View check run for this annotation

Codecov / codecov/patch

packages/socket-mode/src/SocketModeClient.ts#L329

Added line #L329 was not covered by tests
}
await this.send(event.envelope_id, response);
};
Expand Down Expand Up @@ -386,9 +387,8 @@
} else {
this.emit('outgoing_message', message);

const flatMessage = JSON.stringify(message);
this.logger.debug(`Sending a WebSocket message: ${flatMessage}`);
this.websocket.send(flatMessage, (error) => {
this.logger.debug(`Sending a WebSocket message: ${JSON.stringify(SocketModeClient.redact(message))}`);
this.websocket.send(JSON.stringify(message), (error) => {

Check warning on line 391 in packages/socket-mode/src/SocketModeClient.ts

View check run for this annotation

Codecov / codecov/patch

packages/socket-mode/src/SocketModeClient.ts#L390-L391

Added lines #L390 - L391 were not covered by tests
if (error) {
this.logger.error(`Failed to send a WebSocket message (error: ${error})`);
return reject(websocketErrorWithOriginal(error));
Expand All @@ -398,6 +398,37 @@
}
});
}

/**
* Removes secrets and tokens from socket request and response objects
* before logging.
* @param body - the object with values for redaction.
* @returns the same object with redacted values.
*/
private static redact(body?: Record<string, unknown>): Record<string, unknown> | unknown[] | undefined {
Copy link
Contributor

Choose a reason for hiding this comment

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

What may help in this function is to define a recursive utility type that encapsulates Record<string, unknown>; something that signifies "an array of objects that itself can contain objects (..and so on), or an object that itself can contain objects (..and so on)." I think that could help you eliminate the type casting below. Perhaps something like:

type Something = Record<string, Something> | Something[];

In theory, in my head, this could be helpful to hint to TS how to derive types as you dissect the thing to redact. I have hit problems with recursive types in TS before, but give it a shot and see if that helps!

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted this to work... Found that recursive types sometimes won't work? It might be caused by a certain version of TS cause it seemed to have worked in some earlier version but not in this version 😢

if (body === undefined || body === null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The type of the body parameter and the values we are checking here don't line up. Should the parameter be optional? Since this is a private method, it's up to us; I would imagine we want to ensure this method is always called with something. Additionally, since we are checking for null here, should body be assignable to null?

Copy link
Member Author

Choose a reason for hiding this comment

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

Great callout with the optional body? - that was poor typing on my part! Also a nice catch with null. That shouldn't be possible and I was guarding to much... undefined is possible - via ack() - and seems to work well as is, but the rest was fixed!

return body;
}

Check warning on line 411 in packages/socket-mode/src/SocketModeClient.ts

View check run for this annotation

Codecov / codecov/patch

packages/socket-mode/src/SocketModeClient.ts#L410-L411

Added lines #L410 - L411 were not covered by tests
const record = Object.create(body);
if (Array.isArray(body)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as above: should body be assignable to an Array?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not directly, but it's possible that keys somewhere within the body contain an Array, which would be hit in some recursive case. Possibly with details from blocks or actions or inputs.

return body.map((item) => (
(typeof item === 'object' && item !== null) ?
SocketModeClient.redact(item as Record<string, unknown>) :
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you may require type casting here because my previous comment is unaddressed: if body is assignable to an array (of, presumably, Record<string, unknown>), then the map here could infer that item is a Record<string, unknown>, so then you may not need to type cast anymore.

I know that in a variety of places within node-slack-sdk and bolt-js today we use type casting liberally. In general (not that I'm a TS expert) my understanding is type casting is a sign that the types of variables are not thorough enough. Moving forward with the node SDK and bolt-js, I would like for us to, when possible, consider type casting as a last resort and avoid them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Found the PR that fixes some coverups caused by blunt casting! slackapi/bolt-js#1806

I am wary of casting and ought to take this lesson to heart more 😳

item

Check warning on line 417 in packages/socket-mode/src/SocketModeClient.ts

View check run for this annotation

Codecov / codecov/patch

packages/socket-mode/src/SocketModeClient.ts#L417

Added line #L417 was not covered by tests
));
}
Object.keys(body).forEach((key: string) => {
const value = body[key];
if (typeof value === 'object' && value !== null) {
record[key] = SocketModeClient.redact(value as Record<string, unknown>);
} else if (key.match(/.*token.*/) !== null || key.match(/secret/)) {
record[key] = '[[REDACTED]]';
} else {
record[key] = value;
}
});
return record;
}
}

/* Instrumentation */
Expand Down