httpd: GET /api/v1/read for conditional reads !94

merged merged by cmc on 2026-08-27 04:09 UTC · krz/gitbay:api-get into main

6 files changed, +258 −20

Layout: unified · split

e2e/api_test.go +107

	@@ -6,6 +6,7 @@ import (
6	"fmt"	6	"fmt"
7	"io"	7	"io"
8	"net/http"	8	"net/http"
		9	"net/url"
9	"strings"	10	"strings"
10	"testing"	11	"testing"
11	"time"	12	"time"
	@@ -242,3 +243,109 @@ func mintToken(t testing.T, inst instance, key, name string) string {
242	}	243	}
243	return env.Data.Token	244	return env.Data.Token
244	}	245	}
		246
		247	// apiGet fetches one read command, optionally conditionally.
		248	func (i instance) apiGet(t testing.T, token string, argv []string, ifNoneMatch string) (int, string, string) {
		249	t.Helper()
		250	q := url.Values{}
		251	for _, a := range argv {
		252	q.Add("argv", a)
		253	}
		254	req, err := http.NewRequest("GET",
		255	fmt.Sprintf("http://127.0.0.1:%d/api/v1/read?%s", i.httpPort, q.Encode()), nil)
		256	if err != nil {
		257	t.Fatal(err)
		258	}
		259	if token != "" {
		260	req.Header.Set("Authorization", "Bearer "+token)
		261	}
		262	if ifNoneMatch != "" {
		263	req.Header.Set("If-None-Match", ifNoneMatch)
		264	}
		265	resp, err := http.DefaultClient.Do(req)
		266	if err != nil {
		267	t.Fatal(err)
		268	}
		269	defer resp.Body.Close()
		270	raw, _ := io.ReadAll(resp.Body)
		271	return resp.StatusCode, resp.Header.Get("ETag"), string(raw)
		272	}
		273
		274	// TestAPIReadGET covers the conditional-request surface: reads over GET
		275	// with an ETag, 304 on revalidation, writes refused, and one caller's ETag
		276	// never matching another's.
		277	func TestAPIReadGET(t *testing.T) {
		278	inst := startInstanceWith(t, "[api]\nenabled = true\n")
		279	aliceKey := inst.newKey(t, "alice")
		280	bobKey := inst.newKey(t, "bob")
		281	inst.admin(t, "admin", "user", "create", "alice", "--key", aliceKey+".pub")
		282	inst.admin(t, "admin", "user", "create", "bob", "--key", bobKey+".pub")
		283	aliceTok := mintToken(t, inst, aliceKey, "alice-get")
		284	bobTok := mintToken(t, inst, bobKey, "bob-get")
		285	if _, errOut, code := inst.ssh(t, aliceKey, "", "repo", "create", "alice/app"); code != 0 {
		286	t.Fatalf("repo create: %s", errOut)
		287	}
		288
		289	status, etag, body := inst.apiGet(t, aliceTok, []string{"repo", "show", "alice/app"}, "")
		290	if status != 200 {
		291	t.Fatalf("GET read: %d %s", status, body)
		292	}
		293	if etag == "" {
		294	t.Fatal("no ETag, so a client cannot revalidate")
		295	}
		296	if !strings.Contains(body, `"alice/app"`) {
		297	t.Errorf("body: %s", body)
		298	}
		299
		300	// Revalidation returns 304 with no body — the point of the surface.
		301	status, _, body = inst.apiGet(t, aliceTok, []string{"repo", "show", "alice/app"}, etag)
		302	if status != http.StatusNotModified {
		303	t.Fatalf("revalidation status %d, want 304", status)
		304	}
		305	if body != "" {
		306	t.Errorf("304 carried a body: %q", body)
		307	}
		308	// A weak validator from an intermediary still matches.
		309	if status, _, _ := inst.apiGet(t, aliceTok, []string{"repo", "show", "alice/app"}, "W/"+etag); status != http.StatusNotModified {
		310	t.Errorf("weak ETag not honoured: %d", status)
		311	}
		312
		313	// A stale ETag gets the real body back, not a 304.
		314	if status, _, body := inst.apiGet(t, aliceTok, []string{"repo", "show", "alice/app"}, `"stale"`); status != 200 \|\| body == "" {
		315	t.Errorf("stale ETag: %d %q", status, body)
		316	}
		317
		318	// The ETag is salted per caller, so one account can never be handed a
		319	// 304 for another account's cached answer.
		320	if status, _, _ := inst.apiGet(t, bobTok, []string{"repo", "show", "alice/app"}, etag); status == http.StatusNotModified {
		321	t.Error("another caller's ETag matched")
		322	}
		323
		324	// Responses must not be storable by shared caches.
		325	req, _ := http.NewRequest("GET",
		326	fmt.Sprintf("http://127.0.0.1:%d/api/v1/read?argv=whoami", inst.httpPort), nil)
		327	req.Header.Set("Authorization", "Bearer "+aliceTok)
		328	resp, err := http.DefaultClient.Do(req)
		329	if err != nil {
		330	t.Fatal(err)
		331	}
		332	resp.Body.Close()
		333	if cc := resp.Header.Get("Cache-Control"); !strings.Contains(cc, "private") {
		334	t.Errorf("Cache-Control = %q, want private", cc)
		335	}
		336
		337	// A GET can never mutate: writes are refused by the registry's own
		338	// ReadOnly flag rather than by a hand-kept list.
		339	status, _, body = inst.apiGet(t, aliceTok, []string{"repo", "create", "alice/sneaky"}, "")
		340	if status != http.StatusBadRequest \|\| !strings.Contains(body, "POST it") {
		341	t.Fatalf("write over GET: %d %s", status, body)
		342	}
		343	if _, _, code := inst.ssh(t, aliceKey, "", "repo", "show", "alice/sneaky"); code == 0 {
		344	t.Fatal("a GET created a repository")
		345	}
		346
		347	// Unauthenticated reads are refused like everywhere else.
		348	if status, _, _ := inst.apiGet(t, "", []string{"whoami"}, ""); status != http.StatusUnauthorized {
		349	t.Errorf("anonymous GET status %d, want 401", status)
		350	}
		351	}

internal/httpd/api.go +17 −9

 	}
 	code := control.Dispatch(ctx, req.Argv)
-	status := map[int]int{
+	status := statusForExit(code)
-		protocol.ExitOK:       http.StatusOK,
-		protocol.ExitUsage:    http.StatusBadRequest,
-		protocol.ExitNotFound: http.StatusNotFound,
-		protocol.ExitDenied:   http.StatusForbidden,
-	}[code]
-	if status == 0 {
-		status = http.StatusInternalServerError
-	}
 	// Commands normally emit exactly one JSON envelope; inject exit_code.
 	// A few (mr diff, help) write raw text instead — wrap those.
 	json.NewEncoder(w).Encode(body)
 }
+// statusForExit maps a command's exit code onto an HTTP status, shared by
+// both API surfaces so they cannot answer the same failure differently.
+func statusForExit(code int) int {
+	switch code {
+	case protocol.ExitOK:
+		return http.StatusOK
+	case protocol.ExitUsage:
+		return http.StatusBadRequest
+	case protocol.ExitNotFound:
+		return http.StatusNotFound
+	case protocol.ExitDenied:
+		return http.StatusForbidden
+	}
+	return http.StatusInternalServerError
+}
 // limitKey buckets an authenticated caller by account, so rotating tokens
 // buys no extra budget, and everyone else by peer address.
 func limitKey(r *http.Request, user store.User) string {

internal/httpd/apiread.go added +116

@@ -0,0 +1,116 @@
	1	package httpd
	2
	3	import (
	4	"bytes"
	5	"crypto/sha256"
	6	"encoding/hex"
	7	"encoding/json"
	8	"net/http"
	9	"strings"
	10
	11	"gitbay.org/gitbay/internal/control"
	12	"gitbay.org/gitbay/internal/protocol"
	13	)
	14
	15	// apiRead is the conditional-request half of the API: the same commands as
	16	// /api/v1/cmd, reached with GET so a response can carry an ETag and a
	17	// client can revalidate instead of refetching. A phone on a slow network
	18	// re-renders a screen for a 304 rather than a full body.
	19	//
	20	// It dispatches the same registry — no second implementation, no chance of
	21	// the two surfaces disagreeing — and admits only commands the registry
	22	// marks ReadOnly, so a GET can never mutate.
	23	//
	24	// GET /api/v1/read?argv=repo&argv=tree&argv=owner/name
	25	func (s Server) apiRead(w http.ResponseWriter, r http.Request) {
	26	user, _, ok := s.apiAuth(w, r)
	27	if !ok {
	28	return
	29	}
	30	argv := r.URL.Query()["argv"]
	31	if len(argv) == 0 {
	32	apiError(w, http.StatusBadRequest, "argv is required: ?argv=repo&argv=show&argv=owner/name")
	33	return
	34	}
	35	cmd, _, found := control.Lookup(argv)
	36	if !found {
	37	apiError(w, http.StatusNotFound, "unknown command "+argv[0])
	38	return
	39	}
	40	if !cmd.ReadOnly {
	41	// Not 405: the command exists, it is simply not a read. Saying so
	42	// is more useful than implying the URL is wrong.
	43	apiError(w, http.StatusBadRequest,
	44	joinArgv(cmd.Path)+" changes state; POST it to /api/v1/cmd")
	45	return
	46	}
	47	if allowed, wait := s.apiLimit.allow(limitKey(r, user), false); !allowed {
	48	tooManyRequests(w, wait)
	49	return
	50	}
	51
	52	var stdout, stderr bytes.Buffer
	53	ctx := &control.Ctx{
	54	User: user,
	55	Source: "api",
	56	Scope: "full",
	57	Store: s.st,
	58	Cfg: s.cfg,
	59	Stdin: strings.NewReader(""),
	60	Stdout: &stdout,
	61	Stderr: &stderr,
	62	JSON: true,
	63	ViaAPI: true,
	64	ReadOnly: true,
	65	}
	66	code := control.Dispatch(ctx, argv)
	67
	68	var body map[string]any
	69	if err := json.Unmarshal(stdout.Bytes(), &body); err != nil \|\| body == nil {
	70	body = map[string]any{"protocol_version": protocol.Version, "output": stdout.String()}
	71	}
	72	body["exit_code"] = code
	73	if msg := strings.TrimSpace(stderr.String()); msg != "" {
	74	body["stderr"] = msg
	75	}
	76	payload, err := json.Marshal(body)
	77	if err != nil {
	78	apiError(w, http.StatusInternalServerError, "internal error")
	79	return
	80	}
	81
	82	// Responses are authorized per account, so the ETag is salted with the
	83	// caller: two users asking the same question may get different answers,
	84	// and neither should ever be served the other's.
	85	sum := sha256.Sum256(append([]byte(limitKey(r, user)+"\x00"), payload...))
	86	etag := `"` + hex.EncodeToString(sum[:16]) + `"`
	87
	88	// private keeps this out of shared caches; no-cache requires a
	89	// revalidation rather than forbidding storage, which is what makes the
	90	// 304 worth having.
	91	w.Header().Set("Cache-Control", "private, no-cache")
	92	w.Header().Set("ETag", etag)
	93	w.Header().Set("Content-Type", "application/json")
	94	if match := r.Header.Get("If-None-Match"); match != "" && etagMatches(match, etag) {
	95	w.WriteHeader(http.StatusNotModified)
	96	return
	97	}
	98
	99	status := statusForExit(code)
	100	w.WriteHeader(status)
	101	w.Write(payload)
	102	}
	103
	104	// etagMatches handles the comma-separated If-None-Match list, and the weak
	105	// prefix a cache may add.
	106	func etagMatches(header, etag string) bool {
	107	for _, candidate := range strings.Split(header, ",") {
	108	candidate = strings.TrimSpace(candidate)
	109	if candidate == "*" \|\| strings.TrimPrefix(candidate, "W/") == etag {
	110	return true
	111	}
	112	}
	113	return false
	114	}
	115
	116	func joinArgv(path []string) string { return strings.Join(path, " ") }

internal/httpd/diff.go +6 −11

 }
 // diffFormatter is the blob formatter without line numbers: the diff
-// supplies its own gutters.
+// supplies its own gutters. PreventSurroundingPre also drops chroma's
-var diffFormatter = html.New(html.WithClasses(true))
+// per-line <span class="line"> wrapper, which the generated CSS gives
+// display:flex — inside a diff row that breaks the +/- marker onto a line
+// of its own.
+var diffFormatter = html.New(html.WithClasses(true), html.PreventSurroundingPre(true))
 // highlightFile syntax-highlights a file's diff content one hunk at a time,
 // each side separately. A hunk's context+deletions are contiguous lines of
 	if err := diffFormatter.Format(&buf, styles.Get(lightStyle), it); err != nil {
 		return nil
 	}
-	body := buf.String()
+	body := strings.TrimSuffix(buf.String(), "\n")
-	// Strip the wrapper chroma puts around the whole block.
-	if i := strings.Index(body, "<code"); i >= 0 {
-		if j := strings.IndexByte(body[i:], '>'); j >= 0 {
-			body = body[i+j+1:]
-		}
-	}
-	body = strings.TrimSuffix(strings.TrimSuffix(body, "</pre>"), "</code>")
-	body = strings.TrimSuffix(body, "\n")
 	var out []template.HTML
 	for _, line := range splitHighlighted(body) {

internal/httpd/diff_test.go +11

	@@ -118,6 +118,17 @@ func TestParseDiffHighlighting(t *testing.T) {
118	}	118	}
119	}	119	}
120		120
		121	// chroma wraps each line in <span class="line">, and its stylesheet gives
		122	// that display:flex. A diff row is already one line, so the wrapper only
		123	// serves to break the +/- marker onto a row of its own.
		124	func TestParseDiffHasNoLineWrappers(t *testing.T) {
		125	for _, l := range parseDiff(samplePatch)[0].Lines {
		126	if strings.Contains(string(l.Code), `class="line"`) {
		127	t.Errorf("%s line %q kept chroma's line wrapper: %s", l.Class, l.Content, l.Code)
		128	}
		129	}
		130	}
		131
121	// A file whose type chroma does not know renders as plain text rather than	132	// A file whose type chroma does not know renders as plain text rather than
122	// being guessed at.	133	// being guessed at.
123	func TestParseDiffUnknownType(t *testing.T) {	134	func TestParseDiffUnknownType(t *testing.T) {

internal/httpd/routes.go +1

	@@ -77,6 +77,7 @@ func (s *Server) Routes() []Route {
77	if s.cfg.API.Enabled {	77	if s.cfg.API.Enabled {
78	routes = append(routes,	78	routes = append(routes,
79	Route{Method: "POST", Pattern: "/api/v1/cmd", Mutating: true, Handler: s.apiCmd},	79	Route{Method: "POST", Pattern: "/api/v1/cmd", Mutating: true, Handler: s.apiCmd},
		80	Route{Method: "GET", Pattern: "/api/v1/read", Handler: s.apiRead},
80	)	81	)
81	}	82	}
82		83