From 71e6201790bc09592f9344f93cc07a32207d2ff9 Mon Sep 17 00:00:00 2001 From: Ali jawwad <33836051+jawwad-ali@users.noreply.github.com> Date: Fri, 24 Jul 2026 21:03:06 +0500 Subject: [PATCH] fix(auth): normalize whitespace in auth-config env-var/id references at store time (#3691) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(auth): normalize whitespace in auth-config env-var/id references 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) * test(auth): cover tenant_id/client_id/client_secret_env normalization 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) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- src/specify_cli/authentication/config.py | 22 ++++++++++--- tests/test_authentication.py | 41 ++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 4 deletions(-) diff --git a/src/specify_cli/authentication/config.py b/src/specify_cli/authentication/config.py index 8d1faf80c..829940d6f 100644 --- a/src/specify_cli/authentication/config.py +++ b/src/specify_cli/authentication/config.py @@ -13,6 +13,7 @@ import stat from dataclasses import dataclass from fnmatch import fnmatch from pathlib import Path +from typing import Any from urllib.parse import urlparse @@ -53,6 +54,19 @@ def _is_valid_host_pattern(pattern: str) -> bool: return pattern.startswith("*.") and "*" not in pattern[2:] +def _norm(value: Any) -> Any: + """Strip surrounding whitespace from a whitespace-insignificant string + config reference (env-var names, tenant/client ids) before it is stored. + + These fields are validated on their ``.strip()``ed form, so an accidentally + padded value passes validation but then silently breaks the verbatim + ``os.environ.get(...)`` / URL lookups downstream. Normalizing at store time + mirrors how ``hosts`` is already handled (``h.strip().lower()``). Non-string + values (e.g. ``None``) pass through unchanged. + """ + return value.strip() if isinstance(value, str) else value + + def load_auth_config( path: Path | None = None, ) -> list[AuthConfigEntry]: @@ -182,10 +196,10 @@ def load_auth_config( provider=provider, auth=auth, token=token, - token_env=token_env, - tenant_id=entry_raw.get("tenant_id"), - client_id=entry_raw.get("client_id"), - client_secret_env=entry_raw.get("client_secret_env"), + token_env=_norm(token_env), + tenant_id=_norm(entry_raw.get("tenant_id")), + client_id=_norm(entry_raw.get("client_id")), + client_secret_env=_norm(entry_raw.get("client_secret_env")), ) ) diff --git a/tests/test_authentication.py b/tests/test_authentication.py index 06523c6f2..9edbd9860 100644 --- a/tests/test_authentication.py +++ b/tests/test_authentication.py @@ -92,6 +92,27 @@ class TestLoadAuthConfig: assert entries[0].auth == "bearer" assert entries[0].token_env == "GH_TOKEN" + def test_padded_token_env_is_normalized_and_resolves(self, tmp_path, monkeypatch): + # token_env is validated on its stripped form but was stored raw, so a + # padded env-var name passed validation yet broke the verbatim + # os.environ.get() lookup — resolve_token silently returned None. + monkeypatch.setenv("GH_TOKEN", "secret-tok") + cfg = tmp_path / "auth.json" + cfg.write_text(json.dumps({ + "providers": [{ + "hosts": ["github.com"], + "provider": "github", + "auth": "bearer", + "token_env": " GH_TOKEN ", + }] + })) + entries = load_auth_config(cfg) + assert len(entries) == 1 + # Stored normalized (matching how hosts are normalized), ... + assert entries[0].token_env == "GH_TOKEN" + # ... so the env lookup finds the token instead of returning None. + assert GitHubAuth().resolve_token(entries[0]) == "secret-tok" + def test_valid_ado_config(self, tmp_path): cfg = tmp_path / "auth.json" cfg.write_text(json.dumps({ @@ -136,6 +157,26 @@ class TestLoadAuthConfig: assert entries[0].auth == "azure-ad" assert entries[0].tenant_id == "tid" + def test_padded_azure_ad_refs_are_normalized(self, tmp_path): + # The normalization also covers tenant_id / client_id / client_secret_env + # (used verbatim in the OAuth token URL/body and os.environ.get). Padded + # values were validated on their stripped form but stored raw. + cfg = tmp_path / "auth.json" + cfg.write_text(json.dumps({ + "providers": [{ + "hosts": ["dev.azure.com"], + "provider": "azure-devops", + "auth": "azure-ad", + "tenant_id": " tid ", + "client_id": " cid ", + "client_secret_env": " SECRET ", + }] + })) + entries = load_auth_config(cfg) + assert entries[0].tenant_id == "tid" + assert entries[0].client_id == "cid" + assert entries[0].client_secret_env == "SECRET" + def test_azure_cli_config(self, tmp_path): cfg = tmp_path / "auth.json" cfg.write_text(json.dumps({