fix(telegram): single per-request message, no web page preview

Defer ask approval from SOCKS5 CONNECT to per-request HTTP handler
so only one Telegram message appears per request (not two). The
message combines destination and request info:

  OpenClaw wants to connect to:
  HTTPS httpbin.org:443
  GET https://httpbin.org/ip
  Allow this request?

Also disable Telegram web page preview on approval messages to
prevent link-preview artifacts.

Author: nnemirovsky

internal/proxy/request_policy_context_test.go

@@ -117,7 +117,11 @@ default = "allow"
 	}
 }
 
-func TestAllowAttachesCheckerOnAskApproval(t *testing.T) {
+func TestAllowDefersAskToPerRequest(t *testing.T) {
+	// When a broker is configured and the destination matches an ask rule,
+	// Allow() auto-allows the SOCKS5 CONNECT without consulting the broker.
+	// A checker with no seed credit is attached so every HTTP request
+	// triggers its own per-request approval with method/path visible.
 	fc := newFakeChannel(channel.ResponseAllowOnce)
 	broker := channel.NewBroker([]channel.Channel{fc})
 	fc.broker = broker
@@ -132,40 +136,34 @@ destination = "api.example.com"
 
 	ctx, ok := rules.Allow(context.Background(), mkConnectRequest("api.example.com"))
 	if !ok {
-		t.Fatal("Allow returned false after ask->allow-once approval")
+		t.Fatal("Allow returned false for ask destination with broker")
 	}
 	if skip, _ := ctx.Value(ctxKeySkipPerRequest).(bool); skip {
-		t.Fatal("ctxKeySkipPerRequest should NOT be set for ask-approved connection")
+		t.Fatal("ctxKeySkipPerRequest should NOT be set for ask-deferred connection")
 	}
 	checker, _ := ctx.Value(ctxKeyPerRequestPolicy).(*RequestPolicyChecker)
 	if checker == nil {
-		t.Fatal("ask-approved connection must attach a RequestPolicyChecker so subsequent HTTP requests re-trigger the ask flow")
+		t.Fatal("ask-deferred connection must attach a RequestPolicyChecker")
 	}
 
-	// Sanity check: Allow() must have consulted the broker exactly once for
-	// the CONNECT-level ask.
-	if got := fc.requestCount(); got != 1 {
-		t.Fatalf("broker count after Allow = %d, want 1 (CONNECT ask)", got)
+	// Allow() must NOT have consulted the broker (deferred to per-request).
+	if got := fc.requestCount(); got != 0 {
+		t.Fatalf("broker count after Allow = %d, want 0 (deferred)", got)
 	}
 
-	// The first HTTP request must NOT re-ask the broker (double-prompt
-	// regression guard). The checker was seeded with one prepaid allow
-	// credit from the CONNECT approval, so CheckAndConsume on the first
-	// request is satisfied from the seed without contacting the broker.
+	// First HTTP request asks the broker (no seed credit).
 	verdict, err := checker.CheckAndConsume("api.example.com", 443)
 	if err != nil {
 		t.Fatalf("first CheckAndConsume: %v", err)
 	}
 	if verdict != policy.Allow {
-		t.Fatalf("first HTTP request verdict = %v, want Allow (seeded credit)", verdict)
+		t.Fatalf("first HTTP request verdict = %v, want Allow", verdict)
 	}
 	if got := fc.requestCount(); got != 1 {
-		t.Fatalf("broker count after first HTTP request = %d, want 1 (seed must consume without re-asking broker)", got)
+		t.Fatalf("broker count after first request = %d, want 1", got)
 	}
 
-	// The second HTTP request must re-ask the broker (seed is exhausted).
-	// Flip the response so the second verdict differs from the first,
-	// proving the broker was actually consulted again.
+	// Second HTTP request also asks the broker.
 	fc.setResponse(channel.ResponseDeny)
 	verdict, err = checker.CheckAndConsume("api.example.com", 443)
 	if err != nil {
@@ -175,17 +173,15 @@ destination = "api.example.com"
 		t.Fatalf("second HTTP request verdict = %v, want Deny", verdict)
 	}
 	if got := fc.requestCount(); got != 2 {
-		t.Fatalf("broker count after second HTTP request = %d, want 2 (each subsequent request re-asks)", got)
+		t.Fatalf("broker count after second request = %d, want 2", got)
 	}
 }
 
-func TestAllowSetsSkipAfterAlwaysAllow(t *testing.T) {
-	// When a connection-level ask resolves to Always Allow and the rule
-	// is persisted, the connection should take the fast path because the
-	// new rule will match every subsequent HTTP request via the engine.
-	// This mirrors the SNI path which sets ctxKeySkipPerRequest after
-	// sniSaveRule succeeds. Uses a store-backed rule set because the
-	// persist path now requires a store (there is no in-memory fallback).
+func TestAllowDefersAskWithBrokerDoesNotPrompt(t *testing.T) {
+	// With a broker, ask destinations are auto-allowed at CONNECT time.
+	// The broker is NOT consulted. The checker has no seed credit.
+	// Verify that Always Allow responses are handled per-request (the
+	// checker's persist callback handles rule persistence).
 	fc := newFakeChannel(channel.ResponseAlwaysAllow)
 	broker := channel.NewBroker([]channel.Channel{fc})
 	fc.broker = broker
@@ -200,14 +196,16 @@ destination = "api.example.com"
 
 	ctx, ok := rules.Allow(context.Background(), mkConnectRequest("api.example.com"))
 	if !ok {
-		t.Fatal("Allow returned false after ask->always-allow approval")
+		t.Fatal("Allow returned false for ask destination with broker")
 	}
-	skip, _ := ctx.Value(ctxKeySkipPerRequest).(bool)
-	if !skip {
-		t.Fatal("ctxKeySkipPerRequest should be true after always-allow so per-request checks are skipped")
+	// Broker was NOT consulted at connection level.
+	if got := fc.requestCount(); got != 0 {
+		t.Fatalf("broker count = %d, want 0 (deferred)", got)
 	}
-	if _, present := ctx.Value(ctxKeyPerRequestPolicy).(*RequestPolicyChecker); present {
-		t.Fatal("ctxKeyPerRequestPolicy should NOT be set when skip flag is true")
+	// Checker is attached (not skip).
+	checker, _ := ctx.Value(ctxKeyPerRequestPolicy).(*RequestPolicyChecker)
+	if checker == nil {
+		t.Fatal("checker should be attached for ask-deferred connection")
 	}
 }
 

internal/proxy/server.go

@@ -309,11 +309,6 @@ func (r *policyRuleSet) Allow(ctx context.Context, req *socks5.Request) (context
 	// partial persistence failure does not silently allow all subsequent
 	// requests without re-triggering per-request policy.
 	var alwaysAllowPersisted bool
-	// askApprovedOnce is true when the connection-level ask resolved to
-	// ResponseAllowOnce. Used to seed the per-request checker with a single
-	// prepaid allow credit so the first HTTP request does not re-prompt the
-	// user (double-prompt fix).
-	var askApprovedOnce bool
 	switch verdict {
 	case policy.Allow:
 		allowed = true
@@ -323,39 +318,15 @@ func (r *policyRuleSet) Allow(ctx context.Context, req *socks5.Request) (context
 			reason = "ask treated as deny (no approval broker)"
 			log.Printf("[ASK->DENY] %s:%d (no approval broker)", dest, port)
 		} else {
-			log.Printf("[ASK] %s:%d (waiting for Telegram approval)", dest, port)
-			timeout := time.Duration(eng.TimeoutSec) * time.Second
-			proto := DetectProtocol(port)
-			resp, err := r.broker.Request(dest, port, proto.String(), timeout)
-			if err != nil {
-				effectiveVerdict = policy.Deny
-				reason = fmt.Sprintf("approval timeout: %v", err)
-				log.Printf("[ASK->DENY] %s:%d (timeout: %v)", dest, port, err)
-			} else {
-				switch resp {
-				case channel.ResponseAllowOnce:
-					allowed = true
-					effectiveVerdict = policy.Allow
-					reason = "user approved once"
-					askApprovedOnce = true
-					log.Printf("[ASK->ALLOW] %s:%d (user approved once)", dest, port)
-				case channel.ResponseAlwaysAllow:
-					allowed = true
-					effectiveVerdict = policy.Allow
-					reason = "user approved always"
-					log.Printf("[ASK->ALLOW+SAVE] %s:%d (user approved always)", dest, port)
-					alwaysAllowPersisted = r.persistAlwaysAllow(dest, port)
-				case channel.ResponseAlwaysDeny:
-					effectiveVerdict = policy.Deny
-					reason = "user denied always"
-					log.Printf("[ASK->DENY+SAVE] %s:%d (user denied always)", dest, port)
-					r.persistAlwaysDeny(dest, port)
-				default:
-					effectiveVerdict = policy.Deny
-					reason = "user denied"
-					log.Printf("[ASK->DENY] %s:%d (user denied)", dest, port)
-				}
-			}
+			// Auto-allow the SOCKS5 CONNECT without prompting the user.
+			// Approval happens per-request inside the MITM handler where
+			// the HTTP method and path are known, producing a single
+			// combined Telegram message per request instead of two
+			// separate messages (connection + request).
+			allowed = true
+			effectiveVerdict = policy.Allow
+			reason = "ask deferred to per-request"
+			log.Printf("[ASK->DEFER] %s:%d (approval deferred to per-request)", dest, port)
 		}
 	}
 
@@ -444,13 +415,11 @@ func (r *policyRuleSet) Allow(ctx context.Context, req *socks5.Request) (context
 		if skipPerRequest {
 			ctx = context.WithValue(ctx, ctxKeySkipPerRequest, true)
 		} else {
-			seed := 0
-			if askApprovedOnce {
-				seed = 1
-			}
+			// No seed credit: every HTTP request triggers its own
+			// per-request approval with method and path visible in
+			// the Telegram message.
 			checker := NewRequestPolicyChecker(r.engine, r.broker,
 				WithPersist(r.buildPersistFunc()),
-				WithSeedCredits(seed),
 			)
 			ctx = context.WithValue(ctx, ctxKeyPerRequestPolicy, checker)
 		}
@@ -1447,56 +1416,14 @@ func (s *Server) sniPolicyCheckBeforeDial(ctx context.Context, request *socks5.R
 			log.Printf("[SNI->DENY] %s:%d (hostname %s: ask treated as deny, no broker)", ipStr, port, sni)
 			return nil, ctx, false
 		}
-		log.Printf("[SNI->ASK] %s:%d (hostname %s: waiting for approval)", ipStr, port, sni)
-		timeout := time.Duration(eng.TimeoutSec) * time.Second
-		proto := DetectProtocol(port)
-		resp, reqErr := s.rules.broker.Request(sni, port, proto.String(), timeout)
-		if reqErr != nil {
-			log.Printf("[SNI->DENY] %s:%d (hostname %s: approval timeout: %v)", ipStr, port, sni, reqErr)
-			return nil, ctx, false
-		}
-		switch resp {
-		case channel.ResponseAllowOnce:
-			log.Printf("[SNI->ALLOW] %s:%d (hostname %s: user approved once)", ipStr, port, sni)
-			// Ask-approved once: attach a checker seeded with one prepaid
-			// allow credit so the first HTTP request on the new tunnel does
-			// not re-prompt the user (the SNI-level approval is reused for
-			// the first request). Subsequent requests on the same keep-alive
-			// connection re-trigger the ask flow normally.
-			checker := NewRequestPolicyChecker(s.rules.engine, s.rules.broker,
-				WithPersist(s.rules.buildPersistFunc()),
-				WithSeedCredits(1),
-			)
-			ctx = context.WithValue(ctx, ctxKeyPerRequestPolicy, checker)
-			return reader, ctx, true
-		case channel.ResponseAlwaysAllow:
-			log.Printf("[SNI->ALLOW+SAVE] %s:%d (hostname %s: user approved always)", ipStr, port, sni)
-			// Persist the new allow rule. If the persist path fails we
-			// fall back to attaching a checker as a safety net so the
-			// current keep-alive connection is still policy-checked on
-			// every request (the failure was only logged inside
-			// sniSaveRule, so the in-memory engine may still evaluate
-			// sni:port as ask). The checker is seeded with one credit
-			// because the user already approved this connection.
-			if ok := s.sniSaveRule("allow", sni, port); ok {
-				ctx = context.WithValue(ctx, ctxKeySkipPerRequest, true)
-			} else {
-				log.Printf("[SNI->ALLOW+SAVE] %s:%d persistence failed; attaching per-request checker as safety net", sni, port)
-				checker := NewRequestPolicyChecker(s.rules.engine, s.rules.broker,
-					WithPersist(s.rules.buildPersistFunc()),
-					WithSeedCredits(1),
-				)
-				ctx = context.WithValue(ctx, ctxKeyPerRequestPolicy, checker)
-			}
-			return reader, ctx, true
-		case channel.ResponseAlwaysDeny:
-			log.Printf("[SNI->DENY+SAVE] %s:%d (hostname %s: user denied always)", ipStr, port, sni)
-			s.sniSaveRule("deny", sni, port)
-			return nil, ctx, false
-		default:
-			log.Printf("[SNI->DENY] %s:%d (hostname %s: user denied)", ipStr, port, sni)
-			return nil, ctx, false
-		}
+		// Auto-allow the connection and defer approval to per-request
+		// checks where the HTTP method and path are visible.
+		log.Printf("[SNI->DEFER] %s:%d (hostname %s: approval deferred to per-request)", ipStr, port, sni)
+		checker := NewRequestPolicyChecker(s.rules.engine, s.rules.broker,
+			WithPersist(s.rules.buildPersistFunc()),
+		)
+		ctx = context.WithValue(ctx, ctxKeyPerRequestPolicy, checker)
+		return reader, ctx, true
 	default:
 		log.Printf("[SNI->DENY] %s:%d (hostname %s: default deny)", ipStr, port, sni)
 		return nil, ctx, false

internal/telegram/approval.go

@@ -137,6 +137,7 @@ func (tc *TelegramChannel) sendApprovalMessage(req channel.ApprovalRequest) {
 
 	msg := tgbotapi.NewMessage(tc.chatID, FormatApprovalMessage(req))
 	msg.ParseMode = tgbotapi.ModeHTML
+	msg.DisableWebPagePreview = true
 	msg.ReplyMarkup = tgbotapi.NewInlineKeyboardMarkup(
 		tgbotapi.NewInlineKeyboardRow(
 			tgbotapi.NewInlineKeyboardButtonData("Allow", req.ID+"|allow_once"),

internal/telegram/bot.go

@@ -65,16 +65,17 @@ func FormatApprovalMessage(req channel.ApprovalRequest) string {
 		msg += "\n\nAllow this tool call?"
 		return msg
 	}
+	display := protoDisplayName[req.Protocol]
+	if display == "" {
+		display = req.Protocol
+	}
 	if req.Method != "" {
 		return fmt.Sprintf(
-			"OpenClaw wants to send request (per-request):\n\n%s %s\n\nAllow this request?",
+			"OpenClaw wants to connect to:\n\n%s %s:%d\n%s %s\n\nAllow this request?",
+			htmlEscape(display), htmlEscape(req.Destination), req.Port,
 			htmlEscape(req.Method), htmlEscape(buildRequestURL(req)),
 		)
 	}
-	display := protoDisplayName[req.Protocol]
-	if display == "" {
-		display = req.Protocol
-	}
 	return fmt.Sprintf(
 		"OpenClaw wants to connect to:\n\n%s %s:%d\n\nAllow this connection?",
 		htmlEscape(display), htmlEscape(req.Destination), req.Port,

internal/telegram/bot_test.go

@@ -167,11 +167,11 @@ func TestFormatApprovalMessage(t *testing.T) {
 			Path:        "/users/me",
 		}
 		msg := FormatApprovalMessage(req)
-		if !strings.Contains(msg, "GET https://api.example.com/users/me") {
-			t.Errorf("expected 'GET https://api.example.com/users/me' in message, got: %s", msg)
+		if !strings.Contains(msg, "HTTPS api.example.com:443") {
+			t.Errorf("expected destination line in message, got: %s", msg)
 		}
-		if !strings.Contains(msg, "per-request") {
-			t.Errorf("expected 'per-request' label in message, got: %s", msg)
+		if !strings.Contains(msg, "GET https://api.example.com/users/me") {
+			t.Errorf("expected request line in message, got: %s", msg)
 		}
 		if !strings.Contains(msg, "Allow this request?") {
 			t.Errorf("expected 'Allow this request?' wording, got: %s", msg)