diff --git a/docs/reference/http-api.md b/docs/reference/http-api.md index e9469db1..0a680ac5 100644 --- a/docs/reference/http-api.md +++ b/docs/reference/http-api.md @@ -174,7 +174,7 @@ about the endpoint's rollout status on upstream clusters. | Method & path | Scope | Notes | |---|---|---| | `POST /repos` | repo:write (repo=new id) | Optional body `{repo_name?, default_branch?, upstream?, base_repo?}` (details below). → `{repo_id, repo_name, http_url, message}`. **201** = created; 200 = the name holds a finished repo the request matches; 409 = another live repo holds the name; 404 = a deleted repo holds the name; 503 = a create of the name is not finished (below). | -| `GET /repos` | org:read (repo="org" or no repo) | Query `cursor, limit, q, prefix`. `q` is a case-insensitive substring of repo `url`, and `prefix` a case-insensitive start of it, both trimmed (empty = no filter). With `prefix` set, `q` matches only the rest of `url` after the prefix: `prefix=pierre/&q=sdk` returns `pierre/sdk-documentation`, not `pierre/example`. → `{repos: [{repo_id, repo_name, url (deprecated mirror), default_branch, created_at, upstream?, base_repo?}], next_cursor, has_more}` | +| `GET /repos` | org:read (repo="org" or no repo) | Query `cursor, limit, q, prefix`. `q` is a case-insensitive substring of repo `url`, and `prefix` a case-insensitive start of it, both trimmed (empty = no filter). With `prefix` set, `q` matches only the rest of `url` after the prefix: `prefix=pierre/&q=sdk` returns `pierre/sdk-documentation`, not `pierre/example`. A `q` or `prefix` with a NUL byte or invalid UTF-8 is a 400. → `{repos: [{repo_id, repo_name, url (deprecated mirror), default_branch, created_at, upstream?, base_repo?}], next_cursor, has_more}` | | `GET /repo` | git:read (repo from JWT) | → `{repo_id, repo_name, url (deprecated mirror), default_branch, created_at, upstream?, base_repo?}`; 404 → SDK returns null | | `PATCH /repo` | repo:write | Update the default branch; body `{default_branch}` (details below). → `{repo_id, id (deprecated mirror), repo_name, url (deprecated mirror), default_branch}` (no `created_at`, per the live spec) | | `DELETE /repos/delete` | repo:write | Soft delete + async cleanup. → `{repo_name, repo_id (deprecated mirror), message}`; 404 not found; 409 already deleted | diff --git a/internal/api/api_integration_test.go b/internal/api/api_integration_test.go index dc1b704b..6ec4008a 100644 --- a/internal/api/api_integration_test.go +++ b/internal/api/api_integration_test.go @@ -690,6 +690,46 @@ func testListPrefix(t *testing.T, e *testEnv) { } } +// TestListFilterValues pins q and prefix values that no repo_name holds: +// a value of 200 KiB lists no repo (SQLite refuses a LIKE pattern over +// 50,000 bytes), and a NUL byte or invalid UTF-8, which PostgreSQL refuses +// in a text parameter, is the client's 400. +func TestListFilterValues(t *testing.T) { runBackends(t, testListFilterValues) } + +func testListFilterValues(t *testing.T, e *testEnv) { + org := e.newOrg(t) + createRepo(t, e, org, "acme/alpha", nil, http.StatusCreated) + orgTok := e.mint(t, tokenSpec{iss: org, repo: auth.RepoClaimOrg, scopes: []string{auth.ScopeOrgRead}}) + long := strings.Repeat("a", 200<<10) + for _, tc := range []struct { + name string + params url.Values + status int + }{ + {"q of 200 KiB", url.Values{"q": {long}}, http.StatusOK}, + {"prefix of 200 KiB", url.Values{"prefix": {long}}, http.StatusOK}, + {"q of 25,001 percent signs", url.Values{"q": {strings.Repeat("%", 25_001)}}, http.StatusOK}, + {"q with a NUL byte", url.Values{"q": {"a\x00b"}}, http.StatusBadRequest}, + {"prefix with a NUL byte", url.Values{"prefix": {"acme\x00"}}, http.StatusBadRequest}, + {"q not UTF-8", url.Values{"q": {"a\xffb"}}, http.StatusBadRequest}, + {"prefix not UTF-8", url.Values{"prefix": {"\xff"}}, http.StatusBadRequest}, + } { + t.Run(tc.name, func(t *testing.T) { + resp, body := e.doReq(t, http.MethodGet, "/api/v1/repos?"+tc.params.Encode(), orgTok, nil, "") + if resp.StatusCode != tc.status { + t.Fatalf("status = %d (body %.300s), want %d", resp.StatusCode, body, tc.status) + } + if tc.status != http.StatusOK { + return + } + var page listReposResponse + if err := json.Unmarshal(body, &page); err != nil || len(page.Repos) != 0 || page.HasMore { + t.Fatalf("body %.300s (err %v), want an empty last page", body, err) + } + }) + } +} + // TestAuthMatrix drives the docs/reference/http-api.md 401/403 failure map across the // repo lifecycle endpoints (create and list on both path styles), asserting status, // envelope shape, and WWW-Authenticate on 401. diff --git a/internal/api/handlers.go b/internal/api/handlers.go index 22582d2f..0b63fbd1 100644 --- a/internal/api/handlers.go +++ b/internal/api/handlers.go @@ -13,6 +13,7 @@ import ( "strconv" "strings" "time" + "unicode/utf8" "github.com/coder/openstorage/internal/auth" "github.com/coder/openstorage/internal/blob" @@ -637,9 +638,16 @@ func (s *server) handleListRepos(w http.ResponseWriter, r *http.Request) { limit = n } + q, prefix := strings.TrimSpace(query.Get("q")), strings.TrimSpace(query.Get("prefix")) + for _, f := range []struct{ name, value string }{{"q", q}, {"prefix", prefix}} { + if !utf8.ValidString(f.value) || strings.ContainsRune(f.value, 0) { + writeErr(w, http.StatusBadRequest, "invalid "+f.name+" (want UTF-8 text without NUL)") + return + } + } res, err := s.store.ListRepos(r.Context(), claims.Iss, meta.ListParams{ - Q: strings.TrimSpace(query.Get("q")), - Prefix: strings.TrimSpace(query.Get("prefix")), + Q: q, + Prefix: prefix, Cursor: query.Get("cursor"), Limit: limit, }) diff --git a/internal/meta/meta.go b/internal/meta/meta.go index 51104f91..c1deb4d6 100644 --- a/internal/meta/meta.go +++ b/internal/meta/meta.go @@ -13,7 +13,9 @@ import ( "fmt" "log/slog" "net/url" + "strings" "time" + "unicode/utf8" ) // Sentinel errors implementations must return (possibly wrapped; callers @@ -114,12 +116,23 @@ const ( // value here (default 20, max 100); the opaque cursor is validated by the // store (ErrInvalidCursor). type ListParams struct { - Q string // case-insensitive substring filter on URL, pre-trimmed; "" = no filter; with Prefix set, only the URL after the prefix - Prefix string // case-insensitive filter on the start of URL, pre-trimmed; "" = no filter + Q string // case-insensitive substring filter on URL, pre-trimmed UTF-8 without NUL; "" = no filter; with Prefix set, only the URL after the prefix + Prefix string // case-insensitive filter on the start of URL, pre-trimmed UTF-8 without NUL; "" = no filter Cursor string // opaque cursor from a previous ListResult; "" = first page Limit int // page size, already clamped/validated by the caller } +// assertListFilters checks the ListParams filter contract. PostgreSQL +// refuses a NUL byte and invalid UTF-8 in a text parameter (SQLSTATE +// 22021), so the API refuses them first, and every backend gets the same +// filters. +func assertListFilters(p ListParams) { + for _, v := range []string{p.Q, p.Prefix} { + assertf(v == strings.TrimSpace(v) && utf8.ValidString(v) && !strings.ContainsRune(v, 0), + "ListRepos: filter %.80q is untrimmed, not UTF-8, or holds a NUL byte", v) + } +} + // ListResult is one page of repos ordered by (created_at DESC, repo_id DESC). type ListResult struct { Repos []Repo // never nil; empty slice when no matches diff --git a/internal/meta/pg.go b/internal/meta/pg.go index 08ae7ffa..e5bb24de 100644 --- a/internal/meta/pg.go +++ b/internal/meta/pg.go @@ -615,8 +615,7 @@ func escapeLike(s string) string { // prefix at the start, which consumes exactly the prefix's characters, so q // can only match the rest of the url after it. func listURLPattern(p ListParams) string { - assertf(p.Q == strings.TrimSpace(p.Q) && p.Prefix == strings.TrimSpace(p.Prefix), - "ListRepos: untrimmed filters q=%q prefix=%q", p.Q, p.Prefix) + assertListFilters(p) if p.Q == "" && p.Prefix == "" { return "" } diff --git a/internal/meta/pg_test.go b/internal/meta/pg_test.go index 2c7a0f29..7c914abf 100644 --- a/internal/meta/pg_test.go +++ b/internal/meta/pg_test.go @@ -625,6 +625,10 @@ func testListReposQFilter(t *testing.T, s Backend) { if got := urls(all); !slices.Equal(got, []string{"github.com/acme/Alpha", "github.com/acme/beta-ALPHA"}) { t.Errorf("q=alpha -> %v", got) } + all, _ = listAll(t, s, org, "GAMMA", 10) // q in another case than the url + if got := urls(all); !slices.Equal(got, []string{"github.com/acme/gamma"}) { + t.Errorf("q=GAMMA -> %v, want the gamma url", got) + } // LIKE metacharacters in q are literals, not wildcards. all, _ = listAll(t, s, org, "%", 10) @@ -664,6 +668,7 @@ func testListReposPrefixFilter(t *testing.T, s Backend) { "Pierre/Upper-SDK", "pierre/pierre-mirror", "acme/pierre-tools", + "acme/pierre/inner", // holds the prefix 'pierre/', but not at the start "50%/off", "500/decoy", // matches prefix '50%' only if % is unescaped "a_b/one", @@ -722,6 +727,48 @@ func testListReposPrefixFilter(t *testing.T, s Backend) { } } +func TestListReposLongFilters(t *testing.T) { runBackends(t, testListReposLongFilters) } + +// testListReposLongFilters pins q and prefix of any length: SQLite refuses +// a LIKE pattern over 50,000 bytes (SQLITE_MAX_LIKE_PATTERN_LENGTH), and +// an escaped `%` takes two of them. +func testListReposLongFilters(t *testing.T, s Backend) { + ctx := t.Context() + org := seedOrg(t, s) + percents := strings.Repeat("%", 30_000) + long := "acme/" + percents + "/end" + for _, url := range []string{"acme/alpha", long} { + if _, err := s.CreateRepo(ctx, org, url, "main"); err != nil { + t.Fatalf("create %.40s: %v", url, err) + } + } + for _, tc := range []struct { + name, prefix, q string + want []string + }{ + {"q of 200 KiB", "", strings.Repeat("a", 200<<10), nil}, + {"prefix of 200 KiB", strings.Repeat("a", 200<<10), "", nil}, + {"q of 30,000 percent signs", "", percents, []string{long}}, + {"q one percent sign longer", "", percents + "%", nil}, + {"prefix of 30,000 percent signs and q", "acme/" + percents, "END", []string{long}}, + {"q inside a long prefix", "acme/" + percents, "%", nil}, + } { + t.Run(tc.name, func(t *testing.T) { + res, err := s.ListRepos(ctx, org, ListParams{Q: tc.q, Prefix: tc.prefix, Limit: 10}) + if err != nil { + t.Fatalf("list: %.300v", err) + } + got := make([]string, 0, len(res.Repos)) + for _, r := range res.Repos { + got = append(got, r.URL) + } + if !slices.Equal(got, tc.want) { + t.Errorf("prefix %d bytes, q %d bytes: got %d repos, want %d", len(tc.prefix), len(tc.q), len(got), len(tc.want)) + } + }) + } +} + func TestListReposErrors(t *testing.T) { runBackends(t, testListReposErrors) } func testListReposErrors(t *testing.T, s Backend) { diff --git a/internal/meta/s3walmeta.go b/internal/meta/s3walmeta.go index d7524695..dd732fc9 100644 --- a/internal/meta/s3walmeta.go +++ b/internal/meta/s3walmeta.go @@ -1415,8 +1415,7 @@ func (s *S3WAL) ListRepos(ctx context.Context, org string, params ListParams) (L } cursorNanos, cursorID = createdAt.UnixNano(), repoID } - assertf(params.Q == strings.TrimSpace(params.Q) && params.Prefix == strings.TrimSpace(params.Prefix), - "ListRepos: untrimmed filters q=%q prefix=%q", params.Q, params.Prefix) + assertListFilters(params) q, prefix := strings.ToLower(params.Q), strings.ToLower(params.Prefix) rows := make([]Repo, 0, params.Limit+1) for id, r := range doc.Repos { diff --git a/internal/meta/s3walmeta_test.go b/internal/meta/s3walmeta_test.go index c2270796..174fec5a 100644 --- a/internal/meta/s3walmeta_test.go +++ b/internal/meta/s3walmeta_test.go @@ -462,6 +462,7 @@ func TestS3WALMetaConformance(t *testing.T) { {"ListReposPagination", testListReposPagination}, {"ListReposQFilter", testListReposQFilter}, {"ListReposPrefixFilter", testListReposPrefixFilter}, + {"ListReposLongFilters", testListReposLongFilters}, {"ListReposErrors", testListReposErrors}, {"Webhooks", testWebhooks}, {"GitCredentials", testGitCredentials}, diff --git a/internal/meta/sqlite.go b/internal/meta/sqlite.go index a9c93f8b..e85780fc 100644 --- a/internal/meta/sqlite.go +++ b/internal/meta/sqlite.go @@ -724,7 +724,9 @@ func (s *SQLite) RemoveSigningKey(ctx context.Context, org string, id int64) err // ListRepos pages live repos for the org, keyset-ordered by // (created_at DESC, repo_id DESC). // -// PG's `url ILIKE ?` becomes `lower(url) LIKE lower(?) ESCAPE '\'`. SQLite's +// PG's `url ILIKE ?` becomes a start and a substring test of lower(url), +// not LIKE: SQLite refuses a LIKE pattern over 50,000 bytes +// (SQLITE_MAX_LIKE_PATTERN_LENGTH), and q and prefix can be longer. SQLite's // lower() only folds ASCII, so non-ASCII urls match case-sensitively here // while PG ILIKE folds per locale — an accepted divergence for local mode. func (s *SQLite) ListRepos(ctx context.Context, org string, params ListParams) (ListResult, error) { @@ -739,9 +741,15 @@ func (s *SQLite) ListRepos(ctx context.Context, org string, params ListParams) ( query := `SELECT ` + repoCols + ` FROM repos WHERE org_id = ? AND deleted_at IS NULL` args := []any{orgID} - if pattern := listURLPattern(params); pattern != "" { - args = append(args, pattern) - query += ` AND lower(url) LIKE lower(?) ESCAPE '\'` + assertListFilters(params) + if params.Prefix != "" { + args = append(args, params.Prefix, params.Prefix) + query += ` AND substr(lower(url), 1, length(?)) = lower(?)` + } + if params.Q != "" { + // With a prefix, q matches only the rest of the url after it. + args = append(args, params.Prefix, params.Q) + query += ` AND instr(substr(lower(url), length(?) + 1), lower(?)) > 0` } if params.Cursor != "" { createdAt, repoID, err := decodeCursor(params.Cursor)