Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: main
Are you sure you want to change the base?
socket-mode(fix): redact ephemeral tokens and secrets from debug logs #1832
Changes from 1 commit
d097cd6
b4718d8
e7a1166
3147e08
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 329 in packages/socket-mode/src/SocketModeClient.ts
Codecov / codecov/patch
packages/socket-mode/src/SocketModeClient.ts#L329
Check warning on line 391 in packages/socket-mode/src/SocketModeClient.ts
Codecov / codecov/patch
packages/socket-mode/src/SocketModeClient.ts#L390-L391
There was a problem hiding this comment.
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: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!
There was a problem hiding this comment.
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 😢
There was a problem hiding this comment.
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 fornull
here, shouldbody
be assignable tonull
?There was a problem hiding this comment.
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 withnull
. That shouldn't be possible and I was guarding to much...undefined
is possible - viaack()
- and seems to work well as is, but the rest was fixed!Check warning on line 411 in packages/socket-mode/src/SocketModeClient.ts
Codecov / codecov/patch
packages/socket-mode/src/SocketModeClient.ts#L410-L411
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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
oractions
orinputs
.There was a problem hiding this comment.
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 themap
here could infer thatitem
is aRecord<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.
There was a problem hiding this comment.
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 😳
Check warning on line 417 in packages/socket-mode/src/SocketModeClient.ts
Codecov / codecov/patch
packages/socket-mode/src/SocketModeClient.ts#L417