You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Follow-up from the Copilot review on PR #38. Three inline comments flagged logging-safety issues in backend_auth.py. All are low-severity, log-only (no impact on auth correctness), and the endpoint values come from trusted operator-set env vars rather than user input — so none blocked PR #38.
Crucially, as the PR author (@kolyaopahle) noted, the flagged code is copied verbatim from the pre-existing TokenExchangeStrategy. Fixing it in ClientCredentialsStrategy alone would create an inconsistency. This issue tracks fixing it consistently in both strategies (and the shared factory).
Items to address
1. _safe_endpoint sanitizer doesn't handle IPv6 hosts and keeps query/fragment
The current construction in build_backend_token_provider() (both the token_exchange and client_credentials branches):
IPv6:urlparse('https://[::1]:8080/tok').hostname returns ::1without brackets, so the reassembled URL becomes https://::1:8080/... — malformed. Wrap IPv6 hosts in brackets.
Query/fragment:_replace(netloc=...) preserves .query and .fragment, so a 'safe' endpoint can still surface sensitive query params. Strip them for the log form.
2. RuntimeError messages interpolate the raw token endpoint URL
The error messages in both strategies' get_token() embed the full endpoint. The factory already produces a sanitized _safe_endpoint for its info log — the runtime errors should use the same sanitized form for consistency.
The debug logs in both strategies' get_token() include the raw endpoint. Use the sanitized form (or log only host/path).
Suggested approach
Extract a single _sanitize_endpoint_for_logging(url) helper that: validates the URL, brackets IPv6 hosts, preserves scheme + host + port + path, and drops userinfo, query, and fragment.
Use it in bothTokenExchangeStrategy and ClientCredentialsStrategy — for debug logs and for the RuntimeError messages — and in the factory's info logs.
Add a small unit test covering IPv6 hosts and URLs carrying query params.
Priority
Low. Log-only hardening; trusted-input endpoints make real-world exposure unlikely. Grouped here so all three Copilot comments are resolved once, across both strategies.
Background
Follow-up from the Copilot review on PR #38. Three inline comments flagged logging-safety issues in
backend_auth.py. All are low-severity, log-only (no impact on auth correctness), and the endpoint values come from trusted operator-set env vars rather than user input — so none blocked PR #38.Crucially, as the PR author (@kolyaopahle) noted, the flagged code is copied verbatim from the pre-existing
TokenExchangeStrategy. Fixing it inClientCredentialsStrategyalone would create an inconsistency. This issue tracks fixing it consistently in both strategies (and the shared factory).Items to address
1.
_safe_endpointsanitizer doesn't handle IPv6 hosts and keeps query/fragmentThe current construction in
build_backend_token_provider()(both the token_exchange and client_credentials branches):urlparse('https://[::1]:8080/tok').hostnamereturns::1without brackets, so the reassembled URL becomeshttps://::1:8080/...— malformed. Wrap IPv6 hosts in brackets._replace(netloc=...)preserves.queryand.fragment, so a 'safe' endpoint can still surface sensitive query params. Strip them for the log form.2.
RuntimeErrormessages interpolate the raw token endpoint URLThe error messages in both strategies'
get_token()embed the full endpoint. The factory already produces a sanitized_safe_endpointfor its info log — the runtime errors should use the same sanitized form for consistency.3.
logger.debuglines include the full endpointThe debug logs in both strategies'
get_token()include the raw endpoint. Use the sanitized form (or log only host/path).Suggested approach
_sanitize_endpoint_for_logging(url)helper that: validates the URL, brackets IPv6 hosts, preserves scheme + host + port + path, and drops userinfo, query, and fragment.TokenExchangeStrategyandClientCredentialsStrategy— for debug logs and for theRuntimeErrormessages — and in the factory's info logs.Priority
Low. Log-only hardening; trusted-input endpoints make real-world exposure unlikely. Grouped here so all three Copilot comments are resolved once, across both strategies.
Related