From c961d65092b66b9f926243884377af7ad66f49a8 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Fri, 9 Oct 2026 18:33:57 -0400 Subject: [PATCH] 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) +}