mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
fix(auth): normalize whitespace in auth-config env-var/id references at store time (#3691)
* 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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")),
|
||||
)
|
||||
)
|
||||
|
||||
|
||||
@@ -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({
|
||||
|
||||
Reference in New Issue
Block a user