diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ea0e5f..4be7b8f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,17 @@ APIs change between minors without a deprecation period. this version refuses those weights until its control plane can say `allow_pickle`. +### Fixed + +- **A reused answer could carry another caller's personal data.** With + `restore: true` and the answer cache on, two callers' requests that + redacted to the same text shared one answer — with the first caller's + address put back in it. Such requests are now never answered from the + cache nor kept in it. +- **Similar questions about different pictures got each other's answer.** + A question with a picture, a sound or a file in it is reused only when it + is the same request. + ### Added - **And on a web page, `/admin/mcp`.** For administrators: the same list, diff --git a/docs/install.md b/docs/install.md index 0a2e59d..94aa1f0 100644 --- a/docs/install.md +++ b/docs/install.md @@ -2155,8 +2155,10 @@ llmgw keys cache --project Acme/ml/train --off - **Policies still apply:** the limits are checked and the guardrails judge the request before the cache is looked in, and the output policy judges a reused answer as it would a new one. A request continuing a stored - conversation, or under a `no_store` policy, is never answered from the - cache nor kept in it. + conversation, under a `no_store` policy, or whose redacted values are put + back in its answer (`restore: true`) is never answered from the cache nor + kept in it: redacted, two callers' requests read the same, and the answer + has the first caller's values in it. - **What a hit costs:** nothing. It is recorded as a request — status 200, the model, the key, the rate limit's count — with no tokens and no cost, marked as a cache hit (`llmgw ctl request ` says so), so it spends no @@ -2196,8 +2198,8 @@ router: `router.cache.embedding_model`, against the questions already answered in the same conversation — the same system prompt, the same earlier turns, tools, `response_format` and sampling settings, the same model and - project. A like question in another conversation, or after a tool's - result, is not taken for this one. The exact cache is asked first, and an + project. A like question in another conversation, after a tool's result, + or with a picture, a sound or a file in it, is not taken for this one. The exact cache is asked first, and an exact hit embeds nothing. - **How alike:** the cosine similarity of the two embeddings, at least the project's `--similar`. 0.95 is strict; below 0.9 different questions start diff --git a/internal/router/cache.go b/internal/router/cache.go index 86ede02..424b483 100644 --- a/internal/router/cache.go +++ b/internal/router/cache.go @@ -313,6 +313,12 @@ func reusable(req *parsedRequest, anyAnswer bool) bool { if req.knowledge || req.Header.Get(knowledgeHeader) != "" { return false } + // An answer with the caller's own values put back in it (restore: true) + // is theirs alone: another caller's request, redacted, reads the same, + // and would be given the first caller's address. + if len(req.guardIn.Restore) > 0 { + return false + } if v := strings.ToLower(strings.TrimSpace(req.Header.Get(cacheHeader))); v == "no" || v == "off" || v == "bypass" { return false } diff --git a/internal/router/restore_test.go b/internal/router/restore_test.go index 6d3cc49..b48b5c8 100644 --- a/internal/router/restore_test.go +++ b/internal/router/restore_test.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "regexp" "strings" "testing" @@ -108,3 +109,42 @@ func TestARestorerHoldsOnlyWhatCouldBeAPlaceholder(t *testing.T) { t.Errorf("OnEnd: %v", out) } } + +// anyEmailGuard redacts whichever address a request has to [EMAIL_1], and +// asks for it back: two callers' requests then read the same. +type anyEmailGuard struct{} + +var anEmail = regexp.MustCompile(`[a-z]+@example\.com`) + +func (anyEmailGuard) Input(_ context.Context, _ guard.Subject, body []byte) (guard.Verdict, error) { + addr := anEmail.Find(body) + if addr == nil { + return guard.Verdict{}, nil + } + return guard.Verdict{Action: guard.Redact, Policy: "pii", Category: "email", + Body: anEmail.ReplaceAll(body, []byte("[EMAIL_1]")), + Restore: map[string]string{"[EMAIL_1]": string(addr)}}, nil +} +func (anyEmailGuard) Output(context.Context, guard.Subject) guard.OutputFilter { return nil } +func (anyEmailGuard) Directives(guard.Subject) guard.Directives { return guard.Directives{} } + +func TestAnAnswerWithACallersValuesPutBackIsNeverReusedForAnother(t *testing.T) { + // Redacted, alice's request and bob's read the same; the answer alice + // got has her address in it again, and must not be bob's. + srv := echoEngine(t) + rt, secret := cachingRouter(t, srv.URL, false) + rt.SetGuard(anyEmailGuard{}) + hdr := map[string]string{"Authorization": "Bearer " + secret} + + first := post(t, rt.Handler(), ask(0, "write to alice@example.com"), hdr) + second := post(t, rt.Handler(), ask(0, "write to bob@example.com"), hdr) + if strings.Contains(second.Body.String(), "alice") { + t.Fatalf("bob was given alice's answer: %s", second.Body.String()) + } + if !strings.Contains(second.Body.String(), "bob@example.com") { + t.Errorf("bob's answer lacks his own address: %s", second.Body.String()) + } + if got := first.Header().Get(cacheHeader) + "/" + second.Header().Get(cacheHeader); got != "bypass/bypass" { + t.Errorf("x-llmgw-cache %s, want bypass for both", got) + } +} diff --git a/internal/router/similar.go b/internal/router/similar.go index f2afc64..f06676e 100644 --- a/internal/router/similar.go +++ b/internal/router/similar.go @@ -73,6 +73,16 @@ func similarGroup(project, path, model string, body []byte) (group, question str if last == nil || last["role"] != "user" { return "", "", false } + // A picture, a sound or a file is part of the question, and the text + // beside it is not all of it: "what is in this picture?" asked of two + // pictures is two questions. Reused only when the same, exactly. + if parts, isList := last["content"].([]any); isList { + for _, p := range parts { + if part, _ := p.(map[string]any); part == nil || part["type"] != "text" { + return "", "", false + } + } + } question = lastUserText(body) last["content"] = "" group, ok = keyOf(project, path+"#similar", model, m) diff --git a/internal/router/similar_test.go b/internal/router/similar_test.go index fe29f1a..be1c8ec 100644 --- a/internal/router/similar_test.go +++ b/internal/router/similar_test.go @@ -148,3 +148,17 @@ func TestTheConversationAQuestionIsAskedInIsAllButItsLastMessage(t *testing.T) { t.Error("a tool's result is taken for a question") } } + +func TestAQuestionAboutAPictureIsNotTakenForAnotherPicture(t *testing.T) { + pic := func(url string) []byte { + b, _ := json.Marshal(map[string]any{"model": "m", "temperature": 0, "messages": []any{map[string]any{ + "role": "user", "content": []any{ + map[string]any{"type": "text", "text": "what is in this picture?"}, + map[string]any{"type": "image_url", "image_url": map[string]any{"url": url}}, + }}}}) + return b + } + if _, _, ok := similarGroup("p", "/v1/chat/completions", "m", pic("data:image/png;base64,AAAA")); ok { + t.Error("a question with a picture in it is compared by its text alone") + } +}