a2aproject / a2aproject/a2a-rs

a2a-server: historyLength, idempotent cancel, and an authorize callback

Aperta
#195 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub
Lingua principale
Rust
Stelle
75
Fork
19
Merge medio
11h 27m
PR unite (30g)
21

Descrizione

`DefaultRequestHandler` still has three interop/security gaps that other SDKs already closed. #123 tried to fix them in one patch. Review on that PR is still open, and the authentication piece as written is not safe to take.

## What is wrong today

1. **`get_task` ignores `historyLength`.** `list_tasks` truncates; `get_task` returns the full history. A client that asks for `historyLength=1` still sees every prior message (information disclosure vs Python/JS/Go).
2. **Negative `historyLength` is not normalized on the list path.** The in-memory store casts `i32` to `usize`, so `-1` wraps and returns full history. `get` and `list` must share one clamp (`<= 0` → empty).
3. **`cancel_task` on a terminal task returns `TASK_NOT_CANCELABLE`.** TypeScript and Go treat cancel-on-terminal as idempotent and return the current task. A retry after a timeout then fails only on Rust.
4. **No authorization hook on handler methods.** `ServiceParams` are passed through and ignored. Any caller can `get_task` / `list_tasks` / `cancel_task` (CWE-862).

## What #123 got right, and what it did not

Keep from #123:

- apply `historyLength` on `get_task`
- treat cancel-on-terminal as success
- opt-in so default deployments do not change

Do not take from #123:

- `require_authentication()` that only checks a non-empty `Authorization` value. `Authorization: arbitrary` would pass. That is a header-presence gate, not authentication, and it does not fix CWE-862.
- a lowercase-only `"authorization"` lookup. SlimRPC copies metadata keys as-is; a valid client can be rejected.

## What the replacement should do

- One shared `historyLength` normalization used by `get_task` and `list_tasks`. Prefer in-place truncate on an owned task (no extra history clone).
- Idempotent `cancel_task` when the loaded task is already terminal.
- An **authorizer callback** on `DefaultRequestHandler`, for example `authorize(&ServiceParams, Option<&task_id>) -> Result<(), A2AError>`. Default is a no-op. The handler must not invent a principal by testing that a header exists.
- Header-key comparison, if an implementation looks at headers, must be case-insensitive.
- `task_id` is reserved for later owner/tenant checks; the first cut only needs the callback seat.

A presence check cannot be that seat. Applications that already validate a token, DID, or session implement the callback with that result.

## Plan

1. Land the three handler fixes in one `a2a-server` PR against this issue.
2. Close #123 in favor of that PR (credit the original `historyLength` / cancel work there).
3. Callers that need real admit implement the callback. They do not call a built-in "require header" helper as authentication.

## Test plan

- [ ] `get_task` with `historyLength=1` / `0` / `-1` matches `list_tasks`
- [ ] cancel on COMPLETED/CANCELED/FAILED returns the current terminal task
- [ ] default handler still accepts requests with empty `ServiceParams`
- [ ] a callback that rejects unauthenticated params is invoked on send, get, list, cancel, subscribe, and push-config methods
- [ ] existing `a2a-server` unit tests stay green

Guida per i contributori

Apri la guida per i contributori

Valutazione

Questa issue non è ancora stata valutata.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.