mirror of
https://github.com/pewdiepie-archdaemon/odysseus.git
synced 2026-06-17 02:05:22 -04:00
fix(endpoint): scope secondary endpoint lookups by owner
* Scope secondary endpoint lookups by owner * Reject unregistered image endpoint URLs for non-admins * Adjust owner-scope tests for rebased routes * Allow non-admins to compare endpoints they own The compare owner-scope guard called _reject_raw_endpoint_url_for_non_admin with endpoint_id=None, so it rejected every signed-in non-admin /api/compare/start request — even for endpoints the caller owns — because compare resolves endpoints by URL and carries no endpoint_id. That locked non-admins out of compare entirely. Resolve the owned ModelEndpoint first and pass its id, so a registered endpoint the caller owns is allowed while only truly raw, unregistered URLs are rejected (mirrors the gallery inpaint/harmonize checks in this PR). Replace the source-only reject test with deterministic reject + allow regressions that no longer depend on the dev DB contents. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Bind compare sessions to the resolved owner-scoped endpoint /api/compare/start created the [CMP] helper sessions with the raw caller-supplied endpoint URL and only used the owner-scoped lookup to decide whether to copy an API key. That stopped key borrowing but still let a non-admin inject an arbitrary raw endpoint URL into the compare session path. Now, when the supplied URL resolves to a registered endpoint visible to the caller, the session binds to that row's own normalized base URL (build_chat_url(normalize_base(ep.base_url))) plus its headers — the same registered-endpoint shape session_routes uses. The raw URL survives only when ep is None, which non-admins already hit a 403 on, leaving raw URLs reachable solely for admins / single-user mode with no borrowed key. Adds compare-specific behavior tests: another user's private endpoint is rejected (nothing created), the session binds to the stored URL rather than the raw input, and an admin raw URL is allowed but carries no inherited key. Addresses the review on #1511. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Validate both compare endpoints before creating any session start_comparison resolved + created each [CMP] session inside one loop, so a request pairing a valid owned endpoint A with an unregistered raw endpoint B raised 403 only after A's session was already created — and its Authorization header copied in. The rejected request left a partial compare session with that header behind. Split the flow into two phases: phase 1 resolves and owner-validates both endpoints (running the raw-URL reject helper) and stashes the session URL + headers; phase 2 creates the two sessions only once both passed. A 403 on either endpoint now aborts with nothing created and no header copied. Adds a regression test: owned endpoint A + unregistered/raw endpoint B -> 403 with no sessions created. Addresses the follow-up review on #1511. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Resolve compare credentials by endpoint id, not URL alone Two endpoints visible to a caller can share a base_url but hold different api_keys. _owned_endpoint_by_url returned whichever row sorted first, so /api/compare/start could copy the wrong key into the [CMP] session. Add _owned_endpoint_by_id (same owner scoping) and optional endpoint_a_id/ endpoint_b_id form fields. The id pins the exact registered endpoint; URL resolution remains only for legacy/admin raw-URL callers. An id the caller can't see 404s instead of falling back to a same-URL row. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Loosen research-routes owner-scope assertion to the stable substring The rebased _resolve_research_endpoint generalized its owner derivation to honor an explicit owner arg first (owner = owner or getattr(sess, ...)), so the exact-line assertion broke CI. Assert the stable session-derivation substring instead of the full line. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Vykos
+110
-22
@@ -12,6 +12,7 @@ import logging
|
||||
from core.database import Comparison, SessionLocal
|
||||
from core.session_manager import SessionManager
|
||||
from src.auth_helpers import get_current_user
|
||||
from routes.session_routes import _reject_raw_endpoint_url_for_non_admin
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
@@ -38,6 +39,24 @@ def _owned_endpoint_by_url(db, base_url, owner):
|
||||
return owner_filter(q, ModelEndpoint, owner).first()
|
||||
|
||||
|
||||
def _owned_endpoint_by_id(db, endpoint_id, owner):
|
||||
"""ModelEndpoint whose id == `endpoint_id` and is VISIBLE to `owner` (their
|
||||
own rows + legacy null-owner "shared" rows); None otherwise.
|
||||
|
||||
Preferred over _owned_endpoint_by_url for credential resolution: two visible
|
||||
endpoints can share the same base_url but hold DIFFERENT api_keys (e.g. two
|
||||
accounts on the same provider). A base_url-only match returns whichever row
|
||||
sorts first, so it can copy the WRONG owner-scoped key into the [CMP] session.
|
||||
An id pins the exact registered endpoint, so /api/compare/start prefers it and
|
||||
only falls back to URL matching for legacy / admin raw-URL callers. Owner
|
||||
scoping is identical to _owned_endpoint_by_url (a null/empty owner is a no-op).
|
||||
"""
|
||||
from core.database import ModelEndpoint
|
||||
from src.auth_helpers import owner_filter
|
||||
q = db.query(ModelEndpoint).filter(ModelEndpoint.id == endpoint_id)
|
||||
return owner_filter(q, ModelEndpoint, owner).first()
|
||||
|
||||
|
||||
class RecordVoteRequest(BaseModel):
|
||||
prompt: str
|
||||
models: List[str]
|
||||
@@ -54,8 +73,10 @@ def setup_compare_routes(session_manager: SessionManager):
|
||||
prompt: str = Form(...),
|
||||
model_a: str = Form(...),
|
||||
model_b: str = Form(...),
|
||||
endpoint_a: str = Form(...),
|
||||
endpoint_b: str = Form(...),
|
||||
endpoint_a: str = Form(""),
|
||||
endpoint_b: str = Form(""),
|
||||
endpoint_a_id: str = Form(""),
|
||||
endpoint_b_id: str = Form(""),
|
||||
is_blind: str = Form("true"),
|
||||
):
|
||||
"""Create two ephemeral sessions and a comparison record.
|
||||
@@ -63,10 +84,10 @@ def setup_compare_routes(session_manager: SessionManager):
|
||||
Returns the comparison ID and the two session IDs so the client
|
||||
can fire two independent SSE streams to /api/chat_stream.
|
||||
"""
|
||||
user = getattr(request.state, 'current_user', None)
|
||||
comp_id = str(uuid.uuid4())
|
||||
sid_a = str(uuid.uuid4())
|
||||
sid_b = str(uuid.uuid4())
|
||||
user = getattr(request.state, 'current_user', None)
|
||||
|
||||
# Blind mapping: randomly assign left/right
|
||||
blind = str(is_blind).lower() == "true"
|
||||
@@ -87,31 +108,94 @@ def setup_compare_routes(session_manager: SessionManager):
|
||||
# de-anonymizing the comparison before the user votes (issue #1285).
|
||||
slot_name = {session_left: "Model A", session_right: "Model B"}
|
||||
|
||||
# Create ephemeral sessions (prefixed [CMP])
|
||||
for sid, model, endpoint in [(sid_a, model_a, endpoint_a), (sid_b, model_b, endpoint_b)]:
|
||||
# SECURITY: resolve and validate BOTH endpoints before creating any
|
||||
# session. Compare copies a registered endpoint's Authorization header
|
||||
# into the [CMP] session, so validating one endpoint while creating its
|
||||
# session, then rejecting the other, would leave a partial compare
|
||||
# session behind with that header attached. Doing all the owner-scope
|
||||
# resolution + raw-URL rejection up front means a 403 on either endpoint
|
||||
# aborts the whole request with nothing created and no header copied.
|
||||
from src.endpoint_resolver import build_chat_url, build_headers, normalize_base
|
||||
resolved = []
|
||||
db = SessionLocal()
|
||||
try:
|
||||
for sid, model, endpoint, endpoint_id in [
|
||||
(sid_a, model_a, endpoint_a, endpoint_a_id),
|
||||
(sid_b, model_b, endpoint_b, endpoint_b_id),
|
||||
]:
|
||||
# Prefer an explicit endpoint id: it pins the EXACT registered
|
||||
# endpoint (and its api_key), even when two endpoints visible to
|
||||
# the caller share a base_url with different keys — a URL-only
|
||||
# match would copy whichever row sorts first, i.e. possibly the
|
||||
# wrong key. Fall back to URL resolution only for legacy / admin
|
||||
# raw-URL callers that don't send an id.
|
||||
eid = endpoint_id.strip() if isinstance(endpoint_id, str) else ""
|
||||
if eid:
|
||||
ep = _owned_endpoint_by_id(db, eid, user)
|
||||
if ep is None:
|
||||
# An id the caller can't see (wrong owner / deleted) must
|
||||
# NOT silently fall back to a same-URL row with a different
|
||||
# key — that's exactly the mix-up ids exist to prevent.
|
||||
raise HTTPException(404, "Model endpoint not found")
|
||||
# The id already resolved the endpoint; ignore any raw URL the
|
||||
# caller also sent and dial the stored config instead.
|
||||
endpoint = ep.base_url
|
||||
elif not endpoint:
|
||||
raise HTTPException(
|
||||
422, "endpoint_a/endpoint_b or endpoint_a_id/endpoint_b_id is required"
|
||||
)
|
||||
else:
|
||||
# Resolve the supplied URL to a ModelEndpoint the caller owns
|
||||
# (their own rows + legacy null-owner shared rows), scoped so a
|
||||
# comparison can't borrow another user's private endpoint key.
|
||||
base = normalize_base(endpoint)
|
||||
ep = _owned_endpoint_by_url(db, base, user)
|
||||
# Reject *unregistered* raw URLs for signed-in non-admins; a
|
||||
# matched registered endpoint supplies an id so the caller can
|
||||
# still compare endpoints they own. Blanket-rejecting here (the
|
||||
# earlier `endpoint_id=None` call) locked non-admins out of
|
||||
# compare entirely, since compare resolves endpoints by URL with
|
||||
# no endpoint_id. Mirrors the gallery inpaint/harmonize checks.
|
||||
# Raised here (phase 1), before any session exists.
|
||||
_reject_raw_endpoint_url_for_non_admin(
|
||||
request, user, str(ep.id) if ep is not None else None, endpoint
|
||||
)
|
||||
# Bind the [CMP] session to the RESOLVED endpoint, not the raw
|
||||
# caller-supplied string. When the URL matches a registered
|
||||
# endpoint visible to the caller, use that row's own normalized
|
||||
# base URL (the same value owner scoping + endpoint validation
|
||||
# already vetted) so the session dials exactly where the stored
|
||||
# config points. The raw `endpoint` only survives for callers
|
||||
# allowed to pass one — admins / single-user mode, where
|
||||
# `_reject_raw_endpoint_url_for_non_admin` is a no-op and `ep`
|
||||
# is None. Mirrors the registered-endpoint path in session_routes.
|
||||
session_endpoint_url = (
|
||||
build_chat_url(normalize_base(ep.base_url)) if ep is not None else endpoint
|
||||
)
|
||||
# Headers come only from a matched endpoint's key; None when
|
||||
# `ep` is None (raw admin URL or no match), so a comparison can
|
||||
# never inherit another user's key/headers.
|
||||
headers = build_headers(ep.api_key, ep.base_url) if (ep and ep.api_key) else None
|
||||
resolved.append((sid, model, session_endpoint_url, headers))
|
||||
finally:
|
||||
db.close()
|
||||
|
||||
# Both endpoints validated — only now create the ephemeral [CMP]
|
||||
# sessions and copy any resolved headers.
|
||||
for sid, model, session_endpoint_url, headers in resolved:
|
||||
name = f"[CMP] {slot_name[sid]}" if blind else f"[CMP] {model.split('/')[-1]}"
|
||||
session_manager.create_session(
|
||||
session_id=sid,
|
||||
name=name,
|
||||
endpoint_url=endpoint,
|
||||
endpoint_url=session_endpoint_url,
|
||||
model=model,
|
||||
rag=False,
|
||||
owner=user,
|
||||
)
|
||||
# Copy API key from endpoint config
|
||||
db = SessionLocal()
|
||||
try:
|
||||
from src.endpoint_resolver import build_headers, normalize_base
|
||||
# Find matching endpoint by URL, scoped to the caller so a
|
||||
# comparison can't borrow another user's private endpoint key.
|
||||
base = normalize_base(endpoint)
|
||||
ep = _owned_endpoint_by_url(db, base, user)
|
||||
if ep and ep.api_key:
|
||||
s = session_manager.sessions.get(sid)
|
||||
if s:
|
||||
s.headers = build_headers(ep.api_key, ep.base_url)
|
||||
finally:
|
||||
db.close()
|
||||
if headers:
|
||||
s = session_manager.sessions.get(sid)
|
||||
if s:
|
||||
s.headers = headers
|
||||
|
||||
# Store comparison record
|
||||
db = SessionLocal()
|
||||
@@ -121,8 +205,12 @@ def setup_compare_routes(session_manager: SessionManager):
|
||||
prompt=prompt,
|
||||
model_a=model_a,
|
||||
model_b=model_b,
|
||||
endpoint_a=endpoint_a,
|
||||
endpoint_b=endpoint_b,
|
||||
# Record the URL the session actually dials. For URL callers this
|
||||
# is their raw input; for id-only callers (empty endpoint_a/_b)
|
||||
# fall back to the resolved endpoint URL so the column stays
|
||||
# meaningful and non-null. resolved is in [a, b] order.
|
||||
endpoint_a=endpoint_a or resolved[0][2],
|
||||
endpoint_b=endpoint_b or resolved[1][2],
|
||||
is_blind=blind,
|
||||
blind_mapping=json.dumps(mapping),
|
||||
owner=user,
|
||||
|
||||
@@ -12,7 +12,7 @@ from fastapi import APIRouter, HTTPException, Query, Request
|
||||
|
||||
from core.database import SessionLocal, GalleryImage, GalleryAlbum, ModelEndpoint
|
||||
from core.database import Session as DbSession
|
||||
from src.auth_helpers import get_current_user, require_privilege
|
||||
from src.auth_helpers import get_current_user, owner_filter, require_privilege
|
||||
from src.upload_limits import read_upload_limited
|
||||
from src.constants import GENERATED_IMAGES_DIR
|
||||
|
||||
@@ -26,6 +26,19 @@ GALLERY_UPLOAD_MAX_BYTES = int(os.getenv("ODYSSEUS_GALLERY_UPLOAD_MAX_BYTES", st
|
||||
GALLERY_TRANSFORM_UPLOAD_MAX_BYTES = int(os.getenv("ODYSSEUS_GALLERY_TRANSFORM_UPLOAD_MAX_BYTES", str(25 * 1024 * 1024)))
|
||||
|
||||
|
||||
def _current_user_is_admin(request: Request, user: str | None) -> bool:
|
||||
if not user:
|
||||
return False
|
||||
auth_mgr = getattr(request.app.state, "auth_manager", None)
|
||||
is_admin = getattr(auth_mgr, "is_admin", None)
|
||||
if not callable(is_admin):
|
||||
return False
|
||||
try:
|
||||
return bool(is_admin(user))
|
||||
except Exception:
|
||||
return False
|
||||
|
||||
|
||||
def _sanitize_gallery_filename(filename: str) -> str:
|
||||
"""Return a local filename safe to join under generated_images."""
|
||||
safe_name = re.sub(r"[^A-Za-z0-9._-]", "_", Path(str(filename or "")).name)[:128]
|
||||
@@ -1043,7 +1056,10 @@ def setup_gallery_routes() -> APIRouter:
|
||||
try:
|
||||
ep = _visible_image_endpoint_for_base(db, _target, user)
|
||||
if ep:
|
||||
base = (ep.base_url or base).rstrip("/")
|
||||
api_key = ep.api_key
|
||||
elif user and not _current_user_is_admin(request, user):
|
||||
raise HTTPException(403, "Choose a registered image endpoint")
|
||||
finally:
|
||||
db.close()
|
||||
|
||||
@@ -1234,7 +1250,10 @@ def setup_gallery_routes() -> APIRouter:
|
||||
try:
|
||||
ep = _visible_image_endpoint_for_base(db, base, user)
|
||||
if ep:
|
||||
base = (ep.base_url or base).rstrip("/")
|
||||
api_key = ep.api_key
|
||||
elif user and not _current_user_is_admin(request, user):
|
||||
raise HTTPException(403, "Choose a registered image endpoint")
|
||||
finally:
|
||||
db.close()
|
||||
|
||||
|
||||
@@ -38,9 +38,9 @@ def _first_chat_model(models) -> str:
|
||||
return (models[0] if models else "")
|
||||
|
||||
|
||||
def _resolve_research_endpoint(sess) -> tuple:
|
||||
def _resolve_research_endpoint(sess, owner: Optional[str] = None) -> tuple:
|
||||
"""Return (endpoint_url, model, headers) for Deep Research, checking admin overrides."""
|
||||
owner = getattr(sess, "owner", None) or None
|
||||
owner = owner or getattr(sess, "owner", None) or None
|
||||
url, model, headers = resolve_endpoint(
|
||||
"research",
|
||||
fallback_url=sess.endpoint_url,
|
||||
|
||||
Reference in New Issue
Block a user