Skip to content

Commit dda6883

Browse files
Copilotmnriem
andauthored
fix: validate host patterns, cache auth config per-process
Agent-Logs-Url: https://github.com/github/spec-kit/sessions/889b58a7-7f8c-47e2-8056-931ebcc671cc Co-authored-by: mnriem <15701806+mnriem@users.noreply.github.com>
1 parent ef5deb7 commit dda6883

4 files changed

Lines changed: 128 additions & 3 deletions

File tree

src/specify_cli/authentication/config.py

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,22 @@ def _default_config_path() -> Path:
3636
return Path.home() / ".specify" / "auth.json"
3737

3838

39+
def _is_valid_host_pattern(pattern: str) -> bool:
40+
"""Return True for safe host patterns: exact hostnames or ``*.suffix`` only.
41+
42+
Rejects patterns like ``*github.com`` (which would match
43+
``github.com.evil.com``) or multi-wildcard forms. Only these two
44+
forms are accepted:
45+
46+
* ``example.com`` — exact hostname
47+
* ``*.example.com`` — single leading wildcard for one sub-domain level
48+
"""
49+
if "*" not in pattern:
50+
return True # exact hostname — already validated as non-empty
51+
# Only *.suffix is allowed; no other wildcard positions
52+
return pattern.startswith("*.") and "*" not in pattern[2:]
53+
54+
3955
def load_auth_config(
4056
path: Path | None = None,
4157
) -> list[AuthConfigEntry]:
@@ -92,6 +108,14 @@ def load_auth_config(
92108
raise ValueError(f"providers[{i}]: each host must be a non-empty string")
93109
# Normalize hosts: strip whitespace and lowercase
94110
hosts = [h.strip().lower() for h in hosts]
111+
# Reject dangerous wildcard forms (e.g. *github.com matches github.com.evil.com)
112+
for h in hosts:
113+
if not _is_valid_host_pattern(h):
114+
raise ValueError(
115+
f"providers[{i}]: invalid host pattern {h!r}. "
116+
"Only exact hostnames or '*.suffix' forms are allowed "
117+
"(e.g. 'github.com' or '*.visualstudio.com')."
118+
)
95119

96120
provider = entry_raw.get("provider", "")
97121
if not isinstance(provider, str) or not provider:

src/specify_cli/authentication/http.py

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,14 +21,22 @@
2121

2222

2323
_config_override: list[AuthConfigEntry] | None = None
24+
_config_cache: list[AuthConfigEntry] | None = None # None = not yet loaded
2425

2526

2627
def _load_config() -> list[AuthConfigEntry]:
27-
"""Load auth config, using override if set (for testing)."""
28+
"""Load auth config, using override if set (for testing).
29+
30+
The result is cached per-process so ``auth.json`` is read at most once,
31+
and any warning about a malformed file fires only once.
32+
"""
33+
global _config_cache
2834
if _config_override is not None:
2935
return _config_override
36+
if _config_cache is not None:
37+
return _config_cache
3038
try:
31-
return load_auth_config()
39+
_config_cache = load_auth_config()
3240
except (ValueError, OSError) as exc:
3341
import warnings
3442
config_path = _default_config_path()
@@ -38,7 +46,8 @@ def _load_config() -> list[AuthConfigEntry]:
3846
UserWarning,
3947
stacklevel=2,
4048
)
41-
return []
49+
_config_cache = []
50+
return _config_cache
4251

4352

4453
def _hostname_in_hosts(hostname: str, hosts: tuple[str, ...]) -> bool:

tests/conftest.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,3 +78,6 @@ def _isolate_auth_config(monkeypatch):
7878
"""Ensure no test reads the real ~/.specify/auth.json."""
7979
from specify_cli.authentication import http as _auth_http
8080
monkeypatch.setattr(_auth_http, "_config_override", [])
81+
# Also clear the per-process cache so tests that unset _config_override
82+
# won't see a previously cached real-file result.
83+
monkeypatch.setattr(_auth_http, "_config_cache", None)

tests/test_authentication.py

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -244,6 +244,30 @@ def test_incompatible_provider_scheme_raises(self, tmp_path):
244244
with pytest.raises(ValueError, match="does not support"):
245245
load_auth_config(cfg)
246246

247+
def test_dangerous_wildcard_host_raises(self, tmp_path):
248+
cfg = tmp_path / "auth.json"
249+
cfg.write_text(json.dumps({
250+
"providers": [{"hosts": ["*github.com"], "provider": "github", "auth": "bearer", "token_env": "X"}]
251+
}))
252+
with pytest.raises(ValueError, match="invalid host pattern"):
253+
load_auth_config(cfg)
254+
255+
def test_multi_wildcard_host_raises(self, tmp_path):
256+
cfg = tmp_path / "auth.json"
257+
cfg.write_text(json.dumps({
258+
"providers": [{"hosts": ["*.*.example.com"], "provider": "github", "auth": "bearer", "token_env": "X"}]
259+
}))
260+
with pytest.raises(ValueError, match="invalid host pattern"):
261+
load_auth_config(cfg)
262+
263+
def test_valid_star_dot_host_accepted(self, tmp_path):
264+
cfg = tmp_path / "auth.json"
265+
cfg.write_text(json.dumps({
266+
"providers": [{"hosts": ["*.visualstudio.com"], "provider": "azure-devops", "auth": "basic-pat", "token_env": "X"}]
267+
}))
268+
entries = load_auth_config(cfg)
269+
assert entries[0].hosts == ("*.visualstudio.com",)
270+
247271
@pytest.mark.skipif(os.name == "nt", reason="POSIX permission bits not supported on Windows")
248272
def test_world_readable_warns(self, tmp_path):
249273
import stat
@@ -658,6 +682,71 @@ def test_timeout_propagates(self, monkeypatch):
658682
open_url("https://example.com/file")
659683

660684

685+
# ---------------------------------------------------------------------------
686+
# _load_config caching
687+
# ---------------------------------------------------------------------------
688+
689+
690+
class TestLoadConfigCaching:
691+
def test_config_cached_after_first_load(self, monkeypatch):
692+
"""_load_config() should call load_auth_config only once per process."""
693+
from unittest.mock import patch
694+
from specify_cli.authentication import http as _mod
695+
from specify_cli.authentication.config import AuthConfigEntry
696+
# Allow the real load path (no override)
697+
monkeypatch.setattr(_mod, "_config_override", None)
698+
monkeypatch.setattr(_mod, "_config_cache", None)
699+
700+
entry = _github_entry()
701+
call_count = 0
702+
703+
def fake_load(path=None):
704+
nonlocal call_count
705+
call_count += 1
706+
return [entry]
707+
708+
with patch.object(_mod, "load_auth_config", side_effect=fake_load):
709+
_mod._load_config()
710+
_mod._load_config()
711+
_mod._load_config()
712+
713+
assert call_count == 1
714+
715+
def test_cache_bypassed_by_override(self, monkeypatch):
716+
"""When _config_override is set, the cache is ignored entirely."""
717+
from specify_cli.authentication import http as _mod
718+
sentinel: list = [object()] # type: ignore[list-item]
719+
monkeypatch.setattr(_mod, "_config_override", sentinel)
720+
monkeypatch.setattr(_mod, "_config_cache", None)
721+
722+
result = _mod._load_config()
723+
assert result is sentinel
724+
# Cache must not have been populated when override is active
725+
assert _mod._config_cache is None
726+
727+
def test_failed_load_warns_once_and_caches_empty(self, monkeypatch):
728+
"""A bad auth.json emits exactly one warning and subsequent calls use cache."""
729+
from unittest.mock import patch
730+
from specify_cli.authentication import http as _mod
731+
import warnings as _warnings
732+
monkeypatch.setattr(_mod, "_config_override", None)
733+
monkeypatch.setattr(_mod, "_config_cache", None)
734+
735+
def fail_load(path=None):
736+
raise ValueError("bad config")
737+
738+
with patch.object(_mod, "load_auth_config", side_effect=fail_load):
739+
with _warnings.catch_warnings(record=True) as w:
740+
_warnings.simplefilter("always")
741+
_mod._load_config()
742+
_mod._load_config()
743+
_mod._load_config()
744+
745+
user_warnings = [x for x in w if issubclass(x.category, UserWarning)]
746+
assert len(user_warnings) == 1, "Expected exactly one warning"
747+
assert _mod._config_cache == []
748+
749+
661750
# ---------------------------------------------------------------------------
662751
# Redirect stripping
663752
# ---------------------------------------------------------------------------

0 commit comments

Comments
 (0)