fix(auth): normalize whitespace in auth-config env-var/id references at store time#3691
Merged
mnriem merged 2 commits intoJul 24, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Normalizes whitespace in authentication config references before downstream use.
Changes:
- Adds
_normfor string references. - Normalizes token and Azure credential references.
- Adds end-to-end coverage for padded
token_env.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/authentication/config.py |
Normalizes stored authentication references. |
tests/test_authentication.py |
Tests padded token environment resolution. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Medium
Collaborator
|
Please address Copilot feedback |
…at store time token_env, client_secret_env, tenant_id, and client_id were VALIDATED on their .strip()ed form but STORED raw, so an accidentally padded value passed validation yet silently broke the downstream verbatim os.environ.get(name) / OAuth-URL lookups — load_auth_config succeeded but resolve_token returned None and the request quietly downgraded to unauthenticated (401/403) with no diagnostic. Normalize these whitespace-insignificant string references with a _norm helper at store time, mirroring how `hosts` is already normalized (h.strip().lower()). `token` is unchanged (already stripped at resolve time). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review: the regression test only covered token_env, but the fix also normalizes tenant_id, client_id, and client_secret_env. Add a padded azure-ad entry asserting all three are stored stripped. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jawwad-ali
force-pushed
the
fix/auth-config-normalize-env-refs
branch
from
July 24, 2026 08:28
dea5ad8 to
a910632
Compare
Collaborator
|
Thank you! |
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.
What
In
load_auth_config, the fieldstoken_env,client_secret_env,tenant_id, andclient_idare validated on their.strip()ed form (proving they're non-empty ignoring surrounding whitespace) but then stored raw on theAuthConfigEntry.So an accidentally padded value (e.g.
"token_env": " GH_TOKEN ") passes validation, yet downstream:base.py→os.environ.get(entry.token_env)azure_devops.py→os.environ.get(entry.client_secret_env, ""), andtenant_id/client_idused verbatim in the OAuth URL/body…all use the padded name literally, so the env var is never found.
load_auth_configsucceeds with no error, butresolve_tokenreturnsNoneand the request silently downgrades to unauthenticated (401/403 on private resources) with no diagnostic — the classic validate-normalized-but-store-original bug.The
hostsfield in the same function already does it right (hosts = [h.strip().lower() for h in hosts]); these references were just never given the same normalization.Fix
A module-level
_normhelper strips whitespace-insignificant string references at store time (non-strings likeNonepass through). Applied totoken_env,tenant_id,client_id,client_secret_env.tokenis left unchanged (already stripped at resolve time inbase.py).Tests
tests/test_authentication.py::TestLoadAuthConfig::test_padded_token_env_is_normalized_and_resolves— a" GH_TOKEN "config now stores"GH_TOKEN"andGitHubAuth().resolve_token(...)finds the token. Fails before the fix (resolve_tokenreturnsNone). Full auth suite (140 tests) passes;ruffclean.AI-assisted: authored with Claude Code. Verified the validate-vs-store asymmetry against the
hostssibling and confirmed fail-before/pass-after end-to-end throughresolve_token.