From 1eafe812ec701dae0f0c5ee953f8bcc2543844f1 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Fri, 9 Oct 2026 18:07:21 -0400 Subject: [PATCH 1/2] Withhold unsafe recipe sources from idea job results Cook from what I have and Quick dinners ideas returned the grounded recipe's raw source_url and site_name, so a link carrying a token in its query or userinfo, or a stored name taken from its host, reached the client in the job response (BASE-01). dinners.SourceOf is now the one way a fetched recipe's source reaches a client: the dinner detail and both idea result builders use it. An idea's source_url and site_name are null when they could leak a secret, and a source that does not parse fails the job instead of being shown. Task: c15ed4b5-25ae-43c6-8d36-f4a6dd51e5af Co-Authored-By: Claude Opus 5.5 --- internal/dinners/detail.go | 17 ++- internal/ideas/ideas.go | 73 +++++++++---- internal/ideas/quick_test.go | 2 +- internal/server/ideasource_test.go | 124 ++++++++++++++++++++++ web/src/ideas/CookFromPantryPage.test.tsx | 4 +- web/src/ideas/QuickDinnersPage.test.tsx | 4 +- 6 files changed, 196 insertions(+), 28 deletions(-) create mode 100644 internal/server/ideasource_test.go diff --git a/internal/dinners/detail.go b/internal/dinners/detail.go index dfaf779..0eb1f06 100644 --- a/internal/dinners/detail.go +++ b/internal/dinners/detail.go @@ -147,11 +147,11 @@ func readRecipe(ctx context.Context, q *db.Queries, recipeID string, d *Detail) return fmt.Errorf("recipe %s: %w", recipeID, err) } if !IsManualRecipe(rec.SourceUrl) { - site, err := pageSiteName(rec.SiteName, rec.SourceUrl) + src, err := SourceOf(rec.SiteName, rec.SourceUrl) if err != nil { return err } - d.Source = &Source{SiteName: site, URL: safeSourceURL(rec.SourceUrl)} + d.Source = &src } ings, err := q.ListRecipeIngredients(ctx, recipeID) if err != nil { @@ -182,6 +182,19 @@ func readRecipe(ctx context.Context, q *db.Queries, recipeID string, d *Detail) return nil } +// SourceOf is the Source of a recipe fetched from sourceURL whose stored +// site name is siteName: the name only when the page gave it (see +// pageSiteName) and the link only when it is free of secrets (see +// safeSourceURL). It is the one way a fetched recipe's source reaches a +// client. +func SourceOf(siteName sql.NullString, sourceURL string) (Source, error) { + site, err := pageSiteName(siteName, sourceURL) + if err != nil { + return Source{}, err + } + return Source{SiteName: site, URL: safeSourceURL(sourceURL)}, nil +} + // pageSiteName is the stored site name of a recipe fetched from sourceURL // when it is one the page gave, else nil. A page that names no site is // stored under recipes.HostSiteName of the URL it was served from: the diff --git a/internal/ideas/ideas.go b/internal/ideas/ideas.go index 41d1dc2..c95524c 100644 --- a/internal/ideas/ideas.go +++ b/internal/ideas/ideas.go @@ -11,6 +11,7 @@ package ideas import ( "context" + "database/sql" "encoding/json" "errors" "fmt" @@ -197,11 +198,13 @@ type Idea struct { // Difficulty is easy, medium or hard. Difficulty string `json:"difficulty"` // Quick says whether the idea is a Quick dinner (see IsQuick). - Quick bool `json:"quick"` - RecipeID string `json:"recipe_id"` - PhotoURL string `json:"photo_url"` - SourceURL string `json:"source_url"` - SiteName string `json:"site_name"` + Quick bool `json:"quick"` + RecipeID string `json:"recipe_id"` + PhotoURL string `json:"photo_url"` + // SourceURL and SiteName are the grounded recipe's source as + // dinners.SourceOf gives it: each null when it could leak a secret. + SourceURL *string `json:"source_url"` + SiteName *string `json:"site_name"` } // Result is the KindFromIngredients result, published as the job goes: @@ -312,9 +315,9 @@ func (s *Service) fill(ctx context.Context, p FromIngredientsJob, publish func(R for i, idea := range out.Ideas { reqs[i] = recipes.GroundRequest{DishName: idea.Name, Query: idea.SearchQuery} } - err = keepGrounded(ctx, s.grounder, reqs, skills.IdeasKept, &res.Ideas, &res.Dropped, func(i int, gr recipes.Grounded) (Idea, bool) { - idea := ideaOf(out.Ideas[i], gr) - return idea, !p.QuickOnly || idea.Quick + err = keepGrounded(ctx, s.grounder, reqs, skills.IdeasKept, &res.Ideas, &res.Dropped, func(i int, gr recipes.Grounded) (Idea, bool, error) { + idea, err := ideaOf(out.Ideas[i], gr) + return idea, !p.QuickOnly || idea.Quick, err }, func() error { return publish(res) }) if err != nil { return res, err @@ -333,9 +336,9 @@ func (s *Service) fill(ctx context.Context, p FromIngredientsJob, publish func(R // order (an idea that grounds sooner than one before it is shown first, // and the earlier one is put back in its place when it grounds) and // publish is called. An idea no recipe grounds, or that keep refuses, -// counts in *dropped. Any other grounding failure, or publish failing, is -// returned (see ground). -func keepGrounded[T any](ctx context.Context, g *recipes.Grounder, reqs []recipes.GroundRequest, want int, shown *[]T, dropped *int, keep func(i int, gr recipes.Grounded) (T, bool), publish func() error) error { +// counts in *dropped. Any other grounding failure, or keep or publish +// failing, is returned (see ground). +func keepGrounded[T any](ctx context.Context, g *recipes.Grounder, reqs []recipes.GroundRequest, want int, shown *[]T, dropped *int, keep func(i int, gr recipes.Grounded) (T, bool, error), publish func() error) error { // order[k] is the index in reqs of (*shown)[k], so each idea is put in // its place in reqs' order as it grounds. var order []int @@ -348,7 +351,10 @@ func keepGrounded[T any](ctx context.Context, g *recipes.Grounder, reqs []recipe *dropped++ return nil } - idea, ok := keep(first+i, *gr) + idea, ok, err := keep(first+i, *gr) + if err != nil { + return err + } if !ok { *dropped++ return nil @@ -418,7 +424,11 @@ func withoutIdeas[R shownResult[R]](stored json.RawMessage) (json.RawMessage, er } // ideaOf is idea as grounded by gr. -func ideaOf(idea skills.Idea, gr recipes.Grounded) Idea { +func ideaOf(idea skills.Idea, gr recipes.Grounded) (Idea, error) { + src, err := sourceOf(gr) + if err != nil { + return Idea{}, err + } return Idea{ Name: idea.Name, Uses: idea.Uses, @@ -430,9 +440,20 @@ func ideaOf(idea skills.Idea, gr recipes.Grounded) Idea { Quick: IsQuick(gr.Method, gr.MinutesTotal), RecipeID: gr.RecipeID, PhotoURL: dinners.PhotoURL(gr.PhotoID), - SourceURL: gr.SourceURL, - SiteName: gr.SiteName, + SourceURL: src.URL, + SiteName: src.SiteName, + }, nil +} + +// sourceOf is gr's recipe source as a client may see it (see +// dinners.SourceOf): a grounded recipe's site name is stored only when +// not empty. +func sourceOf(gr recipes.Grounded) (dinners.Source, error) { + src, err := dinners.SourceOf(sql.NullString{String: gr.SiteName, Valid: gr.SiteName != ""}, gr.SourceURL) + if err != nil { + return dinners.Source{}, fmt.Errorf("ideas: recipe %s: %w", gr.RecipeID, err) } + return src, nil } // QuickJob is the KindQuick payload: the kind of Quick dinner wanted, @@ -478,8 +499,9 @@ type QuickIdea struct { Difficulty string `json:"difficulty"` RecipeID string `json:"recipe_id"` PhotoURL string `json:"photo_url"` - SourceURL string `json:"source_url"` - SiteName string `json:"site_name"` + // SourceURL and SiteName are as in Idea. + SourceURL *string `json:"source_url"` + SiteName *string `json:"site_name"` } // QuickResult is the KindQuick result, published as the job goes: its @@ -564,8 +586,9 @@ func (s *Service) fillQuick(ctx context.Context, p QuickJob, publish func(QuickR for i, idea := range out.Ideas { reqs[i] = recipes.GroundRequest{DishName: idea.Name, Query: idea.SearchQuery} } - err = keepGrounded(ctx, s.grounder, reqs, skills.QuickIdeasKept, &res.Ideas, &res.Dropped, func(i int, gr recipes.Grounded) (QuickIdea, bool) { - return quickIdeaOf(out.Ideas[i], gr), OfQuickKind(p.Kind, gr.Method, gr.MinutesTotal) + err = keepGrounded(ctx, s.grounder, reqs, skills.QuickIdeasKept, &res.Ideas, &res.Dropped, func(i int, gr recipes.Grounded) (QuickIdea, bool, error) { + idea, err := quickIdeaOf(out.Ideas[i], gr) + return idea, OfQuickKind(p.Kind, gr.Method, gr.MinutesTotal), err }, func() error { return publish(res) }) if err != nil { return res, err @@ -578,7 +601,11 @@ func (s *Service) fillQuick(ctx context.Context, p QuickJob, publish func(QuickR } // quickIdeaOf is idea as grounded by gr. -func quickIdeaOf(idea skills.QuickIdea, gr recipes.Grounded) QuickIdea { +func quickIdeaOf(idea skills.QuickIdea, gr recipes.Grounded) (QuickIdea, error) { + src, err := sourceOf(gr) + if err != nil { + return QuickIdea{}, err + } return QuickIdea{ Name: idea.Name, Method: gr.Method, @@ -586,9 +613,9 @@ func quickIdeaOf(idea skills.QuickIdea, gr recipes.Grounded) QuickIdea { Difficulty: gr.Difficulty, RecipeID: gr.RecipeID, PhotoURL: dinners.PhotoURL(gr.PhotoID), - SourceURL: gr.SourceURL, - SiteName: gr.SiteName, - } + SourceURL: src.URL, + SiteName: src.SiteName, + }, nil } // Origins of a dinner made from an idea (the dinners.origin column). diff --git a/internal/ideas/quick_test.go b/internal/ideas/quick_test.go index 04bc7d2..d7acbbc 100644 --- a/internal/ideas/quick_test.go +++ b/internal/ideas/quick_test.go @@ -369,7 +369,7 @@ func TestStartQuickRunsTheJob(t *testing.T) { if res.Stage != StageDone || res.Kind != skills.QuickCrockpot || !reflect.DeepEqual(res.Exclude, []string{"Beef Stew"}) || len(res.Ideas) != 1 { t.Fatalf("result = %+v", res) } - if got := res.Ideas[0]; got.Name != "Pulled Pork" || got.Method != "crockpot" || got.SiteName != "R Kitchen" { + if got := res.Ideas[0]; got.Name != "Pulled Pork" || got.Method != "crockpot" || got.SiteName == nil || *got.SiteName != "R Kitchen" { t.Errorf("idea = %+v", got) } } diff --git a/internal/server/ideasource_test.go b/internal/server/ideasource_test.go new file mode 100644 index 0000000..a858bca --- /dev/null +++ b/internal/server/ideasource_test.go @@ -0,0 +1,124 @@ +package server + +import ( + "encoding/json" + "fmt" + "net/http" + "reflect" + "strings" + "testing" + + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ai/llm" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/recipes" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/recipes/recipestest" +) + +// sourceCase is a recipe page an idea grounds in: its link and the site +// name its page gives (empty for none, so the host is stored instead). +type sourceCase struct { + dish, link, site string + // want is the idea's source_url and site_name; nil is null. + wantURL, wantSite any +} + +const ( + cleanSourceLink = "https://kitchen.example/recipes/chicken-curry" + queryToken = "query-token-abc123" + queryHost = "query-host-q1.example" + userinfoToken = "user-token-def456" + userinfoHost = "userinfo-host-u2.example" +) + +// sourceCases are a clean link, credited by its page's name, and links +// carrying a token in the query and the userinfo whose pages name no site, +// so the stored name is the link's host. +var sourceCases = []sourceCase{ + {"Chicken Curry", cleanSourceLink, "Curry House", cleanSourceLink, "Curry House"}, + {"Beef Stew", "https://" + queryHost + "/stew?access_token=" + queryToken, "", nil, nil}, + {"Bean Chili", "https://" + userinfoToken + "@" + userinfoHost + "/chili", "", nil, nil}, +} + +// sourceSecrets are what no idea job response may contain. +var sourceSecrets = []string{queryToken, "access_token", queryHost, userinfoToken, userinfoHost} + +// sourceFakes makes the search for each case's dish find its page and +// scripts the normalize_recipe reply grounding it takes. +func sourceFakes(t *testing.T, minutes int) (*llm.Fake, *llm.Fake, *recipestest.Search, *recipestest.Pages, *recipestest.Images) { + t.Helper() + model, normalize, search, pages, images := newQuickFakes() + for i, c := range sourceCases { + photo := fmt.Sprintf("https://img.example/%d.jpg", i) + search.Results[c.dish] = []recipes.Candidate{{URL: c.link}} + pages.Bodies[c.link] = recipestest.RecipePage(c.dish, c.site, photo, minutes) + images.JPEGs[photo] = recipestest.JPEG(t, 400, 320, uint8(i+1)) + normalize.QueueGenerate(recipestest.NormalizedReply("stovetop", minutes)) + } + return model, normalize, search, pages, images +} + +// checkIdeaSources checks the finished job id: each idea shows its case's +// safe source, and the response holds nothing of a token-bearing link. +func checkIdeaSources(t *testing.T, h http.Handler, id string) { + t.Helper() + job := waitJob(t, h, id) + if job["status"] != "succeeded" { + t.Fatalf("GET job = %v", job) + } + rec, _ := callDinners(t, h, http.MethodGet, "/api/v1/jobs/"+id, "") + for _, secret := range sourceSecrets { + if strings.Contains(rec.Body.String(), secret) { + t.Errorf("job response contains %q: %s", secret, rec.Body) + } + } + list := job["result"].(map[string]any)["ideas"].([]any) + if len(list) != len(sourceCases) { + t.Fatalf("ideas = %v, want %d", list, len(sourceCases)) + } + for i, c := range sourceCases { + idea := list[i].(map[string]any) + if idea["name"] != c.dish { + t.Fatalf("idea %d = %v, want %s", i, idea, c.dish) + } + for key, want := range map[string]any{"source_url": c.wantURL, "site_name": c.wantSite} { + got, ok := idea[key] + if !ok || !reflect.DeepEqual(got, want) { + t.Errorf("%s %s = %v (present %t), want %v", c.dish, key, got, ok, want) + } + } + if idea["recipe_id"] == "" || idea["photo_url"] == "" { + t.Errorf("idea %s = %v, want a recipe and photo", c.dish, idea) + } + } +} + +// An ideas_from_ingredients job shows a clean link and its page's name, +// and neither a token-bearing link nor a name taken from its host. +func TestIdeasJobShowsOnlySafeSources(t *testing.T) { + t.Parallel() + model, normalize, search, pages, images := sourceFakes(t, 25) + list := make([]string, len(sourceCases)) + for i, c := range sourceCases { + list[i] = fmt.Sprintf(`{"name":%q,"uses":["rice"],"missing":[],"est_minutes":25,"method":"stovetop","difficulty":"easy","search_query":%q}`, c.dish, c.dish) + } + js := `{"ideas":[` + strings.Join(list, ",") + `]}` + model.QueueGenerate(llm.GenerateResult{Response: llm.Response{Text: js, JSON: json.RawMessage(js)}}) + h, _, _ := newIdeasRouter(t, model, normalize, search, pages, images, true) + checkIdeaSources(t, h, startIdeasJob(t, h)) +} + +// A quick_ideas job shows sources as an ideas_from_ingredients job does. +func TestQuickJobShowsOnlySafeSources(t *testing.T) { + t.Parallel() + model, normalize, search, pages, images := sourceFakes(t, 25) + ideas := make([][3]any, len(sourceCases)) + for i, c := range sourceCases { + ideas[i] = [3]any{c.dish, "stovetop", 20} + } + quickReply(model, ideas...) + h, _, _ := newIdeasRouter(t, model, normalize, search, pages, images, true) + rec, body := callDinners(t, h, http.MethodPost, quickPath, `{"kind": "under30", "exclude": []}`) + if rec.Code != http.StatusAccepted { + t.Fatalf("POST = %d: %s", rec.Code, rec.Body) + } + checkIdeaSources(t, h, body["jobId"].(string)) +} diff --git a/web/src/ideas/CookFromPantryPage.test.tsx b/web/src/ideas/CookFromPantryPage.test.tsx index 6733bf8..f17c70d 100644 --- a/web/src/ideas/CookFromPantryPage.test.tsx +++ b/web/src/ideas/CookFromPantryPage.test.tsx @@ -33,7 +33,9 @@ function idea(name: string, summary: string, minutes: number | null = 30) { // The ideas of mock 1i. const CURRY = idea('Coconut chicken curry', 'Uses all 5 · missing: 1 lime') -const BOWLS = idea('Garlic spinach rice bowls', 'Uses 4 of 5 · nothing missing', 25) +// BOWLS's source is withheld, as the server sends one whose link could +// carry a secret: the card shows all the same. +const BOWLS = { ...idea('Garlic spinach rice bowls', 'Uses 4 of 5 · nothing missing', 25), source_url: null, site_name: null } const ONE_PAN = idea('One-pan chicken & rice', 'Uses 3 of 5 · missing: onion') type JobBody = { status: string; result: unknown; error: string | null } diff --git a/web/src/ideas/QuickDinnersPage.test.tsx b/web/src/ideas/QuickDinnersPage.test.tsx index fe41e8c..90fbe39 100644 --- a/web/src/ideas/QuickDinnersPage.test.tsx +++ b/web/src/ideas/QuickDinnersPage.test.tsx @@ -30,7 +30,9 @@ function idea(name: string, method: string, minutes: number | null, difficulty: // The ideas of mock 1j, and more for later pages and the Crock pot tab. const ORZO = idea('Lemon garlic shrimp orzo', 'stovetop', 25, 'medium') -const QUESADILLAS = idea('Chicken quesadillas', 'stovetop', 20, 'easy') +// QUESADILLAS's source is withheld, as the server sends one whose link +// could carry a secret: the card shows all the same. +const QUESADILLAS = { ...idea('Chicken quesadillas', 'stovetop', 20, 'easy'), source_url: null, site_name: null } const BROCCOLI = idea('Beef & broccoli', 'stovetop', 30, 'easy') const GNOCCHI = idea('Pesto gnocchi & peas', 'stovetop', 15, 'easy') const CHILI = idea('Crock pot chili', 'crockpot', 360, 'easy') -- 2.55.0 From c961d65092b66b9f926243884377af7ad66f49a8 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Fri, 9 Oct 2026 18:33:57 -0400 Subject: [PATCH 2/2] Show idea job results' sources safely on every read GET /jobs/{id} served a succeeded idea job's stored result as it was, so a result stored before sources were made safe still showed a token-bearing source_url and a site_name taken from the link's host (BASE-01-R1). jobs.Kind.FailedResult becomes ShownResult, applied to every stored result with the job's status: ideas_from_ingredients and quick_ideas results show no ideas when failed and, otherwise, each idea's source through dinners.SourceOf, the one rule, so no job old or new serves an unsafe source. Other kinds' results are untouched. dinners.SourceOf now parses the link before reading the site name, so a link that does not parse is an error with or without a stored name, and an idea job grounded in one fails instead of succeeding (BASE-01-R2). Task: c15ed4b5-25ae-43c6-8d36-f4a6dd51e5af Co-Authored-By: Claude Opus 5.5 --- internal/dinners/detail.go | 27 +++++----- internal/dinners/dinners_test.go | 15 +++--- internal/dinners/export_test.go | 3 -- internal/ideas/ideas.go | 84 +++++++++++++++++++++++++----- internal/jobs/jobs.go | 47 +++++++++-------- internal/jobs/jobs_test.go | 28 ++++++---- internal/server/ideasource_test.go | 73 ++++++++++++++++++++++++++ 7 files changed, 207 insertions(+), 70 deletions(-) diff --git a/internal/dinners/detail.go b/internal/dinners/detail.go index 0eb1f06..6ccb079 100644 --- a/internal/dinners/detail.go +++ b/internal/dinners/detail.go @@ -186,16 +186,18 @@ func readRecipe(ctx context.Context, q *db.Queries, recipeID string, d *Detail) // site name is siteName: the name only when the page gave it (see // pageSiteName) and the link only when it is free of secrets (see // safeSourceURL). It is the one way a fetched recipe's source reaches a -// client. +// client. A sourceURL that does not parse is an error, whether or not a +// site name is stored. func SourceOf(siteName sql.NullString, sourceURL string) (Source, error) { - site, err := pageSiteName(siteName, sourceURL) + link, err := url.Parse(sourceURL) if err != nil { - return Source{}, err + // Not wrapped: url.Parse's error quotes the link. + return Source{}, errors.New("source url does not parse") } - return Source{SiteName: site, URL: safeSourceURL(sourceURL)}, nil + return Source{SiteName: pageSiteName(siteName, link), URL: safeSourceURL(sourceURL)}, nil } -// pageSiteName is the stored site name of a recipe fetched from sourceURL +// pageSiteName is the stored site name of a recipe fetched from link // when it is one the page gave, else nil. A page that names no site is // stored under recipes.HostSiteName of the URL it was served from: the // pasted link's host, which may hold a personal token (a private @@ -205,23 +207,18 @@ func SourceOf(siteName sql.NullString, sourceURL string) (Source, error) { // is treated as one and withheld. A site that names itself as a domain, // such as "Food.com", loses its name with them: a credit that does not // name the site is better than one that leaks a link. -func pageSiteName(stored sql.NullString, sourceURL string) (*string, error) { +func pageSiteName(stored sql.NullString, link *url.URL) *string { if !stored.Valid { - return nil, nil - } - link, err := url.Parse(sourceURL) - if err != nil { - // Not wrapped: url.Parse's error quotes the link. - return nil, errors.New("source url does not parse") + return nil } name := stored.String if strings.EqualFold(name, recipes.HostSiteName(link)) || net.ParseIP(name) != nil { - return nil, nil + return nil } if u, err := url.Parse("https://" + name + "/"); err == nil && u.Host == name && strings.Contains(name, ".") { - return nil, nil + return nil } - return &name, nil + return &name } // safeSourceURL is sourceURL when it is verifiably free of secrets — an diff --git a/internal/dinners/dinners_test.go b/internal/dinners/dinners_test.go index dbd8bce..14e0ff6 100644 --- a/internal/dinners/dinners_test.go +++ b/internal/dinners/dinners_test.go @@ -99,16 +99,19 @@ func TestPageSiteNameWithholdsHosts(t *testing.T) { {"Bon Appétit", true, ptr("Bon Appétit")}, {"Mr. Food Test Kitchen", true, ptr("Mr. Food Test Kitchen")}, } { - got, err := dinners.PageSiteName(sql.NullString{String: tc.stored, Valid: tc.valid}, link) + src, err := dinners.SourceOf(sql.NullString{String: tc.stored, Valid: tc.valid}, link) if err != nil { - t.Fatalf("PageSiteName(%q): %v", tc.stored, err) + t.Fatalf("SourceOf(%q): %v", tc.stored, err) } - if (got == nil) != (tc.want == nil) || (got != nil && *got != *tc.want) { - t.Errorf("PageSiteName(%q) = %v, want %v", tc.stored, deref(got), deref(tc.want)) + if got := src.SiteName; (got == nil) != (tc.want == nil) || (got != nil && *got != *tc.want) { + t.Errorf("SourceOf(%q) site name = %v, want %v", tc.stored, deref(got), deref(tc.want)) } } - if _, err := dinners.PageSiteName(sql.NullString{String: "Delish", Valid: true}, "https://secret-token\x7f.example/%zz"); err == nil || strings.Contains(err.Error(), "secret-token") { - t.Errorf("PageSiteName of an unparseable link: err = %v, want one not quoting the link", err) + // A link that does not parse is an error with or without a site name. + for _, site := range []sql.NullString{{String: "Delish", Valid: true}, {}} { + if _, err := dinners.SourceOf(site, "https://secret-token\x7f.example/%zz"); err == nil || strings.Contains(err.Error(), "secret-token") { + t.Errorf("SourceOf(%v) of an unparseable link: err = %v, want one not quoting the link", site, err) + } } } diff --git a/internal/dinners/export_test.go b/internal/dinners/export_test.go index 1d62211..0f4a731 100644 --- a/internal/dinners/export_test.go +++ b/internal/dinners/export_test.go @@ -72,8 +72,5 @@ func (s *Service) RunGroundJob(ctx context.Context, p GroundJob) (string, error) return r.GroundingStatus, err } -// PageSiteName is pageSiteName. -var PageSiteName = pageSiteName - // SafeSourceURL is safeSourceURL. var SafeSourceURL = safeSourceURL diff --git a/internal/ideas/ideas.go b/internal/ideas/ideas.go index c95524c..557b512 100644 --- a/internal/ideas/ideas.go +++ b/internal/ideas/ideas.go @@ -124,18 +124,18 @@ func NewService(st *store.Store, grounder *recipes.Grounder, generate Generator, } s := &Service{st: st, grounder: grounder, runner: runner, generate: generate, quick: quick, logger: logger} var err error - if s.job, err = register(runner, KindFromIngredients, s.run, withoutIdeas[Result]); err != nil { + if s.job, err = register(runner, KindFromIngredients, s.run, shownJobResult[Result]); err != nil { return nil, err } - if s.quickJob, err = register(runner, KindQuick, s.runQuick, withoutIdeas[QuickResult]); err != nil { + if s.quickJob, err = register(runner, KindQuick, s.runQuick, shownJobResult[QuickResult]); err != nil { return nil, err } return s, nil } -// register registers the service-only job name, run by run, whose failed -// jobs show their result through failed. -func register[P any, R jobs.Public](runner *jobs.Runner, name string, run jobs.Handler[P, R], failed func(json.RawMessage) (json.RawMessage, error)) (*jobs.Kind[P], error) { +// register registers the service-only job name, run by run, whose jobs +// show their result through show. +func register[P any, R jobs.Public](runner *jobs.Runner, name string, run jobs.Handler[P, R], show func(jobs.Status, json.RawMessage) (json.RawMessage, error)) (*jobs.Kind[P], error) { kind, err := jobs.Register(runner, name, run, Reasons...) if err != nil { return nil, err @@ -143,7 +143,7 @@ func register[P any, R jobs.Public](runner *jobs.Runner, name string, run jobs.H if err := kind.ServiceOnly(); err != nil { return nil, err } - if err := kind.FailedResult(failed); err != nil { + if err := kind.ShownResult(show); err != nil { return nil, err } return kind, nil @@ -254,6 +254,9 @@ type shownResult[R any] interface { shown() int // cleared is the result showing no ideas. cleared() R + // withSafeSources is the result with each idea's source as + // shownSource gives it. + withSafeSources() (R, error) } func (r Result) shown() int { return len(r.Ideas) } @@ -263,11 +266,25 @@ func (r Result) cleared() Result { return r } +func (r Result) withSafeSources() (Result, error) { + ideas := make([]Idea, len(r.Ideas)) + for i, idea := range r.Ideas { + src, err := shownSource(idea.SourceURL, idea.SiteName) + if err != nil { + return Result{}, fmt.Errorf("ideas: idea %d: %w", i, err) + } + idea.SourceURL, idea.SiteName = src.URL, src.SiteName + ideas[i] = idea + } + r.Ideas = ideas + return r, nil +} + // finish ends a job whose work returned res and err. A job that failed // has its result published again with no ideas, to tidy the stored // result. That write is not what keeps a failed job from showing ideas, // since it may fail and a job interrupted by restart never makes it: -// withoutIdeas, applied whenever a failed job is read, is. +// shownJobResult, applied whenever a failed job is read, is. func finish[R shownResult[R]](ctx context.Context, logger *slog.Logger, what string, res R, err error, publish func(R) error) (R, error) { var zero R if err == nil { @@ -412,15 +429,24 @@ func ground(ctx context.Context, g *recipes.Grounder, reqs []recipes.GroundReque return failed } -// withoutIdeas is a failed job's stored result as the job shows it: with no -// ideas, however the job failed, so a failed job never shows ideas kept -// before the failure. -func withoutIdeas[R shownResult[R]](stored json.RawMessage) (json.RawMessage, error) { +// shownJobResult is a job's stored result as the job shows it. A failed +// job's shows no ideas, however the job failed, so a failed job never +// shows ideas kept before the failure. Any other job's shows each idea's +// source as shownSource gives it, so a result stored before sources were +// made safe never shows a link or name that could leak a secret. +func shownJobResult[R shownResult[R]](status jobs.Status, stored json.RawMessage) (json.RawMessage, error) { var res R if err := json.Unmarshal(stored, &res); err != nil { - return nil, fmt.Errorf("ideas: decode failed job's result: %w", err) + return nil, fmt.Errorf("ideas: decode %s job's result: %w", status, err) } - return res.cleared().PublicJSON() + if status == jobs.StatusFailed { + return res.cleared().PublicJSON() + } + safe, err := res.withSafeSources() + if err != nil { + return nil, err + } + return safe.PublicJSON() } // ideaOf is idea as grounded by gr. @@ -456,6 +482,24 @@ func sourceOf(gr recipes.Grounded) (dinners.Source, error) { return src, nil } +// shownSource is the source of an idea as stored in a job's result, with +// link sourceURL and site name siteName, as a client may see it (see +// dinners.SourceOf). A result stored now already holds it, and one stored +// before sources were made safe holds the grounded recipe's own, with "" +// for no site name, as sourceOf reads it; a null link, which only a result +// stored now holds, is checked as "". +func shownSource(sourceURL, siteName *string) (dinners.Source, error) { + var link string + if sourceURL != nil { + link = *sourceURL + } + var site sql.NullString + if siteName != nil { + site = sql.NullString{String: *siteName, Valid: *siteName != ""} + } + return dinners.SourceOf(site, link) +} + // QuickJob is the KindQuick payload: the kind of Quick dinner wanted, // skills.QuickCrockpot or skills.QuickUnder30, and the names already shown // to the household, as skills.CleanExclude returns them. @@ -526,6 +570,20 @@ func (r QuickResult) cleared() QuickResult { return r } +func (r QuickResult) withSafeSources() (QuickResult, error) { + ideas := make([]QuickIdea, len(r.Ideas)) + for i, idea := range r.Ideas { + src, err := shownSource(idea.SourceURL, idea.SiteName) + if err != nil { + return QuickResult{}, fmt.Errorf("ideas: idea %d: %w", i, err) + } + idea.SourceURL, idea.SiteName = src.URL, src.SiteName + ideas[i] = idea + } + r.Ideas = ideas + return r, nil +} + // OfQuickKind reports whether a recipe cooked by method in minutes (nil // when unknown) is a Quick dinner of kind: for skills.QuickCrockpot, // cooked in a crock pot (normalize_recipe writes a slow cooker as diff --git a/internal/jobs/jobs.go b/internal/jobs/jobs.go index e6b6561..34856cf 100644 --- a/internal/jobs/jobs.go +++ b/internal/jobs/jobs.go @@ -136,13 +136,13 @@ type runFunc func(ctx context.Context, payload string, publish func(json.RawMess // handler is a registered kind: run runs a job, check reports whether a // payload decodes into the kind's payload type, serviceOnly is set when -// Runner.Enqueue refuses the kind, and failedResult, when set, is what Get -// shows of a failed job's stored result (see Kind.FailedResult). +// Runner.Enqueue refuses the kind, and shownResult, when set, is what Get +// shows of a job's stored result (see Kind.ShownResult). type handler struct { - run runFunc - check func(payload []byte) bool - serviceOnly bool - failedResult func(stored json.RawMessage) (json.RawMessage, error) + run runFunc + check func(payload []byte) bool + serviceOnly bool + shownResult func(status Status, stored json.RawMessage) (json.RawMessage, error) } // Runner owns the job handlers and the worker pool. @@ -309,23 +309,24 @@ func (k *Kind[P]) ServiceOnly() error { return nil } -// FailedResult makes Get return, for a failed job of the kind with a -// stored result, show's view of that result instead of the result itself. -// It is applied on every read, so it holds however the job failed: by its -// handler, as interrupted by restart, or after a write meant to tidy the -// result failed. A show error fails Get. A kind that never calls it shows a -// failed job's last result as stored. It must be called before Start. -func (k *Kind[P]) FailedResult(show func(stored json.RawMessage) (json.RawMessage, error)) error { +// ShownResult makes Get return, for a job of the kind with a stored +// result, show's view of that result, given the job's status, instead of +// the result itself. It is applied on every read, so it holds for every +// stored result: however a job failed (by its handler, as interrupted by +// restart, or after a write meant to tidy the result failed) and whatever +// version of the kind stored it. A show error fails Get. A kind that never +// calls it shows a job's result as stored. It must be called before Start. +func (k *Kind[P]) ShownResult(show func(status Status, stored json.RawMessage) (json.RawMessage, error)) error { if show == nil { - return fmt.Errorf("jobs: failed result %q: nil func", k.name) + return fmt.Errorf("jobs: shown result %q: nil func", k.name) } k.r.mu.Lock() defer k.r.mu.Unlock() if k.r.started { - return fmt.Errorf("jobs: failed result %q: runner already started", k.name) + return fmt.Errorf("jobs: shown result %q: runner already started", k.name) } h := k.r.handlers[k.name] - h.failedResult = show + h.shownResult = show k.r.handlers[k.name] = h return nil } @@ -425,8 +426,8 @@ func (r *Runner) Wait() { r.wg.Wait() } -// Get returns the job with id, or ErrNotFound. A failed job's result is -// shown through its kind's FailedResult, when the kind set one. +// Get returns the job with id, or ErrNotFound. A job's result is shown +// through its kind's ShownResult, when the kind set one. func (r *Runner) Get(ctx context.Context, id string) (Job, error) { row, err := db.New(r.st.Reader).GetJob(ctx, id) if errors.Is(err, sql.ErrNoRows) { @@ -442,17 +443,17 @@ func (r *Runner) Get(ctx context.Context, id string) (Job, error) { if row.Error.Valid { job.Error = &row.Error.String } - if job.Status == StatusFailed && job.Result != nil { + if job.Result != nil { r.mu.Lock() - show := r.handlers[job.Kind].failedResult + show := r.handlers[job.Kind].shownResult r.mu.Unlock() if show != nil { - shown, err := show(job.Result) + shown, err := show(job.Status, job.Result) if err != nil { - return Job{}, fmt.Errorf("jobs: show failed %s result: %w", job.Kind, err) + return Job{}, fmt.Errorf("jobs: show %s %s result: %w", job.Status, job.Kind, err) } if !json.Valid(shown) { - return Job{}, fmt.Errorf("jobs: show failed %s result: not one JSON value", job.Kind) + return Job{}, fmt.Errorf("jobs: show %s %s result: not one JSON value", job.Status, job.Kind) } job.Result = shown } diff --git a/internal/jobs/jobs_test.go b/internal/jobs/jobs_test.go index 79cb103..837f497 100644 --- a/internal/jobs/jobs_test.go +++ b/internal/jobs/jobs_test.go @@ -773,7 +773,7 @@ func TestServiceOnlyKind(t *testing.T) { } } -func TestFailedResultAppliesOnlyToItsKindsFailedJobs(t *testing.T) { +func TestShownResultAppliesOnlyToItsKindsJobs(t *testing.T) { t.Parallel() st := migratedStore(t) r := newRunner(t, st, 1) @@ -788,17 +788,24 @@ func TestFailedResultAppliesOnlyToItsKindsFailedJobs(t *testing.T) { } hidden := register(t, r, "hidden", h, "stopped") plain := register(t, r, "plain", h, "stopped") - if err := hidden.FailedResult(func(json.RawMessage) (json.RawMessage, error) { - return json.RawMessage(`{"done":0}`), nil + if err := hidden.ShownResult(func(status Status, stored json.RawMessage) (json.RawMessage, error) { + if status == StatusFailed { + return json.RawMessage(`{"done":0}`), nil + } + var res countResult + if err := json.Unmarshal(stored, &res); err != nil { + return nil, err + } + return json.Marshal(countResult{Done: res.Done * 10}) }); err != nil { t.Fatal(err) } - if err := hidden.FailedResult(nil); err == nil { - t.Error("FailedResult(nil) succeeded, want an error") + if err := hidden.ShownResult(nil); err == nil { + t.Error("ShownResult(nil) succeeded, want an error") } start(t, r) - if err := hidden.FailedResult(func(s json.RawMessage) (json.RawMessage, error) { return s, nil }); err == nil { - t.Error("FailedResult after Start succeeded, want an error") + if err := hidden.ShownResult(func(_ Status, s json.RawMessage) (json.RawMessage, error) { return s, nil }); err == nil { + t.Error("ShownResult after Start succeeded, want an error") } for _, tc := range []struct { @@ -806,9 +813,10 @@ func TestFailedResultAppliesOnlyToItsKindsFailedJobs(t *testing.T) { steps int want string }{ - {hidden, 0, `{"done":0}`}, // failed: shown through FailedResult - {hidden, 3, `{"done":3}`}, // succeeded: as stored - {plain, 0, `{"done":5}`}, // failed, kind without FailedResult: as stored + {hidden, 0, `{"done":0}`}, // failed: shown through ShownResult + {hidden, 3, `{"done":30}`}, // succeeded: shown through ShownResult + {plain, 0, `{"done":5}`}, // failed, kind without ShownResult: as stored + {plain, 3, `{"done":3}`}, // succeeded, kind without ShownResult: as stored } { job := waitFor(t, r, enqueue(t, tc.kind, countPayload{Steps: tc.steps}), finished) if string(job.Result) != tc.want { diff --git a/internal/server/ideasource_test.go b/internal/server/ideasource_test.go index a858bca..2aae059 100644 --- a/internal/server/ideasource_test.go +++ b/internal/server/ideasource_test.go @@ -9,6 +9,8 @@ import ( "testing" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ai/llm" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ideas" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/jobs" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/recipes" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/recipes/recipestest" ) @@ -122,3 +124,74 @@ func TestQuickJobShowsOnlySafeSources(t *testing.T) { } checkIdeaSources(t, h, body["jobId"].(string)) } + +// legacyIdea is an idea as the result builders stored it before sources +// were made safe: the grounded recipe's raw link and stored site name, +// which is the link's host when its page named no site. +func legacyIdea(name, link, site string) map[string]any { + return map[string]any{ + "name": name, "method": "stovetop", "minutes_total": 25, "difficulty": "easy", + "recipe_id": "01JLEGACY" + strings.ToUpper(strings.ReplaceAll(name, " ", "")), "photo_url": "/api/v1/photos/legacy", + "source_url": link, "site_name": site, + } +} + +// A job of either idea kind whose result was stored before sources were +// made safe is served with the sources a job stored now holds: neither a +// token-bearing link nor a name taken from its host. +func TestJobServesLegacyIdeaResultsWithOnlySafeSources(t *testing.T) { + t.Parallel() + model, normalize, search, pages, images := newQuickFakes() + h, _, st := newIdeasRouter(t, model, normalize, search, pages, images, false) + legacy := []any{ + legacyIdea("Chicken Curry", cleanSourceLink, "Curry House"), + legacyIdea("Beef Stew", "https://"+queryHost+"/stew?access_token="+queryToken, queryHost), + legacyIdea("Bean Chili", "https://"+userinfoToken+"@"+userinfoHost+"/chili", userinfoHost), + } + for kind, result := range map[string]map[string]any{ + ideas.KindFromIngredients: {"stage": "done", "ingredients": []string{"rice"}, "quick_only": false, "ideas": legacy, "dropped": 0}, + ideas.KindQuick: {"stage": "done", "kind": "under30", "exclude": []string{}, "ideas": legacy, "dropped": 0}, + } { + js, err := json.Marshal(result) + if err != nil { + t.Fatal(err) + } + id := "legacy-" + kind + if _, err := st.Writer.ExecContext(t.Context(), + `INSERT INTO jobs (id, kind, status, payload, result, created_at, updated_at) VALUES (?, ?, 'succeeded', '{}', ?, '2026-10-01T00:00:00Z', '2026-10-01T00:00:00Z')`, + id, kind, string(js)); err != nil { + t.Fatal(err) + } + checkIdeaSources(t, h, id) + } +} + +// An idea job whose grounded recipe's stored link does not parse fails, +// even when no site name is stored, rather than show the idea without a +// source. +func TestIdeaJobFailsOnUnparseableSourceWithoutSiteName(t *testing.T) { + t.Parallel() + const badLink = "https://kitchen.example/%zz" + model, normalize, search, pages, images := sourceFakes(t, 25) + idea := fmt.Sprintf(`{"name":%q,"uses":["rice"],"missing":[],"est_minutes":25,"method":"stovetop","difficulty":"easy","search_query":%q}`, sourceCases[0].dish, sourceCases[0].dish) + js := `{"ideas":[` + idea + `]}` + for range 2 { + model.QueueGenerate(llm.GenerateResult{Response: llm.Response{Text: js, JSON: json.RawMessage(js)}}) + } + h, _, st := newIdeasRouter(t, model, normalize, search, pages, images, true) + if job := waitJob(t, h, startIdeasJob(t, h)); job["status"] != "succeeded" { + t.Fatalf("grounding job = %v", job) + } + // The recipe is now stored under badLink with no site name, and the + // search finds it there. + res, err := st.Writer.ExecContext(t.Context(), `UPDATE recipes SET source_url = ?, site_name = NULL WHERE source_url = ?`, badLink, cleanSourceLink) + if err != nil { + t.Fatal(err) + } + if n, err := res.RowsAffected(); err != nil || n != 1 { + t.Fatalf("updated %d recipes (%v), want 1", n, err) + } + search.Results[sourceCases[0].dish] = []recipes.Candidate{{URL: badLink}} + job := waitJob(t, h, startIdeasJob(t, h)) + assertFailedWithoutIdeas(t, job, jobs.CodeInternal) +} -- 2.55.0