SLACK_API_TOKEN in SlackError

[levenleven]

SlackError contains env.response, including request headers and SLACK_API_TOKEN

https://github.com/slack-ruby/slack-ruby-client/blob/f2423d46e86f655444c2e2d4e51e0eff285a5ecf/lib/slack/web/faraday/response/raise_error.rb#L19

This can be a foot gun as the token can be accidentally leaked by e.g. logging the error puts err.inspect.

Maybe we could auto redact auth header or even avoid storing whole response in the error instance and keep only e.g. body/status (but I guess this would be a breaking change)?

[dblock]

You raise a good point. I'd rather not introduce a breaking change here eliminating the header. How do other libraries deal with this?

[dblock]

Claude suggested two approaches, both modifying inspect.

⏺ Update(lib/slack/web/api/errors/slack_error.rb)
  ⎿  User rejected update to lib/slack/web/api/errors/slack_error.rb
       22              def response_metadata
       23                response.body.response_metadata
       24              end
       25 +  
       26 +            def inspect
       27 +              redacted_response = if @response
       28 +                                    response_copy = @response.dup
       29 +                                    if response_copy.respond_to?(:env) && response_copy.env[:request_headers]
       30 +                                      headers = response_copy.env[:request_headers].dup
       31 +                                      headers['Authorization'] = '[REDACTED]' if headers['Authorization']
       32 +                                      response_copy.instance_variable_set(:@env, response_copy.env.merge(request_headers: headers))
       33 +                                    end
       34 +                                    response_copy
       35 +                                  end
       36 +  
       37 +              "#<#{self.class.name}: #{message.inspect}, response=#{redacted_response.inspect}>"
       38 +            end
       39            end
       40          end
       41        end

or

│ │   25 +                                                                                                                                                                              │ │
│ │   26 +            def inspect                                                                                                                                                       │ │
│ │   27 +              response_info = if @response                                                                                                                                    │ │
│ │   28 +                                status = @response.status if @response.respond_to?(:status)                                                                                   │ │
│ │   29 +                                body = @response.body if @response.respond_to?(:body)                                                                                         │ │
│ │   30 +                                "status=#{status}, body=#{body.inspect}"                                                                                                      │ │
│ │   31 +                              else                                                                                                                                            │ │
│ │   32 +                                'nil'                                                                                                                                         │ │
│ │   33 +                              end                                                                                                                                             │ │
│ │   34 +                                                                                                                                                                              │ │
│ │   35 +              "#<#{self.class.name}: #{message.inspect}, response=(#{response_info})>"                                                                                        │ │
│ │   36 +            end                                                                                                                                                               │ │
│ │   37            end                                                                                                                                                                 │ │
│ │   38          end                                                                                                                                                                   │ │
│ │   39        end                          

Thoughts?

[levenleven]

both modifying inspect

That works, but I'd rather .dup and redact the header before storing response in the error as we still keep it accessible from the error instance. This would cover any interaction with it and not only err.inspect case.

[dblock]

@levenleven want to make a PR along those lines? doesn't have to be that code