Fix 500 on every provider resolution when BYOK key matches the demo key
The demo-key backstop added in 1dd3108 keyed its check on
`api_key == DEMO_API_KEY` rather than on `using_demo`. That looked stricter but
was wrong: the demo key is an ordinary OpenRouter key, so a user can
legitimately paste that same value into their own Settings as BYOK. The guard
then raised on every resolve_provider_config() call for that account.
Because me_payload() resolves a provider config, this 500'd GET /api/auth/me —
the SPA's bootstrap call — so the frontend's `me` never resolved and the nav
(including the AI Chat link) never rendered, on top of chat itself failing.
`using_demo` is the flag that actually means "the server is paying", and only
resolve_provider_config's demo branch sets it, so the pinning guarantee is
unchanged: server-funded turns still can't reach an off-whitelist model.
Adds a regression test for a BYOK user whose key equals the demo key value, and
corrects the test that had asserted the buggy behaviour.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014FGY1yvzSeKgTtRfeVtDmx
This commit is contained in:
co-authored by
Claude Opus 5
parent
1dd31086c1
commit
49e2dde5cf
+14
-8
@@ -89,14 +89,20 @@ class ProviderConfig:
|
|||||||
using_demo: bool
|
using_demo: bool
|
||||||
|
|
||||||
def __post_init__(self) -> None:
|
def __post_init__(self) -> None:
|
||||||
# Belt and braces around the shared demo key. resolve_provider_config()
|
# Belt and braces around server-funded turns: resolve_provider_config()
|
||||||
# already pins the model, but this makes it a property of the config
|
# already pins the model, and this makes it a property of the config
|
||||||
# object itself: however it was built, and by whichever caller, the
|
# object too, so a future caller can't construct an unpinned one.
|
||||||
# server-funded key can never be paired with an off-whitelist (i.e.
|
# Unreachable by design — a raise here means a new code path bypassed
|
||||||
# possibly paid) model. Unreachable by design — a 500 here means a new
|
# the pinning, which is worth failing loudly rather than billing.
|
||||||
# code path tried to bypass the pinning, which is worth failing loudly
|
#
|
||||||
# rather than silently billing.
|
# The test is `using_demo`, NOT `api_key == DEMO_API_KEY`. Keying it on
|
||||||
if DEMO_API_KEY and self.api_key == DEMO_API_KEY and self.model not in DEMO_MODELS:
|
# the key value looks stricter but is wrong: the demo key is a normal
|
||||||
|
# OpenRouter key, so a user can legitimately paste that same key into
|
||||||
|
# their own Settings as BYOK — and then every resolution raised, 500ing
|
||||||
|
# even GET /auth/me and taking the whole SPA down with it. `using_demo`
|
||||||
|
# is what actually means "the server is paying", and only the demo
|
||||||
|
# branch below sets it.
|
||||||
|
if self.using_demo and self.model not in DEMO_MODELS:
|
||||||
raise ValueError(
|
raise ValueError(
|
||||||
f"Refusing to use the shared demo key with non-whitelisted model {self.model!r}"
|
f"Refusing to use the shared demo key with non-whitelisted model {self.model!r}"
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -180,20 +180,45 @@ def test_demo_key_endpoint_cannot_be_redirected(client, monkeypatch):
|
|||||||
assert FakeProvider.last_key == "demo-key"
|
assert FakeProvider.last_key == "demo-key"
|
||||||
|
|
||||||
|
|
||||||
def test_provider_config_refuses_demo_key_with_paid_model(monkeypatch):
|
def test_provider_config_refuses_server_funded_paid_model(monkeypatch):
|
||||||
"""The structural backstop: even a hand-built config (a future code path
|
"""The structural backstop: a hand-built config (a future code path that
|
||||||
that forgets to go through resolve_provider_config) can't pair them."""
|
forgets to go through resolve_provider_config) can't run a server-funded
|
||||||
|
turn on an off-whitelist model."""
|
||||||
monkeypatch.setattr(auth, "DEMO_API_KEY", "demo-key")
|
monkeypatch.setattr(auth, "DEMO_API_KEY", "demo-key")
|
||||||
monkeypatch.setattr(auth, "DEMO_MODELS", ["free/allowed"])
|
monkeypatch.setattr(auth, "DEMO_MODELS", ["free/allowed"])
|
||||||
with pytest.raises(ValueError):
|
with pytest.raises(ValueError):
|
||||||
auth.ProviderConfig("http://demo", "demo-key", "expensive/paid-model", True)
|
auth.ProviderConfig("http://demo", "demo-key", "expensive/paid-model", True)
|
||||||
# Mislabelling it as non-demo doesn't help: the key is what's checked.
|
auth.ProviderConfig("http://demo", "demo-key", "free/allowed", True) # whitelisted: fine
|
||||||
with pytest.raises(ValueError):
|
|
||||||
auth.ProviderConfig("http://demo", "demo-key", "expensive/paid-model", False)
|
|
||||||
# The user's own key with any model stays fine.
|
# The user's own key with any model stays fine.
|
||||||
auth.ProviderConfig("http://any", "sk-mine", "expensive/paid-model", False)
|
auth.ProviderConfig("http://any", "sk-mine", "expensive/paid-model", False)
|
||||||
|
|
||||||
|
|
||||||
|
def test_byok_user_may_reuse_the_demo_keys_value(client, monkeypatch):
|
||||||
|
"""Regression: the demo key is just an OpenRouter key, so a user can paste
|
||||||
|
that same value into their own Settings. That's BYOK — they're paying — and
|
||||||
|
it must not trip the guard. It used to raise on every resolution, which
|
||||||
|
500'd GET /auth/me and took the whole SPA down (no nav, no chat)."""
|
||||||
|
monkeypatch.setattr(auth, "demo_enabled", lambda: True)
|
||||||
|
monkeypatch.setattr(auth, "DEMO_API_KEY", "shared-key")
|
||||||
|
monkeypatch.setattr(auth, "DEMO_ENDPOINT_URL", "http://demo")
|
||||||
|
monkeypatch.setattr(auth, "DEMO_MODELS", ["free/allowed"])
|
||||||
|
db = SessionLocal()
|
||||||
|
try:
|
||||||
|
settings = db.query(models.Settings).first()
|
||||||
|
settings.api_key = "shared-key" # same value, but supplied by the user
|
||||||
|
settings.model = "expensive/paid-model" # their spend, their choice
|
||||||
|
db.commit()
|
||||||
|
finally:
|
||||||
|
db.close()
|
||||||
|
|
||||||
|
assert client.get("/api/auth/me").status_code == 200
|
||||||
|
assert client.get("/api/chat/config").status_code == 200
|
||||||
|
resp = _send(client)
|
||||||
|
assert resp.status_code == 200, resp.text
|
||||||
|
assert FakeProvider.last_model == "expensive/paid-model"
|
||||||
|
assert FakeProvider.last_key == "shared-key"
|
||||||
|
|
||||||
|
|
||||||
def test_resolve_provider_config_is_the_single_choke_point(monkeypatch):
|
def test_resolve_provider_config_is_the_single_choke_point(monkeypatch):
|
||||||
"""Turns, AI Chat and the connection test all resolve through this one
|
"""Turns, AI Chat and the connection test all resolve through this one
|
||||||
function, so pinning it here pins every caller. No DB or HTTP needed."""
|
function, so pinning it here pins every caller. No DB or HTTP needed."""
|
||||||
|
|||||||
Reference in New Issue
Block a user