diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 161102d..9a33eec 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -180,7 +180,8 @@ automatic** (D-021). each skill run, so a saved change applies to the next run. - Dinners (`dinners.Service`). `POST /api/v1/dinners` adds a Regular from exactly one of `{name}` or `{recipe_url}` (an https:// link, kept in - `recipe_url` for grounding) with status and origin `regular` and + `recipe_url` for grounding and never returned in a dinner response, as + it may carry a personal token) with status and origin `regular` and grounding_status `pending`, answering 201 with the dinner. A link's dinner is named from its last path segment (separators become spaces, an extension and all-digit words are dropped: `…/12345/beef-stew/` is diff --git a/internal/dinners/dinners.go b/internal/dinners/dinners.go index 605d675..7a39832 100644 --- a/internal/dinners/dinners.go +++ b/internal/dinners/dinners.go @@ -55,7 +55,9 @@ var listable = []string{StatusRegular, StatusQuestLog} // ErrNotFound is returned for an unknown dinner id. var ErrNotFound = errors.New("dinner not found") -// Dinner is a dinner as the API shows it. +// Dinner is a dinner as the API shows it. It has no recipe_url: a pasted +// link may carry a personal token, so it stays in the database for +// grounding and never leaves the server. type Dinner struct { ID string `json:"id"` Name string `json:"name"` @@ -67,7 +69,6 @@ type Dinner struct { Cuisine *string `json:"cuisine"` IsFavorite bool `json:"is_favorite"` RecipeID *string `json:"recipe_id"` - RecipeURL *string `json:"recipe_url"` PhotoURL *string `json:"photo_url"` GroundingStatus string `json:"grounding_status"` // GroundingError is why grounding failed, one of the Reason codes; @@ -565,7 +566,6 @@ func fromRow(r db.Dinner) Dinner { Cuisine: nullString(r.Cuisine), IsFavorite: r.IsFavorite == 1, RecipeID: nullString(r.RecipeID), - RecipeURL: nullString(r.RecipeUrl), GroundingStatus: r.GroundingStatus, GroundingError: nullString(r.GroundingError), UpdatedBy: nullString(r.UpdatedBy), diff --git a/internal/dinners/grounding_test.go b/internal/dinners/grounding_test.go index 2578083..c6ada37 100644 --- a/internal/dinners/grounding_test.go +++ b/internal/dinners/grounding_test.go @@ -383,8 +383,11 @@ func TestRenameLinkWhilePendingGroundsNewName(t *testing.T) { if err != nil { t.Fatal(err) } - if d.GroundingStatus != dinners.GroundingPending || d.RecipeURL != nil { - t.Fatalf("renamed = %+v, want still pending with no recipe_url", d) + if d.GroundingStatus != dinners.GroundingPending { + t.Fatalf("renamed = %+v, want still pending", d) + } + if row, err := db.New(e.st.Reader).GetDinner(t.Context(), c.ID); err != nil || row.RecipeUrl.Valid { + t.Fatalf("renamed row = %+v (err %v), want no recipe_url", row, err) } e.start(t) @@ -749,12 +752,15 @@ func TestFixReplacesPendingLink(t *testing.T) { t.Fatal(err) } e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 25)) - d, err := e.svc.Ground(t.Context(), c.ID, tt.fix, member) + if _, err := e.svc.Ground(t.Context(), c.ID, tt.fix, member); err != nil { + t.Fatal(err) + } + row, err := db.New(e.st.Reader).GetDinner(t.Context(), c.ID) if err != nil { t.Fatal(err) } - if d.RecipeURL != nil { - t.Errorf("recipe_url = %q after the fix, want the unusable link dropped", *d.RecipeURL) + if row.RecipeUrl.Valid { + t.Errorf("recipe_url = %q after the fix, want the unusable link dropped", row.RecipeUrl.String) } if _, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{RecipeURL: ptr(lost)}, member); err != nil { diff --git a/internal/server/dinners_test.go b/internal/server/dinners_test.go index 402b90d..a8285e2 100644 --- a/internal/server/dinners_test.go +++ b/internal/server/dinners_test.go @@ -159,7 +159,6 @@ func TestCreateDinnerByName(t *testing.T) { "origin": "regular", "grounding_status": "pending", "is_favorite": false, - "recipe_url": nil, "photo_url": nil, "method": nil, "minutes_total": nil, @@ -170,6 +169,9 @@ func TestCreateDinnerByName(t *testing.T) { t.Errorf("%s = %#v (present %v), want %#v", k, got, ok, v) } } + if got, ok := body["recipe_url"]; ok { + t.Errorf("recipe_url = %#v, want no recipe_url in a dinner response", got) + } var normalized, updatedBy string if err := st.Reader.QueryRow(`SELECT normalized_name, updated_by FROM dinners WHERE id = ?`, body["id"]).Scan(&normalized, &updatedBy); err != nil { t.Fatal(err) @@ -190,7 +192,6 @@ func TestCreateDinnerByURL(t *testing.T) { } for k, v := range map[string]any{ "name": "Beef and broccoli stir fry", - "recipe_url": link, "status": "regular", "origin": "regular", "grounding_status": "pending", @@ -200,6 +201,10 @@ func TestCreateDinnerByURL(t *testing.T) { t.Errorf("%s = %#v, want %#v", k, body[k], v) } } + // The link stays on the server for grounding; responses never carry it. + if got, ok := body["recipe_url"]; ok { + t.Errorf("recipe_url = %#v, want no recipe_url in a dinner response", got) + } // The same link again is a duplicate even though its name would differ. rec, body = callDinners(t, router, http.MethodPost, dinnersPath, `{"name":"Something else entirely"}`) @@ -227,6 +232,40 @@ func TestCreateDinnerByURL(t *testing.T) { } } +// TestDinnerResponsesHideRecipeURL: a pasted link may carry a personal +// token, so neither the create response nor a later list read shows any +// part of it, while the stored link still awaits grounding. +func TestDinnerResponsesHideRecipeURL(t *testing.T) { + t.Parallel() + router, st := newDinnersRouter(t) + + const link = "https://c.example/recipes/stew?access_token=personal-token-abc123" + rec, body := callDinners(t, router, http.MethodPost, dinnersPath, `{"recipe_url":"`+link+`"}`) + if rec.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201: %s", rec.Code, rec.Body) + } + created := rec.Body.String() + rec, _ = callDinners(t, router, http.MethodGet, dinnersPath+"?status=regular", "") + if rec.Code != http.StatusOK { + t.Fatalf("GET status=regular = %d: %s", rec.Code, rec.Body) + } + for _, resp := range []struct{ name, body string }{{"create", created}, {"list", rec.Body.String()}} { + for _, secret := range []string{link, "c.example", "access_token", "personal-token-abc123", "recipe_url"} { + if strings.Contains(resp.body, secret) { + t.Errorf("%s response contains %q: %s", resp.name, secret, resp.body) + } + } + } + + var stored string + if err := st.Reader.QueryRow(`SELECT recipe_url FROM dinners WHERE id = ?`, body["id"]).Scan(&stored); err != nil { + t.Fatal(err) + } + if stored != link { + t.Errorf("stored recipe_url = %q, want %q kept for grounding", stored, link) + } +} + func TestCreateDinnerDuplicate(t *testing.T) { t.Parallel() router, st := newDinnersRouter(t) diff --git a/web/src/settings/RegularsSettingsPage.test.tsx b/web/src/settings/RegularsSettingsPage.test.tsx index ba7371c..990081b 100644 --- a/web/src/settings/RegularsSettingsPage.test.tsx +++ b/web/src/settings/RegularsSettingsPage.test.tsx @@ -196,7 +196,7 @@ describe('RegularsSettingsPage', () => { const link = 'https://example.com/recipes/12345/beef-stew/' const server = stubServer([], { 'POST /api/v1/dinners': () => { - const created = dinnerBody('d9', 'Beef stew', { grounding_status: 'pending', recipe_url: link }) + const created = dinnerBody('d9', 'Beef stew', { grounding_status: 'pending' }) server.regulars = [created] return jsonResponse(201, { ...created, grounding_job_id: 'job-1' }) }, diff --git a/web/src/test/dinners.ts b/web/src/test/dinners.ts index f2a1cb2..f316391 100644 --- a/web/src/test/dinners.ts +++ b/web/src/test/dinners.ts @@ -10,7 +10,6 @@ export type DinnerBody = { cuisine: string | null is_favorite: boolean recipe_id: string | null - recipe_url: string | null photo_url: string | null grounding_status: string grounding_error: string | null @@ -33,7 +32,6 @@ export function dinnerBody(id: string, name: string, overrides: Partial