diff --git a/crates/api/src/lib.rs b/crates/api/src/lib.rs index 3524327..4a9473f 100644 --- a/crates/api/src/lib.rs +++ b/crates/api/src/lib.rs @@ -107,16 +107,16 @@ pub fn build_router(state: AppState) -> Router { .get(routes::apikeys::get_key) .delete(routes::apikeys::delete_key), ) - // Custodial signing tombstones (410 Gone since the non-custodial cutover). + // Custodial signing tombstone (410 Gone since the non-custodial cutover). .route( "/v1/wallets/:id/withdraw", post(routes::withdrawals::withdraw), ) + // Non-custodial path: clients sign locally and relay through these. .route( "/v1/wallets/:id/trustlines", post(routes::trustlines::add_trustline), ) - // Non-custodial path: clients sign locally and relay through these. .route( "/v1/wallets/:id/submit-signed", post(routes::submit::submit_signed), diff --git a/crates/api/src/routes/trustlines.rs b/crates/api/src/routes/trustlines.rs index 1c22d71..b4c4786 100644 --- a/crates/api/src/routes/trustlines.rs +++ b/crates/api/src/routes/trustlines.rs @@ -1,19 +1,109 @@ -//! Tombstone for the custodial trustline endpoint. +//! Non-custodial trustline endpoint: returns what a client needs to build and sign a ChangeTrust. //! -//! Removed in the non-custodial cutover: the server no longer holds user wallet keys, so it -//! cannot sign a ChangeTrust on the user's behalf. Clients build + sign the ChangeTrust locally -//! (dashboard/SDK) and relay through `POST /v1/wallets/:id/submit-signed`. +//! The server never signs for a user wallet. The client builds the ChangeTrust locally with the +//! data returned here, signs it with its own key, and relays it via +//! `POST /v1/wallets/:id/submit-signed` (whose op allowlist already admits change-trust). -use crate::error::{ApiError, ApiResult}; -use axum::extract::Path; -use axum::response::Response; +use crate::auth::authorize_wallet; +use crate::error::{ApiError, ApiResult, Envelope}; +use crate::json::parse_optional; +use crate::state::AppState; +use axum::body::Bytes; +use axum::extract::{Path, State}; +use axum::http::HeaderMap; +use axum::Json; +use octo_wallet_core::{validate_change_trust, WalletError}; +use serde::{Deserialize, Serialize}; use uuid::Uuid; -/// `POST /v1/wallets/:id/trustlines` — 410 Gone. -pub async fn add_trustline(Path(_wallet_id): Path) -> ApiResult { - Err(ApiError::Gone( - "custodial trustlines were removed: sign the ChangeTrust client-side and POST it to \ - /v1/wallets/:id/submit-signed" +/// Stellar's minimum base fee per operation, in stroops (matches `signing_info`). +const BASE_FEE_STROOPS: i64 = 100; + +#[derive(Debug, Default, Deserialize)] +pub struct TrustlineRequest { + /// Asset code to trust (1–12 bytes, e.g. `"USDC"`). + pub asset_code: Option, + /// Issuer of the asset (`G...`). + pub asset_issuer: Option, + /// Trust limit in stroops. Omitted => unlimited; `0` removes the trustline. + pub limit_stroops: Option, +} + +#[derive(Debug, Serialize)] +pub struct TrustlineSigningInfo { + /// The wallet's account — the ChangeTrust source. + pub account: String, + /// Current sequence, as a string (see `SigningInfo::sequence` for why). + #[serde(with = "crate::json::i64_as_string")] + pub sequence: i64, + pub network_passphrase: String, + pub base_fee_stroops: i64, + pub asset_code: String, + pub asset_issuer: String, + /// Limit to put on the operation, as a string (`i64::MAX` = unlimited exceeds JS safe ints). + #[serde(with = "crate::json::i64_as_string")] + pub limit_stroops: i64, + /// Where to send the signed envelope. + pub submit_url: String, +} + +/// `POST /v1/wallets/:id/trustlines` — validate a ChangeTrust request and return signing info. +pub async fn add_trustline( + State(state): State, + Path(wallet_id): Path, + headers: HeaderMap, + body: Bytes, +) -> ApiResult>> { + authorize_wallet(&headers, &state, wallet_id).await?; + let req: TrustlineRequest = parse_optional(&body)?; + let asset_code = req + .asset_code + .ok_or_else(|| ApiError::BadRequest("asset_code is required".into()))?; + let asset_issuer = req + .asset_issuer + .ok_or_else(|| ApiError::BadRequest("asset_issuer is required".into()))?; + + // Same validator the ChangeTrust signer uses, so both paths agree on what's acceptable. + validate_change_trust(&asset_code, &asset_issuer, req.limit_stroops).map_err(|e| { + ApiError::BadRequest( + match e { + WalletError::InvalidAssetCode => "asset_code must be 1-12 bytes", + WalletError::InvalidAddress => "asset_issuer must be a valid G... account", + _ => "limit_stroops must be >= 0", + } .into(), - )) + ) + })?; + + let wallet = state.store().get_wallet(wallet_id).await?; + // A trustline to one's own asset is malformed on-chain; fail fast instead of burning a fee. + if asset_issuer == wallet.stellar_account_g { + return Err(ApiError::BadRequest( + "a wallet cannot add a trustline to an asset it issues".into(), + )); + } + + let sequence = state + .horizon() + .account_sequence(&wallet.stellar_account_g) + .await + .map_err(|e| match e { + ApiError::NotFound => ApiError::BadRequest( + "This wallet is not funded on-chain yet. Fund it with XLM (testnet friendbot) first." + .into(), + ), + other => other, + })?; + + Ok(Envelope::ok(TrustlineSigningInfo { + account: wallet.stellar_account_g, + sequence, + network_passphrase: state.network().passphrase().to_string(), + base_fee_stroops: BASE_FEE_STROOPS, + asset_code, + asset_issuer, + // Stellar treats a missing limit as 0 (= remove), so "unlimited" must be explicit. + limit_stroops: req.limit_stroops.unwrap_or(i64::MAX), + submit_url: format!("/v1/wallets/{wallet_id}/submit-signed"), + })) } diff --git a/crates/api/tests/api_tests.rs b/crates/api/tests/api_tests.rs index 9a4eab3..df64975 100644 --- a/crates/api/tests/api_tests.rs +++ b/crates/api/tests/api_tests.rs @@ -649,32 +649,132 @@ async fn submit_signed_requires_transaction_xdr() { assert_eq!(resp.status(), StatusCode::BAD_REQUEST); } +const TRUSTLINE_ISSUER: &str = "GBBD47IF6LWK7P7MDEVSCWR7DPUWV3NY3DTQEVFL4NAT4AQH3ZLLFLA5"; + +/// Local mock Horizon serving `GET /accounts/:id` with a fixed sequence, so the trustline success +/// path runs on every build instead of needing a funded testnet account. +async fn start_mock_horizon_accounts() -> String { + async fn account() -> axum::Json { + axum::Json(serde_json::json!({ + "sequence": "15942562120466433", + "balances": [], + "subentry_count": 0, + "num_sponsoring": 0, + "num_sponsored": 0 + })) + } + let app = Router::new().route("/accounts/:id", axum::routing::get(account)); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + tokio::spawn(async move { axum::serve(listener, app).await.unwrap() }); + format!("http://{addr}") +} + #[tokio::test] -async fn custodial_trustline_is_gone() { - let Some(state) = test_state().await else { - return; - }; +async fn add_trustline_returns_signing_info_for_a_valid_asset() { + let Some(url) = database_url() else { return }; + let store = Store::connect(&url).await.expect("connect"); + store.migrate().await.expect("migrate"); + let state = AppState::new( + store, + [42u8; 32], + StellarNetwork::Testnet, + start_mock_horizon_accounts().await, + None, + octo_email::EmailSender::new_captured(), + ); let app = build_router(state.clone()); let token = auth_token(&app, &state).await; - let resp = app - .clone() - .oneshot(create_wallet_req(&app, &token).await) - .await - .unwrap(); - let wallet_id = body_json(resp).await["data"]["id"] - .as_str() - .unwrap() - .to_string(); + let wallet_id = create_wallet_for(&app, &token).await; + let body = format!(r#"{{"asset_code":"USDC","asset_issuer":"{TRUSTLINE_ISSUER}"}}"#); let resp = app .oneshot(post_json_auth( &format!("/v1/wallets/{wallet_id}/trustlines"), - r#"{"asset_code":"USDC","asset_issuer":"GBBD47IF6LWK7P7MDEVSCWR7DPUWV3NY3DTQEVFL4NAT4AQH3ZLLFLA5"}"#, + &body, &token, )) .await .unwrap(); - assert_eq!(resp.status(), StatusCode::GONE); + assert_eq!(resp.status(), StatusCode::OK); + let data = &body_json(resp).await["data"]; + assert_eq!(data["sequence"], "15942562120466433"); + assert_eq!(data["asset_code"], "USDC"); + assert_eq!(data["asset_issuer"], TRUSTLINE_ISSUER); + assert_eq!(data["base_fee_stroops"], 100); + assert_eq!( + data["limit_stroops"], + i64::MAX.to_string(), + "omitted limit = unlimited" + ); + assert_eq!( + data["network_passphrase"], + StellarNetwork::Testnet.passphrase() + ); + assert!(data["account"].as_str().unwrap().starts_with('G')); + assert_eq!( + data["submit_url"], + format!("/v1/wallets/{wallet_id}/submit-signed") + ); +} + +#[tokio::test] +async fn add_trustline_rejects_an_invalid_asset_code() { + let Some(state) = test_state().await else { + return; + }; + let app = build_router(state.clone()); + let token = auth_token(&app, &state).await; + let wallet_id = create_wallet_for(&app, &token).await; + let uri = format!("/v1/wallets/{wallet_id}/trustlines"); + + // Empty code, 13-byte code, bad issuer, negative limit: all rejected before Horizon is hit. + for body in [ + format!(r#"{{"asset_code":"","asset_issuer":"{TRUSTLINE_ISSUER}"}}"#), + format!(r#"{{"asset_code":"ABCDEFGHIJKLM","asset_issuer":"{TRUSTLINE_ISSUER}"}}"#), + r#"{"asset_code":"USDC","asset_issuer":"not-a-strkey"}"#.to_string(), + format!( + r#"{{"asset_code":"USDC","asset_issuer":"{TRUSTLINE_ISSUER}","limit_stroops":-1}}"# + ), + format!(r#"{{"asset_issuer":"{TRUSTLINE_ISSUER}"}}"#), + ] { + let resp = app + .clone() + .oneshot(post_json_auth(&uri, &body, &token)) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::BAD_REQUEST, "body: {body}"); + } +} + +#[tokio::test] +async fn add_trustline_requires_wallet_authorization() { + let Some(state) = test_state().await else { + return; + }; + let app = build_router(state.clone()); + let token = auth_token(&app, &state).await; + let wallet_id = create_wallet_for(&app, &token).await; + let uri = format!("/v1/wallets/{wallet_id}/trustlines"); + let body = format!(r#"{{"asset_code":"USDC","asset_issuer":"{TRUSTLINE_ISSUER}"}}"#); + + // No credentials → 401. + let unauth = Request::builder() + .method("POST") + .uri(&uri) + .header("content-type", "application/json") + .body(Body::from(body.clone())) + .unwrap(); + let resp = app.clone().oneshot(unauth).await.unwrap(); + assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); + + // Another user must not learn whether this wallet exists → 404. + let other = auth_token(&app, &state).await; + let resp = app + .oneshot(post_json_auth(&uri, &body, &other)) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::NOT_FOUND); } /// Regression coverage for the withdrawal route's use of the shared diff --git a/crates/wallet-core/src/signer.rs b/crates/wallet-core/src/signer.rs index cab39df..642e30e 100644 --- a/crates/wallet-core/src/signer.rs +++ b/crates/wallet-core/src/signer.rs @@ -18,9 +18,9 @@ use stellar_base::network::Network; // sign_fee_bump (production, not test-gated) rejects sub-minimum fees against this constant. use stellar_base::transaction::MIN_BASE_FEE; -// Used only by the feature-gated custodial signing fixtures below. -#[cfg(any(test, feature = "test-fixtures"))] +// validate_change_trust (production) checks the issuer strkey. use crate::address::is_valid_account; +// Used only by the feature-gated custodial signing fixtures below. #[cfg(any(test, feature = "test-fixtures"))] use crate::asset::is_valid_asset_code; #[cfg(any(test, feature = "test-fixtures"))] @@ -236,6 +236,27 @@ pub fn sign_payment( }) } +/// Validate ChangeTrust parameters: asset code, `G...` issuer, and a non-negative limit. +/// +/// Shared by server-side validation of client-built trustlines and the test-fixture signer, so +/// both paths accept exactly the same inputs. +pub fn validate_change_trust( + asset_code: &str, + asset_issuer: &str, + limit_stroops: Option, +) -> Result<(), WalletError> { + if !crate::asset::is_valid_asset_code(asset_code) { + return Err(WalletError::InvalidAssetCode); + } + if !is_valid_account(asset_issuer) { + return Err(WalletError::InvalidAddress); + } + if limit_stroops.is_some_and(|l| l < 0) { + return Err(WalletError::InvalidAmount); + } + Ok(()) +} + /// A trustline (ChangeTrust) to build and sign from the master account. /// /// **Test fixture only** since the non-custodial cutover (see [`PaymentRequest`]). @@ -268,14 +289,7 @@ pub fn sign_change_trust( account_index: u32, req: &ChangeTrustRequest<'_>, ) -> Result { - if let Some(limit) = req.limit_stroops { - if limit < 0 { - return Err(WalletError::InvalidAmount); - } - } - if !is_valid_account(req.asset_issuer) { - return Err(WalletError::InvalidAddress); - } + validate_change_trust(req.asset_code, req.asset_issuer, req.limit_stroops)?; let keypair = keypair_from_sealed(master_key, sealed, network, account_index)?; let source = keypair.public_key(); diff --git a/docs/api.md b/docs/api.md index 68ab4d7..e3bc299 100644 --- a/docs/api.md +++ b/docs/api.md @@ -35,8 +35,12 @@ server stores only the public account and an opaque, client-encrypted backup blo decrypt. Consequently: - There is **no endpoint that signs a payment for you.** You build and sign locally, then relay. -- `POST /v1/wallets/:id/withdraw` and `POST /v1/wallets/:id/trustlines` are **`410 Gone` - tombstones**. They exist only to give integrators a clear error pointing at `submit-signed`. +- `POST /v1/wallets/:id/withdraw` is a **`410 Gone` tombstone** pointing integrators at + `submit-signed`. +- `POST /v1/wallets/:id/trustlines` takes `{asset_code, asset_issuer, limit_stroops?}`, validates + them, and returns ChangeTrust signing info (`account`, `sequence`, `network_passphrase`, + `base_fee_stroops`, `limit_stroops`, `submit_url`). The server never signs it — the client builds + and signs the ChangeTrust locally and relays it via `submit-signed`. ## Wallets diff --git a/docs/architecture.md b/docs/architecture.md index 65cd70a..5cc291d 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -51,7 +51,9 @@ The client fetches `GET /signing-info` (sequence, network passphrase, base fee), envelope and submits it to Horizon **unmodified** → record + webhook on confirmation. Horizon's result codes are passed back so the client can correct and re-sign. -The custodial `POST /withdraw` and `POST /trustlines` endpoints are `410 Gone` tombstones. +The custodial `POST /withdraw` endpoint is a `410 Gone` tombstone. `POST /trustlines` validates the +asset and returns ChangeTrust signing info (sequence, passphrase, fee, limit); the client signs +locally and relays via `submit-signed`. ## Signing safety diff --git a/docs/non-custodial-flow.md b/docs/non-custodial-flow.md index a186207..14de2e5 100644 --- a/docs/non-custodial-flow.md +++ b/docs/non-custodial-flow.md @@ -13,7 +13,8 @@ customer balances. - Client key handling / signing: `src/lib/sdk/` (frontend) - Submit + validation: `crates/api/src/routes/submit.rs`, `crates/api/src/submit_validation.rs` - Wallet creation / backup / gas tank: `crates/api/src/routes/wallets.rs` -- Custodial endpoints (`/withdraw`, `/trustlines`) now return **410 Gone**. +- Trustlines: `crates/api/src/routes/trustlines.rs` (signing info only; client signs) +- The custodial `/withdraw` endpoint now returns **410 Gone**. --- @@ -26,7 +27,8 @@ customer balances. | Who signs a withdrawal | Octo's server | The user, locally | | Withdraw request body | `destination` + `amount` (server signs) | A fully **signed transaction (XDR)** (server relays) | | Full server breach | Can drain every wallet | Cannot move funds — no keys to steal | -| `POST /withdraw`, `POST /trustlines` | Server-signs | **410 Gone** → `POST /submit-signed` | +| `POST /withdraw` | Server-signs | **410 Gone** → `POST /submit-signed` | +| `POST /trustlines` | Server-signs | Returns signing info → client signs → `POST /submit-signed` | | Gas sponsorship | Fee-bump from the wallet's own seed | Fee-bump from a separate gas tank (fee float only) | | Lost password **and** phrase | Octo could reset | Unrecoverable (the non-custodial trade-off) | | Deposits / addresses / balances | — | Unchanged (never needed a key) |