2 Commits

Author	SHA1	Message	Date
Sunshine	a956bbc030	fix(runs): reject cancel actions on GET stream joins (#5092) ... * fix(runs): reject cancel actions on GET stream joins stream_existing_run is registered for both GET and POST, and its ?action=interrupt|rollback branch cancels the run. The CSRF middleware exempts GET, so a session-authenticated browser could be forced cross-site (img/script/top-level navigation) into GET /api/threads/{id}/runs/{run_id}/stream?action=interrupt|rollback — a state-changing GET that bypasses the CSRF protection guarding the POST variant. Introduced with the dual registration in #1403. The handler's docstring already documents cancel-then-stream as POST-only (the LangGraph SDK's joinStream/useStream stop button uses POST); enforce it: GET with an action answers 405, action-less GET joins and POST cancel-then-stream are unchanged. Regression drives the real router: GET+action is 405 with the run left running, plain GET join still streams, POST+action still cancels. * fix(runs): scope the 405 detail to the action requirement "GET is a read-only stream join" overstates the current main: on a locally-owned run with the default on_disconnect=cancel, a GET join's disconnect can still trigger cancellation. That observer-disconnect vector is closed by #5041; the detail here should only claim what this guard enforces. * fix(runs): harden GET stream action rejection * fix(runs): align stream schema with method contract * test(runs): pin GET stream action 405 through the production stack Review follow-up (defence-in-depth): the GET-action suite drove bare FastAPI() apps, so nothing pinned that a session-authenticated cross-site GET reaches the route gate at all once CSRF exempts the safe method. test_pat_auth.py already assembles the production middleware order (AuthMiddleware inner, CSRFMiddleware outer), so its mirror app now registers the real _reject_get_stream_action dependency on a GET join route. The new case pins the end-to-end premise: an authenticated GET ?action=interrupt is answered 405 + Allow: POST by the production route dependency, while the same unauthenticated GET dies at AuthMiddleware's 401 before any route logic runs. Validation: focused suites (test_pat_auth, test_stream_get_action, test_csrf_middleware) — 62 passed; ruff check + format clean; the new case errors on the pre-fix baseline (guard absent), confirming the pin.	2026-09-01 15:51:37 +08:00
Lucy Shen	37451500eb	fix(gateway): split stream_existing_run into per-method routes for unique OpenAPI operationIds (#3228) ... * fix(gateway): split stream_existing_run into per-method routes for unique OpenAPI operationIds `@router.api_route("/.../stream", methods=["GET", "POST"])` registers a single FastAPI route that holds both methods. FastAPI's auto-generated `operationId` is computed once per route from a single method picked out of `route.methods`, so when OpenAPI generation iterates over every method on that route both end up sharing the same `operationId`. That triggers `UserWarning: Duplicate Operation ID stream_existing_run_..._stream_(get|post) for function stream_existing_run` during `app.openapi()` and produces an invalid OpenAPI spec for SDK / codegen consumers. Register GET and POST as two separate routes on the same handler so each method gets a distinct auto-generated `operationId` ("..._stream_get" and "..._stream_post"). Behavior is otherwise unchanged: same handler, same `require_permission` decoration, same response. Add `tests/test_openapi_operation_ids.py` to lock in the invariant: no duplicate-operationId warnings during spec generation, globally unique operationIds across the spec, and distinct GET / POST operationIds on the stream endpoint specifically. Reverted the source change locally and confirmed all three tests fail before the fix. * test(runtime): widen CancelledError catch in _ScriptedAgent to fix cancel-race flake `_ScriptedAgent.astream()` previously only caught `asyncio.CancelledError` inside the inner `if self.block_after_first_chunk:` while-loop. Cancellation arriving during any earlier `await` in the same body (`self.model.ainvoke`, `_write_checkpoint`, the `yield`) would propagate without setting `controller.cancelled`, so callers waiting on `controller.cancelled.wait(5)` after `POST /cancel` returned 204 could race and time out. `test_cancel_interrupt_stops_running_background_run` waits only for the `started` event (set on the first line of `astream`) before issuing cancel, so its race window spans all three pre-loop `await`s. On a clean `main` checkout, stress-running the test 20× reproduces the failure 6/20 (~30%). `test_cancel_rollback_restores_pre_run_checkpoint`, which waits for the later `checkpoint_written` event, passes 20/20 — confirming the race lives entirely in the gap between `started.set()` and the cancellation-aware block. Widen the try/except to cover the entire `astream` body so any `CancelledError` sets the controller event; the non-cancel path is unchanged (no exception means no event set). After this change the previously flaky test passes 50/50, the rollback test still passes 30/30, and the full backend suite remains at 3649 passed / 19 skipped. Test-only change — `backend/tests/test_runtime_lifecycle_e2e.py` is the only file touched; the production cancel pipeline is unaffected.	2026-05-28 08:20:52 +08:00