Skip to content

fix: properly set token_expiry_is_time_of_expiration and mask access token when logging #637

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.

Sign up for GitHub

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

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

fix: properly set token_expiry_is_time_of_expiration and mask access token when logging #637

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0764b63
set token_expiry_is_time_of_expiration in DeclarativeSingleUseRefresh…
lmossman Jul 3, 2025
4e6b9f2
mask access token when logging OAuth token refresh response
lmossman Jul 3, 2025
b1d5d05
properly handle case where no access key can be found in response
lmossman Jul 3, 2025
bf1eaf2
properly handle json exception
lmossman Jul 3, 2025
10a27b1
initialize response_json and json_exception to None at start
lmossman Jul 3, 2025
ab062f8
refactor _make_handled_request to be simpler
lmossman Jul 3, 2025
b24db1b
handle max recursion error
lmossman Jul 3, 2025
503dac6
ruff format
lmossman Jul 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2801,6 +2801,7 @@ def create_oauth_authenticator(
).eval(config),
scopes=model.scopes,
token_expiry_date_format=model.token_expiry_date_format,
token_expiry_is_time_of_expiration=bool(model.token_expiry_date_format),
message_repository=self._message_repository,
refresh_token_error_status_codes=model.refresh_token_updater.refresh_token_error_status_codes,
refresh_token_error_key=model.refresh_token_updater.refresh_token_error_key,
Expand Down
29 changes: 20 additions & 9 deletions airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,10 +217,26 @@ def _make_handled_request(self) -> Any:
data=self.build_refresh_request_body(),
headers=self.build_refresh_request_headers(),
)
# log the response even if the request failed for troubleshooting purposes

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: but wonder if there is a more straightforward way to write this logic. Something like

            # log the response even if the request failed for troubleshooting purposes
            self._log_response(response)
            response.raise_for_status()
            
            response_json = response.json()

            try:
                # extract the access token and add to secrets to avoid logging the raw value
                access_key = self._extract_access_token(response_json)
                if access_key is not None:
                  add_to_secrets(access_key)
            except ResponseKeysMaxRecurtionReached as e:
                # could not find the access token in the response, so do nothing
                pass
   
            return response_json

Copy link
Contributor

Choose a reason for hiding this comment

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

also, are we sure it makes sense to ignore the ResponseKeysMaxRecurtionReached exception? Admittedly this code is weird, but it seems like we could theoretically hit a max recursive depth but not fully explore the response json and therefore if we ignore the error, the access_key can actually be returned and we would not mask it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed this over zoom and came to a simpler implementation that we both liked

if not response.ok:
# log the response even if the request failed for troubleshooting purposes
self._log_response(response)
response.raise_for_status()

response_json = response.json()

try:
# extract the access token and add to secrets to avoid logging the raw value
access_key = self._extract_access_token(response_json)
if access_key:
add_to_secrets(access_key)
except ResponseKeysMaxRecurtionReached as e:
# could not find the access token in the response, so do nothing
pass

self._log_response(response)
response.raise_for_status()
return response.json()

return response_json
except requests.exceptions.RequestException as e:
if e.response is not None:
if e.response.status_code == 429 or e.response.status_code >= 500:
Expand All @@ -240,9 +256,7 @@ def _ensure_access_token_in_response(self, response_data: Mapping[str, Any]) ->

This method attempts to extract the access token from the provided response data.
If the access token is not found, it raises an exception indicating that the token
refresh API response was missing the access token. If the access token is found,
it adds the token to the list of secrets to ensure it is replaced before logging
the response.
refresh API response was missing the access token.

Args:
response_data (Mapping[str, Any]): The response data from which to extract the access token.
Expand All @@ -257,9 +271,6 @@ def _ensure_access_token_in_response(self, response_data: Mapping[str, Any]) ->
raise Exception(
f"Token refresh API response was missing access token {self.get_access_token_name()}"
)
# Add the access token to the list of secrets so it is replaced before logging the response
# An argument could be made to remove the prevous access key from the list of secrets, but unmasking values seems like a security incident waiting to happen...
add_to_secrets(access_key)
except ResponseKeysMaxRecurtionReached as e:
raise e

Expand Down
Loading