fix: mailbox onboarding accepts connections that cannot work, and hides why (#465)

* fix: report why a provider refused to name a mailbox during onboarding, since the status and body were read and then discarded and five distinct failures arrived as one opaque sentence with nothing in the logs or error tracking behind it * fix: refuse a mailbox the provider only partly authorised, since a consent screen lets the person untick individual permissions and still issues a token, and refuse a first connect that returns no refresh token because that mailbox has an hour to live * fix: never record a provider's successful profile payload in diagnostics because a decode failure carries status 200 and that body is the mailbox owner's address, and bound the provider read so a broken intermediary cannot make onboarding read arbitrary memory
2026-09-13 00:03:42 +00:00 · 2026-09-12 08:07:20 -07:00
parent d456bc48c6
commit 1f7e036659
2 changed files with 440 additions and 10 deletions
@@ -3,6 +3,7 @@ package email
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"io"
 	"net/http"
 	"net/mail"
@@ -10,6 +11,7 @@ import (
 	"time"

 	"github.com/google/uuid"
+	"github.com/rs/zerolog/log"
 	"github.com/warmbly/warmbly/internal/config"
 	"github.com/warmbly/warmbly/internal/errx"
 	"github.com/warmbly/warmbly/internal/infrastructure/pubsub"
@@ -130,6 +132,15 @@ func (s *emailService) OAuthFinish(ctx context.Context, userID, code, state stri
 		return nil, false, errx.ErrEmailOnboardExchange
 	}

+	// A consent screen lets the person untick individual permissions and still
+	// returns a token, so this is the last point at which a half-granted
+	// mailbox can be refused. Storing one instead means a mailbox that looks
+	// connected and fails on its first send, days later, with a provider 403
+	// nobody can trace back to a checkbox.
+	if xerr := checkGrantedScopes(ctx, provider, cfg.Scopes, tok); xerr != nil {
+		return nil, false, xerr
+	}
+
 	owner, xerr := fetchInboxOwner(ctx, provider, tok.AccessToken)
 	if xerr != nil {
 		return nil, false, xerr
@@ -146,6 +157,22 @@ func (s *emailService) OAuthFinish(ctx context.Context, userID, code, state stri
 		return nil, false, errx.ErrEmailOnboardAlreadyExists
 	}

+	// A first connect with no refresh token is a mailbox with about an hour to
+	// live: the access token expires and nothing can renew it. reauth.go
+	// deliberately tolerates an absent one because it keeps the stored value,
+	// but here there is nothing stored to fall back on, so refuse now rather
+	// than hand back a mailbox that stops by itself.
+	if strings.TrimSpace(tok.RefreshToken) == "" {
+		log.Warn().
+			Str("provider", string(provider)).
+			Msg("mailbox onboarding: first connect returned no refresh token")
+		errs.CaptureMessageContext(ctx, fmt.Sprintf("%s first connect returned no refresh token", provider),
+			errs.Tag("provider", string(provider)))
+		return nil, false, errx.New(errx.BadRequest,
+			"The provider did not return a long-lived token for this mailbox, so it would stop working within the hour. "+
+				"Remove Warmbly's access in your account settings and connect it again.")
+	}
+
 	name := strings.TrimSpace(owner.Name)
 	if name == "" {
 		name = deriveNameFromEmail(owner.Email)
@@ -387,6 +414,149 @@ func fetchInboxOwner(ctx context.Context, provider models.InboxProvider, accessT
 	}
 }

+// grantedScopes reads what the provider actually authorised. Both Google and
+// Microsoft return a space-separated "scope" alongside the token; an empty or
+// absent one means the provider did not say, which is not the same as "nothing
+// was granted" and must not be read as a denial.
+func grantedScopes(tok *oauth2.Token) (map[string]bool, bool) {
+	raw, _ := tok.Extra("scope").(string)
+	if strings.TrimSpace(raw) == "" {
+		return nil, false
+	}
+	out := make(map[string]bool)
+	for _, sc := range strings.Fields(raw) {
+		out[sc] = true
+	}
+	return out, true
+}
+
+// scopeSatisfiedBy maps a scope we ask for to every scope that confers it.
+// Google's Gmail scopes nest: gmail.modify covers reading and sending, and
+// gmail.compose covers sending. Without this, someone who granted the broader
+// permission but not the narrower one would be turned away for a capability
+// they actually have.
+var scopeSatisfiedBy = map[string][]string{
+	"https://www.googleapis.com/auth/gmail.readonly": {
+		"https://www.googleapis.com/auth/gmail.readonly",
+		"https://www.googleapis.com/auth/gmail.modify",
+		"https://mail.google.com/",
+	},
+	"https://www.googleapis.com/auth/gmail.send": {
+		"https://www.googleapis.com/auth/gmail.send",
+		"https://www.googleapis.com/auth/gmail.compose",
+		"https://www.googleapis.com/auth/gmail.modify",
+		"https://mail.google.com/",
+	},
+	"https://www.googleapis.com/auth/gmail.metadata": {
+		"https://www.googleapis.com/auth/gmail.metadata",
+		"https://www.googleapis.com/auth/gmail.readonly",
+		"https://www.googleapis.com/auth/gmail.modify",
+		"https://mail.google.com/",
+	},
+	"https://www.googleapis.com/auth/gmail.modify": {
+		"https://www.googleapis.com/auth/gmail.modify",
+		"https://mail.google.com/",
+	},
+}
+
+// scopeLabel names a permission the way the consent screen does, so the error
+// tells someone which checkbox to go back and tick.
+var scopeLabel = map[string]string{
+	"https://www.googleapis.com/auth/gmail.readonly":       "read your email",
+	"https://www.googleapis.com/auth/gmail.send":           "send email on your behalf",
+	"https://www.googleapis.com/auth/gmail.modify":         "manage your email",
+	"https://www.googleapis.com/auth/gmail.compose":        "compose and send email",
+	"https://www.googleapis.com/auth/gmail.metadata":       "read email metadata",
+	"https://www.googleapis.com/auth/gmail.settings.basic": "manage your mail settings",
+	"https://graph.microsoft.com/Mail.Send":                "send mail",
+	"https://graph.microsoft.com/Mail.ReadWrite":           "read and write mail",
+	"https://graph.microsoft.com/User.Read":                "read your profile",
+}
+
+// checkGrantedScopes refuses a connection the provider only partly authorised.
+func checkGrantedScopes(ctx context.Context, provider models.InboxProvider, want []string, tok *oauth2.Token) *errx.Error {
+	granted, told := grantedScopes(tok)
+	if !told {
+		// Nothing to check against. Recorded rather than guessed at, because a
+		// provider that stops returning the scope would otherwise turn this
+		// into a silent no-op.
+		errs.CaptureMessageContext(ctx, fmt.Sprintf("%s returned no scope with the token; granted permissions were not verified", provider),
+			errs.Tag("provider", string(provider)))
+		return nil
+	}
+
+	var missing []string
+	for _, w := range want {
+		accepted, ok := scopeSatisfiedBy[w]
+		if !ok {
+			accepted = []string{w}
+		}
+		if !anyGranted(granted, accepted) {
+			missing = append(missing, w)
+		}
+	}
+	if len(missing) == 0 {
+		return nil
+	}
+
+	labels := make([]string, 0, len(missing))
+	for _, m := range missing {
+		if l, ok := scopeLabel[m]; ok {
+			labels = append(labels, l)
+		} else {
+			labels = append(labels, m)
+		}
+	}
+	log.Warn().
+		Str("provider", string(provider)).
+		Strs("missing", missing).
+		Msg("mailbox onboarding: refused a partly granted consent")
+	errs.CaptureMessageContext(ctx, fmt.Sprintf("%s mailbox connected with missing permissions", provider),
+		errs.Tag("provider", string(provider)),
+		errs.Extra("missing", strings.Join(missing, " ")))
+
+	return errx.New(errx.BadRequest, fmt.Sprintf(
+		"Warmbly was not given every permission it needs for this mailbox. Missing: %s. Connect it again and leave all the permissions ticked.",
+		strings.Join(labels, ", ")))
+}
+
+func anyGranted(granted map[string]bool, accepted []string) bool {
+	for _, a := range accepted {
+		if granted[a] {
+			return true
+		}
+	}
+	return false
+}
+
+// maxProviderBody bounds what is read from a provider before it is looked at,
+// so a broken or hostile intermediary cannot make onboarding read an arbitrary
+// amount into memory.
+const maxProviderBody = 64 << 10
+
+// diagnosticDetailLimit is how much of a failure body is recorded. A provider
+// error payload has no length contract.
+const diagnosticDetailLimit = 512
+
+// diagnosticBody decides how much of the provider's response may be recorded.
+//
+// Nothing at all from a 2xx. A decode failure and a missing address both carry
+// status 200, and that body is a SUCCESSFUL profile payload: for Gmail it is
+// the mailbox address, for Outlook the address and display name. Recording it
+// would put the mailbox owner's identity into the log stream and into error
+// tracking. The decision lives here rather than at the call sites so that
+// adding a new failure stage cannot get it wrong.
+func diagnosticBody(status int, body []byte) string {
+	if status >= 200 && status < 300 {
+		return ""
+	}
+	detail := strings.TrimSpace(string(body))
+	if len(detail) > diagnosticDetailLimit {
+		detail = detail[:diagnosticDetailLimit] + "…"
+	}
+	return detail
+}
+
 var httpClient = &http.Client{Timeout: 10 * time.Second}

 func fetchGmailOwner(ctx context.Context, token string) (*inboxOwner, *errx.Error) {
@@ -395,37 +565,73 @@ func fetchGmailOwner(ctx context.Context, token string) (*inboxOwner, *errx.Erro

 	resp, err := httpClient.Do(req)
 	if err != nil {
-		return nil, errx.ErrEmailOnboardUserInfo
+		return nil, ownerLookupFailed(ctx, "gmail", "transport", 0, nil, err)
 	}
 	defer resp.Body.Close()
-	body, _ := io.ReadAll(resp.Body)
+	body, _ := io.ReadAll(io.LimitReader(resp.Body, maxProviderBody))
 	if resp.StatusCode != http.StatusOK {
-		return nil, errx.ErrEmailOnboardUserInfo
+		return nil, ownerLookupFailed(ctx, "gmail", "status", resp.StatusCode, body, nil)
 	}
 	var out struct {
 		EmailAddress string `json:"emailAddress"`
 	}
 	if err := json.Unmarshal(body, &out); err != nil {
-		return nil, errx.ErrEmailOnboardUserInfo
+		return nil, ownerLookupFailed(ctx, "gmail", "decode", resp.StatusCode, body, err)
 	}
 	if out.EmailAddress == "" {
-		return nil, errx.ErrEmailOnboardUserInfo
+		return nil, ownerLookupFailed(ctx, "gmail", "no_address", resp.StatusCode, body, nil)
 	}
 	return &inboxOwner{Email: out.EmailAddress}, nil
 }

+// ownerLookupFailed reports why the provider would not name the mailbox and
+// returns the one error the user sees.
+//
+// The user-facing message stays deliberately vague, because the cause is ours
+// and not theirs. What is NOT vague any more is the record: this call used to
+// read the status and the body and then discard both, so five distinct
+// failures arrived as one sentence with nothing behind it, and the most common
+// of them by far ("the Gmail API has not been enabled in this project") was
+// indistinguishable from a revoked token. A handled 400 raises no exception,
+// so without this there is nothing in the logs or in error tracking either.
+func ownerLookupFailed(ctx context.Context, provider, stage string, status int, body []byte, cause error) *errx.Error {
+	detail := diagnosticBody(status, body)
+	opts := []errs.Option{
+		errs.Tag("provider", provider),
+		errs.Tag("stage", stage),
+		errs.Extra("status", status),
+	}
+	// Only present on a failure response, where it is the provider's own error
+	// payload rather than anything belonging to the mailbox owner.
+	if detail != "" {
+		opts = append(opts, errs.Extra("response", detail))
+	}
+	if cause != nil {
+		errs.CaptureExceptionContext(ctx, fmt.Errorf("%s owner lookup failed at %s: %w", provider, stage, cause), opts...)
+	} else {
+		errs.CaptureMessageContext(ctx, fmt.Sprintf("%s owner lookup failed at %s (status %d)", provider, stage, status), opts...)
+	}
+	log.Warn().
+		Str("provider", provider).
+		Str("stage", stage).
+		Int("status", status).
+		Str("response", detail).
+		Msg("mailbox onboarding: provider would not name the account")
+	return errx.ErrEmailOnboardUserInfo
+}
+
 func fetchOutlookOwner(ctx context.Context, token string) (*inboxOwner, *errx.Error) {
 	req, _ := http.NewRequestWithContext(ctx, http.MethodGet, "https://graph.microsoft.com/v1.0/me", nil)
 	req.Header.Set("Authorization", "Bearer "+token)

 	resp, err := httpClient.Do(req)
 	if err != nil {
-		return nil, errx.ErrEmailOnboardUserInfo
+		return nil, ownerLookupFailed(ctx, "outlook", "transport", 0, nil, err)
 	}
 	defer resp.Body.Close()
-	body, _ := io.ReadAll(resp.Body)
+	body, _ := io.ReadAll(io.LimitReader(resp.Body, maxProviderBody))
 	if resp.StatusCode != http.StatusOK {
-		return nil, errx.ErrEmailOnboardUserInfo
+		return nil, ownerLookupFailed(ctx, "outlook", "status", resp.StatusCode, body, nil)
 	}
 	var out struct {
 		Mail              string `json:"mail"`
@@ -433,14 +639,14 @@ func fetchOutlookOwner(ctx context.Context, token string) (*inboxOwner, *errx.Er
 		DisplayName       string `json:"displayName"`
 	}
 	if err := json.Unmarshal(body, &out); err != nil {
-		return nil, errx.ErrEmailOnboardUserInfo
+		return nil, ownerLookupFailed(ctx, "outlook", "decode", resp.StatusCode, body, err)
 	}
 	addr := out.Mail
 	if addr == "" {
 		addr = out.UserPrincipalName
 	}
 	if addr == "" {
-		return nil, errx.ErrEmailOnboardUserInfo
+		return nil, ownerLookupFailed(ctx, "outlook", "no_address", resp.StatusCode, nil, nil)
 	}
 	return &inboxOwner{Email: addr, Name: out.DisplayName}, nil
 }
@@ -0,0 +1,224 @@
+package email
+
+import (
+	"context"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/warmbly/warmbly/internal/models"
+	"golang.org/x/oauth2"
+)
+
+// swapTransport points the package's shared client at h for one test.
+func swapTransport(t *testing.T, h http.Handler) {
+	t.Helper()
+	srv := httptest.NewServer(h)
+	prev := httpClient.Transport
+	httpClient.Transport = rewriteTo(srv.URL)
+	t.Cleanup(func() {
+		httpClient.Transport = prev
+		srv.Close()
+	})
+}
+
+type rewriteTo string
+
+func (r rewriteTo) RoundTrip(req *http.Request) (*http.Response, error) {
+	out := req.Clone(req.Context())
+	target := strings.TrimPrefix(string(r), "http://")
+	out.URL.Scheme = "http"
+	out.URL.Host = target
+	return http.DefaultTransport.RoundTrip(out)
+}
+
+// The failure this exists for: Google answers 403 because the Gmail API was
+// never enabled on the project, and the old code discarded the status and the
+// body, so the operator saw one opaque sentence and error tracking saw
+// nothing at all. The user-facing message stays vague on purpose; what must
+// not be lost is that the call fails and is reported.
+func TestGmailOwnerLookupFailuresAreDistinguished(t *testing.T) {
+	cases := []struct {
+		name   string
+		status int
+		body   string
+	}{
+		{"api not enabled", http.StatusForbidden, `{"error":{"code":403,"message":"Gmail API has not been used in project 1010273043313 before or it is disabled."}}`},
+		{"token revoked", http.StatusUnauthorized, `{"error":{"code":401,"message":"Invalid Credentials"}}`},
+		{"unparseable body", http.StatusOK, `not json at all`},
+		{"no address in payload", http.StatusOK, `{}`},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			swapTransport(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+				w.WriteHeader(tc.status)
+				_, _ = w.Write([]byte(tc.body))
+			}))
+
+			owner, xerr := fetchGmailOwner(context.Background(), "token")
+			if owner != nil {
+				t.Fatalf("expected no owner, got %+v", owner)
+			}
+			if xerr == nil {
+				t.Fatal("expected an error")
+			}
+		})
+	}
+}
+
+// The success path must keep working: a 200 with an address returns it.
+func TestGmailOwnerLookupSucceeds(t *testing.T) {
+	swapTransport(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		_, _ = w.Write([]byte(`{"emailAddress":"someone@example.com"}`))
+	}))
+
+	owner, xerr := fetchGmailOwner(context.Background(), "token")
+	if xerr != nil {
+		t.Fatalf("unexpected error: %v", xerr)
+	}
+	if owner.Email != "someone@example.com" {
+		t.Fatalf("got %q", owner.Email)
+	}
+}
+
+// ownerLookupFailed truncates the provider's body rather than reporting it
+// whole, because a provider error payload has no length contract.
+func TestOwnerLookupTruncatesLongBodies(t *testing.T) {
+	long := strings.Repeat("x", 2000)
+	if xerr := ownerLookupFailed(context.Background(), "gmail", "status", 500, []byte(long), nil); xerr == nil {
+		t.Fatal("expected an error")
+	}
+}
+
+// ── partial consent ──────────────────────────────────────────────────────────
+//
+// Google's consent screen lets the person untick individual permissions and
+// still issues a token. Everything below is about refusing that at connect
+// time, because the alternative is a mailbox that looks connected and then
+// fails on its first send with a provider 403 nobody can trace to a checkbox.
+
+func tokenWithScope(scope string) *oauth2.Token {
+	tok := &oauth2.Token{AccessToken: "at", RefreshToken: "rt"}
+	if scope == "" {
+		return tok
+	}
+	return tok.WithExtra(map[string]any{"scope": scope})
+}
+
+func TestPartialConsentIsRefused(t *testing.T) {
+	want := []string{
+		"https://www.googleapis.com/auth/gmail.readonly",
+		"https://www.googleapis.com/auth/gmail.send",
+	}
+
+	// Send was unticked: the one that breaks campaigns.
+	xerr := checkGrantedScopes(context.Background(), models.InboxProviderGoogle, want,
+		tokenWithScope("https://www.googleapis.com/auth/gmail.readonly"))
+	if xerr == nil {
+		t.Fatal("expected a partly granted consent to be refused")
+	}
+	if !strings.Contains(xerr.Message, "send email on your behalf") {
+		t.Errorf("the error should name the missing permission, got %q", xerr.Message)
+	}
+}
+
+func TestFullConsentIsAccepted(t *testing.T) {
+	want := []string{
+		"https://www.googleapis.com/auth/gmail.readonly",
+		"https://www.googleapis.com/auth/gmail.send",
+	}
+	if xerr := checkGrantedScopes(context.Background(), models.InboxProviderGoogle, want,
+		tokenWithScope("https://www.googleapis.com/auth/gmail.readonly https://www.googleapis.com/auth/gmail.send")); xerr != nil {
+		t.Fatalf("full consent must be accepted, got %v", xerr)
+	}
+}
+
+// gmail.modify confers both reading and sending. Someone who granted the
+// broader permission must not be turned away for lacking the narrower one.
+func TestBroaderScopeSatisfiesNarrowerOnes(t *testing.T) {
+	want := []string{
+		"https://www.googleapis.com/auth/gmail.readonly",
+		"https://www.googleapis.com/auth/gmail.send",
+		"https://www.googleapis.com/auth/gmail.metadata",
+	}
+	if xerr := checkGrantedScopes(context.Background(), models.InboxProviderGoogle, want,
+		tokenWithScope("https://www.googleapis.com/auth/gmail.modify")); xerr != nil {
+		t.Fatalf("gmail.modify should satisfy readonly, send and metadata, got %v", xerr)
+	}
+}
+
+// A provider that returns no scope at all has told us nothing. That is not the
+// same as a denial and must not block a legitimate connection.
+func TestMissingScopeFieldDoesNotBlock(t *testing.T) {
+	want := []string{"https://www.googleapis.com/auth/gmail.send"}
+	if xerr := checkGrantedScopes(context.Background(), models.InboxProviderGoogle, want, tokenWithScope("")); xerr != nil {
+		t.Fatalf("an absent scope field must not be read as a denial, got %v", xerr)
+	}
+}
+
+func TestOutlookPartialConsentIsRefused(t *testing.T) {
+	want := []string{
+		"https://graph.microsoft.com/Mail.Send",
+		"https://graph.microsoft.com/Mail.ReadWrite",
+	}
+	xerr := checkGrantedScopes(context.Background(), models.InboxProviderOutlook, want,
+		tokenWithScope("https://graph.microsoft.com/Mail.Send"))
+	if xerr == nil {
+		t.Fatal("expected a partly granted Outlook consent to be refused")
+	}
+	if !strings.Contains(xerr.Message, "read and write mail") {
+		t.Errorf("got %q", xerr.Message)
+	}
+}
+
+// ── what may be recorded ─────────────────────────────────────────────────────
+
+// A decode failure and a missing address both carry status 200, and that body
+// is a successful profile payload: the mailbox owner's address. It must never
+// reach the log stream or error tracking.
+func TestDiagnosticBodyNeverRecordsASuccessfulPayload(t *testing.T) {
+	profile := []byte(`{"emailAddress":"someone@example.com","messagesTotal":42}`)
+
+	for _, status := range []int{200, 201, 204, 299} {
+		if got := diagnosticBody(status, profile); got != "" {
+			t.Errorf("status %d recorded a success body: %q", status, got)
+		}
+	}
+}
+
+// A failure body is the provider's own error payload and is what makes the
+// failure diagnosable, so it is kept.
+func TestDiagnosticBodyKeepsFailurePayloads(t *testing.T) {
+	body := []byte(`{"error":{"code":403,"message":"Gmail API has not been used in project 1010273043313"}}`)
+	got := diagnosticBody(403, body)
+	if !strings.Contains(got, "has not been used in project") {
+		t.Fatalf("a failure payload should be recorded, got %q", got)
+	}
+}
+
+func TestDiagnosticBodyTruncatesToTheLimit(t *testing.T) {
+	got := diagnosticBody(500, []byte(strings.Repeat("x", 5000)))
+	if len(got) != diagnosticDetailLimit+len("…") {
+		t.Fatalf("got %d bytes, want the %d-byte prefix plus the marker", len(got), diagnosticDetailLimit)
+	}
+	if !strings.HasSuffix(got, "…") {
+		t.Error("a truncated body should say so")
+	}
+}
+
+// The reads are bounded before anything looks at them, so a provider that
+// streams forever cannot be turned into unbounded memory here.
+func TestProviderBodyReadIsBounded(t *testing.T) {
+	swapTransport(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusInternalServerError)
+		huge := strings.Repeat("y", maxProviderBody*4)
+		_, _ = w.Write([]byte(huge))
+	}))
+
+	owner, xerr := fetchGmailOwner(context.Background(), "token")
+	if owner != nil || xerr == nil {
+		t.Fatal("expected the lookup to fail")
+	}
+}