fix: Redact tokens in query and response logs (#545)#694
Closed
ad-claw000 wants to merge 1 commit into
Closed
Conversation
Contributor
Author
|
Closing in favor of older PR #672 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #545 by preventing authentication secrets (passwords and tokens) from being exposed in SDK logs when Authenticate (or subsequent queries around authentication) fail, by scrubbing sensitive fields before formatting them for logging.
Changes:
- Redacts
session_token/refresh_tokenwhen stringifyingConnector.last_responseviaConnector.get_last_response_str(). - Redacts
password/token/session_token/refresh_tokeninexecute_query()error logs by deep-copying and scrubbing the query/response objects before logging.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| aperturedb/Connector.py | Scrubs auth tokens from the “last response” string representation used in error/diagnostic logging. |
| aperturedb/CommonLibrary.py | Scrubs auth secrets from failed-transaction error logs to avoid leaking credentials/tokens. |
Comments suppressed due to low confidence (2)
aperturedb/Connector.py:656
- Redaction here only looks for
Authenticateresponses.last_responsecan also be aRefreshTokenresponse (see_refresh_token()which readssession_token/refresh_tokenfromresponse[0]["RefreshToken"]), andget_last_response_str()would currently serialize those tokens unredacted. Expanding the scrub to coverRefreshTokenwould prevent token leakage when a refresh occurs before a subsequent failing query.
if isinstance(safe_response, list):
for item in safe_response:
if isinstance(item, dict) and "Authenticate" in item:
auth_dict = item["Authenticate"]
if isinstance(auth_dict, dict):
if "session_token" in auth_dict:
auth_dict["session_token"] = "***"
if "refresh_token" in auth_dict:
auth_dict["refresh_token"] = "***"
aperturedb/CommonLibrary.py:333
- This redaction logic only handles
Authenticateobjects; queries/responses involvingRefreshTokencan also containrefresh_token/session_tokenfields (Connector._refresh_token expects them). As a result,execute_querycan still log sensitive tokens when a refresh-token transaction fails. Consider scrubbing the same fields underRefreshTokenas well (and ideally using a small shared scrubber to avoid missing future auth-related commands).
for obj in (safe_query, safe_r):
if isinstance(obj, list):
for item in obj:
if isinstance(item, dict) and "Authenticate" in item:
auth_dict = item["Authenticate"]
if isinstance(auth_dict, dict):
if "password" in auth_dict:
auth_dict["password"] = "***"
if "token" in auth_dict:
auth_dict["token"] = "***"
if "session_token" in auth_dict:
auth_dict["session_token"] = "***"
if "refresh_token" in auth_dict:
auth_dict["refresh_token"] = "***"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+646
to
+659
| try: | ||
| safe_response = copy.deepcopy(self.last_response) | ||
| if isinstance(safe_response, list): | ||
| for item in safe_response: | ||
| if isinstance(item, dict) and "Authenticate" in item: | ||
| auth_dict = item["Authenticate"] | ||
| if isinstance(auth_dict, dict): | ||
| if "session_token" in auth_dict: | ||
| auth_dict["session_token"] = "***" | ||
| if "refresh_token" in auth_dict: | ||
| auth_dict["refresh_token"] = "***" | ||
| return json.dumps(safe_response, indent=4, sort_keys=False) | ||
| except Exception: | ||
| return json.dumps(self.last_response, indent=4, sort_keys=False) |
Comment on lines
+316
to
+318
| import copy | ||
| safe_query = copy.deepcopy(query) | ||
| safe_r = copy.deepcopy(r) |
Comment on lines
641
to
+657
| def get_last_response_str(self): | ||
| import copy | ||
| if not self.last_response: | ||
| return "[]" | ||
|
|
||
| return json.dumps(self.last_response, indent=4, sort_keys=False) | ||
| try: | ||
| safe_response = copy.deepcopy(self.last_response) | ||
| if isinstance(safe_response, list): | ||
| for item in safe_response: | ||
| if isinstance(item, dict) and "Authenticate" in item: | ||
| auth_dict = item["Authenticate"] | ||
| if isinstance(auth_dict, dict): | ||
| if "session_token" in auth_dict: | ||
| auth_dict["session_token"] = "***" | ||
| if "refresh_token" in auth_dict: | ||
| auth_dict["refresh_token"] = "***" | ||
| return json.dumps(safe_response, indent=4, sort_keys=False) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When an
Authenticatequery fails or a subsequent query fails and the last response was from authentication, the error logs were exposingrefresh_token,session_token, and passwords. This change redacts them.Changes
aperturedb/Connector.py: Modifiedget_last_response_strto deepcopy and scrubsession_tokenandrefresh_tokeninsideAuthenticate.aperturedb/CommonLibrary.py: Modified error logging inexecute_queryto also redact tokens/passwords in the query and response JSON before logging.Fixes #545