Skip to content

fix(web-api): redact response body in debug logs#2652

Merged
srtaalej merged 8 commits into
mainfrom
fix/redact-response-body-debug-logs
Jul 9, 2026
Merged

fix(web-api): redact response body in debug logs#2652
srtaalej merged 8 commits into
mainfrom
fix/redact-response-body-debug-logs

Conversation

@srtaalej

@srtaalej srtaalej commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Apply redact() to API response bodies before debug logging, consistent with how request bodies and headers are already handled
  • Widen redact() parameter type from Record<string, unknown> to object so it accepts WebAPICallResult without casting
  • Prevents tokens returned in API responses (e.g., oauth.v2.access, openid.connect.token, tooling.tokens.rotate) from being written to logs in plaintext when debug logging is enabled

Test plan

  • npm run build --workspace=packages/web-api passes
  • npm test --workspace=packages/web-api — all 155 tests pass
  • Verify manually: enable LogLevel.DEBUG, call oauth.v2.access, confirm access_token shows as [[REDACTED]] in output

Fixes #2651

Apply redact() to API response bodies before debug logging, consistent
with how request bodies and headers are already handled. This prevents
tokens returned in API responses (e.g., oauth.v2.access, openid.connect.token)
from being written to logs in plaintext when debug logging is enabled.

Fixes #2651
@srtaalej srtaalej requested a review from a team as a code owner July 8, 2026 20:01
@changeset-bot

changeset-bot Bot commented Jul 8, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 4fc31ac

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@slack/web-api Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.04%. Comparing base (2b6a0ce) to head (4fc31ac).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2652      +/-   ##
==========================================
- Coverage   89.06%   89.04%   -0.02%     
==========================================
  Files          65       65              
  Lines       10436    10436              
  Branches      471      468       -3     
==========================================
- Hits         9295     9293       -2     
  Misses       1120     1120              
- Partials       21       23       +2     
Flag Coverage Δ
cli-hooks 89.04% <100.00%> (-0.02%) ⬇️
cli-test 89.04% <100.00%> (-0.02%) ⬇️
logger 89.04% <100.00%> (-0.02%) ⬇️
oauth 89.04% <100.00%> (-0.02%) ⬇️
socket-mode 89.04% <100.00%> (-0.02%) ⬇️
web-api 89.04% <100.00%> (-0.02%) ⬇️
webhook 89.04% <100.00%> (-0.02%) ⬇️

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

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@srtaalej srtaalej self-assigned this Jul 8, 2026
@srtaalej srtaalej added pkg:web-api applies to `@slack/web-api` bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented semver:patch labels Jul 8, 2026
@srtaalej srtaalej requested a review from WilliamBergamin July 8, 2026 20:07

@WilliamBergamin WilliamBergamin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a solid change 💯

I think we might want to gate the redaction logic behind a try catch so that it does not raise an error if it fails and rather logs the problem 🤔 let me know what you think of this

If we could add some tests for this it would be nice

Comment thread packages/web-api/src/WebClient.ts

@srtaalej srtaalej left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

thank you for reviewing @WilliamBergamin! i agree with these changes except that the existing debug lines on 702 and 721 don't include these guards. should we add them there too to stay consistent?

@WilliamBergamin

Copy link
Copy Markdown
Contributor

Ohh actually by bad, the object we pass to redact was already processed by buildResult it isn't the raw response 😅
I don't think this guard is needed after all

What do you think about just adding a unit test to make sure the redaction works 🙏

Adds a unit test verifying that tokens in API response bodies are
redacted before being written to debug logs.

Fixes #2651
@srtaalej srtaalej requested a review from WilliamBergamin July 9, 2026 14:37
@srtaalej

srtaalej commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

added unit test and left redact changes as is! can i get a quick approve @WilliamBergamin?

@WilliamBergamin WilliamBergamin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking good 💯 left one suggestion for the test 🙏

Comment thread packages/web-api/src/WebClient.test.ts Outdated
Comment thread packages/web-api/src/WebClient.test.ts Outdated
srtaalej and others added 5 commits July 9, 2026 10:56
Co-authored-by: William Bergamin <wbergamin@salesforce.com>
Co-authored-by: William Bergamin <wbergamin@salesforce.com>
The redact() function previously only operated on top-level keys,
meaning nested tokens (e.g., authed_user.access_token in oauth.v2.access
responses) would still appear in debug logs. Now recurses into nested
objects to redact token keys at any depth.
@srtaalej srtaalej requested a review from WilliamBergamin July 9, 2026 17:47

@WilliamBergamin WilliamBergamin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice fix 💯

@srtaalej srtaalej merged commit bb49d99 into main Jul 9, 2026
22 of 24 checks passed
@srtaalej srtaalej deleted the fix/redact-response-body-debug-logs branch July 9, 2026 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented pkg:web-api applies to `@slack/web-api` semver:patch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Response body not redacted in debug logs

2 participants