From 85c84308e2a56bc6d9671ee753d1a4be10c99ba5 Mon Sep 17 00:00:00 2001 From: Petr Date: Wed, 19 Aug 2026 18:09:50 -0400 Subject: [PATCH 1/2] feat(token): add `token list`, stop retrying non-idempotent writes (#599) Issue #599 reported a persistent upstream 500 from `POST /v2/storage/tokens` on the europe-west3.gcp stack and flagged two client-side gaps it exposed. Both are real, and the first is worse than reported. Retry policy (http_base.py) kbagent retried every request on 429/500/502/503/504 regardless of method. Reading connection's `Storage_Service_Tokens::createToken` shows why that is unsafe for a write: the token row is saved, its secret generated, and only then are bucket permissions resolved -- outside any transaction (unlike `updateToken` right below it, which wraps the same work in one). A 500 raised in that block leaves a live token behind that the caller never sees, so three attempts could leave three of them. The reporter framed the duplicate-mint risk as hypothetical; that branch is reachable. - 5xx is retried only on GET/HEAD/OPTIONS/PUT/DELETE (RETRY_SAFE_METHODS). - 429 is still retried on every method -- the server states it did not process the request. - Transport failures split by what they prove: a refused connection or a connect/pool timeout never delivered the request and is still retried on any method; a read/write timeout means the request WAS sent, so it is not retried on POST/PATCH. That path is the same hazard as the 500 one, just quieter, and fixing only the status-code branch would have left it open. - A 500 on an unretried write reports retryable=false, so callers do not treat it as a transient blip. This covers all 29 POST call sites, not just the token mint -- `config oauth-url` mints a token through the very same endpoint. Error guidance (_raise_api_error) The parser picked up Keboola's generic `error: "Application error."` and dropped the `exceptionId` beside it -- the only handle Keboola support can trace an incident by. It is now surfaced, plus one of two hints: the request was not retried because the method is not idempotent (verify before repeating), or the same 5xx survived all attempts and is an upstream incident (escalate with the id). 4xx is untouched -- the incident hint would mislead. `kbagent token list` (issue #599 comment) The group could mint, revoke and rotate but not enumerate, so there was no way to get the `--token-id` that delete/refresh need without the web UI. It is also the check the new retry behaviour tells you to run after an unretried mint failure, which is why it ships here rather than separately. Secrets are stripped from every row, `--json` included: on a project carrying `force-decrypted-token` the Storage API embeds live values in the listing, and reproducing them would break the group's "revealed once, at mint" contract for every token at once. Stripped at the service layer and again in the SDK facade. Full surface: client `list_tokens()`, `TokenService.list_tokens()`, the CLI command, `GET /token/{project}/list` on serve, `token.list: read` in the permission registry, and `Client.list_tokens() -> list[TokenListEntryResult]` on the SDK. Version is deliberately NOT bumped -- this lands in a stack of PRs released as one version. The `(since v0.86.0)` tags in gotchas.md / commands-reference / sdk.md assume 0.86.0; the bump PR must confirm that. --- CLAUDE.md | 12 +- docs/sdk.md | 9 +- plugins/kbagent/agents/keboola-expert.md | 11 + plugins/kbagent/skills/kbagent/SKILL.md | 1 + .../kbagent/references/commands-reference.md | 1 + .../skills/kbagent/references/gotchas.md | 62 +++++ src/keboola_agent_cli/__init__.py | 2 + src/keboola_agent_cli/auth/auth_client.py | 25 +- src/keboola_agent_cli/client/tokens.py | 22 ++ src/keboola_agent_cli/commands/context.py | 4 + src/keboola_agent_cli/commands/token.py | 58 +++++ src/keboola_agent_cli/constants.py | 9 + src/keboola_agent_cli/http_base.py | 118 ++++++++- src/keboola_agent_cli/lib.py | 17 ++ src/keboola_agent_cli/permissions.py | 1 + src/keboola_agent_cli/result_models.py | 30 +++ src/keboola_agent_cli/server/routers/token.py | 11 + .../services/token_service.py | 23 ++ tests/test_client.py | 17 +- tests/test_client_device_enrollment.py | 44 ++++ tests/test_dev_portal_client.py | 16 +- tests/test_e2e.py | 19 +- tests/test_http_base.py | 236 ++++++++++++++++++ tests/test_lib_device_enrollment.py | 38 +++ tests/test_metastore_client.py | 29 +-- tests/test_token_cli.py | 73 ++++++ tests/test_token_service.py | 51 ++++ 27 files changed, 886 insertions(+), 53 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 3a5660e3..df3b7cb2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -511,12 +511,18 @@ kbagent feature user-add --project ALIAS --email EMAIL --feature NAME [--dry-run kbagent feature user-remove --project ALIAS --email EMAIL --feature NAME [--dry-run] [--yes] # token: scoped Storage tokens (Keboola single-bucket-write pattern; acting token needs canManageTokens; secret shown once). +kbagent token list --project NAME kbagent token create --project NAME --description DESC [--bucket-write BUCKET ...] [--bucket-read BUCKET ...] [--component-access ID ...] [--can-read-all-file-uploads] [--expires-in N] kbagent token delete --project NAME --token-id ID [--yes] kbagent token refresh --project NAME --token-id ID [--yes] -# SDK (importable Client(url,token)) now exposes create_scoped_token / delete_token / refresh_token / -# create_stream_source / get_stream_source / list_stream_sources / delete_stream_source: dicts on .raw, -# typed ScopedTokenResult / StreamSourceResult on the facade. See docs/sdk.md. +# `token list` (issue #599): GET /v2/storage/tokens -- the only way to see what already exists and to +# get the id `delete`/`refresh` need, without the web UI. Secrets are STRIPPED from every row: a +# project with the `force-decrypted-token` feature has the API embed live values in the listing, and +# echoing those would break the group's "revealed once, at mint" contract for every token at once. +# SDK (importable Client(url,token)) now exposes create_scoped_token / list_tokens / delete_token / +# refresh_token / create_stream_source / get_stream_source / list_stream_sources / +# delete_stream_source: dicts on .raw, typed ScopedTokenResult / TokenListEntryResult / +# StreamSourceResult on the facade. See docs/sdk.md. # permissions: session write/destructive firewall. The top-level --deny-writes / --deny-destructive # flags are the one-shot form; `permissions set` persists a policy (mode allow|deny + allow/deny patterns diff --git a/docs/sdk.md b/docs/sdk.md index e921ddb0..2d388156 100644 --- a/docs/sdk.md +++ b/docs/sdk.md @@ -175,9 +175,10 @@ print(res.imported_rows, res.warnings) Seven methods for the "provision an OTLP ingest endpoint, then mint a narrowly-scoped Storage token a device can hold" flow. They live on the facade and delegate straight to `KeboolaClient`; the token/stream ones return the typed models in [§5](#5-typed-result-models). -**Scoped Storage tokens** — mint, revoke, rotate: +**Scoped Storage tokens** — mint, list, revoke, rotate: - **`create_scoped_token(*, description, bucket_permissions=None, component_access=None, can_read_all_file_uploads=False, expires_in=None) -> ScopedTokenResult`** — `POST /v2/storage/tokens`. `bucket_permissions` is `{bucket_id: "read"|"write"}`; `expires_in` is seconds. The acting token must carry **`canManageTokens`** or the create 403s. +- **`list_tokens() -> list[TokenListEntryResult]`** — `GET /v2/storage/tokens` (`0.86.0+`). Where the `token_id` for `delete_token` / `refresh_token` comes from. **Secrets are stripped before validation**: a project carrying the `force-decrypted-token` feature has the API embed live values in the listing, and `create_scoped_token` is meant to be the only reveal. The acting token needs `canManageTokens`. - **`delete_token(token_id) -> None`** — `DELETE /v2/storage/tokens/{id}` (204, no body). Revokes. - **`refresh_token(token_id) -> ScopedTokenResult`** — `POST .../tokens/{id}/refresh`. Rotates the secret in place; the returned `.token` is the new secret. @@ -236,7 +237,7 @@ Escape hatch for endpoints the facade omits. See [§7](#7-clientraw-the-escape-h ## 5. Typed result models -`result_models.py` defines the **stable return shapes** (`JobResult`, `QueryResult`, `UploadTableResult`, `ConfigDetailResult`, `SyncPushResult`, `CloneResult`, and the `0.66.0+` device-enrollment pair `ScopedTokenResult` / `StreamSourceResult`), all re-exported from the package root. They exist so a downstream consumer types against a **semver-versioned contract** instead of an undocumented `dict[str, Any]` — a contract change then surfaces at *type-check* time, not at runtime against a customer build. +`result_models.py` defines the **stable return shapes** (`JobResult`, `QueryResult`, `UploadTableResult`, `ConfigDetailResult`, `SyncPushResult`, `CloneResult`, the `0.66.0+` device-enrollment pair `ScopedTokenResult` / `StreamSourceResult`, and `TokenListEntryResult` from `0.86.0+`), all re-exported from the package root. They exist so a downstream consumer types against a **semver-versioned contract** instead of an undocumented `dict[str, Any]` — a contract change then surfaces at *type-check* time, not at runtime against a customer build. Two design rules every model follows (`_ApiResultModel` base): @@ -257,7 +258,9 @@ The two device-enrollment models (`0.66.0+`) commit these named fields: - **`ScopedTokenResult`** — `id`, `token` (the one-time secret, see the gotcha in §4), `description`, `expires` (`str | None`), `can_read_all_file_uploads` (alias `canReadAllFileUploads`). - **`StreamSourceResult`** — `id`, `source_id`, `name`, `type`, `description`, `branch_id` (default `"default"`), `otlp_url` (ingest URL, secret in the path — unmasked), `otlp_secret`, `base_endpoint`, `sink_bucket_id` (`str | None`; the `in.c-otlp-` bucket to grant a device token write on). -> **The committed surface is `__all__`.** Anything exported from `keboola_agent_cli` (`Client`, `Files`, `FileEntry`, the six result models, `JobIdempotencyStore`, `__version__`) is public API under semver. Renaming or removing a named field, or tightening a type, is a breaking change. +`TokenListEntryResult` (`0.86.0+`) commits `id`, `description`, `created` (`str | None`), `expires` (`str | None`), `is_expired` (alias `isExpired`), `is_master_token` (alias `isMasterToken`). It has **no secret field by design** — see `list_tokens` above. + +> **The committed surface is `__all__`.** Anything exported from `keboola_agent_cli` (`Client`, `Files`, `FileEntry`, the result models, `JobIdempotencyStore`, `__version__`) is public API under semver. Renaming or removing a named field, or tightening a type, is a breaking change. --- diff --git a/plugins/kbagent/agents/keboola-expert.md b/plugins/kbagent/agents/keboola-expert.md index 30b76e06..18be314d 100644 --- a/plugins/kbagent/agents/keboola-expert.md +++ b/plugins/kbagent/agents/keboola-expert.md @@ -169,6 +169,17 @@ read it when a trigger fires. Each `(X.Y.Z+)` tag is the version floor. list` flags them). No migration command -- you do the argv mapping. Tool->command map in `docs/mcp-migration.md`; recipe in gotchas.md. +**A write that failed with a 5xx** +- `POST`/`PATCH` is NOT retried on 5xx any more (0.86.0+); `retryable: false` + there is deliberate -- never wrap it in your own retry loop. The work may have + landed: check with `token list` / `job list` before repeating. The message + carries the `exceptionId` -- quote it when escalating. gotchas.md. + +**Finding an existing Storage token** +- `kbagent token list -p P` (0.86.0+) -- the only source of the `--token-id` + that `token delete`/`refresh` need. Secrets are stripped from every row, + `--json` included; do not route around it with `kbagent http get`. + **Upgrading kbagent itself** - `install_channel` in `kbagent --json version` => native binary; `kbagent update` REFUSES by design. Quote `upgrade_command` (choco/winget/brew/apt/dnf); diff --git a/plugins/kbagent/skills/kbagent/SKILL.md b/plugins/kbagent/skills/kbagent/SKILL.md index c50e205f..42fd015c 100644 --- a/plugins/kbagent/skills/kbagent/SKILL.md +++ b/plugins/kbagent/skills/kbagent/SKILL.md @@ -99,6 +99,7 @@ When working inside a git repository or project directory, run `kbagent init` (o | Enable a feature flag on a user | `kbagent feature user-add --project PROJECT --email EMAIL --feature FEATURE` | | Disable a feature flag on a user (destructive) | `kbagent feature user-remove --project PROJECT --email EMAIL --feature FEATURE` | | Mint a scoped Storage API token (secret shown once) | `kbagent token create --project PROJECT --description DESCRIPTION` | +| List the project's Storage API tokens (no secrets -- those are mint-only) | `kbagent token list --project PROJECT` | | Revoke a Storage API token immediately (destructive; only non-master tokens) | `kbagent token delete --project PROJECT --token-id TOKEN-ID` | | Rotate a token: generate a new value and invalidate the old one (secret shown once) | `kbagent token refresh --project PROJECT --token-id TOKEN-ID` | | Show the current PAYG credit balance for one or more projects | `kbagent billing credits` | diff --git a/plugins/kbagent/skills/kbagent/references/commands-reference.md b/plugins/kbagent/skills/kbagent/references/commands-reference.md index b4cd7b2d..da57bf02 100644 --- a/plugins/kbagent/skills/kbagent/references/commands-reference.md +++ b/plugins/kbagent/skills/kbagent/references/commands-reference.md @@ -200,6 +200,7 @@ Uses the per-project Storage token (no manage token). Control plane = `stream. 204). Destructive; confirms via prompt unless `--yes` or `--json`. - `token refresh --project NAME --token-id ID [--yes]` -- rotate a token's secret (POST `.../refresh`); the new secret is printed ONCE. Confirms unless `--yes` or `--json`. diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index 598a3f97..fdb350f6 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -3709,3 +3709,65 @@ mapping in favor of `changed_since: adaptive`, which tracks a assumption: here the empty state is the expensive, surprising path, and a seeded checkpoint is the conservative one. Do not assume "no state = safe default" when adaptive is involved. + +## A failed POST/PATCH is no longer retried -- and the error now says what to do (since v0.86.0) + +kbagent used to retry every request on `429/500/502/503/504`, regardless of +HTTP method. That is safe for a read and unsafe for a write, because a 5xx does +not mean "nothing happened": + +- **Keboola's token mint persists the token row before the step that can + throw.** `POST /v2/storage/tokens` saves the new token, generates its secret, + and only then resolves bucket permissions -- and that last block is outside + any transaction. A 500 raised there leaves a live token behind that the + caller never saw, so three attempts could leave three of them. This is what + [issue #599](https://github.com/keboola/cli/issues/599) reported from the + `europe-west3.gcp` stack. +- The same hazard applies to every other non-idempotent call: `job run`, + `job terminate`, `config new`, `flow new`, `config oauth-url` (which mints a + token of its own), `data-app deploy`. + +What changed: + +- **5xx is retried only on `GET/HEAD/OPTIONS/PUT/DELETE`.** A `POST`/`PATCH` + answered with a 5xx fails on the first attempt. +- **429 is still retried on every method** -- the server is stating it did not + process the request, so repeating it is safe. +- **Transport failures are split by what they prove.** A refused connection + (`ConnectError`) or a connect/pool timeout never delivered the request and is + still retried on any method. A read/write timeout means the request was sent + and the outcome is unknown, so it is not retried on a `POST`/`PATCH`. + +Two consequences for an agent reading an error: + +1. **`retryable: false` on a 500 from a write is deliberate, not a + misclassification.** Do not paper over it with a retry loop of your own. The + correct next step is to check what the server already did -- for a token + mint, `kbagent token list`; for a job, `kbagent job list`. +2. **A 5xx message now carries the `exceptionId`** the Keboola API returns + alongside its generic `"Application error."`, plus one of two hints: the + request was not retried because the method is not idempotent, or the same + 5xx survived all 3 attempts and is therefore an upstream incident. Quote the + `exceptionId` when escalating to Keboola support -- it is the only handle + that traces back to the actual server-side exception. + +## `token list` is the only way to see what exists -- and it never shows secrets (since v0.86.0) + +`kbagent token list --project P` (`GET /v2/storage/tokens`) closes the gap where +the `token` group could mint, revoke, and rotate but not enumerate -- so there +was no way to obtain the `--token-id` that `delete` / `refresh` require without +the web UI. + +- **Secrets are stripped from every row, including under `--json`.** On a + project carrying the `force-decrypted-token` feature the Storage API embeds + each token's live value in the listing. kbagent removes it at the service + layer (and in the SDK facade) before anything is rendered: `token create` / + `token refresh` are the only reveals, and a listing that dumped live values + would break that contract for every token in the project at once. Do not + reach for `kbagent http get` to work around this. +- **It needs `canManageTokens`, same as `create`.** A plain Storage token gets + a 403 -> `ACCESS_DENIED`. +- The master token appears in the listing with `isMasterToken: true` and cannot + be deleted -- the API refuses. +- SDK parity: `Client.list_tokens() -> list[TokenListEntryResult]`, secrets + stripped there too. diff --git a/src/keboola_agent_cli/__init__.py b/src/keboola_agent_cli/__init__.py index dadbbfda..9b00981d 100644 --- a/src/keboola_agent_cli/__init__.py +++ b/src/keboola_agent_cli/__init__.py @@ -12,6 +12,7 @@ ScopedTokenResult, StreamSourceResult, SyncPushResult, + TokenListEntryResult, UploadTableResult, ) from .services.job_idempotency_store import JobIdempotencyStore @@ -33,6 +34,7 @@ "ScopedTokenResult", "StreamSourceResult", "SyncPushResult", + "TokenListEntryResult", "UploadTableResult", "__version__", ] diff --git a/src/keboola_agent_cli/auth/auth_client.py b/src/keboola_agent_cli/auth/auth_client.py index 8b38e75f..7259194a 100644 --- a/src/keboola_agent_cli/auth/auth_client.py +++ b/src/keboola_agent_cli/auth/auth_client.py @@ -770,7 +770,14 @@ def _extract_error_message(response: httpx.Response) -> str: # Shared error mapping # ------------------------------------------------------------------ - def _raise_api_error(self, response: httpx.Response, base_url: str | None = None) -> None: + def _raise_api_error( + self, + response: httpx.Response, + base_url: str | None = None, + *, + hint: str | None = None, + retryable: bool | None = None, + ) -> None: """Escalate a 404 before falling back to the shared error mapping. `BaseHttpClient._do_request` calls this method for every @@ -778,10 +785,20 @@ def _raise_api_error(self, response: httpx.Response, base_url: str | None = None (rather than adding a check in each method) covers all of them at once. `poll_device_token` bypasses `_do_request` entirely and calls `_map_auth_error` directly for the same 404 case. + + `hint` / `retryable` are the base class's 5xx guidance; they are + forwarded untouched so an auth-endpoint 500 reads the same as any + other (issue #599). A 404 is escalated before they matter. """ - self._map_auth_error(response) + self._map_auth_error(response, hint=hint, retryable=retryable) - def _map_auth_error(self, response: httpx.Response) -> NoReturn: + def _map_auth_error( + self, + response: httpx.Response, + *, + hint: str | None = None, + retryable: bool | None = None, + ) -> NoReturn: """Map a failed auth-endpoint response, escalating 404 to a dedicated code. `login_password`/`verify_mfa_totp` never reach this method -- both @@ -812,7 +829,7 @@ def _map_auth_error(self, response: httpx.Response) -> NoReturn: error_code=ErrorCode.AUTH_NOT_SUPPORTED_ON_STACK, retryable=False, ) - super()._raise_api_error(response, self._base_url) + super()._raise_api_error(response, self._base_url, hint=hint, retryable=retryable) # BaseHttpClient._raise_api_error always raises; the static type # checker cannot see that across the base-class call, so make the # divergence explicit rather than relax this method's NoReturn type. diff --git a/src/keboola_agent_cli/client/tokens.py b/src/keboola_agent_cli/client/tokens.py index 623cb80d..71a70b2e 100644 --- a/src/keboola_agent_cli/client/tokens.py +++ b/src/keboola_agent_cli/client/tokens.py @@ -159,6 +159,28 @@ def create_scoped_token( response = self._request("POST", "/v2/storage/tokens", data=data) return response.json() + def list_tokens(self) -> list[dict[str, Any]]: + """List the project's Storage API tokens (``GET /v2/storage/tokens``). + + Returns the API's array verbatim -- one dict per token, carrying ``id``, + ``description``, ``created``, ``expires``, ``isExpired``, + ``isMasterToken``, the ``can*`` grants, ``bucketPermissions`` and (when + the token was minted by another token) ``creatorToken``. + + The acting token must carry ``canManageTokens``; the API answers 403 + otherwise (surfaced as ``ACCESS_DENIED``). + + A **secret is never listed here as a rule, but the API is not a + guarantee**: on a project carrying the ``force-decrypted-token`` + feature the response embeds each token's live value in a ``token`` + field. Callers that render this must strip it -- ``TokenService`` + does. Anything past the documented array shape (an envelope object, + say) degrades to an empty list rather than blowing up in the caller. + """ + response = self._request("GET", "/v2/storage/tokens") + payload = response.json() + return payload if isinstance(payload, list) else [] + def delete_token(self, token_id: str) -> None: """Revoke a Storage API token immediately (``DELETE /v2/storage/tokens/{id}``). diff --git a/src/keboola_agent_cli/commands/context.py b/src/keboola_agent_cli/commands/context.py index c77adc68..274d1380 100644 --- a/src/keboola_agent_cli/commands/context.py +++ b/src/keboola_agent_cli/commands/context.py @@ -788,6 +788,10 @@ ### Scoped Storage Tokens + kbagent token list --project NAME + List the project's Storage API tokens (id, description, created, expires, master flag, + creating token). Secrets are never listed -- `token create` is the only reveal. This is where + the --token-id for delete/refresh comes from. Acting token needs canManageTokens. kbagent token create --project NAME --description DESC [--bucket-write BUCKET ...] [--bucket-read BUCKET ...] [--component-access ID ...] [--can-read-all-file-uploads] [--expires-in N] Create a scoped Storage API token (Keboola single-bucket-write pattern). --bucket-write / --bucket-read (repeatable) grant per-bucket write/read; write wins when a bucket is on both. diff --git a/src/keboola_agent_cli/commands/token.py b/src/keboola_agent_cli/commands/token.py index a8c3b471..71792aeb 100644 --- a/src/keboola_agent_cli/commands/token.py +++ b/src/keboola_agent_cli/commands/token.py @@ -19,6 +19,7 @@ import typer from rich.console import Console from rich.panel import Panel +from rich.table import Table from ..errors import ConfigError, ErrorCode, KeboolaApiError from ._helpers import ( @@ -71,6 +72,43 @@ def _format_created_token(console: Console, data: dict[str, Any]) -> None: console.print(Panel("\n".join(lines), title="Scoped token created", expand=False)) +def _format_token_list(console: Console, data: dict[str, Any]) -> None: + """Render the project's tokens as a table -- never their secret values.""" + tokens = data.get("tokens") or [] + alias = data.get("alias", "") + if not tokens: + console.print( + f"No tokens visible in project [cyan]{alias}[/cyan]. " + "Mint one with [bold]kbagent token create[/bold]." + ) + return + table = Table(title=f"Storage API tokens -- {alias} ({len(tokens)})") + table.add_column("ID", style="bold cyan") + table.add_column("Description") + table.add_column("Created", style="dim") + table.add_column("Expires", style="dim") + table.add_column("Master", justify="center") + table.add_column("Created by", style="dim") + for token in tokens: + expires = token.get("expires") + if not expires: + expires_label = "never" + elif token.get("isExpired"): + expires_label = f"[red]{expires} (expired)[/red]" + else: + expires_label = str(expires) + creator = token.get("creatorToken") or {} + table.add_row( + str(token.get("id", "")), + str(token.get("description", "")), + str(token.get("created") or ""), + expires_label, + "yes" if token.get("isMasterToken") else "", + str(creator.get("description") or ""), + ) + console.print(table) + + def _format_deleted_token(console: Console, data: dict[str, Any]) -> None: """Render the outcome of a token delete.""" console.print( @@ -145,6 +183,26 @@ def token_create( formatter.output(result, _format_created_token) +@token_app.command("list") +def token_list( + ctx: typer.Context, + project: str = typer.Option(..., "--project", "-p", help="Project alias"), +) -> None: + """List the project's Storage API tokens (no secrets -- those are mint-only). + + Answers "what already exists" and hands you the token id that `token delete` + and `token refresh` require, without a detour through the web UI. The acting + project token must carry canManageTokens, same as `token create`. + """ + formatter = get_formatter(ctx) + service = get_service(ctx, "token_service") + try: + result = service.list_tokens(alias=project) + except (ConfigError, KeboolaApiError) as exc: + _handle_errors(formatter, exc) + formatter.output(result, _format_token_list) + + @token_app.command("delete") def token_delete( ctx: typer.Context, diff --git a/src/keboola_agent_cli/constants.py b/src/keboola_agent_cli/constants.py index e65d562d..63fe7052 100644 --- a/src/keboola_agent_cli/constants.py +++ b/src/keboola_agent_cli/constants.py @@ -54,6 +54,15 @@ def _resolve_app_name() -> str: MAX_RETRIES: int = 3 BACKOFF_BASE: float = 1.0 # seconds; delays: 1s, 2s, 4s +# RFC 9110 idempotent methods -- the only ones a 5xx or a read timeout may be +# repeated on. A failed POST/PATCH can have taken effect server-side before the +# error surfaced: Keboola's own token mint persists the token row BEFORE the +# step that can throw, so retrying a 500 from `POST /v2/storage/tokens` can +# leave live credentials behind that the caller never sees (issue #599). +# 429 is exempt from this gate -- the server states it did not process the +# request, so repeating it is safe regardless of method. +RETRY_SAFE_METHODS: frozenset[str] = frozenset({"GET", "HEAD", "OPTIONS", "PUT", "DELETE"}) + # --- HTTP Timeout --- DEFAULT_TIMEOUT: httpx.Timeout = httpx.Timeout(connect=5.0, read=30.0, write=10.0, pool=5.0) diff --git a/src/keboola_agent_cli/http_base.py b/src/keboola_agent_cli/http_base.py index 91b5262f..b6661d95 100644 --- a/src/keboola_agent_cli/http_base.py +++ b/src/keboola_agent_cli/http_base.py @@ -22,6 +22,7 @@ MAX_API_ERROR_LENGTH, MAX_RETRIES, MAX_RETRY_AFTER_SECONDS, + RETRY_SAFE_METHODS, RETRYABLE_STATUS_CODES, ) from .errors import ErrorCode, KeboolaApiError, mask_token @@ -151,6 +152,13 @@ def _do_request( Retries on status codes 429, 500, 502, 503, 504 up to MAX_RETRIES times with exponential backoff (1s, 2s, 4s). + A 5xx (and a read/write timeout) is only repeated on an idempotent + method -- see ``RETRY_SAFE_METHODS``. Repeating a failed POST/PATCH can + duplicate server-side state the caller never gets to see, so those fail + on the first attempt with a message saying so (issue #599). A 429 and a + refused connection are repeated on every method: in both cases the + server provably did not process the request. + Args: method: HTTP method (GET, POST, etc.). path: URL path relative to base_url. @@ -168,6 +176,7 @@ def _do_request( http_client = client or self._client url_label = base_url or self._base_url last_response: httpx.Response | None = None + retry_safe = method.upper() in RETRY_SAFE_METHODS for attempt in range(MAX_RETRIES): try: @@ -176,7 +185,13 @@ def _do_request( if response.status_code < 400: return response - if response.status_code in RETRYABLE_STATUS_CODES and attempt < MAX_RETRIES - 1: + # 429 is repeatable on any method; a 5xx only on an idempotent one. + may_repeat = retry_safe or response.status_code == 429 + if ( + response.status_code in RETRYABLE_STATUS_CODES + and may_repeat + and attempt < MAX_RETRIES - 1 + ): if response.status_code == 429: retry_after = response.headers.get("Retry-After") if retry_after: @@ -201,10 +216,23 @@ def _do_request( last_response = response continue - self._raise_api_error(response, url_label) + hint = self._server_error_hint(method, response.status_code, attempt + 1) + self._raise_api_error( + response, + url_label, + hint=hint, + retryable=may_repeat and response.status_code in RETRYABLE_STATUS_CODES, + ) except httpx.TimeoutException as exc: - if attempt < MAX_RETRIES - 1: + # A connect/pool timeout never delivered the request, so it is + # repeatable on any method. A read/write timeout means the + # request WAS sent and the outcome is unknown -- only repeat it + # when the method is idempotent. + timeout_repeatable = retry_safe or isinstance( + exc, httpx.ConnectTimeout | httpx.PoolTimeout + ) + if timeout_repeatable and attempt < MAX_RETRIES - 1: delay = BACKOFF_BASE * (2**attempt) logger.debug( "Retry attempt %d/%d for %s %s (timeout), delay %.1fs", @@ -216,11 +244,15 @@ def _do_request( ) time.sleep(delay) continue + unsafe_note = "" if timeout_repeatable else f" {self._non_idempotent_note(method)}" raise KeboolaApiError( - message=f"Request timed out connecting to {url_label} (token: {self._masked_token})", + message=( + f"Request timed out connecting to {url_label} " + f"(token: {self._masked_token}){unsafe_note}" + ), status_code=0, error_code=ErrorCode.TIMEOUT, - retryable=True, + retryable=timeout_repeatable, ) from exc except httpx.ConnectError as exc: @@ -244,7 +276,11 @@ def _do_request( ) from exc if last_response is not None: - self._raise_api_error(last_response, url_label) + self._raise_api_error( + last_response, + url_label, + hint=self._server_error_hint(method, last_response.status_code, MAX_RETRIES), + ) raise KeboolaApiError( message=f"Request failed after {MAX_RETRIES} retries to {url_label} (token: {self._masked_token})", @@ -253,7 +289,46 @@ def _do_request( retryable=True, ) - def _raise_api_error(self, response: httpx.Response, base_url: str | None = None) -> None: + @staticmethod + def _non_idempotent_note(method: str) -> str: + """One sentence naming the partial-effect risk of an unrepeated write.""" + return ( + f"{method.upper()} is not idempotent, so this request was not retried -- " + "the operation may already have taken effect server-side; verify the resource " + "state before trying again." + ) + + @classmethod + def _server_error_hint(cls, method: str, status: int, attempts: int) -> str | None: + """Return the actionable next step for a 5xx, or None if there isn't one. + + Two situations need two different answers (issue #599). A 5xx that + survived every retry is an upstream incident nobody on this side can + fix, and the operator's next step is to escalate with the exceptionId. + A 5xx on the single attempt of a non-idempotent write is the opposite: + the server may already have done the work, so the next step is to check + before repeating. A generic "API error 500" said neither. + """ + if status < 500: + return None + if attempts > 1: + return ( + f"The same 5xx came back on all {attempts} attempts, which points at an " + "upstream Keboola incident rather than a caller mistake -- check " + "status.keboola.com and contact Keboola support, quoting the exceptionId above." + ) + if method.upper() not in RETRY_SAFE_METHODS: + return cls._non_idempotent_note(method) + return None + + def _raise_api_error( + self, + response: httpx.Response, + base_url: str | None = None, + *, + hint: str | None = None, + retryable: bool | None = None, + ) -> None: """Convert an HTTP error response into a KeboolaApiError. Parses the response body for error messages, truncates long messages @@ -263,6 +338,12 @@ def _raise_api_error(self, response: httpx.Response, base_url: str | None = None Args: response: The HTTP error response. base_url: Optional URL label for error messages. + hint: Optional actionable next step appended to a 5xx message + (see :meth:`_server_error_hint`). + retryable: Overrides the status-derived ``retryable`` flag. A 500 + on a POST is in RETRYABLE_STATUS_CODES but must NOT be + advertised as retryable -- kbagent deliberately did not repeat + it, and neither should the caller without checking first. Raises: KeboolaApiError: Always raised with appropriate error code and message. @@ -270,8 +351,17 @@ def _raise_api_error(self, response: httpx.Response, base_url: str | None = None status = response.status_code url_label = base_url or self._base_url + exception_id = "" try: body = response.json() + # Keboola answers a 5xx with a generic `error` ("Application + # error.") plus an `exceptionId` -- the ONLY handle Keboola support + # can trace the incident by. Dropping it, as this parser used to, + # left the operator with nothing to escalate (issue #599). + if isinstance(body, dict): + raw_exception_id = body.get("exceptionId") + if isinstance(raw_exception_id, str) and raw_exception_id: + exception_id = raw_exception_id # Real Keboola APIs answer with one of these keys in priority # order. Two caveats: # 1. Keboola Metastore puts the HTTP status code into `error` @@ -327,10 +417,18 @@ def _raise_api_error(self, response: httpx.Response, base_url: str | None = None retryable=False, ) - retryable = status in RETRYABLE_STATUS_CODES + # Truncation above guards the API's own text; the id and the hint are + # kbagent-authored and short, so they are appended after it and always + # survive into the message the operator actually reads. + suffix = f" [exceptionId: {exception_id}]" if exception_id else "" + if hint: + suffix += f" {hint}" raise KeboolaApiError( - message=f"API error {status} from {url_label} (token: {self._masked_token}): {api_message}", + message=( + f"API error {status} from {url_label} " + f"(token: {self._masked_token}): {api_message}{suffix}" + ), status_code=status, error_code=ErrorCode.API_ERROR, - retryable=retryable, + retryable=status in RETRYABLE_STATUS_CODES if retryable is None else retryable, ) diff --git a/src/keboola_agent_cli/lib.py b/src/keboola_agent_cli/lib.py index 46b0056a..147c69c8 100644 --- a/src/keboola_agent_cli/lib.py +++ b/src/keboola_agent_cli/lib.py @@ -57,6 +57,7 @@ QueryResult, ScopedTokenResult, StreamSourceResult, + TokenListEntryResult, UploadTableResult, ) from .services.job_idempotency_store import JobIdempotencyStore, run_idempotent_job @@ -567,6 +568,22 @@ def create_scoped_token( ) ) + def list_tokens(self) -> list[TokenListEntryResult]: + """List the project's Storage API tokens as typed entries. + + The counterpart to :meth:`create_scoped_token`: it answers "what did I + already mint" and hands back the ``id`` that :meth:`delete_token` / + :meth:`refresh_token` need. Secrets are stripped before validation -- + the mint is the only reveal (see :class:`TokenListEntryResult`). The + acting token must carry ``canManageTokens``. + """ + return [ + TokenListEntryResult.model_validate( + {key: value for key, value in token.items() if key != "token"} + ) + for token in self._client.list_tokens() + ] + def delete_token(self, token_id: str) -> None: """Revoke a Storage API token immediately (active per-device revocation).""" self._client.delete_token(token_id) diff --git a/src/keboola_agent_cli/permissions.py b/src/keboola_agent_cli/permissions.py index 4efcfcf0..440b89d1 100644 --- a/src/keboola_agent_cli/permissions.py +++ b/src/keboola_agent_cli/permissions.py @@ -61,6 +61,7 @@ "stream.delete": "destructive", # Storage API scoped tokens: minting/rotating is a write (creates a # credential), revoking is destructive (a live token stops working). + "token.list": "read", "token.create": "write", "token.refresh": "write", "token.delete": "destructive", diff --git a/src/keboola_agent_cli/result_models.py b/src/keboola_agent_cli/result_models.py index f7f8543c..db535105 100644 --- a/src/keboola_agent_cli/result_models.py +++ b/src/keboola_agent_cli/result_models.py @@ -297,6 +297,36 @@ class ScopedTokenResult(_ApiResultModel): ) +class TokenListEntryResult(_ApiResultModel): + """One Storage API token as listed by :meth:`keboola_agent_cli.Client.list_tokens`. + + Deliberately carries **no** secret field. `create_scoped_token` is the one + and only reveal in this SDK; a listing that returned live values would + break that contract for every token in the project at once, so the facade + drops the field before validating even when the API includes it (projects + with the ``force-decrypted-token`` feature do). Everything else the API + reports -- ``bucketPermissions``, ``componentAccess``, the remaining + ``can*`` grants -- is preserved as model extras. + """ + + id: str = Field(default="", description="Token ID (use with delete_token / refresh_token).") + description: str = Field(default="", description="Human-readable token description.") + created: str | None = Field(default=None, description="ISO creation timestamp.") + expires: str | None = Field( + default=None, description="ISO expiry timestamp; None = never expires." + ) + is_expired: bool = Field( + default=False, + validation_alias=AliasChoices("isExpired", "is_expired"), + description="True once the token is past its expiry.", + ) + is_master_token: bool = Field( + default=False, + validation_alias=AliasChoices("isMasterToken", "is_master_token"), + description="True for the project's master token (cannot be deleted).", + ) + + class StreamSourceResult(_ApiResultModel): """A per-device Data Streams (OTLP) source (issue: device enrollment). diff --git a/src/keboola_agent_cli/server/routers/token.py b/src/keboola_agent_cli/server/routers/token.py index fdc5b740..d3546a08 100644 --- a/src/keboola_agent_cli/server/routers/token.py +++ b/src/keboola_agent_cli/server/routers/token.py @@ -32,6 +32,17 @@ class TokenIdBody(BaseModel): token_id: str +@router.get("/{project}/list", summary="List the project's Storage tokens") +def list_tokens( + project: str, + registry: ServiceRegistry = Depends(get_registry), +) -> dict[str, Any]: + """List the project's tokens without their secrets. Mirrors + `kbagent token list`. The acting project token must have canManageTokens. + """ + return registry.token.list_tokens(alias=project) + + @router.post("/{project}/create", summary="Mint a scoped Storage token") def create_token( project: str, diff --git a/src/keboola_agent_cli/services/token_service.py b/src/keboola_agent_cli/services/token_service.py index 7d425349..e58e796a 100644 --- a/src/keboola_agent_cli/services/token_service.py +++ b/src/keboola_agent_cli/services/token_service.py @@ -96,6 +96,29 @@ def create_scoped_token( finally: client.close() + def list_tokens(self, *, alias: str) -> dict[str, Any]: + """List every Storage token in ``alias``'s project. + + Returns ``{"alias", "count", "tokens"}``. Each entry is the raw API + token dict **minus its ``token`` field**: a project with the + ``force-decrypted-token`` feature has the Storage API embed live secret + values in the listing, and echoing those would break the group's + "the secret is revealed once, at mint" contract for every token at + once. Everything else is passed through untouched. + + The acting token needs ``canManageTokens``, same as create/refresh. + """ + creds = self._resolve_project(alias) + client = self._client_factory(creds.stack_url, creds.token) + try: + tokens = [ + {key: value for key, value in token.items() if key != "token"} + for token in client.list_tokens() + ] + return {"alias": alias, "count": len(tokens), "tokens": tokens} + finally: + client.close() + def delete_token(self, *, alias: str, token_id: str) -> dict[str, Any]: """Revoke a token immediately in ``alias``'s project.""" creds = self._resolve_project(alias) diff --git a/tests/test_client.py b/tests/test_client.py index d43dcdcd..8e6fc3eb 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -3092,14 +3092,14 @@ def test_kill_job_500_404_mismatch(self, httpx_mock) -> None: missing jobs. Client passes the error through; service layer distinguishes the two cases via a follow-up GET. """ - # HTTP 500 triggers retry path (MAX_RETRIES total attempts); stub each. - for _ in range(MAX_RETRIES): - httpx_mock.add_response( - url="https://queue.keboola.com/jobs/1/kill", - method="POST", - json={"error": "Internal Server Error occurred.", "code": 404}, - status_code=500, - ) + # A kill is a POST, so the 500 is NOT retried -- one stub, one attempt + # (issue #599). Retrying it would be a second terminate command. + httpx_mock.add_response( + url="https://queue.keboola.com/jobs/1/kill", + method="POST", + json={"error": "Internal Server Error occurred.", "code": 404}, + status_code=500, + ) with ( KeboolaClient(stack_url=_BASE, token=_TOKEN) as client, @@ -3108,6 +3108,7 @@ def test_kill_job_500_404_mismatch(self, httpx_mock) -> None: client.kill_job("1") assert exc_info.value.status_code == 500 + assert len(httpx_mock.get_requests()) == 1 def test_kill_job_url_encodes_job_id(self, httpx_mock) -> None: """kill_job() URL-encodes the job ID to prevent path injection.""" diff --git a/tests/test_client_device_enrollment.py b/tests/test_client_device_enrollment.py index 547f1c22..2ce6717b 100644 --- a/tests/test_client_device_enrollment.py +++ b/tests/test_client_device_enrollment.py @@ -166,6 +166,50 @@ def test_refresh_returns_new_token_dict(self, httpx_mock) -> None: assert str(request.url) == f"{STACK_URL}/v2/storage/tokens/9001/refresh" +class TestListTokens: + def test_list_returns_raw_array(self, httpx_mock) -> None: + """GET /v2/storage/tokens -> the API's array, passed through verbatim.""" + httpx_mock.add_response( + url=f"{STACK_URL}/v2/storage/tokens", + method="GET", + json=[ + {"id": "9001", "description": "device 42", "isMasterToken": False}, + {"id": "1", "description": "master", "isMasterToken": True}, + ], + status_code=200, + ) + + client = _make_client() + try: + result = client.list_tokens() + finally: + client.close() + + assert [t["id"] for t in result] == ["9001", "1"] + request = httpx_mock.get_requests()[0] + assert request.method == "GET" + assert str(request.url) == f"{STACK_URL}/v2/storage/tokens" + + def test_non_list_body_returns_empty(self, httpx_mock) -> None: + """A non-array body cannot be iterated as tokens -- degrade to empty. + + Guards the caller from a TypeError if the endpoint ever answers with an + envelope object instead of the bare array it documents today. + """ + httpx_mock.add_response( + url=f"{STACK_URL}/v2/storage/tokens", + method="GET", + json={"unexpected": "envelope"}, + status_code=200, + ) + + client = _make_client() + try: + assert client.list_tokens() == [] + finally: + client.close() + + # ---------------------------------------------------------------------------- # Per-device stream-source lifecycle # ---------------------------------------------------------------------------- diff --git a/tests/test_dev_portal_client.py b/tests/test_dev_portal_client.py index d47ab880..c845b3e9 100644 --- a/tests/test_dev_portal_client.py +++ b/tests/test_dev_portal_client.py @@ -292,14 +292,14 @@ def test_upload_icon_presign_failure(self, httpx_mock, monkeypatch): url="https://apps-api.keboola.com/auth/login", json={"token": "Bearer abc"}, ) - # Add 3 responses (MAX_RETRIES=3) since 500 is retryable. - for _ in range(3): - httpx_mock.add_response( - method="POST", - url="https://apps-api.keboola.com/vendors/keboola/apps/keboola.ex-foo/icon", - status_code=500, - json={"error": "boom"}, - ) + # One response: an icon upload is a POST, and a POST is no longer + # retried on 500 (issue #599). + httpx_mock.add_response( + method="POST", + url="https://apps-api.keboola.com/vendors/keboola/apps/keboola.ex-foo/icon", + status_code=500, + json={"error": "boom"}, + ) # Suppress retry sleeps. import keboola_agent_cli.http_base as http_base_module diff --git a/tests/test_e2e.py b/tests/test_e2e.py index c136d1cf..10d994f6 100644 --- a/tests/test_e2e.py +++ b/tests/test_e2e.py @@ -12675,7 +12675,7 @@ def _first_bucket_id(self) -> str | None: # ------------------------------------------------------------------ def test_scoped_token_mint_rotate_revoke(self) -> None: - """`token create` (scoped, expiring) -> `token refresh` -> `token delete`. + """`token create` (scoped, expiring) -> `token list` -> `token refresh` -> `token delete`. Never prints the secret; deletes the token the moment its shape is verified so no live credential outlives the test. @@ -12725,13 +12725,26 @@ def test_scoped_token_mint_rotate_revoke(self) -> None: assert bool(data.get("token")), "minted token must reveal a non-empty secret once" assert data.get("alias") == self.alias - _step(2, "token refresh", "rotate the secret; id is stable, old value dies") + _step(2, "token list", "the minted token is visible, and no row carries a secret") + listed = self._run_ok("token", "list", "--project", self.alias)["data"] + assert listed.get("alias") == self.alias + assert listed.get("count") == len(listed.get("tokens") or []) + rows = {str(row.get("id")): row for row in listed.get("tokens") or []} + assert token_id in rows, "the freshly minted token must appear in the listing" + assert rows[token_id].get("description", "").endswith("e2e device-enrollment token") + # The whole point of the strip: `create` is the only reveal, so not one + # row -- not even another project token's -- may carry a value. + assert all("token" not in row for row in rows.values()), ( + "token list must never carry a secret value" + ) + + _step(3, "token refresh", "rotate the secret; id is stable, old value dies") refreshed = self._run_ok( "token", "refresh", "--project", self.alias, "--token-id", token_id, "--yes" )["data"] assert bool(refreshed.get("token")), "rotated token must reveal a new non-empty secret" - _step(3, "token delete", "revoke immediately -- no live credential left behind") + _step(4, "token delete", "revoke immediately -- no live credential left behind") deleted = self._run_ok( "token", "delete", "--project", self.alias, "--token-id", token_id, "--yes" )["data"] diff --git a/tests/test_http_base.py b/tests/test_http_base.py index 7f68471b..9f60d4fb 100644 --- a/tests/test_http_base.py +++ b/tests/test_http_base.py @@ -222,6 +222,242 @@ def test_alternate_client_parameter(self, httpx_mock) -> None: base_client.close() +class TestNonIdempotentRetryPolicy: + """A 5xx/transport failure on a non-idempotent method must NOT be retried. + + ``POST`` creates server-side state. Keboola's own token mint persists the + token row *before* the step that can fail, so a blind retry of a failed + ``POST /v2/storage/tokens`` can silently leave two live credentials behind + (issue #599). Only the RFC 9110 idempotent methods are safe to repeat. + """ + + def _client(self) -> BaseHttpClient: + return BaseHttpClient( + base_url=STACK_URL, + token=TOKEN, + headers={"Authorization": f"Bearer {TOKEN}"}, + ) + + def test_post_not_retried_on_500(self, httpx_mock) -> None: + """A POST answered with 500 fails on the first attempt -- no second mint.""" + httpx_mock.add_response( + url=f"{STACK_URL}/v2/storage/tokens", + status_code=500, + json={"error": "Application error."}, + ) + + client = self._client() + try: + with pytest.raises(KeboolaApiError) as exc_info: + client._do_request("POST", "/v2/storage/tokens") + assert exc_info.value.status_code == 500 + assert len(httpx_mock.get_requests()) == 1 + finally: + client.close() + + def test_post_not_retried_on_502(self, httpx_mock) -> None: + """The gate is the method, not the specific 5xx code.""" + httpx_mock.add_response(url=f"{STACK_URL}/test-path", status_code=502, text="Bad Gateway") + + client = self._client() + try: + with pytest.raises(KeboolaApiError): + client._do_request("POST", "/test-path") + assert len(httpx_mock.get_requests()) == 1 + finally: + client.close() + + def test_patch_not_retried_on_500(self, httpx_mock) -> None: + """PATCH is not idempotent either (RFC 9110), so it gets the same gate.""" + httpx_mock.add_response(url=f"{STACK_URL}/test-path", status_code=500, text="boom") + + client = self._client() + try: + with pytest.raises(KeboolaApiError): + client._do_request("PATCH", "/test-path") + assert len(httpx_mock.get_requests()) == 1 + finally: + client.close() + + def test_post_still_retried_on_429(self, httpx_mock) -> None: + """429 means the server refused to process it -- repeating is safe.""" + httpx_mock.add_response(url=f"{STACK_URL}/test-path", status_code=429, text="slow down") + httpx_mock.add_response(url=f"{STACK_URL}/test-path", status_code=200, json={"ok": True}) + + client = self._client() + import keboola_agent_cli.http_base as http_base_module + + original_sleep = http_base_module.time.sleep + http_base_module.time.sleep = _noop_sleep # ty: ignore[invalid-assignment] + try: + response = client._do_request("POST", "/test-path") + assert response.status_code == 200 + assert len(httpx_mock.get_requests()) == 2 + finally: + http_base_module.time.sleep = original_sleep + client.close() + + def test_put_retried_on_500(self, httpx_mock) -> None: + """PUT is idempotent, so it keeps the retry safety net.""" + httpx_mock.add_response(url=f"{STACK_URL}/test-path", status_code=500, text="boom") + httpx_mock.add_response(url=f"{STACK_URL}/test-path", status_code=200, json={"ok": True}) + + client = self._client() + import keboola_agent_cli.http_base as http_base_module + + original_sleep = http_base_module.time.sleep + http_base_module.time.sleep = _noop_sleep # ty: ignore[invalid-assignment] + try: + response = client._do_request("PUT", "/test-path") + assert response.status_code == 200 + assert len(httpx_mock.get_requests()) == 2 + finally: + http_base_module.time.sleep = original_sleep + client.close() + + def test_delete_retried_on_503(self, httpx_mock) -> None: + """DELETE is idempotent -- repeating a delete converges on the same state.""" + httpx_mock.add_response(url=f"{STACK_URL}/test-path", status_code=503, text="unavailable") + httpx_mock.add_response(url=f"{STACK_URL}/test-path", status_code=204) + + client = self._client() + import keboola_agent_cli.http_base as http_base_module + + original_sleep = http_base_module.time.sleep + http_base_module.time.sleep = _noop_sleep # ty: ignore[invalid-assignment] + try: + response = client._do_request("DELETE", "/test-path") + assert response.status_code == 204 + assert len(httpx_mock.get_requests()) == 2 + finally: + http_base_module.time.sleep = original_sleep + client.close() + + def test_post_not_retried_on_timeout(self, httpx_mock) -> None: + """A timed-out POST may already have taken effect -- never repeat it.""" + httpx_mock.add_exception(httpx.ReadTimeout("Read timed out"), url=f"{STACK_URL}/test-path") + + client = self._client() + try: + with pytest.raises(KeboolaApiError) as exc_info: + client._do_request("POST", "/test-path") + assert exc_info.value.error_code == "TIMEOUT" + assert len(httpx_mock.get_requests()) == 1 + finally: + client.close() + + def test_post_retried_on_connect_error(self, httpx_mock) -> None: + """A refused connection never reached the server, so a POST may repeat.""" + httpx_mock.add_exception(httpx.ConnectError("Connection refused"), url=f"{STACK_URL}/x") + httpx_mock.add_response(url=f"{STACK_URL}/x", status_code=200, json={"ok": True}) + + client = self._client() + import keboola_agent_cli.http_base as http_base_module + + original_sleep = http_base_module.time.sleep + http_base_module.time.sleep = _noop_sleep # ty: ignore[invalid-assignment] + try: + response = client._do_request("POST", "/x") + assert response.status_code == 200 + assert len(httpx_mock.get_requests()) == 2 + finally: + http_base_module.time.sleep = original_sleep + client.close() + + +class TestServerErrorGuidance: + """A 5xx must tell the operator what to do next (issue #599).""" + + def _client(self) -> BaseHttpClient: + return BaseHttpClient( + base_url=STACK_URL, + token=TOKEN, + headers={"Authorization": f"Bearer {TOKEN}"}, + ) + + def test_unretried_500_names_the_partial_effect_risk(self, httpx_mock) -> None: + """The POST hint must warn the operation may already have taken effect.""" + httpx_mock.add_response( + url=f"{STACK_URL}/v2/storage/tokens", + status_code=500, + json={"error": "Application error."}, + ) + + client = self._client() + try: + with pytest.raises(KeboolaApiError) as exc_info: + client._do_request("POST", "/v2/storage/tokens") + message = exc_info.value.message + assert "POST" in message + assert "not retried" in message + assert "may already have taken effect" in message + finally: + client.close() + + def test_exhausted_500_points_at_an_upstream_incident(self, httpx_mock) -> None: + """A 500 that survives every retry is an incident, not a caller mistake.""" + for _ in range(MAX_RETRIES): + httpx_mock.add_response( + url=f"{STACK_URL}/test-path", + status_code=500, + json={"error": "Application error."}, + ) + + client = self._client() + import keboola_agent_cli.http_base as http_base_module + + original_sleep = http_base_module.time.sleep + http_base_module.time.sleep = _noop_sleep # ty: ignore[invalid-assignment] + try: + with pytest.raises(KeboolaApiError) as exc_info: + client._do_request("GET", "/test-path") + message = exc_info.value.message + assert f"{MAX_RETRIES} attempts" in message + assert "upstream Keboola incident" in message + assert "support" in message + finally: + http_base_module.time.sleep = original_sleep + client.close() + + def test_exception_id_surfaced_for_support(self, httpx_mock) -> None: + """Keboola's exceptionId is the only handle support can trace -- keep it.""" + httpx_mock.add_response( + url=f"{STACK_URL}/test-path", + status_code=500, + json={ + "error": "Application error.", + "exceptionId": "kbc-eu-central-1-connection-abc123", + "message": "Please contact our support", + }, + ) + + client = self._client() + try: + with pytest.raises(KeboolaApiError) as exc_info: + client._do_request("POST", "/test-path") + assert "kbc-eu-central-1-connection-abc123" in exc_info.value.message + finally: + client.close() + + def test_no_hint_appended_to_client_errors(self, httpx_mock) -> None: + """A 4xx is the caller's problem -- the incident hint would be misleading.""" + httpx_mock.add_response( + url=f"{STACK_URL}/test-path", + status_code=400, + json={"error": "Bad request"}, + ) + + client = self._client() + try: + with pytest.raises(KeboolaApiError) as exc_info: + client._do_request("POST", "/test-path") + message = exc_info.value.message + assert "not retried" not in message + assert "upstream Keboola incident" not in message + finally: + client.close() + + class TestBaseHttpClientErrorSanitization: """Verify message truncation and error mapping in the base class.""" diff --git a/tests/test_lib_device_enrollment.py b/tests/test_lib_device_enrollment.py index b79c4416..e131ccbe 100644 --- a/tests/test_lib_device_enrollment.py +++ b/tests/test_lib_device_enrollment.py @@ -253,3 +253,41 @@ def test_delegates_and_returns_none(self) -> None: assert client.delete_stream_source("cam01-src", branch_id="default") is None mock_kc.delete_stream_source.assert_called_once_with("cam01-src", branch_id="default") + + +class TestListTokens: + def test_returns_typed_entries(self) -> None: + mock_kc = MagicMock() + mock_kc.list_tokens.return_value = [ + { + "id": "12345", + "description": "device 42", + "created": "2026-08-01T10:00:00+0200", + "expires": None, + "isExpired": False, + "isMasterToken": False, + } + ] + result = _make_client(mock_kc).list_tokens() + assert len(result) == 1 + entry = result[0] + assert entry.id == "12345" + assert entry.description == "device 42" + assert entry.expires is None + assert entry.is_expired is False + assert entry.is_master_token is False + mock_kc.list_tokens.assert_called_once_with() + + def test_secret_never_reaches_the_caller(self) -> None: + """`create_scoped_token` is the one and only secret reveal.""" + mock_kc = MagicMock() + mock_kc.list_tokens.return_value = [ + {"id": "1", "description": "master", "token": "1-liveSecretValue"} + ] + result = _make_client(mock_kc).list_tokens() + assert "liveSecretValue" not in str(result[0].model_dump()) + + def test_empty_project(self) -> None: + mock_kc = MagicMock() + mock_kc.list_tokens.return_value = [] + assert _make_client(mock_kc).list_tokens() == [] diff --git a/tests/test_metastore_client.py b/tests/test_metastore_client.py index e12fa653..be87cee4 100644 --- a/tests/test_metastore_client.py +++ b/tests/test_metastore_client.py @@ -11,7 +11,6 @@ import pytest -from keboola_agent_cli.constants import MAX_RETRIES from keboola_agent_cli.errors import ErrorCode, KeboolaApiError from keboola_agent_cli.metastore_client import ( SEMANTIC_TYPES, @@ -189,13 +188,16 @@ def test_duplicate_name_409_becomes_already_exists(self, httpx_mock, metastore_c assert excinfo.value.retryable is False def test_duplicate_name_500_becomes_already_exists(self, httpx_mock, metastore_client) -> None: - """Legacy / pre-fix metastore still returns 500 -- retain the workaround.""" - for _ in range(MAX_RETRIES): - httpx_mock.add_response( - url=f"{METASTORE_URL_US}/api/v1/repository/semantic-metric", - status_code=500, - json={"error": "Failed to create meta object: duplicate name 'foo'"}, - ) + """Legacy / pre-fix metastore still returns 500 -- retain the workaround. + + A single response: ``post_item`` is a POST, which is no longer retried + on a 5xx (issue #599), so normalisation has to happen on attempt one. + """ + httpx_mock.add_response( + url=f"{METASTORE_URL_US}/api/v1/repository/semantic-metric", + status_code=500, + json={"error": "Failed to create meta object: duplicate name 'foo'"}, + ) with pytest.raises(KeboolaApiError) as excinfo: metastore_client.post_item("semantic-metric", name="foo", data={"name": "foo"}) assert excinfo.value.error_code == ErrorCode.ALREADY_EXISTS @@ -206,12 +208,11 @@ def test_duplicate_name_500_becomes_already_exists(self, httpx_mock, metastore_c def test_unrelated_500_passes_through(self, httpx_mock, metastore_client) -> None: """A 500 without the magic phrase keeps its API_ERROR code.""" - for _ in range(MAX_RETRIES): - httpx_mock.add_response( - url=f"{METASTORE_URL_US}/api/v1/repository/semantic-metric", - status_code=500, - json={"error": "some unrelated internal error"}, - ) + httpx_mock.add_response( + url=f"{METASTORE_URL_US}/api/v1/repository/semantic-metric", + status_code=500, + json={"error": "some unrelated internal error"}, + ) with pytest.raises(KeboolaApiError) as excinfo: metastore_client.post_item("semantic-metric", name="foo", data={"name": "foo"}) assert excinfo.value.error_code != ErrorCode.ALREADY_EXISTS diff --git a/tests/test_token_cli.py b/tests/test_token_cli.py index 80a8d117..5850f36a 100644 --- a/tests/test_token_cli.py +++ b/tests/test_token_cli.py @@ -188,3 +188,76 @@ def test_api_error_exit_code(self, tmp_path: Path) -> None: ) assert result.exit_code != 0 assert json.loads(result.output)["status"] == "error" + + +class TestList: + def _svc_with(self, tokens: list[dict]) -> MagicMock: + svc = MagicMock() + svc.list_tokens.return_value = { + "alias": ALIAS, + "count": len(tokens), + "tokens": tokens, + } + return svc + + def test_list_json(self, tmp_path: Path) -> None: + config_dir = tmp_path / "c" + config_dir.mkdir() + _seed(config_dir) + svc = self._svc_with( + [ + { + "id": "12345", + "description": "device enrollment", + "created": "2026-08-01T10:00:00+0200", + "expires": "2026-09-01T10:00:00+0200", + "isExpired": False, + "isMasterToken": False, + } + ] + ) + result = _invoke(config_dir, svc, ["--json", "token", "list", "--project", ALIAS]) + assert result.exit_code == 0 + payload = json.loads(result.stdout) + assert payload["data"]["count"] == 1 + assert payload["data"]["tokens"][0]["id"] == "12345" + svc.list_tokens.assert_called_once_with(alias=ALIAS) + + def test_list_human_renders_rows(self, tmp_path: Path) -> None: + config_dir = tmp_path / "c" + config_dir.mkdir() + _seed(config_dir) + svc = self._svc_with( + [ + {"id": "1", "description": "master token", "isMasterToken": True}, + {"id": "2", "description": "device enrollment", "isMasterToken": False}, + ] + ) + result = _invoke(config_dir, svc, ["token", "list", "--project", ALIAS]) + assert result.exit_code == 0 + assert "device enrollment" in result.stdout + assert "12345" not in result.stdout + + def test_list_human_empty(self, tmp_path: Path) -> None: + config_dir = tmp_path / "c" + config_dir.mkdir() + _seed(config_dir) + result = _invoke(config_dir, self._svc_with([]), ["token", "list", "--project", ALIAS]) + assert result.exit_code == 0 + assert "No tokens" in result.stdout + + def test_list_api_error_exits_nonzero(self, tmp_path: Path) -> None: + config_dir = tmp_path / "c" + config_dir.mkdir() + _seed(config_dir) + svc = MagicMock() + svc.list_tokens.side_effect = KeboolaApiError( + message="Access denied", status_code=403, error_code="ACCESS_DENIED" + ) + result = _invoke(config_dir, svc, ["--json", "token", "list", "--project", ALIAS]) + # ACCESS_DENIED is a general error (1) in this CLI's exit-code map; + # only INVALID_TOKEN / session failures are the auth class (3). + assert result.exit_code == 1 + payload = json.loads(result.output) + assert payload["status"] == "error" + assert payload["error"]["code"] == "ACCESS_DENIED" diff --git a/tests/test_token_service.py b/tests/test_token_service.py index 65ef44c9..2a56393b 100644 --- a/tests/test_token_service.py +++ b/tests/test_token_service.py @@ -141,3 +141,54 @@ def test_unknown_alias_raises(self, store, client_factory) -> None: factory, _ = client_factory with pytest.raises(ConfigError): _svc(store, factory).refresh_token(alias="nope", token_id="1") + + +class TestListTokens: + def test_returns_alias_and_tokens(self, store, client_factory) -> None: + factory, mock = client_factory + mock.list_tokens.return_value = [ + {"id": "1", "description": "master", "isMasterToken": True}, + {"id": "2", "description": "device", "isMasterToken": False}, + ] + result = _svc(store, factory).list_tokens(alias=ALIAS) + assert result["alias"] == ALIAS + assert result["count"] == 2 + assert [t["id"] for t in result["tokens"]] == ["1", "2"] + factory.assert_called_once_with(STACK_URL, TOKEN) + mock.close.assert_called_once() + + def test_secret_values_are_stripped(self, store, client_factory) -> None: + """A project with `force-decrypted-token` returns live secrets in the list. + + `token create` reveals a secret ONCE by design; a listing that dumped + every live token's value to stdout would break that contract wholesale. + """ + factory, mock = client_factory + mock.list_tokens.return_value = [ + {"id": "1", "description": "master", "token": "1-liveSecretValue"}, + {"id": "2", "description": "device"}, + ] + result = _svc(store, factory).list_tokens(alias=ALIAS) + assert "token" not in result["tokens"][0] + assert "liveSecretValue" not in str(result) + # everything else survives untouched + assert result["tokens"][0]["description"] == "master" + + def test_empty_list(self, store, client_factory) -> None: + factory, mock = client_factory + mock.list_tokens.return_value = [] + result = _svc(store, factory).list_tokens(alias=ALIAS) + assert result["tokens"] == [] + assert result["count"] == 0 + + def test_unknown_alias_raises(self, store, client_factory) -> None: + factory, _ = client_factory + with pytest.raises(ConfigError): + _svc(store, factory).list_tokens(alias="nope") + + def test_client_closed_when_listing_raises(self, store, client_factory) -> None: + factory, mock = client_factory + mock.list_tokens.side_effect = RuntimeError("boom") + with pytest.raises(RuntimeError): + _svc(store, factory).list_tokens(alias=ALIAS) + mock.close.assert_called_once() From 1865e132c4cb3e7cc2644b8e7344fb1ba480a7c1 Mon Sep 17 00:00:00 2001 From: Petr Date: Wed, 19 Aug 2026 18:24:45 -0400 Subject: [PATCH 2/2] fix(http): correct 5xx hint ordering, bound the server-supplied exceptionId Two findings from Devin Review on #616. 1. A write that was first rate-limited got the wrong recovery advice. A POST can legitimately reach a second attempt via a 429 (which stays retryable on every method). If that attempt answered 500, the hint was picked by attempt count, so the operator was told "the same 5xx came back on all 2 attempts, likely an upstream incident, escalate" -- factually wrong (one 5xx was seen) and, worse, it replaced the "verify what already landed" warning on exactly the request that most needed it. Telling someone to escalate instead of check is how the duplicate this PR exists to prevent gets created. Fixed twice over: the method gate is now evaluated BEFORE the attempt count, and the count itself tallies only 5xx responses rather than total attempts, so the exhausted-retry message is accurate for idempotent methods too (a GET that saw 429, 500, 500 now reports 2, not 3). 2. The exceptionId bypassed the length/markup guard. `_raise_api_error` read `exceptionId` straight off an untrusted response body and appended it AFTER the MAX_API_ERROR_LENGTH truncation the codebase applies explicitly "to prevent Rich markup injection and excessive output". Human-mode errors render through Rich with markup enabled (OutputFormatter.error), so a bracket-laden or unbounded value from a misbehaving endpoint reached the terminal as markup, and embedded newlines could forge extra log lines (CWE-117). The code comment I wrote asserted the field was "kbagent-authored and short", which is true of the hint and false of the id -- the wrong premise is what hid the hole. `_safe_exception_id` now drops everything outside [A-Za-z0-9._:-] and caps at MAX_EXCEPTION_ID_LENGTH (128; real ids run ~70). Dropping rather than escaping keeps a real id intact and still leaves support a usable handle if a value is partially mangled. The comment now states the actual invariant: only self-bounded strings may be appended past the truncation. Five tests: the 429-then-500 POST hint, 5xx-count accuracy, the length cap, markup/newline/control-char stripping, and a non-string exceptionId. --- src/keboola_agent_cli/constants.py | 7 ++ src/keboola_agent_cli/http_base.py | 79 ++++++++++++++----- tests/test_http_base.py | 121 ++++++++++++++++++++++++++++- 3 files changed, 186 insertions(+), 21 deletions(-) diff --git a/src/keboola_agent_cli/constants.py b/src/keboola_agent_cli/constants.py index 63fe7052..36ecbbcf 100644 --- a/src/keboola_agent_cli/constants.py +++ b/src/keboola_agent_cli/constants.py @@ -63,6 +63,13 @@ def _resolve_app_name() -> str: # request, so repeating it is safe regardless of method. RETRY_SAFE_METHODS: frozenset[str] = frozenset({"GET", "HEAD", "OPTIONS", "PUT", "DELETE"}) +# Cap on the server-supplied `exceptionId` echoed into a 5xx error message. +# The value is untrusted input rendered into a Rich-markup console, so it gets +# the same "bounded before it reaches a terminal" treatment as the API's own +# error text (MAX_API_ERROR_LENGTH). Real Keboola ids run ~70 chars +# ("com-keboola-gcp-europe-west3-connection-<32 hex>"), so this is generous. +MAX_EXCEPTION_ID_LENGTH: int = 128 + # --- HTTP Timeout --- DEFAULT_TIMEOUT: httpx.Timeout = httpx.Timeout(connect=5.0, read=30.0, write=10.0, pool=5.0) diff --git a/src/keboola_agent_cli/http_base.py b/src/keboola_agent_cli/http_base.py index b6661d95..e49d3aa0 100644 --- a/src/keboola_agent_cli/http_base.py +++ b/src/keboola_agent_cli/http_base.py @@ -9,6 +9,7 @@ import logging import os import platform +import re import time from typing import Any, Self from urllib.parse import urlparse, urlunparse @@ -20,6 +21,7 @@ BACKOFF_BASE, ENV_CONVERSATION_ID, MAX_API_ERROR_LENGTH, + MAX_EXCEPTION_ID_LENGTH, MAX_RETRIES, MAX_RETRY_AFTER_SECONDS, RETRY_SAFE_METHODS, @@ -29,6 +31,12 @@ logger = logging.getLogger(__name__) +# Everything a real Keboola `exceptionId` is made of. Anything else in that +# server-supplied field -- Rich markup brackets, newlines that would forge an +# extra log line (CWE-117), control characters -- is dropped before the id is +# interpolated into an error message. +_EXCEPTION_ID_DISALLOWED = re.compile(r"[^A-Za-z0-9._:-]+") + def build_user_agent() -> str: """Build the User-Agent that signs every Keboola API call. @@ -177,6 +185,10 @@ def _do_request( url_label = base_url or self._base_url last_response: httpx.Response | None = None retry_safe = method.upper() in RETRY_SAFE_METHODS + # Counted separately from `attempt`: a 429 burns an attempt without + # being a server error, so using the attempt index would report "the + # same 5xx came back on N attempts" after seeing exactly one. + server_error_attempts = 0 for attempt in range(MAX_RETRIES): try: @@ -185,6 +197,8 @@ def _do_request( if response.status_code < 400: return response + if response.status_code >= 500: + server_error_attempts += 1 # 429 is repeatable on any method; a 5xx only on an idempotent one. may_repeat = retry_safe or response.status_code == 429 if ( @@ -216,7 +230,7 @@ def _do_request( last_response = response continue - hint = self._server_error_hint(method, response.status_code, attempt + 1) + hint = self._server_error_hint(method, response.status_code, server_error_attempts) self._raise_api_error( response, url_label, @@ -279,7 +293,9 @@ def _do_request( self._raise_api_error( last_response, url_label, - hint=self._server_error_hint(method, last_response.status_code, MAX_RETRIES), + hint=self._server_error_hint( + method, last_response.status_code, server_error_attempts + ), ) raise KeboolaApiError( @@ -289,6 +305,22 @@ def _do_request( retryable=True, ) + @staticmethod + def _safe_exception_id(raw: object) -> str: + """Bound and de-fang the server-supplied ``exceptionId``. + + The id is untrusted input that ends up in a message rendered through + Rich with markup enabled (``OutputFormatter.error``), so it gets the + same "bounded before it reaches a terminal" treatment the API's own + error text gets from MAX_API_ERROR_LENGTH. Disallowed characters are + dropped rather than escaped: a real Keboola id contains none of them, + so this is lossless in practice, and support still gets a handle out + of a partially mangled value instead of nothing. + """ + if not isinstance(raw, str): + return "" + return _EXCEPTION_ID_DISALLOWED.sub("", raw)[:MAX_EXCEPTION_ID_LENGTH] + @staticmethod def _non_idempotent_note(method: str) -> str: """One sentence naming the partial-effect risk of an unrepeated write.""" @@ -299,26 +331,32 @@ def _non_idempotent_note(method: str) -> str: ) @classmethod - def _server_error_hint(cls, method: str, status: int, attempts: int) -> str | None: + def _server_error_hint(cls, method: str, status: int, server_error_attempts: int) -> str | None: """Return the actionable next step for a 5xx, or None if there isn't one. - Two situations need two different answers (issue #599). A 5xx that - survived every retry is an upstream incident nobody on this side can - fix, and the operator's next step is to escalate with the exceptionId. - A 5xx on the single attempt of a non-idempotent write is the opposite: - the server may already have done the work, so the next step is to check - before repeating. A generic "API error 500" said neither. + Two situations need two different answers (issue #599). A 5xx on a + non-idempotent write means the server may already have done the work, + so the next step is to check before repeating. A 5xx that survived + every retry is the opposite: an upstream incident nobody on this side + can fix, and the next step is to escalate with the exceptionId. A + generic "API error 500" said neither. + + The method gate is checked FIRST and `server_error_attempts` counts + only 5xx responses, because a POST can reach a second attempt via a + 429. Ordering it the other way round told the operator to escalate on + exactly the request where they most needed to go and check what had + already landed. """ if status < 500: return None - if attempts > 1: + if method.upper() not in RETRY_SAFE_METHODS: + return cls._non_idempotent_note(method) + if server_error_attempts > 1: return ( - f"The same 5xx came back on all {attempts} attempts, which points at an " - "upstream Keboola incident rather than a caller mistake -- check " + f"The same 5xx came back on all {server_error_attempts} attempts, which points " + "at an upstream Keboola incident rather than a caller mistake -- check " "status.keboola.com and contact Keboola support, quoting the exceptionId above." ) - if method.upper() not in RETRY_SAFE_METHODS: - return cls._non_idempotent_note(method) return None def _raise_api_error( @@ -359,9 +397,7 @@ def _raise_api_error( # can trace the incident by. Dropping it, as this parser used to, # left the operator with nothing to escalate (issue #599). if isinstance(body, dict): - raw_exception_id = body.get("exceptionId") - if isinstance(raw_exception_id, str) and raw_exception_id: - exception_id = raw_exception_id + exception_id = self._safe_exception_id(body.get("exceptionId")) # Real Keboola APIs answer with one of these keys in priority # order. Two caveats: # 1. Keboola Metastore puts the HTTP status code into `error` @@ -417,9 +453,12 @@ def _raise_api_error( retryable=False, ) - # Truncation above guards the API's own text; the id and the hint are - # kbagent-authored and short, so they are appended after it and always - # survive into the message the operator actually reads. + # Appended AFTER the truncation above so they always survive into the + # message the operator actually reads. That is safe only because each + # is bounded on its own: the hint is a kbagent-authored constant, and + # the id went through `_safe_exception_id`. Never append raw + # server-supplied text here -- the truncation is what keeps the + # console (Rich, markup enabled) from rendering it as markup. suffix = f" [exceptionId: {exception_id}]" if exception_id else "" if hint: suffix += f" {hint}" diff --git a/tests/test_http_base.py b/tests/test_http_base.py index 9f60d4fb..7cf97bff 100644 --- a/tests/test_http_base.py +++ b/tests/test_http_base.py @@ -7,7 +7,12 @@ import httpx import pytest -from keboola_agent_cli.constants import APP_NAME, MAX_API_ERROR_LENGTH, MAX_RETRIES +from keboola_agent_cli.constants import ( + APP_NAME, + MAX_API_ERROR_LENGTH, + MAX_EXCEPTION_ID_LENGTH, + MAX_RETRIES, +) from keboola_agent_cli.errors import KeboolaApiError from keboola_agent_cli.http_base import BaseHttpClient, build_user_agent @@ -439,6 +444,120 @@ def test_exception_id_surfaced_for_support(self, httpx_mock) -> None: finally: client.close() + def test_rate_limited_then_500_on_a_post_warns_about_partial_effect(self, httpx_mock) -> None: + """A POST can reach attempt 2 via a 429 -- the 5xx there is still its first. + + The hint must be the partial-effect warning, not "upstream incident": + telling an operator to escalate instead of checking what already + landed is exactly how the duplicate this PR prevents gets created. + """ + httpx_mock.add_response(url=f"{STACK_URL}/test-path", status_code=429, text="slow down") + httpx_mock.add_response( + url=f"{STACK_URL}/test-path", status_code=500, json={"error": "Application error."} + ) + + client = self._client() + import keboola_agent_cli.http_base as http_base_module + + original_sleep = http_base_module.time.sleep + http_base_module.time.sleep = _noop_sleep # ty: ignore[invalid-assignment] + try: + with pytest.raises(KeboolaApiError) as exc_info: + client._do_request("POST", "/test-path") + message = exc_info.value.message + assert "may already have taken effect" in message + assert "upstream Keboola incident" not in message + assert exc_info.value.retryable is False + finally: + http_base_module.time.sleep = original_sleep + client.close() + + def test_exhausted_hint_counts_server_errors_not_total_attempts(self, httpx_mock) -> None: + """A 429 burned an attempt but was not a 5xx -- do not count it as one.""" + httpx_mock.add_response(url=f"{STACK_URL}/test-path", status_code=429, text="slow down") + for _ in range(MAX_RETRIES - 1): + httpx_mock.add_response( + url=f"{STACK_URL}/test-path", status_code=500, json={"error": "boom"} + ) + + client = self._client() + import keboola_agent_cli.http_base as http_base_module + + original_sleep = http_base_module.time.sleep + http_base_module.time.sleep = _noop_sleep # ty: ignore[invalid-assignment] + try: + with pytest.raises(KeboolaApiError) as exc_info: + client._do_request("GET", "/test-path") + message = exc_info.value.message + assert "upstream Keboola incident" in message + assert f"{MAX_RETRIES - 1} attempts" in message + assert f"{MAX_RETRIES} attempts" not in message + finally: + http_base_module.time.sleep = original_sleep + client.close() + + def test_exception_id_is_length_capped(self, httpx_mock) -> None: + """An untrusted id must not reach the terminal unbounded.""" + httpx_mock.add_response( + url=f"{STACK_URL}/test-path", + status_code=500, + json={"error": "Application error.", "exceptionId": "a" * 5000}, + ) + + client = self._client() + try: + with pytest.raises(KeboolaApiError) as exc_info: + client._do_request("POST", "/test-path") + assert "a" * MAX_EXCEPTION_ID_LENGTH in exc_info.value.message + assert "a" * (MAX_EXCEPTION_ID_LENGTH + 1) not in exc_info.value.message + finally: + client.close() + + def test_exception_id_markup_and_newlines_stripped(self, httpx_mock) -> None: + """Human-mode errors render through Rich with markup ON (output.py). + + A server-supplied id carrying brackets would be interpreted as markup, + and newlines would let it forge extra log lines (CWE-117). Neither may + survive into the message. + """ + httpx_mock.add_response( + url=f"{STACK_URL}/test-path", + status_code=500, + json={ + "error": "Application error.", + "exceptionId": "kbc-1[bold red]spoof[/bold red]\nError: fake\x07", + }, + ) + + client = self._client() + try: + with pytest.raises(KeboolaApiError) as exc_info: + client._do_request("POST", "/test-path") + message = exc_info.value.message + assert "[bold red]" not in message + assert "\n" not in message + assert "\x07" not in message + # the legitimate characters survive so support still gets a handle + assert "kbc-1" in message + finally: + client.close() + + def test_non_string_exception_id_ignored(self, httpx_mock) -> None: + """A numeric/object `exceptionId` is not an id -- drop it silently.""" + httpx_mock.add_response( + url=f"{STACK_URL}/test-path", + status_code=500, + json={"error": "Application error.", "exceptionId": {"nested": 1}}, + ) + + client = self._client() + try: + with pytest.raises(KeboolaApiError) as exc_info: + client._do_request("POST", "/test-path") + assert "exceptionId" not in exc_info.value.message + finally: + client.close() + def test_no_hint_appended_to_client_errors(self, httpx_mock) -> None: """A 4xx is the caller's problem -- the incident hint would be misleading.""" httpx_mock.add_response(