diff --git a/internal/dinners/detail.go b/internal/dinners/detail.go index b2673b2..3764a7b 100644 --- a/internal/dinners/detail.go +++ b/internal/dinners/detail.go @@ -58,15 +58,16 @@ type Ingredient struct { RawText string `json:"raw_text"` } -// Source is the site a recipe was fetched from. It has no url: the page's -// link, pasted or offered as a candidate, may carry a personal token, and -// no stored link can be shown to be free of one, so like a dinner's -// recipe_url and a candidate's page it never leaves the server. SiteName -// is the name the page itself gives its site, and null when it gives none -// or the stored name could have come from a link (see pageSiteName), so a -// client shows "Recipe from {site}" only when SiteName is not null. +// Source is the site a recipe was fetched from. SiteName is the name the +// page itself gives its site, and null when it gives none or the stored +// name could have come from a link (see pageSiteName), so a client shows +// "Recipe from {site}" only when SiteName is not null. URL is the page's +// link only when it is verifiably free of secrets (see safeSourceURL), and +// null otherwise: a link pasted or offered as a candidate may carry a +// personal token, so a client links the credit only when URL is not null. type Source struct { SiteName *string `json:"site_name"` + URL *string `json:"url"` } // Get returns dinner id with its recipe and the reason it was suggested, @@ -149,7 +150,7 @@ func readRecipe(ctx context.Context, q *db.Queries, recipeID string, d *Detail) if err != nil { return err } - d.Source = &Source{SiteName: site} + d.Source = &Source{SiteName: site, URL: safeSourceURL(rec.SourceUrl)} } ings, err := q.ListRecipeIngredients(ctx, recipeID) if err != nil { @@ -209,6 +210,69 @@ func pageSiteName(stored sql.NullString, sourceURL string) (*string, error) { return &name, nil } +// safeSourceURL is sourceURL when it is verifiably free of secrets — an +// https link to a host, with no user or password, no query (not even a +// bare "?") and no fragment (not even a bare "#") — else nil. Those are +// the places a link carries a token; a link failing any check, or not +// parsing, is withheld rather than trimmed, since a trimmed link is not the +// page the recipe came from. The client (sourceUrl in +// web/src/dinners/detail.ts) parses the link with the browser's URL parser +// and refuses the whole recipe when it fails, so the host and port must +// also be ones it parses: see browserHost. +func safeSourceURL(sourceURL string) *string { + u, err := url.Parse(sourceURL) + if err != nil || u.Scheme != "https" || u.Opaque != "" || u.Host == "" || u.User != nil || + u.RawQuery != "" || u.ForceQuery || u.Fragment != "" || strings.Contains(sourceURL, "#") { + return nil + } + // url.Parse decodes a percent-encoded host; the link must spell its + // host out as it is checked. + authority, _, _ := strings.Cut(sourceURL[len(u.Scheme)+len("://"):], "/") + if authority != u.Host || !browserHost(u) { + return nil + } + if port := u.Port(); port != "" { + if n, err := strconv.ParseUint(port, 10, 16); err != nil || n == 0 { + return nil + } + } + return &sourceURL +} + +// browserHost says whether u's host is one the browser's URL parser (the +// WHATWG URL Standard) accepts as it stands, where url.Parse accepts more, +// such as "[bad]". The rule is narrower than the browser's: a bracketed +// host is an IPv6 address without a zone; any other is ASCII letters, +// digits and hyphens in dot-separated labels, none empty but a trailing +// one, none punycode ("xn--"), since the browser refuses invalid punycode +// and checking it is not worth a decoder; and a host whose last label is +// a number, which the browser reads as an IPv4 address, must be a dotted +// quad. A link to any other host is withheld. +func browserHost(u *url.URL) bool { + host := u.Hostname() + if strings.HasPrefix(u.Host, "[") { + ip := net.ParseIP(host) + return ip != nil && strings.Contains(host, ":") + } + labels := strings.Split(strings.TrimSuffix(host, "."), ".") + for _, label := range labels { + if label == "" || len(label) >= 4 && strings.EqualFold(label[:4], "xn--") { + return false + } + for _, c := range label { + if !('a' <= c && c <= 'z' || 'A' <= c && c <= 'Z' || '0' <= c && c <= '9' || c == '-') { + return false + } + } + } + last := labels[len(labels)-1] + if strings.Trim(last, "0123456789") == "" || len(last) >= 2 && strings.EqualFold(last[:2], "0x") { + ip := net.ParseIP(host) + return ip != nil && ip.To4() != nil && !strings.Contains(host, ":") + } + return true +} + // duplicatePosition is the error for two recipe items of kind at position. // Position is the only record of recipe order, and the id that breaks the // tie says nothing about it, so a recipe with a duplicate is refused rather diff --git a/internal/dinners/dinners_test.go b/internal/dinners/dinners_test.go index 16cf234..dbd8bce 100644 --- a/internal/dinners/dinners_test.go +++ b/internal/dinners/dinners_test.go @@ -112,6 +112,63 @@ func TestPageSiteNameWithholdsHosts(t *testing.T) { } } +// TestSafeSourceURLOnlyWithoutSecrets: a recipe link is shown only when it +// is https to a host with no user, password, query or fragment, each of +// which can carry a token; any other link, or one that does not parse, is +// withheld whole. +func TestSafeSourceURLOnlyWithoutSecrets(t *testing.T) { + t.Parallel() + for _, link := range []string{ + "https://www.example.com/recipes/chicken-curry", + "https://example.com", + "https://example.com:8443/stew/", + "https://[2001:db8::1]/curry", + "https://1.2.3.4/curry", + "https://Kitchen-2.Example.com./curry", + } { + if got := dinners.SafeSourceURL(link); got == nil || *got != link { + t.Errorf("SafeSourceURL(%q) = %v, want the link", link, deref(got)) + } + } + for _, link := range []string{ + "https://example.com/curry?access_token=query-token", + "https://example.com/curry?", + "https://user:password@example.com/curry", + "https://token@example.com/curry", + "https://@example.com/curry", + "http://example.com/curry", + "https://example.com/curry#token=fragment-token", + "https://example.com/curry#", + "ftp://example.com/curry", + "https:example.com/curry", + "https:///curry", + "https://example.com:99999/curry", + "https://example.com:0/curry", + "//example.com/curry", + "https://secret-token\x7f.example/%zz", + "", + // Hosts url.Parse accepts but the browser's URL parser refuses + // (or reads as another host), which would fail the whole recipe + // screen. + "https://[bad]/curry", + "https://[fe80::1%25en0]/curry", + "https://[1.2.3.4]/curry", + "https://1.2.3.999/curry", + "https://1.2.3.4.5/curry", + "https://999/curry", + "https://example.0x/curry", + "https://xn--a/curry", + "https://ex%61mple.com/curry", + "https://ex%2fample.com/curry", + "https://a..b/curry", + "https://café.example/curry", + } { + if got := dinners.SafeSourceURL(link); got != nil { + t.Errorf("SafeSourceURL(%q) = %q, want nil", link, *got) + } + } +} + func deref(s *string) any { if s == nil { return nil diff --git a/internal/dinners/export_test.go b/internal/dinners/export_test.go index 711568f..1d62211 100644 --- a/internal/dinners/export_test.go +++ b/internal/dinners/export_test.go @@ -74,3 +74,6 @@ func (s *Service) RunGroundJob(ctx context.Context, p GroundJob) (string, error) // PageSiteName is pageSiteName. var PageSiteName = pageSiteName + +// SafeSourceURL is safeSourceURL. +var SafeSourceURL = safeSourceURL diff --git a/internal/server/dinnerdetail_test.go b/internal/server/dinnerdetail_test.go index f60ddaf..1789e1c 100644 --- a/internal/server/dinnerdetail_test.go +++ b/internal/server/dinnerdetail_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "os" "reflect" "strings" "testing" @@ -111,7 +112,7 @@ func TestGetDinnerGroundedSideQuest(t *testing.T) { {Name: str("onion"), Qty: str("1"), RawText: "1 onion"}, }, Steps: []string{"Fry with the garlic.", "Chop the onion."}, - Source: &dinners.Source{SiteName: str("Curry House")}, + Source: &dinners.Source{SiteName: str("Curry House"), URL: str(curryPage)}, GroundingStatus: "grounded", WhyPicked: str(reason), } @@ -216,51 +217,113 @@ func TestGetDinnerNotFound(t *testing.T) { } } -// TestGetDinnerSourceHidesTokenURL: a recipe grounded in a pasted link that -// carries a token in its path or query shows its site name, but never the -// link or any part of it. -func TestGetDinnerSourceHidesTokenURL(t *testing.T) { +// TestGetDinnerSourceURLOnlyWhenSafe: a recipe grounded in a clean https +// link shows it as source.url; one whose stored link carries a token in its +// query or userinfo, is http, or has a fragment shows its site name but +// source.url null and nothing of the link. +func TestGetDinnerSourceURLOnlyWhenSafe(t *testing.T) { t.Parallel() e := newDinnersEnv(t) const ( - pathToken = "path-token-abc123" + clean = "https://kitchen.example/recipes/chicken-curry" queryToken = "query-token-def456" - pathPage = "https://secret-path.example/u/" + pathToken + "/chicken-curry" queryPage = "https://secret-query.example/curry?access_token=" + queryToken ) - e.pages.Bodies[pathPage] = recipestest.RecipePage("Chicken Curry", "Curry House", "https://a.example/curry.jpg", 40) + e.pages.Bodies[clean] = recipestest.RecipePage("Chicken Curry", "Curry House", "https://a.example/curry.jpg", 40) e.pages.Bodies[queryPage] = recipestest.RecipePage("Weeknight Curry", "B Kitchen", "https://b.example/curry.jpg", 25) e.images.JPEGs["https://a.example/curry.jpg"] = recipestest.JPEG(t, 400, 320, 1) e.images.JPEGs["https://b.example/curry.jpg"] = recipestest.JPEG(t, 400, 320, 2) - for _, tc := range []struct{ dish, page, site string }{ - {"Chicken curry", pathPage, "Curry House"}, - {"Weeknight curry", queryPage, "B Kitchen"}, - } { - id := createDinner(t, e.h, `{"name":"`+tc.dish+`"}`) // runner not started: stays pending + ground := func(dish, page string) (id string) { + t.Helper() + id = createDinner(t, e.h, `{"name":"`+dish+`"}`) // runner not started: stays pending e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 25)) - if rec, _ := callDinners(t, e.h, http.MethodPost, dinnersPath+"/"+id+"/ground", `{"recipe_url":"`+tc.page+`"}`); rec.Code != http.StatusOK { - t.Fatalf("POST ground %s = %d: %s", tc.page, rec.Code, rec.Body) + if rec, _ := callDinners(t, e.h, http.MethodPost, dinnersPath+"/"+id+"/ground", `{"recipe_url":"`+page+`"}`); rec.Code != http.StatusOK { + t.Fatalf("POST ground %s = %d: %s", page, rec.Code, rec.Body) } var stored string if err := e.st.Reader.QueryRow(`SELECT source_url FROM recipes WHERE id = (SELECT recipe_id FROM dinners WHERE id = ?)`, id).Scan(&stored); err != nil { t.Fatal(err) } - if stored != tc.page { - t.Fatalf("stored source_url = %q, want %q", stored, tc.page) + if stored != page { + t.Fatalf("stored source_url = %q, want %q", stored, page) } + return id + } + source := func(what, id string) map[string]any { + t.Helper() + raw, _ := getDetail(t, e.h, id) + src, ok := raw["source"].(map[string]any) + if !ok { + t.Fatalf("source of %s = %v, want an object", what, raw["source"]) + } + return src + } + cleanID := ground("Chicken curry", clean) + if src := source("a clean link", cleanID); !reflect.DeepEqual(src, map[string]any{"site_name": "Curry House", "url": clean}) { + t.Errorf("source of a clean link = %v, want site_name Curry House and url %s", src, clean) + } + + queryID := ground("Weeknight curry", queryPage) + withheld := func(what, id, site string, secrets ...string) { + t.Helper() rec, _ := callDinners(t, e.h, http.MethodGet, dinnersPath+"/"+id, "") if rec.Code != http.StatusOK { - t.Fatalf("GET dinner = %d: %s", rec.Code, rec.Body) + t.Fatalf("GET %s = %d: %s", what, rec.Code, rec.Body) } - for _, secret := range []string{tc.page, "secret-path.example", "secret-query.example", pathToken, queryToken, "access_token", `"url"`} { + for _, secret := range secrets { if strings.Contains(rec.Body.String(), secret) { - t.Errorf("detail of %s contains %q: %s", tc.dish, secret, rec.Body) + t.Errorf("detail of %s contains %q: %s", what, secret, rec.Body) } } - raw, _ := getDetail(t, e.h, id) - if src, _ := raw["source"].(map[string]any); !reflect.DeepEqual(src, map[string]any{"site_name": tc.site}) { - t.Errorf("source of %s = %v, want only site_name %q", tc.dish, raw["source"], tc.site) + if src := source(what, id); !reflect.DeepEqual(src, map[string]any{"site_name": site, "url": nil}) { + t.Errorf("source of %s = %v, want site_name %q and url null", what, src, site) + } + } + withheld("a link with a query token", queryID, "B Kitchen", queryPage, "secret-query.example", queryToken, "access_token") + + // The other unsafe shapes, stored as a link grounded elsewhere would + // leave them. + for _, tc := range []struct{ what, link string }{ + {"a link with a userinfo token", "https://user-token-ghi789@kitchen.example/recipes/chicken-curry"}, + {"a link with a userinfo password", "https://cook:password-jkl012@kitchen.example/recipes/chicken-curry"}, + {"an http link", "http://kitchen.example/recipes/chicken-curry"}, + {"a link with a fragment", "https://kitchen.example/recipes/chicken-curry#token=fragment-mno345"}, + } { + if _, err := e.st.Writer.Exec(`UPDATE recipes SET source_url = ? WHERE id = (SELECT recipe_id FROM dinners WHERE id = ?)`, tc.link, cleanID); err != nil { + t.Fatal(err) + } + withheld(tc.what, cleanID, "Curry House", tc.link, "kitchen.example", "-token-", "password", "fragment") + } + + // web/src/dinners/testdata/source_url_cases.json pins source as this + // handler sends it for each stored link; the recipe screen's tests + // render each one, so a link the server sends and the client refuses + // fails one side or the other. + fixture, err := os.ReadFile("../../web/src/dinners/testdata/source_url_cases.json") + if err != nil { + t.Fatal(err) + } + var shared struct { + Cases []struct { + Stored string + Source map[string]any + } + } + if err := json.Unmarshal(fixture, &shared); err != nil { + t.Fatal(err) + } + if len(shared.Cases) == 0 { + t.Fatal("source_url_cases.json has no cases") + } + for _, c := range shared.Cases { + if _, err := e.st.Writer.Exec(`UPDATE recipes SET source_url = ? WHERE id = (SELECT recipe_id FROM dinners WHERE id = ?)`, c.Stored, cleanID); err != nil { + t.Fatal(err) + } + if src := source(c.Stored, cleanID); !reflect.DeepEqual(src, c.Source) { + got, _ := json.Marshal(src) + want, _ := json.Marshal(c.Source) + t.Errorf("source of %s = %s, want %s", c.Stored, got, want) } } } @@ -270,14 +333,16 @@ func TestGetDinnerSourceHidesTokenURL(t *testing.T) { // token has site_name null and nothing from the link in its detail; a page // that names its site keeps that name, even on a token-bearing host; and a // stored name shaped like another host, as a redirect leaves, is withheld. +// The links carry a query, so source.url is withheld too and nothing of the +// link may appear. func TestGetDinnerSiteNameNeverFromLink(t *testing.T) { t.Parallel() e := newDinnersEnv(t) const ( bareToken = "private-token-abc123" namedToken = "named-token-def456" - bare = "https://" + bareToken + ".example.com/recipe" - named = "https://www." + namedToken + ".example.com/recipe" + bare = "https://" + bareToken + ".example.com/recipe?print=1" + named = "https://www." + namedToken + ".example.com/recipe?print=1" ) e.pages.Bodies[bare] = recipestest.RecipePage("Chicken Curry", "", "https://a.example/curry.jpg", 40) e.pages.Bodies[named] = recipestest.RecipePage("Weeknight Curry", "Curry House", "https://b.example/curry.jpg", 25) @@ -315,8 +380,8 @@ func TestGetDinnerSiteNameNeverFromLink(t *testing.T) { } } raw, _ := getDetail(t, e.h, id) - if src, _ := raw["source"].(map[string]any); !reflect.DeepEqual(src, map[string]any{"site_name": want}) { - t.Errorf("source of %s = %v, want only site_name %v", what, raw["source"], want) + if src, _ := raw["source"].(map[string]any); !reflect.DeepEqual(src, map[string]any{"site_name": want, "url": nil}) { + t.Errorf("source of %s = %v, want site_name %v and url null", what, raw["source"], want) } } @@ -324,13 +389,13 @@ func TestGetDinnerSiteNameNeverFromLink(t *testing.T) { if got := storedSite(bareRecipe); got != bareToken+".example.com" { t.Fatalf("stored site_name = %q, want the link's host", got) } - check("a page naming no site", bareID, []string{bare, bareToken, "example.com", `"url"`}, nil) + check("a page naming no site", bareID, []string{bare, bareToken, "example.com"}, nil) namedID, namedRecipe := ground("Weeknight curry", named) if got := storedSite(namedRecipe); got != "Curry House" { t.Fatalf("stored site_name = %q, want the page's", got) } - check("a page naming its site", namedID, []string{named, namedToken, "example.com", `"url"`}, "Curry House") + check("a page naming its site", namedID, []string{named, namedToken, "example.com"}, "Curry House") const redirectHost = "redirect-token-ghi789.example.net" if _, err := e.st.Writer.Exec(`UPDATE recipes SET site_name = ? WHERE id = ?`, redirectHost, namedRecipe); err != nil { diff --git a/web/src/dinners/DinnerDetailPage.test.tsx b/web/src/dinners/DinnerDetailPage.test.tsx index 52ee959..756b9ab 100644 --- a/web/src/dinners/DinnerDetailPage.test.tsx +++ b/web/src/dinners/DinnerDetailPage.test.tsx @@ -9,6 +9,7 @@ import { errorResponse, jsonResponse, stubFetchByPath } from '../test/fetchStub. import { mockViewport } from '../test/matchMedia.ts' import { GROUNDING_POLL_MS } from './api.ts' import { DinnerDetailPage, REGULAR_DINNERS } from './DinnerDetailPage.tsx' +import sourceCases from './testdata/source_url_cases.json' const DINNER = '/api/v1/dinners/d1' @@ -186,6 +187,23 @@ describe('DinnerDetailPage', () => { expect(screen.queryByRole('link', { name: /Recipe from/ })).not.toBeInTheDocument() }) + // Each source exactly as the Go handler sends it for a stored link + // (internal/server's TestGetDinnerSourceURLOnlyWhenSafe checks the same + // file), so every url the server sends must parse here. + it.each(sourceCases.cases)('renders the source the server sends for $stored', async ({ source }) => { + serve(detail({ source })) + renderAt(['/dinner/d1']) + + const credit = await screen.findByText('Recipe from Curry House') + expect(screen.queryByRole('alert')).not.toBeInTheDocument() + if (source.url === null) { + expect(credit.closest('a')).toBeNull() + expect(screen.queryByRole('link', { name: /Recipe from/ })).not.toBeInTheDocument() + } else { + expect(screen.getByRole('link', { name: 'Recipe from Curry House' })).toHaveAttribute('href', new URL(source.url).href) + } + }) + it('leaves the credit out when the server names no site', async () => { serve(detail({ source: { site_name: null } })) renderAt(['/dinner/d1']) diff --git a/web/src/dinners/testdata/source_url_cases.json b/web/src/dinners/testdata/source_url_cases.json new file mode 100644 index 0000000..4d64542 --- /dev/null +++ b/web/src/dinners/testdata/source_url_cases.json @@ -0,0 +1,52 @@ +{ + "cases": [ + { + "stored": "https://kitchen.example/recipes/chicken-curry", + "source": { "site_name": "Curry House", "url": "https://kitchen.example/recipes/chicken-curry" } + }, + { + "stored": "https://Kitchen-2.Example.com.:8443/recipes/chicken-curry", + "source": { "site_name": "Curry House", "url": "https://Kitchen-2.Example.com.:8443/recipes/chicken-curry" } + }, + { + "stored": "https://[2001:db8::1]/recipes/chicken-curry", + "source": { "site_name": "Curry House", "url": "https://[2001:db8::1]/recipes/chicken-curry" } + }, + { + "stored": "https://192.0.2.7/recipes/chicken-curry", + "source": { "site_name": "Curry House", "url": "https://192.0.2.7/recipes/chicken-curry" } + }, + { + "stored": "https://[bad]/recipes/chicken-curry", + "source": { "site_name": "Curry House", "url": null } + }, + { + "stored": "https://[fe80::1%25en0]/recipes/chicken-curry", + "source": { "site_name": "Curry House", "url": null } + }, + { + "stored": "https://192.0.2.999/recipes/chicken-curry", + "source": { "site_name": "Curry House", "url": null } + }, + { + "stored": "https://xn--a.example/recipes/chicken-curry", + "source": { "site_name": "Curry House", "url": null } + }, + { + "stored": "https://kitchen.example/recipes/chicken-curry?access_token=query-token", + "source": { "site_name": "Curry House", "url": null } + }, + { + "stored": "https://user-token@kitchen.example/recipes/chicken-curry", + "source": { "site_name": "Curry House", "url": null } + }, + { + "stored": "http://kitchen.example/recipes/chicken-curry", + "source": { "site_name": "Curry House", "url": null } + }, + { + "stored": "https://kitchen.example/recipes/chicken-curry#token=fragment-token", + "source": { "site_name": "Curry House", "url": null } + } + ] +}