fix(tasks): omit params from the list-tasks response - #377
Conversation
The list endpoint returned the full task object, including `params`: the
arbitrary, caller-supplied create-time payload. On a broadly-scoped list that
exposes whatever callers store there (per-caller bearer tokens, PII, ...) in
bulk. Return a lean `TaskSummary` from `GET /tasks` instead, and keep the full
record (including `params`) on the single-task `GET /tasks/{id}`.
- Add `TaskSummary` (list shape) that omits `params`; point `list_tasks` at it.
- Update the dev UI task-name fallback to use the task `name` instead of
`params.description`, and regenerate `openapi.yaml`.
- Tests: list omits `params`; single-task GET still includes it.
Breaking change: typed SDK consumers that read `params` off a list item must
fetch the task by id instead.
✱ Stainless preview buildsThis PR will update the openapi python typescript
|
| agents: list["Agent"] | None = Field( | ||
| default=None, | ||
| title="Agents associated with this task (only populated when 'agents' view is requested)", | ||
| ) |
There was a problem hiding this comment.
Shouldn't this be maintained on the agent side instead? The agent should know which tasks its writing to or able to write to instead of the task maintaining which agents are associated to it.
There was a problem hiding this comment.
This field isn't introduced here. TaskResponse already carries agents (populated only with ?relationships=agents), and TaskSummary mirrors that existing shape so list and detail stay consistent. Whether the association should live agent-side is a pre-existing design question that's orthogonal to this fix, so I'd keep it out of scope here. Happy to open a separate issue to revisit it if you want.
| relationships=relationships, | ||
| ) | ||
| return [TaskResponse.model_validate(task_entity) for task_entity in task_entities] | ||
| return [TaskSummary.model_validate(task_entity) for task_entity in task_entities] |
There was a problem hiding this comment.
Maybe an integration test would be nice to make sure the fields we deem "sensitive" are omitted (in the case that someone adds them back later). Can be followup
There was a problem hiding this comment.
This is already covered in the PR: test_list_tasks_omits_params_in_response and test_list_tasks_omits_params_for_null_params_task assert params is absent from list items (populated and null cases), and test_get_task_by_id_includes_params_in_response verifies the single-task fetch still returns it. If you want the regression caught one layer earlier, I can also add a model-level guard asserting "params" not in TaskSummary.model_fields.
|
For future reference, enabling FGAC and authz would fix this. Gating access is an auth concern not app level, task params is not the right location for this. Also if it is sensitive it should live in secrets microservice. |
|
Also to be clear, if someone gets the task ID and does a GET this issue will still be exposed. |
Agentex doesn't seem to have a way to do OBO without saving this params in the task. What is your advice? |
Summary
GET /tasks(list) returned the full task object, the sameTaskResponsethe single-task GET returns, so every item includedparams: the arbitrary, caller-supplied create-time payload. Callers routinely stash sensitive material there (credentials, tokens, PII). Because the list is broadly scoped, that turned a single list call into a bulk read of every task'sparams.This makes the list a lean summary (
TaskSummary, noparams) and keeps the full record on the single-task fetch. A list response should carry only what a collection view needs; the full record belongs on the item fetch.Scope: the list response only. Behavior of
GET /tasks/{id},GET /tasks/name/{name},POST/PATCH, and the domain/repository layers is unchanged.Endpoint contract (after)
flowchart TD Client(["Client"]) Client -->|"GET /tasks (list)"| Summary["list of TaskSummary<br/>id, name, status, status_reason,<br/>created_at, updated_at, cleaned_at,<br/>task_metadata, agents"] Client -->|"GET /tasks/{id} (detail)"| Full["TaskResponse<br/>(all summary fields) + params"] Summary -. "need params for one task?" .-> Client Client ==>|"fetch that id"| Full classDef lean fill:#e8f5e9,stroke:#43a047,color:#1b5e20; classDef full fill:#fdecea,stroke:#e53935,color:#b71c1c; class Summary lean; class Full full;params, and is fetched one id at a time (and is subject to the same per-request authorization as before).Schema
TaskSummaryis a deliberate allowlist. It is a standalone model (not a subclass ofTask) soparamscannot leak in via inheritance, and becauseBaseModelignores extra fields, validating aTaskEntity(which still carriesparams) into aTaskSummarydropsparamsat serialization.classDiagram class TaskSummary { +id +name +status +status_reason +created_at +updated_at +cleaned_at +task_metadata +agents } class TaskResponse { +id +name +status +status_reason +created_at +updated_at +cleaned_at +task_metadata +agents +params } note for TaskSummary "list response, omits params" note for TaskResponse "single-task detail, includes params (may carry credentials or PII)"The two share the same safe fields; only
TaskResponseexposesparams.Before / after
What changes across a whole list call:
flowchart LR subgraph Before["Before: GET /tasks"] direction TB b["N tasks × full object<br/>N × params (creds/PII)"] end subgraph After["After: GET /tasks"] direction TB a["N tasks × lean summary<br/>0 × params"] end Before --> After classDef bad fill:#fdecea,stroke:#e53935,color:#b71c1c; classDef good fill:#e8f5e9,stroke:#43a047,color:#1b5e20; class Before bad; class After good;Field reference
TaskSummary)TaskResponse)idnamestatus/status_reasoncreated_at/updated_at/cleaned_attask_metadataagents?relationships=agentsparamsBreaking change and migration
Removing
paramsfrom the list is an intentional, security-motivated breaking change for typed consumers that readparamsoff a list item.flowchart TD Q{"Reading params off a list item?"} Q -->|No| OK["No change needed"] Q -->|Yes| Fix["Fetch the task by id, then read params"] classDef ok fill:#e8f5e9,stroke:#43a047,color:#1b5e20; class OK ok;The SDKs are generated from
agentex/openapi.yaml(Stainless), so the list-item type regenerates automatically on merge; consumers pick up the change when they bump the SDK. This PR regenerates and commitsopenapi.yaml(thelist_tasks200 now referencesTaskSummary), which theopenapi-specCI check enforces.Consumers touched in this PR
agentex-ui):createTaskNamefell back toparams.descriptionfor the task label; switched to the taskname(task_metadata.display_name → name → "Unnamed task"). No other code readsparamsoff a listed task.Implementation
src/api/schemas/tasks.py: addTaskSummary(allowlist, omitsparams).src/api/routes/tasks.py:list_tasksresponse_model=list[TaskSummary], returnsTaskSummary.model_validate(...)per entity. Single-task routes unchanged.agentex-ui/lib/task-utils.ts+ test: name-derivation fallback.agentex/openapi.yaml: regenerated.Follow-ups / known limitations
task_metadatais still free-form (dict[str, Any]) and partly caller-influenced. By convention it holds non-secret display/routing/filter metadata (and the list can filter on it), so it stays in the summary. If we want the list to be strict rather than convention-safe, a follow-up could givetask_metadataa typed shape so callers cannot stash sensitive data that the list would surface.Test plan
flowchart LR subgraph list["GET /tasks"] L1["task present in list"] L2["params NOT in item"] end subgraph get["GET /tasks/{id}"] G1["params present + correct"] end list --> gettests/integration/api/tasks/test_tasks_api.py): list omitsparamsfor both populated- and null-paramstasks; the schema test assertsparamsabsent on list items; single-task GET (by id and by name) still returnsparams.agentex-ui/lib/task-utils.test.ts): task-name derivation fromdisplay_name/name.openapi.yamlregenerated and committed.TaskSummary.model_validate({... "params": {...}})dropsparamswhileTaskResponsekeeps it;ruffclean.Greptile Summary
This PR fixes an information-disclosure issue where
GET /tasks(list) returned the fullTaskResponse— includingparams, an arbitrary caller-supplied payload that can hold credentials and PII — for every task in a single call. The fix introduces a standaloneTaskSummaryallowlist model that omitsparams, updates the list route'sresponse_model, regenerates the OpenAPI spec, and adapts the UI's task-name derivation to usetask.nameinstead ofparams.description.TaskSummaryis a standaloneBaseModel(not a subclass ofTask) soparamscannot leak via inheritance; Pydantic v2's defaultextra='ignore'ensuresmodel_validatesilently drops the field.GET /tasks/{id},GET /tasks/name/{name}, lifecycle endpoints) are unchanged and continue to returnparams.task_metadata.display_name → task.name → 'Unnamed task'instead of the now-absentparams.description.Confidence Score: 5/5
Safe to merge — the change is a targeted, well-scoped fix that strips a sensitive field from a bulk endpoint without touching any single-task or write paths.
The allowlist model is correctly designed as a standalone BaseModel (no Task inheritance), and Pydantic v2's default extra-field behaviour ensures params is silently dropped at serialization. The custom BaseModel in model_utils.py does not set extra='allow', so there is no bypass path. Single-task routes, lifecycle endpoints, and the domain layer are untouched. Integration tests verify the before/after contract for both populated- and null-params tasks, and the UI fallback is covered by unit tests.
Files Needing Attention: No files require special attention.
Important Files Changed
Reviews (2): Last reviewed commit: "Merge branch 'main' into fix/list-tasks-..." | Re-trigger Greptile