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

zimeg · 2024-06-25T03:12:07Z

Summary

This PR replaces values that should be redacted in debug logs - such as the ephemeral bot_access_token used in custom functions and interactor.secret - with [[REDACTED]].

Preview

Example output differences are found towards the end:

- [DEBUG]  socket-mode:SocketModeClient:0 Received a message on the WebSocket: {"envelope_id":"15726246-7de2-491a-95ed-09ba986b1c6b","payload":{"type":"block_actions","team":{"id":"T038J6TH5PF","domain":"sandbox"},"enterprise":null,"user":{"id":"U03SV4BFTJP","name":"me","team_id":"T038J6TH5PF"},"channel":{"id":"D079CP6N3GW","name":"directmessage"},"message":{"user":"U0794R4RBLP","type":"message","ts":"1719282819.865949","bot_id":"B07A85ZNJMN","app_id":"A079CP1699U","text":"Placeholder text","team":"T038J6TH5PF","blocks":[{"type":"section","block_id":"I9kjf","text":{"type":"mrkdwn","text":"Placeholder text","verbatim":false},"accessory":{"type":"button","action_id":"sample_button","text":{"type":"plain_text","text":"Complete function","emoji":true}}}]},"container":{"type":"message","message_ts":"1719282819.865949","channel_id":"D079CP6N3GW","is_ephemeral":false},"actions":[{"block_id":"I9kjf","action_id":"sample_button","type":"button","text":{"type":"plain_text","text":"Complete function","emoji":true},"action_ts":"1719282823.103643"}],"api_app_id":"A079CP1699U","state":{"values":{}},"bot_access_token":"xwfp-example-001","function_data":{"execution_id":"Fx079KPY1T6X","function":{"callback_id":"sample_function"},"inputs":{"user_id":"U03SV4BFTJP"}},"interactivity":{"interactor":{"secret":"someothervalue","id":"U03SV4BFTJP"},"interactivity_pointer":"7323025435029.3290231583797.b4a081dcb5a60882b1eea4d00e4dbc28"}},"type":"interactive","accepts_response_payload":false}
+ [DEBUG]  socket-mode:SocketModeClient:0 Received a message on the WebSocket: {"envelope_id":"15726246-7de2-491a-95ed-09ba986b1c6b","payload":{"type":"block_actions","team":{"id":"T038J6TH5PF","domain":"sandbox"},"enterprise":null,"user":{"id":"U03SV4BFTJP","name":"me","team_id":"T038J6TH5PF"},"channel":{"id":"D079CP6N3GW","name":"directmessage"},"message":{"user":"U0794R4RBLP","type":"message","ts":"1719282819.865949","bot_id":"B07A85ZNJMN","app_id":"A079CP1699U","text":"Placeholder text","team":"T038J6TH5PF","blocks":[{"type":"section","block_id":"I9kjf","text":{"type":"mrkdwn","text":"Placeholder text","verbatim":false},"accessory":{"type":"button","action_id":"sample_button","text":{"type":"plain_text","text":"Complete function","emoji":true}}}]},"container":{"type":"message","message_ts":"1719282819.865949","channel_id":"D079CP6N3GW","is_ephemeral":false},"actions":[{"block_id":"I9kjf","action_id":"sample_button","type":"button","text":{"type":"plain_text","text":"Complete function","emoji":true},"action_ts":"1719282823.103643"}],"api_app_id":"A079CP1699U","state":{"values":{}},"bot_access_token":"[[REDACTED]]","function_data":{"execution_id":"Fx079KPY1T6X","function":{"callback_id":"sample_function"},"inputs":{"user_id":"U03SV4BFTJP"}},"interactivity":{"interactor":{"secret":"[[REDACTED]]","id":"U03SV4BFTJP"},"interactivity_pointer":"7323025435029.3290231583797.b4a081dcb5a60882b1eea4d00e4dbc28"}},"type":"interactive","accepts_response_payload":false}

Reviewers

These changes resemble those found in #1831 but with updates to match refactors from socket-mode@v2. At this time it's not so quick to test with Bolt but the test cases will hopefully give some confidence 🙏

Notes

Similar changes are applied to both socket-mode@latest on main and the [email protected] branch to backport for Bolt JS.

Requirements

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

codecov · 2024-06-25T03:13:46Z

Codecov Report

Attention: Patch coverage is 84.61538% with 8 lines in your changes missing coverage. Please review.

Project coverage is 82.05%. Comparing base (88e4b38) to head (3147e08).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1832      +/-   ##
==========================================
+ Coverage   81.85%   82.05%   +0.19%     
==========================================
  Files          35       35              
  Lines        7782     7829      +47     
  Branches      318      331      +13     
==========================================
+ Hits         6370     6424      +54     
+ Misses       1400     1393       -7     
  Partials       12       12

Flag	Coverage Δ
cli-hooks	`95.07% <ø> (ø)`
cli-test	`54.16% <ø> (+0.02%)`	⬆️
oauth	`76.53% <ø> (ø)`
socket-mode	`63.07% <84.61%> (+3.48%)`	⬆️
web-api	`96.55% <ø> (ø)`
webhook	`95.27% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

seratch

Thanks for working on this! You can continue the review with @filmaj tomorrow

packages/socket-mode/package.json

packages/socket-mode/src/SocketModeClient.ts

filmaj

Left some comments and things to try out to try to avoid type casting - I would like to stop doing that moving forward in our TS libraries.

filmaj · 2024-06-25T12:22:35Z

packages/socket-mode/src/SocketModeClient.ts

+   * @returns the same object with redacted values.
+   */
+  private static redact(body?: Record<string, unknown>): Record<string, unknown> | unknown[] | undefined {
+    if (body === undefined || body === null) {


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?

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!

filmaj · 2024-06-25T12:23:20Z

packages/socket-mode/src/SocketModeClient.ts

+      return body;
+    }
+    const record = Object.create(body);
+    if (Array.isArray(body)) {


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

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.

filmaj · 2024-06-25T12:24:54Z

packages/socket-mode/src/SocketModeClient.ts

+    if (Array.isArray(body)) {
+      return body.map((item) => (
+        (typeof item === 'object' && item !== null) ?
+          SocketModeClient.redact(item as Record<string, unknown>) :


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.

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 😳

filmaj · 2024-06-25T12:32:48Z

packages/socket-mode/src/SocketModeClient.ts

+   * @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 {


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!

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 😢

zimeg

@filmaj great callouts with the typings! Made a few updates here but still found some troubles with casting one last one... Tested this and things are working though!

zimeg · 2024-06-25T20:51:13Z

packages/socket-mode/src/SocketModeClient.ts

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


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

Type alias 'NestedRecord' circularly references itself.

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!

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 😉 🪄

zimeg · 2024-06-25T20:52:05Z

packages/socket-mode/src/SocketModeClient.ts

+    Object.keys(body).forEach((key: string) => {
+      const value = body[key];
+      if (typeof value === 'object' && value !== null) {
+        record[key] = SocketModeClient.redact(value as NestedRecord);


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'.

zimeg · 2024-06-25T20:54:08Z

packages/socket-mode/src/SocketModeClient.ts

+      return body;
+    }
+    const record = Object.create(body);
+    if (Array.isArray(body)) {


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.

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

d097cd6

zimeg added bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented semver:patch security pkg:socket-mode applies to `@slack/socket-mode` release labels Jun 25, 2024

zimeg added this to the [email protected] milestone Jun 25, 2024

zimeg self-assigned this Jun 25, 2024

seratch reviewed Jun 25, 2024

View reviewed changes

packages/socket-mode/package.json Outdated Show resolved Hide resolved

packages/socket-mode/src/SocketModeClient.ts Outdated Show resolved Hide resolved

seratch requested a review from filmaj June 25, 2024 03:20

zimeg added 2 commits June 24, 2024 20:23

chore(release): revert bump to @slack/[email protected]

b4718d8

style: rename the object records to be the singular record

e7a1166

zimeg removed the release label Jun 25, 2024

filmaj requested changes Jun 25, 2024

View reviewed changes

fix: improve nested record typings and remove a casting

3147e08

zimeg commented Jun 25, 2024

View reviewed changes

zimeg modified the milestones: [email protected], [email protected] Aug 15, 2024

filmaj modified the milestones: [email protected], [email protected] Oct 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

zimeg commented Jun 25, 2024 •

edited

Loading

codecov bot commented Jun 25, 2024 •

edited

Loading

seratch left a comment

filmaj left a comment

filmaj Jun 25, 2024

zimeg Jun 25, 2024

filmaj Jun 25, 2024

zimeg Jun 25, 2024

filmaj Jun 25, 2024

zimeg Jun 25, 2024

filmaj Jun 25, 2024

zimeg Jun 25, 2024

zimeg left a comment

zimeg Jun 25, 2024

filmaj Jun 25, 2024

zimeg Jun 25, 2024

zimeg Jun 25, 2024

zimeg Jun 25, 2024

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

Are you sure you want to change the base?

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

Conversation

zimeg commented Jun 25, 2024 • edited Loading

Summary

Preview

Reviewers

Notes

Requirements

codecov bot commented Jun 25, 2024 • edited Loading

Codecov Report

seratch left a comment

Choose a reason for hiding this comment

filmaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zimeg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zimeg commented Jun 25, 2024 •

edited

Loading

codecov bot commented Jun 25, 2024 •

edited

Loading