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 all 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
58 changes: 52 additions & 6 deletions packages/socket-mode/src/SocketModeClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,21 @@
Authenticated = 'authenticated',
}

/**
* Recursive definition for what a value might contain.
*/
interface Nesting {
[key: string]: NestedRecord | unknown;
}

/**
* Recursive definiton for possible JSON object values.
*
* FIXME: Prefer using a circular reference if allowed:
* Record<string, NestedRecord> | NestedRecord[]
Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately this was tossing an error in the editor... Some prior art was tried but without much luck...

Type alias 'NestedRecord' circularly references itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, don't sweat it! We do some seriously nasty gymnastics in deno-slack-sdk to avoid this; I do not recommend it, though. Roll with the punches and move on!

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 am oddly impressed with this magic. I wish this was magic tsc took care of though and I didn't have to learn the tricks 😉 🪄

*/
type NestedRecord = Nesting | NestedRecord[];

/**
* A Socket Mode Client allows programs to communicate with the
* [Slack Platform's Events API](https://api.slack.com/events-api) over WebSocket connections.
Expand Down Expand Up @@ -284,8 +299,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 +312,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 +341,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 344 in packages/socket-mode/src/SocketModeClient.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L344 was not covered by tests
}
await this.send(event.envelope_id, response);
};
Expand Down Expand Up @@ -386,9 +402,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 406 in packages/socket-mode/src/SocketModeClient.ts

View check run for this annotation

Codecov / codecov/patch

packages/socket-mode/src/SocketModeClient.ts#L405-L406

Added lines #L405 - L406 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 +413,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: NestedRecord): NestedRecord {
if (body === undefined) {
return body;
}

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

View check run for this annotation

Codecov / codecov/patch

packages/socket-mode/src/SocketModeClient.ts#L425-L426

Added lines #L425 - L426 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) :
item

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

View check run for this annotation

Codecov / codecov/patch

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

Added line #L432 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 NestedRecord);
Copy link
Member Author

Choose a reason for hiding this comment

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

This cast seems needed even with the recursive definition above... 🤔 I fumbled a bit with it without too much luck.

Argument of type 'object' is not assignable to parameter of type 'NestedRecord'.

} else if (key.match(/.*token.*/) !== null || key.match(/secret/)) {
record[key] = '[[REDACTED]]';
} else {
record[key] = value;
}
});
return record;
}
}

/* Instrumentation */
Expand Down