diff --git a/STATUS.md b/STATUS.md index a5cadf27..be4df450 100644 --- a/STATUS.md +++ b/STATUS.md @@ -45,7 +45,7 @@ - 10 service templates shipped as OpenAPI 3.1: Eventbrite, GitHub, Gmail, Google Calendar, Google Drive, Google Tasks, Resend, Slack, Stripe, X (plus the `overslash` platform namespace) - `outlook` service — Microsoft Graph mail (Gmail-equivalent): profile, messages (search/read/send/move/delete), drafts (create/send), and mail folders, over `provider: microsoft` (already seeded). Structured `sendMail` JSON gives clean approval disclosure without base64 decoding - OAuth for external MCP-runtime services — `x-overslash-mcp.auth: {kind: oauth, provider: …}` resolves the caller's owner connection (D22) into the outbound MCP bearer via the standard `oauth_providers`/`connections` machinery (shipped for HubSpot). `template_oauth_provider` + `McpDetail.provider` extended so oauth MCP services auto-connect, validate pinned connections, and surface a dashboard connect affordance. -- `slack` service — Slack's Web API (`slack.com/api/*`) with a user token from the `slack` provider: `send_message`, `update_message`, `add_reaction`, `list_channels`, `read_channel_history`, `get_thread_replies`, `get_user`, `list_users`, `search_messages`. Writes disclose the channel by name through a `conversations.info` resolver. Replaces the D24 MCP wrapper, since Slack's MCP server only admits Marketplace-listed apps. Shared-app caveat: Slack caps `conversations.history`/`.replies` at 1 req/min and 15 messages for non-Marketplace apps, and an org's own BYOC Slack app is exempt. +- `slack` service — Slack's Web API (`slack.com/api/*`) with a user token from the `slack` provider: `send_message`, `update_message`, `add_reaction`, `list_channels`, `read_channel_history`, `get_thread_replies`, `get_user`, `list_users`, `search_messages`. Summaries and disclosures name the channel (`'#general'`) through a `conversations.info` resolver on every channel-taking action, and `get_user` names the person (`users.info`); a channel Slack won't name (a DM, a scope the token lacks) falls back to the raw ID. HTTP resolver misses are now logged (`display-param resolver …`, with the upstream's in-band `error` code), so the reason is greppable. Replaces the D24 MCP wrapper, since Slack's MCP server only admits Marketplace-listed apps. Shared-app caveat: Slack caps `conversations.history`/`.replies` at 1 req/min and 15 messages for non-Marketplace apps, and an org's own BYOC Slack app is exempt. - `x-overslash-success` (alias `success`) — a jq predicate over a below-400 JSON body, set under `info` or per operation, HTTP only. A body that fails it is an upstream error for `is_error`, audit, response capture and paging, with the real status kept. It is stored on replay payloads and syntax-checked at template register (`success_invalid_jq`). Slack uses `.ok == true`. - Template/service instance split — templates (OpenAPI 3.1 blueprints with `x-overslash-*` extensions) + service instances (named, with credentials and lifecycle) - Three-tier template registry — global (read-only, shipped OpenAPI) + org (CRUD by org admins) + user (CRUD, gated by `user_template_policy`) (PR #100) diff --git a/crates/overslash-api/src/services/param_resolver.rs b/crates/overslash-api/src/services/param_resolver.rs index 67991e18..8134189f 100644 --- a/crates/overslash-api/src/services/param_resolver.rs +++ b/crates/overslash-api/src/services/param_resolver.rs @@ -87,6 +87,19 @@ fn project( (display, canonical) } +/// A top-level `error` code from a resolver response, when it looks like one: +/// a short identifier, not a sentence that could echo the request back. +fn upstream_error_code(body: &serde_json::Value) -> Option<&str> { + body.get("error") + .and_then(serde_json::Value::as_str) + .filter(|code| { + code.len() <= 64 + && code + .bytes() + .all(|b| b.is_ascii_alphanumeric() || matches!(b, b'_' | b'.' | b'-')) + }) +} + /// Resolve display names for params that declare `source:` — answered from the /// gateway's own records rather than from the upstream. /// @@ -225,12 +238,49 @@ pub async fn resolve_display_params( for (key, value) in &headers { req = req.header(key, value); } - let resp = req.send().await.ok()?; - if !resp.status().is_success() { + // Each miss is logged, as on the MCP path: an approval + // quoting a raw id is otherwise undiagnosable. `get` is the + // unsubstituted template path, never the URL, so the + // caller's argument stays out of the log. + let resp = match req.send().await { + Ok(resp) => resp, + Err(e) => { + // reqwest's Display quotes the URL, which carries + // the substituted argument. + tracing::warn!( + get = %get, + param = %name, + error = %e.without_url(), + "display-param resolver request failed; using the raw argument" + ); + return None; + } + }; + let status = resp.status(); + if !status.is_success() { + tracing::warn!( + get = %get, + param = %name, + status = status.as_u16(), + "display-param resolver got an error status; using the raw argument" + ); return None; } let json: serde_json::Value = resp.json().await.ok()?; - Some(project(&resolver, &json)) + let projected = project(&resolver, &json); + if projected.0.is_none() { + // Providers that answer failures in-band (Slack's + // `200 {"ok": false, "error": "missing_scope"}`) land + // here. Their error code is a short enum, safe to log + // and the one thing that says *why*. + tracing::info!( + get = %get, + param = %name, + upstream_error = upstream_error_code(&json).unwrap_or_default(), + "display-param resolver projected nothing; using the raw argument" + ); + } + Some(projected) } .await; (name, fetched) @@ -473,7 +523,7 @@ fn resolver_body( #[cfg(test)] mod tests { - use super::{project, resolver_body}; + use super::{project, resolver_body, upstream_error_code}; use crate::services::mcp_client::ToolCallResult; use overslash_core::types::ParamResolver; use serde_json::json; @@ -547,4 +597,20 @@ mod tests { (Some("Peluquería canina".to_string()), None) ); } + + #[test] + fn upstream_error_code_takes_only_a_short_identifier() { + assert_eq!( + upstream_error_code(&json!({"ok": false, "error": "missing_scope"})), + Some("missing_scope") + ); + // A sentence could quote the request back, so it is not logged. + assert_eq!( + upstream_error_code(&json!({"error": "no channel named C1 for alice"})), + None + ); + // `error` as an object (OAuth-style APIs) carries no flat code. + assert_eq!(upstream_error_code(&json!({"error": {"code": 404}})), None); + assert_eq!(upstream_error_code(&json!({"ok": true})), None); + } } diff --git a/crates/overslash-api/tests/slack.rs b/crates/overslash-api/tests/slack.rs index 327e4e0f..2deb188f 100644 --- a/crates/overslash-api/tests/slack.rs +++ b/crates/overslash-api/tests/slack.rs @@ -10,7 +10,10 @@ //! * Slack's in-band failure (`200 {"ok": false}`) comes back as `is_error` //! and is audited as one, with the status left at 200; //! * a paged read offers the next cursor, and stops on an in-band failure; -//! * a gated write discloses the channel by name, through the resolver; +//! * a gated write names the channel (`#general`) in its title and +//! disclosure, through the resolver, and falls back to the raw id when +//! Slack can't name it; +//! * read summaries name the thread's channel and the looked-up user; //! * no connection → a clean error before any upstream call. //! //! Nothing reaches the network. @@ -152,7 +155,17 @@ async fn start_fake_slack() -> (String, SeenLog) { "/api/chat.postMessage" => { json!({ "ok": true, "channel": body["channel"], "ts": "1700000000.000100" }) } - "/api/conversations.info" => json!({ "ok": true, "channel": { "name": "general" } }), + // Only `C1` exists. Anything else is Slack's in-band miss, which a + // resolver must survive by falling back to the raw id. + "/api/conversations.info" if query == "channel=C1" => { + json!({ "ok": true, "channel": { "name": "general" } }) + } + "/api/conversations.info" => json!({ "ok": false, "error": "channel_not_found" }), + "/api/conversations.replies" => json!({ "ok": true, "messages": [] }), + "/api/users.info" => json!({ + "ok": true, + "user": { "name": "alice", "real_name": "Alice Liddell" } + }), // Page one offers a cursor. Following it hits an in-band failure, // which must end the walk rather than offer the cursor again. "/api/conversations.list" if query.contains("cursor=") => { @@ -385,6 +398,11 @@ async fn a_gated_send_discloses_the_channel_by_name() { ) .await; assert_eq!(body["status"], "pending_approval", "{body}"); + let title = body["action_description"].as_str().unwrap_or_default(); + assert!( + title.contains("to '#general'"), + "the approval title must name the channel: {title}" + ); let disclosed: std::collections::HashMap = body["disclosed_fields"] .as_array() .expect("disclosed_fields present") @@ -398,7 +416,7 @@ async fn a_gated_send_discloses_the_channel_by_name() { .collect(); assert_eq!( disclosed.get("Channel").map(String::as_str), - Some("general") + Some("#general") ); assert_eq!( disclosed.get("Message").map(String::as_str), @@ -410,6 +428,63 @@ async fn a_gated_send_discloses_the_channel_by_name() { ); } +/// Slack can't name the channel (a DM, a private channel, a token without +/// `channels:read`): the approval still goes up, quoting the raw id. +#[tokio::test] +async fn an_unresolvable_channel_falls_back_to_its_id() { + let h = setup(common::test_pool().await, false).await; + seed_slack_connection(&h).await; + + let (status, body) = call( + &h, + "send_message", + json!({ "channel": "D0PRIVATE", "text": "psst" }), + ) + .await; + assert_eq!(status, 202, "{body}"); + assert_eq!(body["status"], "pending_approval", "{body}"); + let title = body["action_description"].as_str().unwrap_or_default(); + assert!(title.contains("to D0PRIVATE"), "{title}"); + assert!(!title.contains('#'), "{title}"); +} + +/// Reads aren't gated, but their audit rows are what a human scans later. +#[tokio::test] +async fn read_summaries_name_the_channel_and_the_user() { + let h = setup(common::test_pool().await, true).await; + seed_slack_connection(&h).await; + + for (action, params, expected) in [ + ( + "get_thread_replies", + json!({ "channel": "C1", "ts": "1700000000.000100" }), + "in '#general'", + ), + ( + "get_user", + json!({ "user": "U1" }), + "'Alice Liddell (@alice)'", + ), + ] { + let (status, body) = call(&h, action, params).await; + assert_eq!(status, 200, "{action}: {body}"); + let description: Option = sqlx::query_scalar( + "SELECT description FROM audit_log + WHERE org_id = $1 AND action = 'action.executed' + ORDER BY created_at DESC LIMIT 1", + ) + .bind(h.org_id) + .fetch_one(&h.pool) + .await + .expect("the call must write an audit row"); + let description = description.unwrap_or_default(); + assert!( + description.contains(expected), + "{action}: expected {expected:?} in {description:?}" + ); + } +} + // ============================================================================ // No connection // ============================================================================ diff --git a/services/slack.yaml b/services/slack.yaml index 5cf74a9c..88443906 100644 --- a/services/slack.yaml +++ b/services/slack.yaml @@ -71,7 +71,7 @@ paths: description: Channel ID (e.g. C0123456789), or a user ID for a DM. resolve: get: /api/conversations.info?channel={channel} - pick: channel.name + display: '#{channel.name}' text: type: string minLength: 1 @@ -110,7 +110,7 @@ paths: description: Channel ID the message is in. resolve: get: /api/conversations.info?channel={channel} - pick: channel.name + display: '#{channel.name}' ts: type: string description: The message's `ts`. @@ -148,7 +148,7 @@ paths: description: Channel ID the message is in. resolve: get: /api/conversations.info?channel={channel} - pick: channel.name + display: '#{channel.name}' timestamp: type: string description: The message's `ts`. @@ -233,7 +233,7 @@ paths: minLength: 1 resolve: get: /api/conversations.info?channel={channel} - pick: channel.name + display: '#{channel.name}' - name: limit in: query schema: @@ -282,6 +282,9 @@ paths: schema: type: string minLength: 1 + resolve: + get: /api/conversations.info?channel={channel} + display: '#{channel.name}' - name: ts in: query required: true @@ -318,6 +321,9 @@ paths: schema: type: string minLength: 1 + resolve: + get: /api/users.info?user={user} + display: '{user.real_name}[ (@{user.name})]' /api/users.list: get: operationId: list_users