Update debugging for token #142
Conversation
WalkthroughThe change enhances error handling and logging in the identity client by adding detailed context (endpoint, orgId, response body) to token request errors and removing sensitive token string logging from local claims retrieval. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/system/client/identity_client.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/system/client/identity_client.go (2)
internal/system/errors/error.go (2)
NewServerError(48-53)ErrorMessage(23-28)internal/system/errors/error_codes.go (1)
TOKEN_FETCH_FAILED(121-124)
| bodyBytes, _ := io.ReadAll(resp.Body) | ||
| errorMsg := fmt.Sprintf( | ||
| "Token endpoint returned status %d for org:%s. Endpoint: %s. Response: %s", | ||
| resp.StatusCode, orgId, endpoint, strings.TrimSpace(string(bodyBytes)), | ||
| ) | ||
| logger.Debug(errorMsg) | ||
| return "", errors2.NewServerError(errors2.ErrorMessage{ | ||
| Code: errors2.TOKEN_FETCH_FAILED.Code, | ||
| Message: errors2.TOKEN_FETCH_FAILED.Message, | ||
| Description: errorMsg, | ||
| }, err) | ||
| }, fmt.Errorf("token endpoint non-200: %d", resp.StatusCode)) |
There was a problem hiding this comment.
Security concern: Response body may contain sensitive information.
The error handling reads and logs the full response body from the token endpoint, which may contain sensitive details such as error messages with system information, partial credentials, or security-related data. Logging this at debug level could expose secrets in application logs.
Additionally, the error from io.ReadAll is ignored on line 251, which could silently hide I/O failures.
🔎 Suggested improvements
Consider these changes:
if resp.StatusCode != http.StatusOK {
- bodyBytes, _ := io.ReadAll(resp.Body)
+ bodyBytes, readErr := io.ReadAll(resp.Body)
+ if readErr != nil {
+ logger.Debug("Failed to read response body", log.Error(readErr))
+ bodyBytes = []byte("unable to read response body")
+ }
+ // Limit response body to avoid logging excessive data
+ responsePreview := string(bodyBytes)
+ if len(responsePreview) > 200 {
+ responsePreview = responsePreview[:200] + "..."
+ }
errorMsg := fmt.Sprintf(
- "Token endpoint returned status %d for org:%s. Endpoint: %s. Response: %s",
- resp.StatusCode, orgId, endpoint, strings.TrimSpace(string(bodyBytes)),
+ "Token endpoint returned status %d for org:%s. Endpoint: %s. Response: %s",
+ resp.StatusCode, orgId, endpoint, strings.TrimSpace(responsePreview),
)
logger.Debug(errorMsg)Alternatively, consider logging only the status code and endpoint without the response body, as token endpoint errors often don't require full response details for debugging.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bodyBytes, _ := io.ReadAll(resp.Body) | |
| errorMsg := fmt.Sprintf( | |
| "Token endpoint returned status %d for org:%s. Endpoint: %s. Response: %s", | |
| resp.StatusCode, orgId, endpoint, strings.TrimSpace(string(bodyBytes)), | |
| ) | |
| logger.Debug(errorMsg) | |
| return "", errors2.NewServerError(errors2.ErrorMessage{ | |
| Code: errors2.TOKEN_FETCH_FAILED.Code, | |
| Message: errors2.TOKEN_FETCH_FAILED.Message, | |
| Description: errorMsg, | |
| }, err) | |
| }, fmt.Errorf("token endpoint non-200: %d", resp.StatusCode)) | |
| if resp.StatusCode != http.StatusOK { | |
| bodyBytes, readErr := io.ReadAll(resp.Body) | |
| if readErr != nil { | |
| logger.Debug("Failed to read response body", log.Error(readErr)) | |
| bodyBytes = []byte("unable to read response body") | |
| } | |
| // Limit response body to avoid logging excessive data | |
| responsePreview := string(bodyBytes) | |
| if len(responsePreview) > 200 { | |
| responsePreview = responsePreview[:200] + "..." | |
| } | |
| errorMsg := fmt.Sprintf( | |
| "Token endpoint returned status %d for org:%s. Endpoint: %s. Response: %s", | |
| resp.StatusCode, orgId, endpoint, strings.TrimSpace(responsePreview), | |
| ) | |
| logger.Debug(errorMsg) | |
| return "", errors2.NewServerError(errors2.ErrorMessage{ | |
| Code: errors2.TOKEN_FETCH_FAILED.Code, | |
| Message: errors2.TOKEN_FETCH_FAILED.Message, | |
| Description: errorMsg, | |
| }, fmt.Errorf("token endpoint non-200: %d", resp.StatusCode)) | |
| } |
🤖 Prompt for AI Agents
internal/system/client/identity_client.go lines 251-261: the code reads and logs
the entire token endpoint response body (and ignores io.ReadAll error), which
may expose sensitive data; change it to safely read with error handling and
avoid logging full body — call io.ReadAll and handle the error (return a server
error if read fails), but do not include the raw body in logs: either omit the
body from the log or include a sanitized/truncated excerpt (e.g., first N bytes
with non-printables stripped) or just log status code and endpoint; ensure the
returned error description does not contain sensitive response contents.
| bodyBytes, _ := io.ReadAll(resp.Body) | ||
| errorMsg := fmt.Sprintf( | ||
| "Token endpoint returned status %d for org:%s. Endpoint: %s. Response: %s", | ||
| resp.StatusCode, orgId, endpoint, strings.TrimSpace(string(bodyBytes)), | ||
| ) | ||
| logger.Debug(errorMsg) | ||
| return "", errors2.NewServerError(errors2.ErrorMessage{ | ||
| Code: errors2.TOKEN_FETCH_FAILED.Code, | ||
| Message: errors2.TOKEN_FETCH_FAILED.Message, | ||
| Description: errorMsg, | ||
| }, err) | ||
| }, fmt.Errorf("token endpoint non-200: %d", resp.StatusCode)) |
There was a problem hiding this comment.
Security concern: Response body may contain sensitive information.
This error handling has the same issue as in requestToken (lines 251-261): the full response body from the token exchange endpoint is logged and included in error messages, potentially exposing sensitive information. The token exchange flow involves authentication tokens, making this especially sensitive.
Additionally, the error from io.ReadAll is ignored on line 323.
🔎 Suggested improvements
Apply the same fix pattern as suggested for lines 251-261:
if resp.StatusCode != http.StatusOK {
- bodyBytes, _ := io.ReadAll(resp.Body)
+ bodyBytes, readErr := io.ReadAll(resp.Body)
+ if readErr != nil {
+ logger.Debug("Failed to read response body", log.Error(readErr))
+ bodyBytes = []byte("unable to read response body")
+ }
+ // Limit response body to avoid logging excessive data
+ responsePreview := string(bodyBytes)
+ if len(responsePreview) > 200 {
+ responsePreview = responsePreview[:200] + "..."
+ }
errorMsg := fmt.Sprintf(
- "Token endpoint returned status %d for org:%s. Endpoint: %s. Response: %s",
- resp.StatusCode, orgId, endpoint, strings.TrimSpace(string(bodyBytes)),
+ "Token endpoint returned status %d for org:%s. Endpoint: %s. Response: %s",
+ resp.StatusCode, orgId, endpoint, strings.TrimSpace(responsePreview),
)
logger.Debug(errorMsg)Consider whether the response body is necessary at all for token exchange errors, as the status code and endpoint may be sufficient for debugging.
🤖 Prompt for AI Agents
In internal/system/client/identity_client.go around lines 323 to 333, the code
reads and logs the full response body (and ignores the io.ReadAll error), which
may leak sensitive token data; change the logic to handle the ReadAll error
(return or include a safe placeholder if read fails) and avoid logging or
embedding the full response body—either omit the body entirely from logs/errors
or replace it with a redacted/truncated placeholder (e.g., "<redacted-response>"
or first N bytes only) and include only status code and endpoint in the error
message returned to callers; ensure the processLogger.Debug call and the
returned errors2.NewServerError use the sanitized message and not the raw body.
3 participants
Purpose
Goals
Approach
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.