Harden error reporting, retry bounds, and credential handling

Second hardening pass, over the code added in 23c660d. Error reporting: - Decode failures now carry an excerpt of the body that arrived. The serde message alone ("expected value at line 1 column 1") gives no hint that the response was an HTML error page from a proxy or a login redirect from a wrong base URL. The excerpt is collapsed to one line and truncated so a large body cannot become a large error. - API errors with an empty or message-less body report the status reason ("Not Found") rather than rendering blank. - `Error` is `#[non_exhaustive]`, so future variants are not breaking. Retry bounds: - `RetryPolicy::max_elapsed` (120s default) bounds the time one call may spend across attempts. The request timeout is per attempt, so the previous defaults could spend four minutes on a call that kept timing out — a surprise for anyone reading `with_timeout(60)` as a bound on the call. - `backoff` uses saturating arithmetic; a large `max_delay` could overflow the jitter span and panic in a debug build. - Response bodies are drained before a retry so the connection returns to the pool instead of the next attempt opening a fresh one. Credentials: - Test pinning that neither the token nor the password appears in `Debug` output. `reqwest`'s `Client` prints its default headers, so the sensitive flag set when building them is the only thing keeping the token out of logs, and nothing enforced that. Also drops `Option::is_none_or`, which raised the minimum toolchain to 1.82 for no benefit over a match. 91 tests, up from 80. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-10-23 18:33:43 -04:00
parent 23c660d2f9
commit 8ff947455e
8 changed files with 412 additions and 21 deletions
@@ -988,3 +988,191 @@ async fn generic_escape_hatches_still_work() {
        .expect("statics_as should bypass the typed model");
    assert!(raw["sizes"].is_array());
 }
+
+// --- secret handling ----------------------------------------------------
+
+/// `reqwest`'s `Client` Debug prints its default headers, so the only thing
+/// keeping the API token out of every `{:?}` log is the sensitive flag set
+/// when the header is built. Lock that in.
+#[test]
+fn debug_output_never_contains_the_token_or_password() {
+    let client = InvoiceNinjaClient::new("https://example.com/api/v1", "SUPER_SECRET_TOKEN")
+        .unwrap()
+        .with_api_password("SUPER_SECRET_PASSWORD")
+        .unwrap();
+
+    let rendered = format!("{client:?}");
+    assert!(
+        !rendered.contains("SUPER_SECRET_TOKEN"),
+        "api token leaked into Debug output: {rendered}"
+    );
+    assert!(
+        !rendered.contains("SUPER_SECRET_PASSWORD"),
+        "api password leaked into Debug output: {rendered}"
+    );
+    assert!(
+        rendered.contains("Sensitive"),
+        "expected the headers to render as redacted, got: {rendered}"
+    );
+}
+
+// --- error quality ------------------------------------------------------
+
+/// A misconfigured base URL or an intercepting proxy returns HTML with a 200.
+/// The bare serde message ("expected value at line 1 column 1") does not hint
+/// at that, so the body excerpt is what makes it diagnosable.
+#[tokio::test]
+async fn decode_failures_quote_the_body_that_arrived() {
+    let server = MockServer::start().await;
+    Mock::given(method("GET"))
+        .and(path("/clients"))
+        .respond_with(
+            ResponseTemplate::new(200).set_body_string("<html><body>Login required</body></html>"),
+        )
+        .mount(&server)
+        .await;
+
+    let err = client(&server)
+        .list_clients(&ListParams::new())
+        .await
+        .expect_err("HTML is not a decodable list response");
+
+    let rendered = err.to_string();
+    assert!(
+        matches!(err, Error::Json { .. }),
+        "expected a decode error, got {err:?}"
+    );
+    assert!(
+        rendered.contains("Login required"),
+        "the excerpt should name what actually arrived, got: {rendered}"
+    );
+}
+
+#[tokio::test]
+async fn decode_excerpts_are_truncated_and_single_line() {
+    let server = MockServer::start().await;
+    let huge = format!("<html>\n{}\n</html>", "x".repeat(50_000));
+    Mock::given(method("GET"))
+        .and(path("/clients"))
+        .respond_with(ResponseTemplate::new(200).set_body_string(huge))
+        .mount(&server)
+        .await;
+
+    let err = client(&server)
+        .list_clients(&ListParams::new())
+        .await
+        .expect_err("should fail to decode");
+
+    let rendered = err.to_string();
+    assert!(
+        rendered.len() < 400,
+        "a huge body must not become a huge error, got {} chars",
+        rendered.len()
+    );
+    assert!(
+        !rendered.contains('\n'),
+        "the excerpt should be collapsed to one line"
+    );
+    assert!(rendered.contains('…'), "truncation should be marked");
+}
+
+#[tokio::test]
+async fn empty_error_bodies_fall_back_to_the_status_reason() {
+    let server = MockServer::start().await;
+    Mock::given(method("DELETE"))
+        .and(path("/clients/abc"))
+        .respond_with(ResponseTemplate::new(404))
+        .mount(&server)
+        .await;
+
+    let err = client(&server)
+        .with_retry_policy(RetryPolicy::none())
+        .delete_client("abc")
+        .await
+        .expect_err("404 is an error");
+
+    match err {
+        Error::Api { message, .. } => assert_eq!(
+            message, "Not Found",
+            "an empty body should borrow the status reason rather than render blank"
+        ),
+        other => panic!("expected Error::Api, got {other:?}"),
+    }
+}
+
+#[tokio::test]
+async fn a_json_error_body_with_no_message_still_says_something() {
+    let server = MockServer::start().await;
+    Mock::given(method("GET"))
+        .and(path("/clients"))
+        .respond_with(ResponseTemplate::new(403).set_body_json(json!({})))
+        .mount(&server)
+        .await;
+
+    let err = client(&server)
+        .with_retry_policy(RetryPolicy::none())
+        .list_clients(&ListParams::new())
+        .await
+        .expect_err("403 is an error");
+    assert!(err.to_string().contains("Forbidden"), "got {err}");
+}
+
+// --- retry budget -------------------------------------------------------
+
+/// The request timeout is per attempt, so without a total budget four
+/// attempts at 60s each would take four minutes. The budget caps the call.
+#[tokio::test]
+async fn the_total_budget_stops_retrying_even_with_retries_left() {
+    let server = MockServer::start().await;
+    Mock::given(method("GET"))
+        .and(path("/clients"))
+        .respond_with(ResponseTemplate::new(503).set_delay(Duration::from_millis(80)))
+        .mount(&server)
+        .await;
+
+    let policy = RetryPolicy::new(50)
+        .base_delay(Duration::from_millis(1))
+        .max_delay(Duration::from_millis(5))
+        .max_elapsed(Some(Duration::from_millis(250)));
+
+    let started = std::time::Instant::now();
+    let err = client(&server)
+        .with_retry_policy(policy)
+        .list_clients(&ListParams::new())
+        .await
+        .expect_err("should give up once the budget is spent");
+
+    assert!(matches!(err, Error::Api { status, .. } if status == 503));
+    assert!(
+        started.elapsed() < Duration::from_secs(3),
+        "50 retries were allowed but the budget should have ended it, took {:?}",
+        started.elapsed()
+    );
+    let attempts = server.received_requests().await.unwrap().len();
+    assert!(
+        (2..=6).contains(&attempts),
+        "expected the budget to allow a few attempts, got {attempts}"
+    );
+}
+
+#[tokio::test]
+async fn no_budget_means_only_max_retries_bounds_the_call() {
+    let server = MockServer::start().await;
+    Mock::given(method("GET"))
+        .and(path("/clients"))
+        .respond_with(ResponseTemplate::new(503))
+        .mount(&server)
+        .await;
+
+    let policy = RetryPolicy::new(4)
+        .base_delay(Duration::from_millis(1))
+        .max_delay(Duration::from_millis(2))
+        .max_elapsed(None);
+
+    let _ = client(&server)
+        .with_retry_policy(policy)
+        .list_clients(&ListParams::new())
+        .await
+        .expect_err("still fails");
+    assert_eq!(server.received_requests().await.unwrap().len(), 5);
+}
@@ -1,9 +1,10 @@
 use invoiceninjars::models::{
    BankIntegration, BankTransaction, BankTransactionRule, BulkRequest, Client, ClientGatewayToken,
    Company, CompanyGateway, CompanyToken, Credit, Design, Document, Expense, ExpenseCategory,
-    GroupSetting, Invoice, ListParams, ListResponse, Location, PaymentTerm, Project, PurchaseOrder,
-    Quote, RecurringExpense, RecurringInvoice, RecurringQuote, Subscription, SystemLog, Tag, Task,
-    TaskScheduler, TaskStatus, TaxRate, User, Vendor, Webhook,
+    GroupSetting, HealthCheck, Invoice, ListParams, ListResponse, Location, PaymentTerm, Project,
+    PurchaseOrder, Quote, RecurringExpense, RecurringInvoice, RecurringQuote, SearchResults,
+    Statics, Subscription, SystemLog, Tag, Task, TaskScheduler, TaskStatus, TaxRate, User, Vendor,
+    Webhook,
 };
 use serde_json::json;

@@ -694,3 +695,70 @@ fn design_body_round_trips_through_extra() {
    let round_tripped = serde_json::to_value(&design).unwrap();
    assert_eq!(round_tripped["design"]["header"], "<h1>");
 }
+
+#[test]
+fn statics_round_trip_preserves_unmodeled_tables_and_fields() {
+    let payload = json!({
+        "currencies": [{ "id": 1, "name": "US Dollar", "code": "USD", "precision": 2 }],
+        "countries": [{ "id": 840, "name": "United States", "iso_3166_3": "USA" }],
+        "gateways": [{ "id": 1, "name": "Stripe" }],
+        "some_future_table": [{ "x": 1 }]
+    });
+
+    let statics: Statics = serde_json::from_value(payload).unwrap();
+    let back = serde_json::to_value(&statics).unwrap();
+
+    assert_eq!(back["countries"][0]["iso_3166_3"], "USA");
+    assert_eq!(back["some_future_table"][0]["x"], 1);
+    assert_eq!(back["gateways"][0]["name"], "Stripe");
+    assert_eq!(
+        back["currencies"][0]["id"], "1",
+        "ids are normalised to strings on the way back out"
+    );
+}
+
+#[test]
+fn search_results_round_trip_preserves_unknown_buckets() {
+    let payload = json!({
+        "clients": [{ "name": "Acme", "type": "/client", "id": "a1", "path": "/clients/a1" }],
+        "future_bucket": [{ "name": "x" }]
+    });
+
+    let results: SearchResults = serde_json::from_value(payload).unwrap();
+    assert_eq!(results.clients[0].r#type, "/client");
+
+    let back = serde_json::to_value(&results).unwrap();
+    assert_eq!(back["future_bucket"][0]["name"], "x");
+    assert_eq!(back["clients"][0]["type"], "/client");
+}
+
+#[test]
+fn health_check_round_trip_preserves_unknown_fields() {
+    let payload = json!({
+        "system_health": false,
+        "api_version": "5.8.56",
+        "queue_data": { "pending": 3 },
+        "brand_new_probe": true
+    });
+
+    let health: HealthCheck = serde_json::from_value(payload).unwrap();
+    assert_eq!(health.system_health, Some(false));
+
+    let back = serde_json::to_value(&health).unwrap();
+    assert_eq!(back["brand_new_probe"], true);
+    assert_eq!(back["queue_data"]["pending"], 3);
+}
+
+#[test]
+fn statics_ids_accept_both_strings_and_numbers() {
+    let payload = json!({
+        "payment_types": [{ "id": "1", "name": "Bank Transfer" }],
+        "industries": [{ "id": 3, "name": "Advertising" }],
+        "sizes": [{ "id": null, "name": "Unknown" }]
+    });
+
+    let statics: Statics = serde_json::from_value(payload).unwrap();
+    assert_eq!(statics.payment_types[0].id, "1");
+    assert_eq!(statics.industries[0].id, "3");
+    assert_eq!(statics.sizes[0].id, "", "a null id decodes as empty");
+}