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) +}