Repository navigation
Fix security findings reported against 4.5.0rc1 - #4075
Conversation
- fastapi: run before_request hooks on every route, when mounted and on the WebSocket handshake, and send a hook's returned response. - Background callbacks: key results per run when cache_by is unset and per callback always, so equal inputs never share another request's result. - dcc.Location(refresh=True).href and html.Area.href go through clean_url. - Pages path_template matching: no IndexError or regex backtracking on pathnames containing a newline. - MCP: build the callback adapter collection per request so values computed for one user are not served to another.
Dash performance benchmarks✅ all within thresholds
growth = late-third / early-third per-op time; ~1 is flat, a large value means the per-op cost scales with accumulated state. machine scale vs baseline: 0.63x - divided out of the baseline ratios so they compare like for like (the absolute warn/fail ceilings are left un-scaled); calibrated on |
| app.mcp_decorated_functions = dict(MCP_DECORATED_FUNCTIONS) | ||
| # A fresh collection per message: its adapters cache values computed from | ||
| # this caller's request, which must never be served to another user. | ||
| # pylint: disable-next=protected-access | ||
| token = app._mcp_callback_map.set(CallbackAdapterCollection(app)) | ||
| try: | ||
| return _dispatch_mcp_message(method, params, request_id) | ||
| finally: | ||
| app._mcp_callback_map.reset(token) # pylint: disable=protected-access |
There was a problem hiding this comment.
Creating a fresh mcp_callback_map per request will cause significant performance problems in larger apps.
Each request throws away everything the adapters compute lazily. MCP clients in their normal usage will have to walk the layout multiple times and re-run every callback to compute their initial values. (example: a tools/list call will recompute everything every time)
I would suggest keeping the cached mcp_callback_map and instead allow app authors to work around the narrow security issue by supplying their own cache keys. Basically, following the same pattern established by background callbacks’ cache_by kwarg (with a default value that preserves the caching we have today).
There was a problem hiding this comment.
Agreed, the per-request rebuild is reverted. The collection is cached again, keyed by a new configure_mcp_server(cache_by=...). The default is one shared cache, like today, so nothing changes unless an app opts in. Per-user keys live in an LRU capped at 128 collections. test_mcpi003 checks a callback runs once across clients by default, test_mcpi001/002 cover per-user values with cache_by, and a cache_by that raises returns a JSON-RPC error instead of falling back to a shared cache (test_mcpi004).
There was a problem hiding this comment.
This 128 limit on the cache raises some flags for me. It's a new paradigm for Dash to limit like this (background callback caches don't behave the same way!) and I question its effectiveness.
It limits how many users are cached, not how much memory the cache uses, which I assume is what we actually care about here.
I don't think we should have a limit at all, but if you do think it's prudent, I think we should at least allow users to configure it.
There was a problem hiding this comment.
Made it configurable in 24ff68c: configure_mcp_server(cache_maxsize=...), default 128, None for no limit. I kept a default cap because unlike background callbacks (stored in Diskcache/Redis, which have their own size limits and expiry) these collections live in process memory, so with cache_by set and no cap a long-running app keeps one per user forever. Agreed it counts users rather than bytes, same model as functools.lru_cache; apps that want no limit can pass None. test_mcpi005 covers both.
- fastapi: accept Flask style (body, headers) and string status hook tuples. - Log a before_request hook failure before rejecting a WebSocket; Quart now runs before_request hooks on the WebSocket handshake too. - Diskcache: expire uncached background results after a day so results nobody polls for do not pile up. - MCP: keep the cached callback map, keyed by configure_mcp_server(cache_by=...) so apps can cache per user; a failing cache_by returns a JSON-RPC error. - Sanitize html.Base href.
|
2 participants
Fixes five issues from a responsible-disclosure report against 4.5.0rc1.
FastAPI
before_requesthooks (dash/backends/_fastapi.py)Hooks only ran for
_dash-*routes, the returned response was dropped, and nothing ran when the app was mounted under a parent FastAPI app. Hooks now run for every route of the Dash app (assets, hook routes,api_endpoint, pages), use the path relative toroot_path, and gate the WebSocket handshake (failures are logged). A non-None result is sent as the response, accepting the same shapes as Flask (Response, body,(body, status),(body, headers),(body, status, headers)), andHTTPExceptionkeeps its status. Quart now also runsbefore_requesthooks on the WebSocket handshake. User routes still skip the JSON body read and keep their own error handling.Background callback result keys (
background_callback/managers/__init__.py)The key was
sha256(args, fn source, triggered). The signedendIdhandle stops forged keys, but a second client sending the same inputs got a valid handle for the same raw key and could read another user's result. Withoutcache_byeach run now gets a random nonce, and the callback key is always part of the hash, so callbacks from one factory function no longer collide. Diskcache writes these uncached entries with a one day expiry so results nobody polls for do not pile up (Celery's result backend already expires them).URL sanitizer gaps (
Location.react.js, htmlgenerate-components.js)dcc.Location(refresh=True).href,html.Area.hrefandhtml.Base.hrefnow go throughclean_url, raising the same "Dangerous link detected" error asdcc.Link.Pages
path_templatematching (dash/_pages.py)A newline in the pathname passed the
fnmatchgate but not the unanchored(.*)regex, giving anIndexErrorand polynomial backtracking. Matching now uses a precompiledDOTALLfullmatchafter the gate. Variables can still contain/as before.MCP request isolation (
dash/dash.py,dash/mcp/_server.py,dash/mcp/_configure.py)app.mcp_callback_mapcached callback initial outputs and layout values from the first caller and served them to every client. The cache stays (rebuilding it per request is too slow), but it is now keyed by a newconfigure_mcp_server(cache_by=...), like background callbacks'cache_by. The default is one shared cache, so apps whose layout or callbacks depend on the user should pass a function returning the user id. Per-user caches live in an LRU capped at 128, and acache_bythat raises returns a JSON-RPC error instead of falling back to the shared cache.Behavior changes
build_cache_keytakes a newcallback_keyargument.cache_byunset, identical triggers no longer reuse a result.expirewhen set (before,expireonly applied once a result was read).before_requesthooks also run on the WebSocket handshake.Tests
Each new test fails on
devand passes here:tests/backend_tests/test_fastapi_before_request.py,tests/backend_tests/test_quart_before_request.pytests/integration/security/test_background_isolation.pytests/integration/security/test_xss.py(xss004,xss005,xss006; the first two checked against bundles built from unpatched sources)tests/integration/multi_page/test_pages_path_template.pytests/integration/mcp/test_mcp_request_isolation.pytests/unit,tests/backend_tests,tests/integration/{mcp,multi_page,security}andtests/background_callbackpass locally.