Withhold recipe source urls whose host the browser cannot parse
safeSourceURL accepted hosts that Go's url.Parse takes but the browser's URL parser refuses, such as "https://[bad]/curry", so the detail API could send a source.url that sourceUrl() in web/src/dinners/detail.ts rejects, failing the whole recipe screen. The server now also requires a host the browser parses as it stands: a bracketed host must be an IPv6 address without a zone; any other must be ASCII letters, digits and hyphens in non-empty dot-separated labels, with no punycode label and, when its last label is a number, a valid dotted quad; and the host must be spelled out, not percent-encoded. Any other link is withheld as before. web/src/dinners/testdata/source_url_cases.json pins the source object the handler sends for each stored link. The Go server test checks the handler against it, and the recipe screen test renders each source from it, so the two parsers cannot drift apart on those links. Task: c1a97243-3e41-42fc-a1a0-1ac684ccb463 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -216,14 +216,21 @@ func pageSiteName(stored sql.NullString, sourceURL string) (*string, error) {
|
||||
// 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) refuses any other link, so the port must also
|
||||
// be one it parses.
|
||||
// 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
|
||||
@@ -232,6 +239,40 @@ func safeSourceURL(sourceURL string) *string {
|
||||
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
|
||||
|
||||
@@ -122,6 +122,9 @@ func TestSafeSourceURLOnlyWithoutSecrets(t *testing.T) {
|
||||
"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))
|
||||
@@ -144,6 +147,21 @@ func TestSafeSourceURLOnlyWithoutSecrets(t *testing.T) {
|
||||
"//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)
|
||||
|
||||
@@ -5,6 +5,7 @@ import (
|
||||
"encoding/json"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"os"
|
||||
"reflect"
|
||||
"strings"
|
||||
"testing"
|
||||
@@ -294,6 +295,37 @@ func TestGetDinnerSourceURLOnlyWhenSafe(t *testing.T) {
|
||||
}
|
||||
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)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// TestGetDinnerSiteNameNeverFromLink: a page that names no site is stored
|
||||
|
||||
@@ -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'])
|
||||
|
||||
+52
@@ -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 }
|
||||
}
|
||||
]
|
||||
}
|
||||
Reference in New Issue
Block a user