diff --git a/go.mod b/go.mod index 45bd08c1..29aa01c1 100644 --- a/go.mod +++ b/go.mod @@ -52,6 +52,8 @@ require ( require ( cloud.google.com/go/compute/metadata v0.3.0 // indirect + github.com/alicebob/gopher-json v0.0.0-20230218143504-906a9b012302 // indirect + github.com/alicebob/miniredis/v2 v2.32.1 // indirect github.com/andybalholm/brotli v1.1.1 // indirect github.com/cention-sany/utf7 v0.0.0-20170124080048-26cad61bd60a // indirect github.com/cespare/xxhash/v2 v2.3.0 // indirect @@ -78,6 +80,7 @@ require ( github.com/ssor/bom v0.0.0-20170718123548-6386211fdfcf // indirect github.com/stretchr/objx v0.5.2 // indirect github.com/valyala/bytebufferpool v1.0.0 // indirect + github.com/yuin/gopher-lua v1.1.1 // indirect golang.org/x/image v0.41.0 // indirect golang.org/x/sys v0.45.0 // indirect golang.org/x/text v0.37.0 // indirect diff --git a/go.sum b/go.sum index 1dd60d55..154b47cc 100644 --- a/go.sum +++ b/go.sum @@ -4,6 +4,7 @@ filippo.io/edwards25519 v1.1.0 h1:FNf4tywRC1HmFuKW5xopWpigGjJKiJSV0Cqo0cJWDaA= filippo.io/edwards25519 v1.1.0/go.mod h1:BxyFTGdWcka3PhytdK4V28tE5sGfRvvvRV7EaN4VDT4= github.com/abhinavxd/ssrfguard v0.1.0 h1:Ns/llAQ63uGFehxSvhCd+WGDKmBEEmIH+E1AW1CGgWM= github.com/abhinavxd/ssrfguard v0.1.0/go.mod h1:eNVubb+m/r3KrKWYdG6hxzeAfj+t2ZmZss4V/x7D6Ws= +github.com/alicebob/gopher-json v0.0.0-20200520072559-a9ecdc9d1d3a/go.mod h1:SGnFV6hVsYE877CKEZ6tDNTjaSXYUk6QqoIK6PrAtcc= github.com/alicebob/gopher-json v0.0.0-20230218143504-906a9b012302 h1:uvdUDbHQHO85qeSydJtItA4T55Pw6BtAejd0APRJOCE= github.com/alicebob/gopher-json v0.0.0-20230218143504-906a9b012302/go.mod h1:SGnFV6hVsYE877CKEZ6tDNTjaSXYUk6QqoIK6PrAtcc= github.com/alicebob/miniredis/v2 v2.32.1 h1:Bz7CciDnYSaa0mX5xODh6GUITRSx+cVhjNoOR4JssBo= @@ -20,6 +21,9 @@ github.com/cention-sany/utf7 v0.0.0-20170124080048-26cad61bd60a h1:MISbI8sU/PSK/ github.com/cention-sany/utf7 v0.0.0-20170124080048-26cad61bd60a/go.mod h1:2GxOXOlEPAMFPfp014mK1SWq8G8BN8o7/dfYqJrVGn8= github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs= github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs= +github.com/chzyer/logex v1.1.10/go.mod h1:+Ywpsq7O8HXn0nuIou7OrIPyXbp3wmkHB+jjWRnGsAI= +github.com/chzyer/readline v0.0.0-20180603132655-2972be24d48e/go.mod h1:nSuG5e5PlCu98SY8svDHJxuZscDgtXS6KTTbou5AhLI= +github.com/chzyer/test v0.0.0-20180213035817-a1ea475d72b1/go.mod h1:Q3SI9o4m/ZMnBNeIyt5eFwwo7qiLfzFZmjNmxjkiQlU= github.com/coreos/go-oidc/v3 v3.11.0 h1:Ia3MxdwpSw702YW0xgfmP1GVCMA9aEFWu12XUZ3/OtI= github.com/coreos/go-oidc/v3 v3.11.0/go.mod h1:gE3LgjOgFoHi9a4ce4/tJczr0Ai2/BoDhf0r5lltWI0= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= @@ -216,6 +220,7 @@ golang.org/x/oauth2 v0.27.0/go.mod h1:onh5ek6nERTohokkhCD/y2cV4Do3fxFHFuAejCkRWT golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20220722155255-886fb9371eb4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.1.0/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= +golang.org/x/sys v0.0.0-20190204203706-41f3e6584952/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20210423082822-04245dca01da/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= diff --git a/internal/aiagent/otp.go b/internal/aiagent/otp.go index a208218a..34f196b0 100644 --- a/internal/aiagent/otp.go +++ b/internal/aiagent/otp.go @@ -27,8 +27,8 @@ const ( otpMaxConvSends = 6 ) -// checkOTPScript matches the pending code and sets the verified flag on match, all in one step. -// Returns 1 on match, 0 on miss/expiry, -1 on corrupt data (key cleared). +// checkOTPScript matches the pending code and, on match, stores the attested email as the verified +// value, all in one step. Returns 1 on match, 0 on miss/expiry, -1 on corrupt data (key cleared). var checkOTPScript = redis.NewScript(` local raw = redis.call('GET', KEYS[1]) if not raw then @@ -39,9 +39,13 @@ if not ok or type(p) ~= 'table' then redis.call('DEL', KEYS[1]) return -1 end +if type(p.email) ~= 'string' or p.email == '' then + redis.call('DEL', KEYS[1]) + return 0 +end if p.code == ARGV[1] then redis.call('DEL', KEYS[1]) - redis.call('SET', KEYS[2], '1', 'EX', ARGV[3]) + redis.call('SET', KEYS[2], p.email, 'EX', ARGV[3]) return 1 end p.attempts = (p.attempts or 0) + 1 @@ -65,20 +69,32 @@ return n // pendingOTP is the JSON stored at otpPendingKeyPrefix while a code awaits entry. type pendingOTP struct { Code string `json:"code"` + Email string `json:"email"` Attempts int `json:"attempts"` } func otpPendingKey(convUUID string) string { return otpPendingKeyPrefix + convUUID } func otpVerifiedKey(convUUID string) string { return otpVerifiedKeyPrefix + convUUID } +func normalizeOTPEmail(email string) string { + return strings.ToLower(strings.TrimSpace(email)) +} + // otpSendsKey scopes the send budget to one address so correcting a mistyped email gets a fresh one. func otpSendsKey(convUUID, email string) string { - return otpSendsKeyPrefix + convUUID + ":" + strings.ToLower(strings.TrimSpace(email)) + return otpSendsKeyPrefix + convUUID + ":" + normalizeOTPEmail(email) } func otpConvSendsKey(convUUID string) string { return otpSendsKeyPrefix + convUUID } -func (m *Manager) isConversationVerified(convUUID string) bool { +// isConversationVerified holds the invariant "verified == the contact's current email is the one +// proven by OTP": the verified value stores the attested address, and a contact email rebound +// after verification (from this or any sibling conversation) no longer matches. +func (m *Manager) isConversationVerified(convUUID, contactEmail string) bool { + email := normalizeOTPEmail(contactEmail) + if email == "" { + return false + } ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) defer cancel() v, err := m.redis.Get(ctx, otpVerifiedKey(convUUID)).Result() @@ -88,7 +104,7 @@ func (m *Manager) isConversationVerified(convUUID string) bool { } return false } - return v == "1" + return v == email } // clearConversationVerified drops the verified flag and any pending code so a changed email must be @@ -121,10 +137,10 @@ func (m *Manager) incrOTPSends(convUUID, email string) error { return incrOTPSendsScript.Run(ctx, m.redis, []string{otpConvSendsKey(convUUID)}, ttl).Err() } -func (m *Manager) storePendingOTP(convUUID, code string) error { +func (m *Manager) storePendingOTP(convUUID, code, email string) error { ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) defer cancel() - b, err := json.Marshal(pendingOTP{Code: code}) + b, err := json.Marshal(pendingOTP{Code: code, Email: normalizeOTPEmail(email)}) if err != nil { return err } diff --git a/internal/aiagent/otp_test.go b/internal/aiagent/otp_test.go new file mode 100644 index 00000000..a9dd97e8 --- /dev/null +++ b/internal/aiagent/otp_test.go @@ -0,0 +1,236 @@ +package aiagent + +import ( + "testing" + "time" + + "github.com/alicebob/miniredis/v2" + "github.com/redis/go-redis/v9" + "github.com/zerodha/logf" +) + +func newOTPTestManager(t *testing.T) (*Manager, *miniredis.Miniredis) { + t.Helper() + mr := miniredis.RunT(t) + lo := logf.New(logf.Opts{}) + return &Manager{ + lo: &lo, + redis: redis.NewClient(&redis.Options{Addr: mr.Addr()}), + }, mr +} + +func TestIsConversationVerified(t *testing.T) { + m, mr := newOTPTestManager(t) + const conv = "conv-a" + + if m.isConversationVerified(conv, "john@example.com") { + t.Error("no verified key must mean unverified") + } + + mr.Set(otpVerifiedKey(conv), "john@example.com") + if !m.isConversationVerified(conv, "john@example.com") { + t.Error("matching email must be verified") + } + if !m.isConversationVerified(conv, " John@Example.COM ") { + t.Error("email comparison must be case- and whitespace-insensitive") + } + + if m.isConversationVerified(conv, "victim@example.com") { + t.Error("a rebound contact email must not stay verified") + } + if m.isConversationVerified(conv, "") { + t.Error("empty contact email must be unverified") + } + if m.isConversationVerified("conv-b", "john@example.com") { + t.Error("another conversation must not inherit the verified flag") + } + + // Value written by a build that stored a bare flag instead of the email. + mr.Set(otpVerifiedKey("conv-legacy"), "1") + if m.isConversationVerified("conv-legacy", "john@example.com") { + t.Error("legacy flag value must be unverified") + } +} + +func TestCheckPendingOTP(t *testing.T) { + m, mr := newOTPTestManager(t) + const conv, email, code = "conv-a", "john@example.com", "123456" + + ok, err := m.checkPendingOTP(conv, code) + if err != nil || ok { + t.Errorf("check without a pending code = (%v, %v), want (false, nil)", ok, err) + } + + if err := m.storePendingOTP(conv, code, " John@Example.COM "); err != nil { + t.Fatal(err) + } + ok, err = m.checkPendingOTP(conv, "999999") + if err != nil || ok { + t.Errorf("wrong code = (%v, %v), want (false, nil)", ok, err) + } + ok, err = m.checkPendingOTP(conv, code) + if err != nil || !ok { + t.Fatalf("correct code = (%v, %v), want (true, nil)", ok, err) + } + if got, _ := mr.Get(otpVerifiedKey(conv)); got != email { + t.Errorf("verified value = %q, want the normalized attested email %q", got, email) + } + if mr.Exists(otpPendingKey(conv)) { + t.Error("pending key must be deleted on match") + } + if !m.isConversationVerified(conv, email) { + t.Error("conversation must be verified after a correct code") + } + + ok, err = m.checkPendingOTP(conv, code) + if err != nil || ok { + t.Errorf("code replay = (%v, %v), want (false, nil)", ok, err) + } +} + +func TestCheckPendingOTPAttemptCap(t *testing.T) { + m, mr := newOTPTestManager(t) + const conv, code = "conv-a", "123456" + + if err := m.storePendingOTP(conv, code, "john@example.com"); err != nil { + t.Fatal(err) + } + for i := 0; i < otpMaxAttempts; i++ { + if ok, _ := m.checkPendingOTP(conv, "000000"); ok { + t.Fatal("wrong code must not verify") + } + } + if mr.Exists(otpPendingKey(conv)) { + t.Error("pending key must be deleted at the attempt cap") + } + if ok, _ := m.checkPendingOTP(conv, code); ok { + t.Error("correct code must not verify after the attempt cap") + } +} + +func TestCheckPendingOTPLegacyAndCorruptPayloads(t *testing.T) { + m, mr := newOTPTestManager(t) + const conv = "conv-a" + + // Pending record from a build that stored no attested email. + mr.Set(otpPendingKey(conv), `{"code":"123456"}`) + ok, err := m.checkPendingOTP(conv, "123456") + if err != nil || ok { + t.Errorf("legacy pending without email = (%v, %v), want (false, nil)", ok, err) + } + if mr.Exists(otpPendingKey(conv)) { + t.Error("legacy pending record must be deleted") + } + if mr.Exists(otpVerifiedKey(conv)) { + t.Error("legacy pending record must never set the verified key") + } + + mr.Set(otpPendingKey(conv), "not-json") + if ok, err := m.checkPendingOTP(conv, "123456"); err == nil || ok { + t.Errorf("corrupt pending = (%v, %v), want (false, error)", ok, err) + } + if mr.Exists(otpPendingKey(conv)) { + t.Error("corrupt pending record must be deleted") + } +} + +func TestClearConversationVerified(t *testing.T) { + m, mr := newOTPTestManager(t) + const conv, email = "conv-a", "john@example.com" + + mr.Set(otpVerifiedKey(conv), email) + if err := m.storePendingOTP(conv, "123456", email); err != nil { + t.Fatal(err) + } + if err := m.clearConversationVerified(conv); err != nil { + t.Fatal(err) + } + if m.isConversationVerified(conv, email) { + t.Error("conversation must be unverified after clear") + } + if mr.Exists(otpPendingKey(conv)) { + t.Error("pending key must be deleted on clear") + } +} + +// The cross-conversation rebind: verified on conversation A must not survive the shared +// contact's email being rewritten from a sibling conversation. +func TestVerifiedDoesNotSurviveEmailRebind(t *testing.T) { + m, _ := newOTPTestManager(t) + const convA, attacker, victim = "conv-a", "attacker@evil.com", "victim@example.com" + + if err := m.storePendingOTP(convA, "123456", attacker); err != nil { + t.Fatal(err) + } + if ok, _ := m.checkPendingOTP(convA, "123456"); !ok { + t.Fatal("verification must succeed for the attested email") + } + if !m.isConversationVerified(convA, attacker) { + t.Fatal("conversation A must be verified for the attested email") + } + + // Sibling conversation B rewrites the shared contact row; A's Redis keys are untouched. + if m.isConversationVerified(convA, victim) { + t.Error("conversation A must not stay verified once the contact email is rebound") + } +} + +func TestVerifiedTTL(t *testing.T) { + m, mr := newOTPTestManager(t) + const conv, email = "conv-a", "john@example.com" + + if err := m.storePendingOTP(conv, "123456", email); err != nil { + t.Fatal(err) + } + if got := mr.TTL(otpPendingKey(conv)); got != otpPendingTTL { + t.Errorf("pending TTL = %v, want %v", got, otpPendingTTL) + } + if ok, _ := m.checkPendingOTP(conv, "123456"); !ok { + t.Fatal("verification must succeed") + } + if got := mr.TTL(otpVerifiedKey(conv)); got != otpVerifiedTTL { + t.Errorf("verified TTL = %v, want %v", got, otpVerifiedTTL) + } + mr.FastForward(otpVerifiedTTL + time.Second) + if m.isConversationVerified(conv, email) { + t.Error("conversation must be unverified after the verified TTL") + } +} + +func TestWrongAttemptKeepsPendingTTL(t *testing.T) { + m, mr := newOTPTestManager(t) + const conv = "conv-a" + + if err := m.storePendingOTP(conv, "123456", "john@example.com"); err != nil { + t.Fatal(err) + } + mr.FastForward(otpPendingTTL / 2) + if ok, _ := m.checkPendingOTP(conv, "000000"); ok { + t.Fatal("wrong code must not verify") + } + if got := mr.TTL(otpPendingKey(conv)); got != otpPendingTTL/2 { + t.Errorf("pending TTL after a wrong attempt = %v, want %v", got, otpPendingTTL/2) + } +} + +// Guards against the redis client failing open on connection errors. +func TestVerifiedFailsClosedOnRedisError(t *testing.T) { + m, mr := newOTPTestManager(t) + mr.Close() + if m.isConversationVerified("conv-a", "john@example.com") { + t.Error("a redis error must mean unverified") + } +} + +func TestNormalizeOTPEmail(t *testing.T) { + cases := map[string]string{ + " John@Example.COM ": "john@example.com", + "": "", + "a@b.c": "a@b.c", + } + for in, want := range cases { + if got := normalizeOTPEmail(in); got != want { + t.Errorf("normalizeOTPEmail(%q) = %q, want %q", in, got, want) + } + } +} diff --git a/internal/aiagent/tools.go b/internal/aiagent/tools.go index bc790b6b..c5125c1e 100644 --- a/internal/aiagent/tools.go +++ b/internal/aiagent/tools.go @@ -248,7 +248,7 @@ func (t *sendEmailVerificationTool) Execute(ctx context.Context, args string) (s if err != nil { return "", err } - if err := t.m.storePendingOTP(t.conv.UUID, code); err != nil { + if err := t.m.storePendingOTP(t.conv.UUID, code, email); err != nil { return "", err } body := t.m.i18n.Ts("ai.agent.verificationEmailBody", "code", code) diff --git a/internal/aiagent/worker.go b/internal/aiagent/worker.go index 6a58dbfb..2348fbc2 100644 --- a/internal/aiagent/worker.go +++ b/internal/aiagent/worker.go @@ -279,7 +279,7 @@ func (m *Manager) handle(ctx context.Context, convID int) { if conv.InboxChannel != channelEmail && conv.Contact.Type == umodels.UserTypeContact { return true } - return m.isConversationVerified(conv.UUID) + return m.isConversationVerified(conv.UUID, conv.Contact.Email.String) } // Snapshot for the run-start registration decisions (one Redis read); tctx still gets the live // closure so mid-turn verification is picked up per tool call.