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

Add debug authorization header flag #754

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

mgyucht
Copy link
Contributor

@mgyucht mgyucht commented Jan 3, 2024

Changes

DebugHeaders by default prints all headers, including the sensitive Authorization header. To allow users to print debug logs with headers but without including this sensitive header, we introduce a new flag, DebugAuthorizationHeader, which is disabled by default for users. This way, enabling debug headers does not automatically leak the token used to authenticate to our REST API.

Tests

  • make test passing
  • make fmt applied
  • relevant integration tests applied

@mgyucht mgyucht requested a review from tanmay-db January 3, 2024 09:38
@@ -257,8 +258,8 @@ func (c *ApiClient) recordRequestLog(
RequestBody: requestBody,
ResponseBody: responseBody,
DebugHeaders: c.config.DebugHeaders,
DebugAuthorizationHeader: c.config.DebugAuthorizationHeader,
Copy link
Contributor

Choose a reason for hiding this comment

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

you're missing the X-Databricks-Azure-SP-Management-Token and X-Databricks-GCP-SA-Access-Token headers in https://github.com/databricks/databricks-sdk-go/blob/main/logger/httplog/round_trip_stringer.go#L33-L35

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you reviewed a stale version but I added this in the last commit

@codecov-commenter
Copy link

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (16e1116) 17.78% compared to head (e877dbd) 17.79%.
Report is 3 commits behind head on main.

Files Patch % Lines
apierr/errors.go 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #754      +/-   ##
==========================================
+ Coverage   17.78%   17.79%   +0.01%     
==========================================
  Files         106      106              
  Lines       14199    14201       +2     
==========================================
+ Hits         2525     2527       +2     
  Misses      11463    11463              
  Partials      211      211              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@tanmay-db tanmay-db left a comment

Choose a reason for hiding this comment

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

Since by default we prevent printing of token, I think this makes user experience more safe. LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants