diff --git a/.github/workflows/fuzz-nightly.yml b/.github/workflows/fuzz-nightly.yml index b73e1298..4e19f561 100644 --- a/.github/workflows/fuzz-nightly.yml +++ b/.github/workflows/fuzz-nightly.yml @@ -52,6 +52,9 @@ jobs: ./internal/jobs:FuzzPackObjectCount ./internal/api:FuzzClientKey ./internal/api:FuzzCoreCursor + ./internal/api:FuzzDecodeRawPath + ./internal/api:FuzzDecodeRawPatterns + ./internal/api:FuzzCommitPackFilePath ./internal/api:FuzzApplyRejection ./internal/api:FuzzPatchModeError ./internal/sync:FuzzParsePushRejections diff --git a/docs/explanation/streaming-commit-protocols.md b/docs/explanation/streaming-commit-protocols.md index 3e9161c0..26e953fd 100644 --- a/docs/explanation/streaming-commit-protocols.md +++ b/docs/explanation/streaming-commit-protocols.md @@ -68,6 +68,11 @@ Lines 2..N — one or more blob chunk frames per `upsert` file, in order: get 400 `invalid`. - Clients normalize paths (leading slashes stripped); the backend should still reject `..` traversal and absolute paths defensively. +- A file whose name is not valid UTF-8 sends `path_base64`, the base64 of the raw + bytes, in place of `path`: exactly one of the two, and an empty one counts as absent. + A bad value gets 400 `invalid` + ([path encoding](../reference/http-api.md#conventions)). A replica older than + v0.20.0 does not read `path_base64` and answers 400 `invalid`. - Modes: `100644`, `100755`, `120000` (symlink), `160000` (submodule). ### Server semantics diff --git a/docs/explanation/wire-compatibility.md b/docs/explanation/wire-compatibility.md index dbafa3c0..63886ea3 100644 --- a/docs/explanation/wire-compatibility.md +++ b/docs/explanation/wire-compatibility.md @@ -210,7 +210,7 @@ the [archived phase notes](../archive/phases/)): | `GET .../file` and `POST .../archive`: no documented file-size cap and no `422`; `archive` has only the caller-chosen `max_blob_size` | `file` and `archive` answer 422 for a file over 100 MiB (`archive` can skip such files with `max_blob_size`), before any response byte. The other read-side git [resource limits](../reference/http-api.md#resource-limits-on-git-reads) apply too. After an `archive` response has started, a limit closes the connection instead of returning a status | Hardening ([ADR-49](../adr/049-read-side-git-children-run-under-one-per-request-budget.md)): the limits bound server memory, temporary disk and delivery time for data that anyone who can push controls. Fetch larger files with git | | Every `503` response can carry an optional `Retry-After` header, the minimum delay in seconds before a retry (spec 2026-10-21) | Two 503s leave `Retry-After` out: a git read that exceeds its [time budget](../reference/http-api.md#resource-limits-on-git-reads), and a sync create whose upstream link was removed before its first sync was scheduled ([pending creates](../reference/http-api.md#repository-lifecycle-repowrite--orgread)). The 503 for exhausted git concurrency and the 503s for a create that is not finished carry `Retry-After` | A retry of either request fails the same way, so a `Retry-After` would invite a useless retry. The same page needs the same git time ([ADR-49](../adr/049-read-side-git-children-run-under-one-per-request-budget.md)). The pending create stays pending until `DELETE /repos/delete` clears the name ([#388](https://github.com/coder/openstorage/issues/388)) | | A token that fails verification (malformed, bad signature, expired, unknown issuer): upstream's live docs and servers answer `403` `forbidden` (`Invalid or expired token`) on the REST API, and `403` on the Git and LFS transports too (checked 2026-11-04). The vendored spec documents no status for it | The REST API answers `403` `forbidden` (`Invalid or expired token`), as upstream does ([#664](https://github.com/coder/openstorage/issues/664)). The Git and LFS transports answer `401` with `WWW-Authenticate: Basic`, as for a missing token ([authentication](../reference/authentication.md#failure-mapping)) | Git asks for credentials only after a `401`, and only a `401` makes it erase a stored credential. After a `403`, git fails and keeps the expired token in the credential helper, so every later command fails the same way until the user removes the token | -| Every path field and `path` parameter is a plain `type: string`. The spec says nothing about tree entry names whose bytes are not valid UTF-8 | A response path that is not valid UTF-8 still lists with one U+FFFD per invalid byte, and gets an extra sibling `_base64` with the raw bytes in standard base64 (for example `path_base64`, `paths_base64`, `old_path_base64`, `original_path_base64`). The sibling is absent for valid UTF-8 paths, so those responses are unchanged. This covers `files`, `files/metadata`, the diff routes, `blame`, `grep` and the merge conflict paths (`conflict_paths_base64` and `path_base64`). JSON request bodies cannot name such a file yet. The `path` query parameter takes the raw bytes percent-encoded ([path encoding](../reference/http-api.md#conventions)) | JSON strings hold Unicode text, so such a byte has no exact JSON form. A sibling field (like GitHub's `pathRaw`) is additive: no SDK reads it, and the SDKs ignore response keys they do not know. Upstream's schemas for these responses are closed (`additionalProperties: false`), so a client that validates a response against the vendored spec as written rejects one that has a sibling. No SDK validates that way. If upstream documents another form, we carry both ([#587](https://github.com/coder/openstorage/issues/587)) | +| Every path field and `path` parameter is a plain `type: string`. The spec says nothing about tree entry names whose bytes are not valid UTF-8 | A response path that is not valid UTF-8 still lists with one U+FFFD per invalid byte, and gets an extra sibling `_base64` with the raw bytes in standard base64 (for example `path_base64`, `paths_base64`, `old_path_base64`, `original_path_base64`). The sibling is absent for valid UTF-8 paths, so those responses are unchanged. This covers `files`, `files/metadata`, the diff routes, `blame`, `grep` and the merge conflict paths (`conflict_paths_base64` and `path_base64`). A JSON request body names such a path with `paths_base64` and `include_globs_base64`/`exclude_globs_base64` (grep and archive) or `path_base64` (a commit-pack file operation), from v0.20.0 on. An older replica ignores the request fields: grep and archive apply only the plain filters, so the result can hold more or fewer files, and commit-pack answers 400. The `path` query parameter takes the raw bytes percent-encoded ([path encoding](../reference/http-api.md#conventions)) | JSON strings hold Unicode text, so such a byte has no exact JSON form. A sibling field (like GitHub's `pathRaw`) is additive: no SDK reads it, and the SDKs ignore response keys they do not know. Upstream's schemas for these responses are closed (`additionalProperties: false`), so a client that validates a response against the vendored spec as written rejects one that has a sibling. No SDK validates that way. If upstream documents another form, we carry both ([#587](https://github.com/coder/openstorage/issues/587)) | Extensions that upstream's spec does not list: @@ -231,6 +231,11 @@ Extensions that upstream's spec does not list: `paths_base64`, `old_path_base64`, `original_path_base64`, `conflict_paths_base64`), present only for a path that is not valid UTF-8 ([path encoding](../reference/http-api.md#conventions)); +- the matching request fields: `paths_base64` and `file_filters.include_globs_base64` and + `exclude_globs_base64` on `POST .../grep`, `include_globs_base64` and + `exclude_globs_base64` on `POST .../archive`, and `files[].path_base64` on + `POST .../commit-pack`. Upstream's request schemas for these are closed too, so + upstream's own servers reject them; - the unauthenticated `/healthz` liveness probe. ## How drift is tracked diff --git a/docs/reference/http-api.md b/docs/reference/http-api.md index a5531ab2..13b58433 100644 --- a/docs/reference/http-api.md +++ b/docs/reference/http-api.md @@ -133,11 +133,29 @@ are in [`../upstream/openapi.yaml`](../upstream/openapi.yaml). raw bytes percent-encoded in the `path` query parameter, for example `file?path=a%FFb`. `file`, `files`, `files/metadata`, `commits`, `diff`, `branches/diff` and `blame` read `path` byte for byte. An `archive` (tar member - names) or a git clone gives the raw names. A JSON request body cannot hold such a - name yet: grep `paths` and commit-pack file paths cannot name the file, and the - archive and grep globs match it only through a wildcard (`a?b`, `a*`), which can - match other names too. Upstream documents no path encoding, so the siblings are an - extension ([#587](https://github.com/coder/openstorage/issues/587)). + names) or a git clone gives the raw names. + + A JSON request body names such a path with a base64 form of the field: + - `grep`: `paths_base64` adds prefixes to `paths`, and `file_filters` takes + `include_globs_base64` and `exclude_globs_base64` next to the plain globs. + - `archive`: `include_globs_base64` and `exclude_globs_base64`. + - `commit-pack`: a file operation sends `path_base64` instead of `path`. It has exactly + one of the two, and an empty one counts as absent. + + A value must be canonical base64 as in the siblings: standard alphabet, padding, no + line breaks. In grep and archive, a value that is not, or that decodes to a blank name + or one with a NUL byte, is a 400 with an `invalid_params` entry that names it + (`paths_base64.1`). In commit-pack, a value that is not canonical base64 is a 400 + `invalid` with a message, and a decoded path goes through the checks of `path`. A + decoded glob matches the + raw bytes: an invalid byte matches only itself, `?` matches one byte of such a name, + and `*` matches any run. A `[...]` class that holds an invalid byte is malformed and + matches nothing. A replica older than v0.20.0 ignores the grep and archive fields, so + it applies only the plain `paths` and globs of the request: the result can hold more + files (only a base64 field was sent) or fewer (a plain filter was sent next to it). It + answers a commit-pack `path_base64` with 400 `invalid`. Upstream documents no path + encoding, so the siblings and these fields are an extension + ([#587](https://github.com/coder/openstorage/issues/587)). - **No client retries**: no SDK retries; every error surfaces immediately. Python client-side timeouts: 30s default, 60s diff/grep, 180s branch-create/merge/merge-preview/commit/tag/restore-commit. @@ -341,8 +359,8 @@ or NUL fails the sync as a configuration error. | `HEAD /repos/file` | same | headers only | | `GET /repos/files` | `ref, ephemeral, path, recursive, cursor, limit` | `{paths: [..], paths_base64?: [..], ref, entries: [{path, path_base64?, type: blob\|tree\|symlink\|submodule, mode}], next_cursor, has_more}` — trees have no trailing `/`; branch on `type`. Without `recursive`, with an empty `recursive=`, or with `recursive=true`, it lists the contents of every folder under `path` and no `tree` entries. `recursive=false` lists only the direct children of `path`, folders as `tree` entries. No `cursor` and no `limit` (or `limit=0`): every entry, or 422 when the listing passes the git [output limit](#resource-limits-on-git-reads). Big trees must page: a recursive call with `limit` or `cursor` reads the tree only up to the end of its page, so a deep cursor takes longer than the first page but needs no more memory. A tree that git stores out of order (one that `git fsck` rejects as `treeNotSorted`) can page in git's order and skip entries; git itself cannot look such entries up by path | | `GET /repos/files/metadata` | same | `{files: [{path, path_base64?, mode, size, last_commit_sha, type}], commits: {: {author, date, message, order}}, ref, next_cursor, has_more}`. `order` is described below the table. `message` is the subject line only, as in the commits list ([Commits](#commits)). No `cursor` and no `limit` (or `limit=0`): every file. **429** when the files under `path` have more than 50,000 distinct last commits, also with `limit` or `cursor`: a retry gets the same 429 (no `Retry-After`), so send a narrower `path`. **404 on unborn branch** | -| `POST /repos/archive` | body `{ref?, include_globs?, exclude_globs?, max_blob_size?, archive?: {prefix}}` | tar.gz stream | -| `POST /repos/grep` | body `{query: {pattern, case_sensitive?}, ref?, ephemeral?, paths?, file_filters?: {include_globs, exclude_globs, extension_filters}, context?: {before, after}, limits?: {max_lines, max_matches_per_file}, pagination?: {cursor, limit}}` | `{query, repo: {ref, commit}, matches: [{path, path_base64?, lines: [{line_number, text, type: "match"\|context}]}], next_cursor, has_more}` | +| `POST /repos/archive` | body `{ref?, include_globs?, include_globs_base64?, exclude_globs?, exclude_globs_base64?, max_blob_size?, archive?: {prefix}}` | tar.gz stream | +| `POST /repos/grep` | body `{query: {pattern, case_sensitive?}, ref?, ephemeral?, paths?, paths_base64?, file_filters?: {include_globs, include_globs_base64?, exclude_globs, exclude_globs_base64?, extension_filters}, context?: {before, after}, limits?: {max_lines, max_matches_per_file}, pagination?: {cursor, limit}}` | `{query, repo: {ref, commit}, matches: [{path, path_base64?, lines: [{line_number, text, type: "match"\|context}]}], next_cursor, has_more}` | | `GET /repos/blame` | `path` (req), `ref`, `ephemeral`, `range` (repeatable, `git blame -L` specs like `1,10`), `detect_moves` | `{ref, path, path_base64?, commit_sha, lines: [{line_number, commit_sha, original_line_number, original_path, original_path_base64?, previous_commit_sha?, author_name, author_email, author_time, committer_name, committer_email, committer_time, summary}]}` | `order` in `files/metadata` is the commit's history-order rank relative to the resolved ref: its 0-based position in `git rev-list --topo-order `. The ref's own commit is `0`, and larger values are older. A commit always ranks higher than every one of its descendants, whatever its author or committer date. Among a folder's files, the one with the smallest `order` was changed most recently in the ref's history. diff --git a/internal/api/fuzz_test.go b/internal/api/fuzz_test.go index 904ac6eb..6ecb8f47 100644 --- a/internal/api/fuzz_test.go +++ b/internal/api/fuzz_test.go @@ -157,6 +157,88 @@ func FuzzRawPathBase64(f *testing.F) { }) } +// FuzzDecodeRawPath: a request's path_base64 text decodes only when it is +// the canonical padded standard base64 of the bytes it yields, so no two +// texts name one path. +func FuzzDecodeRawPath(f *testing.F) { + for _, seed := range []string{"", "YQ==", "YQ=", "YQ", "YR==", "Yf9i", "!!", "Yf9\ni", "Yf9i\r\n", "-_8=", "+/8=", "AA==", "////", "Y W I=", "YQ==YQ=="} { + f.Add(seed) + } + f.Fuzz(func(t *testing.T, s string) { + p, ok := decodeRawPath(s) + if ok != (encodeRawPath(p) == s) || (!ok && p != "") { + t.Fatalf("decodeRawPath(%q) = %q, %v", s, p, ok) + } + // Every byte string has one text, and it decodes back. + if q, ok := decodeRawPath(encodeRawPath(s)); !ok || q != s { + t.Fatalf("decodeRawPath(encodeRawPath(%q)) = %q, %v", s, q, ok) + } + }) +} + +// FuzzDecodeRawPatterns: a decoded list holds one value per element, each +// not blank and with no NUL, and a failure names the first bad element. +func FuzzDecodeRawPatterns(f *testing.F) { + for _, seed := range [][2]string{ + {"YQ==", "Yf9i"}, {"", "YQ=="}, {"YQ==", ""}, {"AA==", "YQ=="}, {"IA==", "YQ=="}, + {"CQ==", "Yf9i"}, {"!!", "!!"}, {"YR==", "YQ=="}, {"YQ==", "YQ=="}, + } { + f.Add(seed[0], seed[1]) + } + f.Fuzz(func(t *testing.T, a, b string) { + list := []string{a, b} + decoded, bad := decodeRawPatterns(list) + valid := func(enc string) bool { + p, ok := decodeRawPath(enc) + return ok && strings.TrimSpace(p) != "" && !strings.ContainsRune(p, 0) + } + if bad < 0 { + if len(decoded) != 2 || encodeRawPath(decoded[0]) != a || encodeRawPath(decoded[1]) != b || !valid(a) || !valid(b) { + t.Fatalf("decodeRawPatterns(%q) = %q, %d", list, decoded, bad) + } + return + } + if decoded != nil || bad > 1 || valid(list[bad]) || (bad == 1 && !valid(a)) { + t.Fatalf("decodeRawPatterns(%q) = %q, %d", list, decoded, bad) + } + }) +} + +// FuzzCommitPackFilePath: a file operation names its path with exactly one +// of path and path_base64 (an empty one is absent), and the path it ends with +// is the one it names. +func FuzzCommitPackFilePath(f *testing.F) { + for _, seed := range [][2]string{ + {"a.txt", ""}, {"", "Yf9i"}, {"a.txt", "Yf9i"}, {"", ""}, {"", "YR=="}, {"", "Li4v"}, + {"", "LmdpdC9jb25maWc="}, {"", "YQBi"}, {"", "AA=="}, {"../a", ""}, {"a\xffb", ""}, + } { + f.Add(seed[0], seed[1]) + } + f.Fuzz(func(t *testing.T, path, enc string) { + files := []wcFileOp{{Path: path, PathBase64: enc, ContentID: "c", Operation: "upsert"}} + err := wcValidateFiles(files) + decoded, decodedOK := decodeRawPath(enc) + want := path + if enc != "" { + want = decoded + } + switch { + case enc != "" && (path != "" || !decodedOK): + if err == nil { + t.Fatalf("path %q, path_base64 %q: accepted", path, enc) + } + case err == nil: + if files[0].Path != want || wcValidPath(want) != nil { + t.Fatalf("path %q, path_base64 %q: resolved to %q, want the valid path %q", path, enc, files[0].Path, want) + } + default: + if wcValidPath(want) == nil { + t.Fatalf("path %q, path_base64 %q: refused with %v though %q is valid", path, enc, err, want) + } + } + }) +} + func FuzzCoreCursor(f *testing.F) { f.Add(coreCursorEncode(cursorKindCommits, "abc123", "2024-01-01T00:00:00Z"), "main", "src/a|b.txt") f.Add("", "", "") diff --git a/internal/api/handlers_read_query.go b/internal/api/handlers_read_query.go index a9fcaefd..a309b422 100644 --- a/internal/api/handlers_read_query.go +++ b/internal/api/handlers_read_query.go @@ -580,11 +580,16 @@ type grepRequestBody struct { Rev string `json:"rev"` // legacy alias (openapi); `ref` wins Ephemeral bool `json:"ephemeral"` Paths []string `json:"paths"` + // PathsBase64 and the *_base64 globs hold the base64 of raw bytes, for + // names that are not valid UTF-8. They add to the plain lists (#587). + PathsBase64 []string `json:"paths_base64,omitempty"` FileFilters struct { - IncludeGlobs []string `json:"include_globs"` - ExcludeGlobs []string `json:"exclude_globs"` - ExtensionFilters []string `json:"extension_filters"` + IncludeGlobs []string `json:"include_globs"` + IncludeGlobsBase64 []string `json:"include_globs_base64,omitempty"` + ExcludeGlobs []string `json:"exclude_globs"` + ExcludeGlobsBase64 []string `json:"exclude_globs_base64,omitempty"` + ExtensionFilters []string `json:"extension_filters"` } `json:"file_filters"` Context struct { Before *int `json:"before"` @@ -655,6 +660,20 @@ func decodeGrepCursor(cursor string) (commit, path string, line int, err error) return parts[0], parts[1], n, nil } +// mergeRawPatterns appends the decoded elements of a _base64 request +// list to the plain list it stands for. A bad element is a 400 that names +// it, and mergeRawPatterns reports false. +func mergeRawPatterns(w http.ResponseWriter, field string, encoded []string, into *[]string) bool { + decoded, bad := decodeRawPatterns(encoded) + if bad >= 0 { + name := field + "." + strconv.Itoa(bad) + writeInvalidField(w, name, "must be the base64 of a non-empty name", "invalid "+name) + return false + } + *into = append(*into, decoded...) + return true +} + // grepFileWanted applies the request's path prefixes and file filters to // one candidate path. Filtering runs server-side over the full grep result // (one uniform glob dialect — gitread.MatchGlob — shared with archive). @@ -774,6 +793,19 @@ func (s *server) handleGrep(w http.ResponseWriter, r *http.Request) { return } } + for _, in := range []struct { + field string + list []string + into *[]string + }{ + {"paths_base64", body.PathsBase64, &body.Paths}, + {"file_filters.include_globs_base64", body.FileFilters.IncludeGlobsBase64, &body.FileFilters.IncludeGlobs}, + {"file_filters.exclude_globs_base64", body.FileFilters.ExcludeGlobsBase64, &body.FileFilters.ExcludeGlobs}, + } { + if !mergeRawPatterns(w, in.field, in.list, in.into) { + return + } + } // Narrow the git-side search to the requested path prefixes (openapi: // "path prefixes used to narrow the search before pattern matching"). // `:(literal)` magic keeps the documented plain-prefix semantics (no @@ -919,11 +951,13 @@ truncated: // --- POST /api/v1/repos/archive (2d) -------------------------------------- type archiveRequestBody struct { - Ref string `json:"ref"` - IncludeGlobs []string `json:"include_globs"` - ExcludeGlobs []string `json:"exclude_globs"` - MaxBlobSize *int64 `json:"max_blob_size"` - Archive struct { + Ref string `json:"ref"` + IncludeGlobs []string `json:"include_globs"` + IncludeGlobsBase64 []string `json:"include_globs_base64,omitempty"` + ExcludeGlobs []string `json:"exclude_globs"` + ExcludeGlobsBase64 []string `json:"exclude_globs_base64,omitempty"` + MaxBlobSize *int64 `json:"max_blob_size"` + Archive struct { Prefix string `json:"prefix"` } `json:"archive"` } @@ -979,6 +1013,18 @@ func (s *server) handleArchive(w http.ResponseWriter, r *http.Request) { } } } + for _, in := range []struct { + field string + list []string + into *[]string + }{ + {"include_globs_base64", body.IncludeGlobsBase64, &body.IncludeGlobs}, + {"exclude_globs_base64", body.ExcludeGlobsBase64, &body.ExcludeGlobs}, + } { + if !mergeRawPatterns(w, in.field, in.list, in.into) { + return + } + } // Reject prefixes that could make extracted entries escape the // destination root (tar slip): absolute paths, "."/".." segments, // NUL bytes, backslashes. diff --git a/internal/api/handlers_write_commit.go b/internal/api/handlers_write_commit.go index 5b9c5b47..30f82cc1 100644 --- a/internal/api/handlers_write_commit.go +++ b/internal/api/handlers_write_commit.go @@ -366,10 +366,30 @@ type wcAuthor struct { } type wcFileOp struct { - Path string `json:"path"` - ContentID string `json:"content_id"` - Operation string `json:"operation"` - Mode string `json:"mode"` + Path string `json:"path"` + // PathBase64 is the base64 of the raw path bytes, for a name that is not + // valid UTF-8. A file operation has exactly one of path and path_base64, + // and an empty one counts as absent (#587). + PathBase64 string `json:"path_base64,omitempty"` + ContentID string `json:"content_id"` + Operation string `json:"operation"` + Mode string `json:"mode"` +} + +// resolvePath moves a path_base64 into Path. +func (f *wcFileOp) resolvePath(i int) error { + if f.PathBase64 == "" { + return nil + } + if f.Path != "" { + return fmt.Errorf("files[%d] has both path and path_base64", i) + } + p, ok := decodeRawPath(f.PathBase64) + if !ok { + return fmt.Errorf("files[%d].path_base64 is not canonical base64", i) + } + f.Path, f.PathBase64 = p, "" + return nil } type wcMetadata struct { @@ -562,6 +582,9 @@ func wcValidateFiles(files []wcFileOp) error { ids := make(map[string]bool, len(files)) for i := range files { f := &files[i] + if err := f.resolvePath(i); err != nil { + return err + } if err := wcValidPath(f.Path); err != nil { return err } diff --git a/internal/api/rawpath.go b/internal/api/rawpath.go index c47ca8f7..15c54119 100644 --- a/internal/api/rawpath.go +++ b/internal/api/rawpath.go @@ -4,6 +4,7 @@ import ( "encoding/base64" "io" "slices" + "strings" "unicode/utf8" ) @@ -19,6 +20,10 @@ import ( // Every sibling field is `omitempty` or `omitzero`, so a response for names // that are valid UTF-8 stays byte for byte what it was. The path-field guard // (rawpath_guard_test.go) fails the build when a path field has no sibling. +// +// A request that names a path has the same sibling, which holds the base64 of +// the raw bytes: paths_base64 and *_globs_base64 in grep and archive bodies, +// path_base64 in a commit-pack file operation. decodeRawPath reads them. // rawPathBase64 returns the base64 of p's bytes when p is not valid UTF-8, // and "" when p is valid UTF-8 (including the empty path). @@ -77,3 +82,30 @@ func (s base64JSONString) writeJSONItem(w io.Writer) error { func anyInvalidUTF8(paths []string) bool { return slices.ContainsFunc(paths, func(p string) bool { return !utf8.ValidString(p) }) } + +// decodeRawPath returns the bytes of s when s is canonical padded standard +// base64. Go's decoder alone also accepts CR and LF inside the text and +// non-zero padding bits, so two texts could name one path. +func decodeRawPath(s string) (string, bool) { + raw, err := base64.StdEncoding.Strict().DecodeString(s) + if err != nil || base64.StdEncoding.EncodeToString(raw) != s { + return "", false + } + return string(raw), true +} + +// decodeRawPatterns decodes the elements of a request list such as +// paths_base64. Each decoded value must pass the check that the plain list +// applies: not blank, and no NUL byte. bad is the index of the first +// element that fails, or -1. +func decodeRawPatterns(list []string) (decoded []string, bad int) { + decoded = make([]string, 0, len(list)) + for i, enc := range list { + p, ok := decodeRawPath(enc) + if !ok || strings.TrimSpace(p) == "" || strings.ContainsRune(p, 0) { + return nil, i + } + decoded = append(decoded, p) + } + return decoded, -1 +} diff --git a/internal/api/rawpath_guard_test.go b/internal/api/rawpath_guard_test.go index a7d31fc8..478ce64e 100644 --- a/internal/api/rawpath_guard_test.go +++ b/internal/api/rawpath_guard_test.go @@ -1,10 +1,10 @@ package api -// #587 guard: a JSON field that carries a repository path has a _base64 -// sibling (rawpath.go). The scan reads every struct type in the -// package's own sources, so a new such field without a sibling fails here, in -// the PR that adds it. It does not check that a handler fills the sibling: -// the route tests do. +// #587 guard: a JSON field that carries a repository path or a path glob, in +// a response or a request, has a _base64 sibling (rawpath.go). The scan reads +// every struct type in the package's own sources, so a new such field without +// a sibling fails here, in the PR that adds it. It does not check that a +// handler fills the sibling: the route tests do. import ( "go/ast" @@ -18,23 +18,18 @@ import ( "testing" ) -// rpGuardExempt lists path fields without a sibling, as Type.json_name. -// Both take a path in a request body, which has no response sibling to fill. -var rpGuardExempt = map[string]string{ - "grepRequestBody.paths": "grep request input", - "wcFileOp.path": "commit-pack request input", -} - -// isPathFieldName reports whether a JSON name carries a repository path. +// isPathFieldName reports whether a JSON name carries a repository path or +// a glob over paths. func isPathFieldName(name string) bool { - return name == "path" || name == "paths" || strings.HasSuffix(name, "_path") || strings.HasSuffix(name, "_paths") + return name == "path" || name == "paths" || strings.HasSuffix(name, "_path") || + strings.HasSuffix(name, "_paths") || strings.HasSuffix(name, "_globs") } // isPathGoName reports whether the Go name of a field suggests a path. // encoding/json names an untagged field by its Go name, which the rule above // cannot see. func isPathGoName(name string) bool { - return strings.HasSuffix(name, "Path") || strings.HasSuffix(name, "Paths") + return strings.HasSuffix(name, "Path") || strings.HasSuffix(name, "Paths") || strings.HasSuffix(name, "Globs") } type jsonField struct { @@ -75,7 +70,7 @@ func structJSONFields(fset *token.FileSet, typeName string, st *ast.StructType) // rawPathViolations returns one message per path field in src that has no // valid sibling, and how many path fields it checked. -func rawPathViolations(t *testing.T, filename, src string, exempt map[string]string) (violations []string, checked int) { +func rawPathViolations(t *testing.T, filename, src string) (violations []string, checked int) { t.Helper() fset := token.NewFileSet() f, err := parser.ParseFile(fset, filename, src, 0) @@ -105,7 +100,7 @@ func rawPathViolations(t *testing.T, filename, src string, exempt map[string]str break nearest } } - v, c := structPathViolations(fset, typeName, st, exempt) + v, c := structPathViolations(fset, typeName, st) violations, checked = append(violations, v...), checked+c } return true @@ -113,10 +108,10 @@ func rawPathViolations(t *testing.T, filename, src string, exempt map[string]str return violations, checked } -func structPathViolations(fset *token.FileSet, typeName string, st *ast.StructType, exempt map[string]string) (violations []string, checked int) { +func structPathViolations(fset *token.FileSet, typeName string, st *ast.StructType) (violations []string, checked int) { fields, violations := structJSONFields(fset, typeName, st) for name, f := range fields { - if !isPathFieldName(name) || exempt[typeName+"."+name] != "" { + if !isPathFieldName(name) { continue } checked++ @@ -134,7 +129,7 @@ func structPathViolations(fset *token.FileSet, typeName string, st *ast.StructTy return violations, checked } -func TestEveryResponsePathFieldHasABase64Sibling(t *testing.T) { +func TestEveryPathFieldHasABase64Sibling(t *testing.T) { entries, err := os.ReadDir(".") if err != nil { t.Fatal(err) @@ -150,14 +145,14 @@ func TestEveryResponsePathFieldHasABase64Sibling(t *testing.T) { if err != nil { t.Fatal(err) } - v, c := rawPathViolations(t, name, string(src), rpGuardExempt) + v, c := rawPathViolations(t, name, string(src)) all, checked = append(all, v...), checked+c } for _, v := range all { t.Error(v) } - if checked < 10 { - t.Errorf("the scan checked %d path fields, want at least 10: it reads too little", checked) + if checked < 16 { + t.Errorf("the scan checked %d path fields, want at least 16: it reads too little", checked) } } @@ -221,8 +216,16 @@ type multi struct { type notAPath struct { Name string ` + "`json:\"name\"`" + ` } -type exempt struct { - Paths []string ` + "`json:\"paths\"`" + ` +type okGlobs struct { + Include []string ` + "`json:\"include_globs\"`" + ` + IncludeBase64 []string ` + "`json:\"include_globs_base64,omitempty\"`" + ` +} +type missingGlobs struct { + Include []string ` + "`json:\"include_globs\"`" + ` +} +type optionalPath struct { + Path string ` + "`json:\"path\"`" + ` + PathBase64 *string ` + "`json:\"path_base64,omitempty\"`" + ` } var packageVar = struct { P string ` + "`json:\"path\"`" + ` @@ -236,13 +239,14 @@ func f() { }{} } ` - got, checked := rawPathViolations(t, "x.go", src, map[string]string{"exempt.paths": "request input"}) + got, checked := rawPathViolations(t, "x.go", src) for _, want := range []string{ "missing.path has no path_base64 sibling", "missingList.paths has no paths_base64 sibling", "wrongType.path: sibling is []string, want string", "alwaysPresent.original_path: sibling is not omitempty or omitzero", "nestedMissing.old_path has no old_path_base64 sibling", + "missingGlobs.include_globs has no include_globs_base64 sibling", "namedMissing.path has no path_base64 sibling", "pointerMissing.paths has no paths_base64 sibling", "untagged.OldPath has no json name", @@ -258,12 +262,13 @@ func f() { if !slicesContainsSuffix(got, "has no paths_base64 sibling") || !strings.Contains(strings.Join(got, "\n"), "struct at x.go:") { t.Errorf("no violation for the unnamed struct in %q", got) } - if len(got) != 12 { - t.Errorf("violations %d (%q), want 12", len(got), got) + if len(got) != 13 { + t.Errorf("violations %d (%q), want 13", len(got), got) } - // ok holds 5 path fields (one nested), and 11 other types 1 each. - if checked != 16 { - t.Errorf("checked %d path fields, want 16", checked) + // ok holds 5 path fields (one nested), okGlobs and optionalPath 1 each, + // and 12 other types 1 each. + if checked != 19 { + t.Errorf("checked %d path fields, want 19", checked) } } diff --git a/internal/api/rawpath_input_test.go b/internal/api/rawpath_input_test.go new file mode 100644 index 00000000..9dfd18f7 --- /dev/null +++ b/internal/api/rawpath_input_test.go @@ -0,0 +1,192 @@ +package api + +// #587: request inputs that name a path with raw bytes. grep paths and +// globs, archive globs and commit-pack file paths take a base64 form. + +import ( + "bytes" + "encoding/base64" + "net/http" + "slices" + "testing" + + "github.com/coder/openstorage/internal/auth" + "github.com/coder/openstorage/internal/ref" +) + +func rpB64(s string) string { return base64.StdEncoding.EncodeToString([]byte(s)) } + +func (fx *rpFixture) grep(t *testing.T, body map[string]any) []string { + t.Helper() + body["query"] = map[string]any{"pattern": "needle"} + doc := rpJSON(t, fx.post(t, "/api/v1/repos/grep", body)) + return rpRawSet(t, "grep", rpObjects(t, doc, "matches"), "path") +} + +func TestRawPathInputs(t *testing.T) { runBackends(t, testRawPathInputs) } + +func testRawPathInputs(t *testing.T, e *testEnv) { + fx := rpBuild(t, e) + + t.Run("grep paths_base64 names one of two colliding files", func(t *testing.T) { + for _, c := range []struct { + name string + body map[string]any + want []string + }{ + {"paths_base64 ff", map[string]any{"paths_base64": []string{rpB64(rpFF)}}, []string{rpFF}}, + {"paths_base64 fe", map[string]any{"paths_base64": []string{rpB64(rpFE)}}, []string{rpFE}}, + {"the listed form names the valid file", map[string]any{"paths": []string{rpReal}}, []string{rpReal}}, + {"a directory prefix", map[string]any{"paths_base64": []string{rpB64("d\xfe")}}, []string{rpDirFE}}, + {"union with paths", map[string]any{"paths": []string{rpPlain}, "paths_base64": []string{rpB64(rpFE)}}, []string{rpFE, rpPlain}}, + {"a valid name in paths_base64", map[string]any{"paths_base64": []string{rpB64(rpPlain)}}, []string{rpPlain}}, + {"include_globs_base64 is literal", map[string]any{"file_filters": map[string]any{"include_globs_base64": []string{rpB64(rpLat1)}}}, []string{rpLat1}}, + {"include and exclude", map[string]any{"file_filters": map[string]any{"include_globs": []string{"a*"}, "exclude_globs_base64": []string{rpB64(rpFF)}}}, []string{rpFE, rpReal}}, + {"a slash glob", map[string]any{"file_filters": map[string]any{"include_globs_base64": []string{rpB64("d\xfe/*.txt")}}}, []string{rpDirFE}}, + } { + if got := fx.grep(t, c.body); !slices.Equal(rpSorted(got...), rpSorted(c.want...)) { + t.Errorf("%s: grep paths = %q, want %q", c.name, got, rpSorted(c.want...)) + } + } + }) + + t.Run("a bad base64 input is a 400 that names it", func(t *testing.T) { + for _, c := range []struct{ field, in string }{ + {"paths_base64", ""}, + {"paths_base64", "!!"}, + {"paths_base64", "YQ"}, // no padding + {"paths_base64", "YR=="}, // padding bits set + {"paths_base64", "Yf9\ni"}, + {"paths_base64", "-_8="}, // URL-safe alphabet + {"paths_base64", rpB64(" ")}, + {"paths_base64", rpB64("a\x00b")}, + {"file_filters.include_globs_base64", "!!"}, + {"file_filters.exclude_globs_base64", rpB64("\t")}, + } { + list := []string{rpB64(rpPlain), c.in} + body := map[string]any{"query": map[string]any{"pattern": "needle"}} + switch c.field { + case "paths_base64": + body["paths_base64"] = list + case "file_filters.include_globs_base64": + body["file_filters"] = map[string]any{"include_globs_base64": list} + default: + body["file_filters"] = map[string]any{"exclude_globs_base64": list} + } + resp, raw := e.doReq(t, http.MethodPost, "/api/v1/repos/grep", fx.token, rqGrepBody(t, body), "") + if resp.StatusCode != http.StatusBadRequest { + t.Errorf("%s %q: status = %d, want 400 (%s)", c.field, c.in, resp.StatusCode, raw) + continue + } + params, _ := rpJSON(t, raw)["invalid_params"].([]any) + if len(params) != 1 || params[0].(map[string]any)["name"] != c.field+".1" { + t.Errorf("%s %q: invalid_params = %v, want one entry named %s.1", c.field, c.in, params, c.field) + } + } + }) + + t.Run("archive globs", func(t *testing.T) { + archive := func(body map[string]any) []string { + t.Helper() + resp, raw := e.doReq(t, http.MethodPost, "/api/v1/repos/archive", fx.token, rqGrepBody(t, body), "") + coreRequireStatus(t, resp, raw, http.StatusOK) + var names []string + for name := range rqReadArchive(t, raw) { + names = append(names, name) + } + return rpSorted(names...) + } + if got := archive(map[string]any{"include_globs_base64": []string{rpB64(rpFE)}}); !slices.Equal(got, []string{rpFE}) { + t.Errorf("include_globs_base64 [a<0xfe>b] = %q", got) + } + all := rpSorted(rpFF, rpFE, rpLat1, rpUTF8, rpDirFE, rpReal, rpPlain, rpNew, rpBin) + want := slices.DeleteFunc(slices.Clone(all), func(n string) bool { return n == rpFF }) + if got := archive(map[string]any{"exclude_globs_base64": []string{rpB64(rpFF)}}); !slices.Equal(got, want) { + t.Errorf("exclude_globs_base64 [a<0xff>b] = %q, want %q", got, want) + } + resp, raw := e.doReq(t, http.MethodPost, "/api/v1/repos/archive", fx.token, rqGrepBody(t, map[string]any{"include_globs_base64": []string{"!!"}}), "") + if resp.StatusCode != http.StatusBadRequest { + t.Errorf("a bad include_globs_base64: status = %d (%s)", resp.StatusCode, raw) + } + }) + + t.Run("commit-pack path_base64", func(t *testing.T) { + srv := wctServer(t, e) + write := e.mint(t, tokenSpec{iss: fx.org, repo: fx.repoURL, scopes: []string{auth.ScopeGitWrite}}) + commit := func(files ...map[string]any) (*http.Response, []byte) { + frames := []any{wctMeta("main", "raw paths", files, nil)} + for _, f := range files { + if f["operation"] == "upsert" { + frames = append(frames, wctChunk(f["content_id"].(string), []byte("rewritten "+f["content_id"].(string)+"\n"), true)) + } + } + return wctPost(t, srv, "/api/v1/repos/commit-pack", write, bytes.NewReader(wctLines(t, frames...))) + } + upsert := func(id string, extra map[string]any) map[string]any { + f := map[string]any{"content_id": id, "operation": "upsert", "mode": "100644"} + for k, v := range extra { + f[k] = v + } + return f + } + + resp, raw := commit( + upsert("ff", map[string]any{"path_base64": rpB64(rpFF)}), + upsert("new", map[string]any{"path_base64": rpB64("added\xfd.txt")}), + map[string]any{"path_base64": rpB64(rpFE), "content_id": "gone", "operation": "delete"}, + ) + wctRequireOK(t, resp, raw) + read := func(name string) (int, string) { + resp, body := coreGet(t, e, http.MethodGet, "/api/v1/repos/file?path="+rpQuery(name), fx.token, nil) + return resp.StatusCode, string(body) + } + if code, body := read(rpFF); code != http.StatusOK || body != "rewritten ff\n" { + t.Errorf("a<0xff>b = %d %q, want the rewritten content", code, body) + } + if code, body := read("added\xfd.txt"); code != http.StatusOK || body != "rewritten new\n" { + t.Errorf("added<0xfd>.txt = %d %q, want the new file", code, body) + } + if code, _ := read(rpFE); code != http.StatusNotFound { + t.Errorf("a<0xfe>b after its delete: status = %d, want 404", code) + } + if code, body := read(rpReal); code != http.StatusOK || body != fx.content[rpReal] { + t.Errorf("the valid U+FFFD file = %d %q, want it untouched", code, body) + } + listed := rpObjects(t, rpJSON(t, fx.get(t, "/api/v1/repos/files")), "entries") + if got := rpRawSet(t, "entries", listed, "path"); !slices.Contains(got, "added\xfd.txt") || slices.Contains(got, rpFE) { + t.Errorf("entries = %q, want the added file and no a<0xfe>b", got) + } + + // An empty path_base64 counts as absent, as an empty path does. + resp, raw = commit(upsert("plain", map[string]any{"path": "plain-with-empty-b64.txt", "path_base64": ""})) + wctRequireOK(t, resp, raw) + if code, body := read("plain-with-empty-b64.txt"); code != http.StatusOK || body != "rewritten plain\n" { + t.Errorf("plain-with-empty-b64.txt = %d %q, want the written file", code, body) + } + + // The base64 form and the plain form name one path, so both is a + // duplicate, and path plus path_base64 on one operation is refused. + for name, files := range map[string][]map[string]any{ + "both on one operation": {upsert("x", map[string]any{"path": "p.txt", "path_base64": rpB64("p.txt")})}, + "neither": {upsert("x", nil)}, + "not canonical": {upsert("x", map[string]any{"path_base64": "YR=="})}, + "empty": {upsert("x", map[string]any{"path_base64": ""})}, + "traversal": {upsert("x", map[string]any{"path_base64": rpB64("a/../b")})}, + "a .git component": {upsert("x", map[string]any{"path_base64": rpB64(".git/config")})}, + "a control byte": {upsert("x", map[string]any{"path_base64": rpB64("a\x00b")})}, + "duplicate across forms": { + upsert("x", map[string]any{"path": "dup.txt"}), + upsert("y", map[string]any{"path_base64": rpB64("dup.txt")}), + }, + } { + tip := wctBranchTip(t, e, fx.token, "main", false) + resp, raw := commit(files...) + if resp.StatusCode != http.StatusBadRequest { + t.Errorf("%s: status = %d, want 400 (%s)", name, resp.StatusCode, raw) + } + if got := wctBranchTip(t, e, fx.token, "main", false); got != tip || !ref.ValidSHA(got) { + t.Errorf("%s: main moved from %s to %s", name, tip, got) + } + } + }) +} diff --git a/internal/api/rawpath_test.go b/internal/api/rawpath_test.go index f04b846f..e59d965b 100644 --- a/internal/api/rawpath_test.go +++ b/internal/api/rawpath_test.go @@ -37,6 +37,8 @@ const ( // eight, c2 (main) renames rpOld to rpNew and adds rpBin. Branch base is c1. type rpFixture struct { e *testEnv + org string + repoURL string token string c1, c2 string content map[string]string // name -> file content at c2 @@ -64,7 +66,7 @@ func rpBuild(t *testing.T, e *testEnv) *rpFixture { } dir := t.TempDir() coreGit(t, dir, "init", "-q", "-b", "main", ".") - fx := &rpFixture{e: e, content: map[string]string{}} + fx := &rpFixture{e: e, org: org, repoURL: repoURL, content: map[string]string{}} stage := func(name, content string) { rpStage(t, dir, name, content) fx.content[name] = content