Skip to content

Commit 25cc2e6

Browse files
committed
fix: gate launch tracebacks on logs:list and harden copy/stringify
Address review feedback. Launching a model only requires `models:write`, but reading logs requires `logs:list`. Returning a full traceback to every caller who may launch a model therefore handed internal filesystem paths and runtime details to operators who cannot reach them through the log center, bypassing that boundary. Gate the `traceback` field on `logs:list` (admins always qualify, and an unauthenticated cluster has no boundary to protect). The normalized root-cause `detail` still goes to anyone who may launch a model, so the UI remains useful without auth changes. API keys are restricted to model query and inference scopes and so never qualify. The check live-reads DB permissions, matching the auth service's own policy so a revoked permission takes effect immediately, and fails closed. Reuse the existing `copyToClipboard` helper for both copy affordances instead of calling `navigator.clipboard` directly: it reports real success or failure rather than always claiming success, and falls back to `execCommand` where the Clipboard API is unavailable (non-HTTPS origins). This also fixes a wrong i18n key on the toast action, which referenced a nonexistent `common.copy` and would have rendered the literal key path. Guard the remaining `JSON.stringify` in `extractDetail`'s array branch, which could throw on a BigInt or circular reference while the interceptor is already handling an error.
1 parent 1f52c48 commit 25cc2e6

6 files changed

Lines changed: 175 additions & 21 deletions

File tree

frontend/src/components/pages/launch-model/launch-dialog/launch-dialog.tsx

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import { useGlobal } from '@/contexts/global-context';
1212
import { useI18n } from '@/contexts/i18n-context';
1313
import { useForm, useFormValues, useWatch } from '@/hooks/use-form';
1414
import { useMenuAuth } from '@/hooks/use-menu-auth';
15-
import { cn } from '@/lib/utils';
15+
import { cn, copyToClipboard } from '@/lib/utils';
1616
import { Progress } from '@/components/ui/progress';
1717
import { Button } from '@/components/ui/button';
1818
import { Switch } from '@/components/ui/switch';
@@ -1350,8 +1350,7 @@ export default function LaunchDialog({
13501350
size="sm"
13511351
className="ml-auto h-7 shrink-0 gap-1 text-xs"
13521352
onClick={() => {
1353-
void navigator.clipboard?.writeText(copyText);
1354-
toast.success(t('common.copySuccess'));
1353+
void copyToClipboard(copyText);
13551354
}}
13561355
>
13571356
<Copy className="size-3" />

frontend/src/contexts/request-context.tsx

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import { RequestEvents } from '@/constants';
88
import { requestManager } from '@/lib/request-manager';
99
import { removeAuthTokens } from '@/lib/auth-storage';
1010
import { translate as t } from '@/contexts/i18n-context';
11+
import { copyToClipboard } from '@/lib/utils';
1112
import type { ServerErrorPayload } from '@/lib/request';
1213

1314
export default function RequestProvider({ children }: PropsWithChildren) {
@@ -54,9 +55,9 @@ export default function RequestProvider({ children }: PropsWithChildren) {
5455
</pre>
5556
),
5657
action: {
57-
label: t('common.copy'),
58+
label: t('launchModel.copyError'),
5859
onClick: () => {
59-
void navigator.clipboard?.writeText(`${message ?? ''}\n\n${traceback}`);
60+
void copyToClipboard(`${message ?? ''}\n\n${traceback}`);
6061
},
6162
},
6263
});

frontend/src/lib/request.ts

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -46,21 +46,25 @@ function extractDetail(data: unknown): string {
4646
? String((item as { msg: unknown }).msg)
4747
: typeof item === 'string'
4848
? item
49-
: JSON.stringify(item)
49+
: safeStringify(item)
5050
)
5151
.filter(Boolean);
5252
if (messages.length) return messages.join('; ');
5353
}
54-
if (data && typeof data === 'object') {
55-
try {
56-
return JSON.stringify(data);
57-
} catch {
58-
return String(data);
59-
}
60-
}
54+
if (data && typeof data === 'object') return safeStringify(data);
6155
return String(data ?? '');
6256
}
6357

58+
/** `JSON.stringify` throws on BigInt and circular references; this runs while
59+
* already handling an error, so it must never throw itself. */
60+
function safeStringify(value: unknown): string {
61+
try {
62+
return JSON.stringify(value) ?? String(value);
63+
} catch {
64+
return String(value);
65+
}
66+
}
67+
6468
// Keep untyped request calls backward-compatible while typed calls can still pass <T>.
6569
// eslint-disable-next-line @typescript-eslint/no-explicit-any
6670
type LooseResponse = any;

xinference/api/restful_api.py

Lines changed: 51 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -280,6 +280,51 @@ def _is_ip_allowed(self, ip: str) -> bool:
280280
def is_authenticated(self):
281281
return self._advanced_auth_service is not None
282282

283+
def _caller_may_see_diagnostics(self, request) -> bool:
284+
"""Whether the caller is allowed to receive internal diagnostics.
285+
286+
Launching a model only requires ``models:write``, but a traceback
287+
exposes filesystem paths and runtime internals that are otherwise
288+
reachable only through the log center, which requires ``logs:list``.
289+
Gate the traceback on that same permission so this endpoint does not
290+
become a way around the diagnostic boundary. Admins always qualify.
291+
292+
When auth is disabled entirely there is no boundary to respect, so
293+
diagnostics are returned to everyone.
294+
"""
295+
if not self._advanced_auth_service:
296+
return True
297+
token = request.headers.get("Authorization", "").replace("Bearer ", "")
298+
if not token:
299+
return False
300+
try:
301+
payload = self._advanced_auth_service.verify_access_token(token)
302+
if not payload:
303+
# An API key rather than a JWT. API keys are restricted to
304+
# model query and inference scopes, so they never qualify.
305+
return False
306+
user = self._advanced_auth_service.db.get_user_by_id(payload.get("user_id"))
307+
if not user:
308+
return False
309+
# Live-read the DB permissions rather than the JWT snapshot, so a
310+
# revoked permission takes effect immediately -- the same policy
311+
# the auth service itself applies.
312+
from .oauth2.scope_aliases import _normalize_scopes
313+
314+
scopes = _normalize_scopes(user.get("permissions", []))
315+
except Exception:
316+
# Never let the diagnostics check break the error response itself;
317+
# fail closed.
318+
logger.debug("Failed to resolve caller scopes", exc_info=True)
319+
return False
320+
return "admin" in scopes or "logs:list" in scopes
321+
322+
def _launch_error_traceback(self, request, exc: BaseException) -> Optional[str]:
323+
"""Format ``exc``'s traceback, or None if the caller may not see it."""
324+
if not self._caller_may_see_diagnostics(request):
325+
return None
326+
return format_error_traceback(exc)
327+
283328
def _check_model_access(
284329
self, request, model_uid: str, model_type: Optional[str] = None
285330
):
@@ -944,17 +989,18 @@ async def launch_model(
944989
)
945990
# A launch failure originates deep inside an engine and crosses several
946991
# actor boundaries before arriving here. Report the root cause of the
947-
# chain rather than the outermost wrapper's message, and pass the
948-
# traceback along so the UI can show where it actually broke.
992+
# chain rather than the outermost wrapper's message. The traceback is
993+
# only attached for callers who may already read it in the log center;
994+
# the root-cause summary goes to everyone who may launch a model.
949995
except ValueError as ve:
950996
logger.error(str(ve), exc_info=True)
951997
raise DetailedHTTPException(
952-
400, format_error_summary(ve), format_error_traceback(ve)
998+
400, format_error_summary(ve), self._launch_error_traceback(request, ve)
953999
)
9541000
except RuntimeError as re:
9551001
logger.error(str(re), exc_info=True)
9561002
raise DetailedHTTPException(
957-
503, format_error_summary(re), format_error_traceback(re)
1003+
503, format_error_summary(re), self._launch_error_traceback(request, re)
9581004
)
9591005
except asyncio.CancelledError as ce:
9601006
# cancelled by user -- not a defect, so no traceback
@@ -963,7 +1009,7 @@ async def launch_model(
9631009
except Exception as e:
9641010
logger.error(str(e), exc_info=True)
9651011
raise DetailedHTTPException(
966-
500, format_error_summary(e), format_error_traceback(e)
1012+
500, format_error_summary(e), self._launch_error_traceback(request, e)
9671013
)
9681014

9691015
# Clear negative cache so that get_model for this uid is not blocked

xinference/api/tests/test_launch_error_response.py

Lines changed: 103 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323
from fastapi.testclient import TestClient
2424

2525
from ...core.error_utils import format_error_summary, format_error_traceback
26-
from ..restful_api import DetailedHTTPException
26+
from ..restful_api import DetailedHTTPException, RESTfulAPI
2727

2828

2929
def _build_app() -> FastAPI:
@@ -113,3 +113,105 @@ def test_exception_subclasses_http_exception():
113113
assert exc.status_code == 503
114114
assert exc.detail == "boom"
115115
assert exc.tb is None
116+
117+
118+
class _FakeDB:
119+
def __init__(self, permissions):
120+
self._permissions = permissions
121+
122+
def get_user_by_id(self, user_id):
123+
if user_id is None:
124+
return None
125+
return {"id": user_id, "permissions": list(self._permissions)}
126+
127+
128+
class _FakeAuthService:
129+
"""Minimal stand-in for AdvancedAuthService's diagnostics-relevant surface."""
130+
131+
def __init__(self, permissions, valid_jwt=True):
132+
self.db = _FakeDB(permissions)
133+
self._valid_jwt = valid_jwt
134+
135+
def verify_access_token(self, token):
136+
# A None payload is how an API key (rather than a JWT) presents here.
137+
return {"user_id": 1} if self._valid_jwt else None
138+
139+
140+
def _api_with_auth(auth_service):
141+
api = RESTfulAPI.__new__(RESTfulAPI)
142+
api._advanced_auth_service = auth_service
143+
return api
144+
145+
146+
def _request_with_token(token="tok"):
147+
headers = [(b"authorization", f"Bearer {token}".encode())] if token else []
148+
return Request({"type": "http", "headers": headers, "method": "POST", "path": "/"})
149+
150+
151+
class TestDiagnosticsPermission:
152+
"""Launching needs models:write; a traceback needs logs:list."""
153+
154+
def test_no_auth_configured_allows_diagnostics(self):
155+
api = _api_with_auth(None)
156+
assert api._caller_may_see_diagnostics(_request_with_token()) is True
157+
158+
def test_logs_list_allows_diagnostics(self):
159+
api = _api_with_auth(_FakeAuthService(["models:write", "logs:list"]))
160+
assert api._caller_may_see_diagnostics(_request_with_token()) is True
161+
162+
def test_admin_allows_diagnostics(self):
163+
api = _api_with_auth(_FakeAuthService(["admin"]))
164+
assert api._caller_may_see_diagnostics(_request_with_token()) is True
165+
166+
def test_models_write_alone_is_denied(self):
167+
# The whole point of the gate: a model operator without log access
168+
# must not receive filesystem paths and runtime internals.
169+
api = _api_with_auth(_FakeAuthService(["models:write"]))
170+
assert api._caller_may_see_diagnostics(_request_with_token()) is False
171+
172+
def test_legacy_scope_alias_is_honoured(self):
173+
# models:start normalizes to models:write -- still not logs:list.
174+
api = _api_with_auth(_FakeAuthService(["models:start"]))
175+
assert api._caller_may_see_diagnostics(_request_with_token()) is False
176+
177+
def test_missing_token_is_denied(self):
178+
api = _api_with_auth(_FakeAuthService(["logs:list"]))
179+
assert api._caller_may_see_diagnostics(_request_with_token(None)) is False
180+
181+
def test_api_key_is_denied(self):
182+
# API keys cannot hold logs:list, so they never qualify.
183+
api = _api_with_auth(_FakeAuthService(["logs:list"], valid_jwt=False))
184+
assert api._caller_may_see_diagnostics(_request_with_token()) is False
185+
186+
def test_unknown_user_is_denied(self):
187+
service = _FakeAuthService(["logs:list"])
188+
service.db.get_user_by_id = lambda _uid: None
189+
api = _api_with_auth(service)
190+
assert api._caller_may_see_diagnostics(_request_with_token()) is False
191+
192+
def test_auth_failure_fails_closed(self):
193+
service = _FakeAuthService(["logs:list"])
194+
195+
def _boom(_token):
196+
raise RuntimeError("auth backend down")
197+
198+
service.verify_access_token = _boom
199+
api = _api_with_auth(service)
200+
# Must not propagate out of the error path, and must deny.
201+
assert api._caller_may_see_diagnostics(_request_with_token()) is False
202+
203+
204+
class TestLaunchErrorTraceback:
205+
def test_traceback_withheld_without_permission(self):
206+
api = _api_with_auth(_FakeAuthService(["models:write"]))
207+
assert (
208+
api._launch_error_traceback(_request_with_token(), ValueError("x")) is None
209+
)
210+
211+
def test_traceback_returned_with_permission(self):
212+
api = _api_with_auth(_FakeAuthService(["logs:list"]))
213+
try:
214+
raise ValueError("boom")
215+
except ValueError as e:
216+
tb = api._launch_error_traceback(_request_with_token(), e)
217+
assert tb is not None and "ValueError: boom" in tb

xinference/core/error_utils.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -116,8 +116,10 @@ def format_error_summary(exc: BaseException) -> str:
116116
def format_error_traceback(exc: BaseException) -> Optional[str]:
117117
"""Render the full traceback of ``exc``, including its cause chain.
118118
119-
Returns ``None`` when ``XINFERENCE_DISABLE_ERROR_TRACEBACK`` is set, for
120-
deployments that would rather not expose filesystem paths over HTTP.
119+
Returns ``None`` when ``XINFERENCE_DISABLE_ERROR_TRACEBACK`` is set, which
120+
withholds tracebacks over HTTP even from callers who would otherwise be
121+
permitted them. Callers are responsible for the permission check itself;
122+
see ``RESTfulAPI._caller_may_see_diagnostics``.
121123
122124
Formats ``exc`` rather than its root cause on purpose: the outer exception
123125
carries the frames from every layer, and xoscar attaches the remote

0 commit comments

Comments
 (0)