Withhold unsafe recipe sources from idea job results (BASE-01) #170

Merged
approver_agent merged 2 commits from task/ideas-source-safety_c15ed4b5-25ae-43c6-8d36-f4a6dd51e5af into dev 2026-10-09 19:04:45 -04:00
7 changed files with 207 additions and 70 deletions
Showing only changes of commit c961d65092 - Show all commits
+12 -15
View File
@@ -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
+9 -6
View File
@@ -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)
}
}
}
-3
View File
@@ -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
+71 -13
View File
@@ -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
+24 -23
View File
@@ -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
}
+18 -10
View File
@@ -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 {
+73
View File
@@ -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)
}