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

Way to disable message content being logged #1751

Closed
gaurav512 opened this issue Feb 23, 2024 · 3 comments · May be fixed by petersem/monocker#96 or petersem/monocker#98
Closed

Way to disable message content being logged #1751

gaurav512 opened this issue Feb 23, 2024 · 3 comments · May be fixed by petersem/monocker#96 or petersem/monocker#98
Labels
enhancement M-T: A feature request for new functionality good first issue pkg:web-api applies to `@slack/web-api`

Comments

@gaurav512
Copy link

Hi,

I am looking for a way to never output / always hide the message contents from the application logs. If such way already exists, I would like to know how.

Use-case

I am building an app which contains sensitive content in the messages that I do not want to be logged in the application logs.

My LogLevel is 'INFO'.
I recently saw some API failure logs in my application, which in turn resulted in the message data being logged in the app logs.

Error: A request error occurred: read ECONNRESET
    at requestErrorWithOriginal (/app/node_modules/@slack/web-api/dist/errors.js:33:33)
    at /app/node_modules/@slack/web-api/dist/WebClient.js:433:65
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async run (/app/node_modules/@slack/web-api/node_modules/p-queue/dist/index.js:163:29) {
  code: 'slack_webapi_request_error',
  original: AxiosError: read ECONNRESET
      at Function.AxiosError.from (/app/node_modules/@slack/web-api/node_modules/axios/dist/node/axios.cjs:837:14)
      at ClientRequest.handleRequestError (/app/node_modules/@slack/web-api/node_modules/axios/dist/node/axios.cjs:3083:25)
      at ClientRequest.emit (node:events:513:28)
      at TLSSocket.socketErrorListener (node:_http_client:494:9)
      at TLSSocket.emit (node:events:513:28)
      at emitErrorNT (node:internal/streams/destroy:157:8)
      at emitErrorCloseNT (node:internal/streams/destroy:122:3)
      at processTicksAndRejections (node:internal/process/task_queues:83:21) {
    syscall: 'read',
    code: 'ECONNRESET',
    errno: -104,
    config: {
      transitional: [Object],
      adapter: [Array],
      transformRequest: [Array],
      transformResponse: [Array],
      timeout: 0,
      xsrfCookieName: 'XSRF-TOKEN',
      xsrfHeaderName: 'X-XSRF-TOKEN',
      maxContentLength: -1,
      maxBodyLength: -1,
      env: [Object],
      validateStatus: [Function: validateStatus],
      headers: [Object [AxiosHeaders]],
      baseURL: 'https://slack.com/api/',
      maxRedirects: 0,
      proxy: false,
      method: 'post',
      url: 'views.publish',
      data: <MY MESSAGE DATA>
	  ...
	  ...

Reproducible in:

The Slack SDK version

"slack/bolt": "^3.13.3",
"slack/logger": "^3.0.0",
"slack/web-api": "^6.9.1"

Node.js runtime version

v16.8.1

OS info

Red Hat Enterprise Linux 8

Steps to reproduce:

  1. App error logs (in case of internal failures) contain message data in the logs

Expected result:

Application logs contain message data.

Actual result:

I want to exclude this message data from the application logs.

Requirements

For general questions/issues about Slack API platform or its server-side, could you submit questions at https://my.slack.com/help/requests/new instead. 🙇

Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you are agreeing to those rules.

@srajiang srajiang added enhancement M-T: A feature request for new functionality and removed untriaged labels Feb 23, 2024
@seratch
Copy link
Member

seratch commented Feb 26, 2024

Thanks for reporting this. This could be improved on the underlying web-api package side (https://github.com/slackapi/node-slack-sdk/blob/%40slack/web-api%407.0.2/packages/web-api/src/errors.ts#L79), so let me transfer this to node-slack-sdk repo.

@seratch seratch transferred this issue from slackapi/bolt-js Feb 26, 2024
@seratch seratch added the pkg:web-api applies to `@slack/web-api` label Feb 26, 2024
@seratch seratch added this to the [email protected] milestone Feb 26, 2024
@Parama92
Copy link
Contributor

Hi! New contributor here 👋

Upon reviewing this issue, I noticed that -

  • The Error type - WebAPIRequestError (the error shared in the issue description), contains a property called original which holds the raw error received from axios

  • The config property within this original is a part of the error data received from axios. However, its presence doesn't seem to depend on any configuration (as evident here) that can be set while creating the axios client here.

Potential solution -

  • Making the original property optional would be the easiest solution, although this might result in losing some crucial details about the initiated request.

  • Alternatively, removing the data property from original.config could address the issue, albeit with the caveat of relying on another library to maintain its contract.

In my opinion, opting for the first solution seems most sensible. What are your thoughts on this approach?

@seratch
Copy link
Member

seratch commented Apr 30, 2024

Thanks for the comment! I agree that the simplest solution would be to provide an opt-out option, allowing for the removal of the original property. Although I haven't explored in depth, adding a flag option like attachOriginalToWebAPIRequestError? : boolean to WebClientOptions should work well. If the property doesn't exist, the flag should default to true for the consistency with existing behavior. The naming may sound somewhat
verbose, but I believe being specific here should not be so bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality good first issue pkg:web-api applies to `@slack/web-api`
Projects
None yet
5 participants