refactor(web): split writing the path list and match raw validators as a list
paths separates authorizing, the 304 and loading from writePathList, which only writes the body. The JSON of the paths is not HTML-escaped. raw reads If-None-Match with etagMatches, so a list of validators, weak ones and * give a 304 as they do for paths.
- commit
- 0219778b77916c30fc21e26cabb155f77512d155
- author
- brice · · 2026-10-12 05:10 UTC
- parent
- b06e385
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
3 files · +40 -8
M internal/adapters/web/paths.go +18 -6
| @@ -82,18 +82,27 @@ func (s *Server) paths(w http.ResponseWriter, r *http.Request) { | |||
| 82 | 82 | s.fail(ctx, w, r, err) | |
| 83 | 83 | return | |
| 84 | 84 | } | |
| 85 | + | // Authorized, not modified and loaded: all that is left is to send it. | |
| 86 | + | validators(w.Header()) | |
| 87 | + | if err := writePathList(w, r, body, gz); err != nil { | |
| 88 | + | s.fail(ctx, w, r, err) | |
| 89 | + | } | |
| 90 | + | } | |
| 91 | + | ||
| 92 | + | // writePathList sends a list (gzipped JSON, as the cache keeps it): as it is | |
| 93 | + | // to a client that accepts gzip, unzipped to the others. The validators are | |
| 94 | + | // already set. It fails only before anything is written. | |
| 95 | + | func writePathList(w http.ResponseWriter, r *http.Request, body []byte, gz bool) error { | |
| 85 | 96 | var plain *gzip.Reader | |
| 86 | 97 | if !gz { | |
| 98 | + | var err error | |
| 87 | 99 | if plain, err = gzip.NewReader(bytes.NewReader(body)); err != nil { | |
| 88 | - | s.fail(ctx, w, r, err) | |
| 89 | - | return | |
| 100 | + | return err | |
| 90 | 101 | } | |
| 91 | 102 | } | |
| 92 | - | ||
| 93 | 103 | h := w.Header() | |
| 94 | 104 | h.Set("Content-Type", "application/json") | |
| 95 | 105 | h.Set("X-Content-Type-Options", "nosniff") | |
| 96 | - | validators(h) | |
| 97 | 106 | if gz { | |
| 98 | 107 | h.Set("Content-Encoding", "gzip") | |
| 99 | 108 | h.Set("Content-Length", strconv.Itoa(len(body))) | |
| @@ -101,12 +110,13 @@ func (s *Server) paths(w http.ResponseWriter, r *http.Request) { | |||
| 101 | 110 | if r.Method != http.MethodHead { | |
| 102 | 111 | _, _ = w.Write(body) | |
| 103 | 112 | } | |
| 104 | - | return | |
| 113 | + | return nil | |
| 105 | 114 | } | |
| 106 | 115 | w.WriteHeader(http.StatusOK) | |
| 107 | 116 | if r.Method != http.MethodHead { | |
| 108 | 117 | _, _ = io.Copy(w, plain) | |
| 109 | 118 | } | |
| 119 | + | return nil | |
| 110 | 120 | } | |
| 111 | 121 | ||
| 112 | 122 | // buildPaths lists the paths of a commit and returns them as gzipped JSON. | |
| @@ -131,7 +141,9 @@ func (s *Server) buildPaths(ctx context.Context, repo, sha string) ([]byte, erro | |||
| 131 | 141 | if err != nil { | |
| 132 | 142 | return nil, err | |
| 133 | 143 | } | |
| 134 | - | if err := json.NewEncoder(zw).Encode(pathList{Paths: paths, Truncated: res.Msg.GetTruncated()}); err != nil { | |
| 144 | + | enc := json.NewEncoder(zw) | |
| 145 | + | enc.SetEscapeHTML(false) // a path is not HTML; "<", ">" and "&" stay as they are | |
| 146 | + | if err := enc.Encode(pathList{Paths: paths, Truncated: res.Msg.GetTruncated()}); err != nil { | |
| 135 | 147 | return nil, err | |
| 136 | 148 | } | |
| 137 | 149 | if err := zw.Close(); err != nil { | |
| @@ -82,18 +82,27 @@ func (s *Server) paths(w http.ResponseWriter, r *http.Request) { | |||||
| 82 | s.fail(ctx, w, r, err) | 82 | s.fail(ctx, w, r, err) | ||
| 83 | return | 83 | return | ||
| 84 | } | 84 | } | ||
| 85 | + | // Authorized, not modified and loaded: all that is left is to send it. | |||
| 86 | + | validators(w.Header()) | |||
| 87 | + | if err := writePathList(w, r, body, gz); err != nil { | |||
| 88 | + | s.fail(ctx, w, r, err) | |||
| 89 | + | } | |||
| 90 | + | } | |||
| 91 | + | ||||
| 92 | + | // writePathList sends a list (gzipped JSON, as the cache keeps it): as it is | |||
| 93 | + | // to a client that accepts gzip, unzipped to the others. The validators are | |||
| 94 | + | // already set. It fails only before anything is written. | |||
| 95 | + | func writePathList(w http.ResponseWriter, r *http.Request, body []byte, gz bool) error { | |||
| 85 | var plain *gzip.Reader | 96 | var plain *gzip.Reader | ||
| 86 | if !gz { | 97 | if !gz { | ||
| 98 | + | var err error | |||
| 87 | if plain, err = gzip.NewReader(bytes.NewReader(body)); err != nil { | 99 | if plain, err = gzip.NewReader(bytes.NewReader(body)); err != nil { | ||
| 88 | - | s.fail(ctx, w, r, err) | 100 | + | return err |
| 89 | - | return | |||
| 90 | } | 101 | } | ||
| 91 | } | 102 | } | ||
| 92 | - | ||||
| 93 | h := w.Header() | 103 | h := w.Header() | ||
| 94 | h.Set("Content-Type", "application/json") | 104 | h.Set("Content-Type", "application/json") | ||
| 95 | h.Set("X-Content-Type-Options", "nosniff") | 105 | h.Set("X-Content-Type-Options", "nosniff") | ||
| 96 | - | validators(h) | |||
| 97 | if gz { | 106 | if gz { | ||
| 98 | h.Set("Content-Encoding", "gzip") | 107 | h.Set("Content-Encoding", "gzip") | ||
| 99 | h.Set("Content-Length", strconv.Itoa(len(body))) | 108 | h.Set("Content-Length", strconv.Itoa(len(body))) | ||
| @@ -101,12 +110,13 @@ func (s *Server) paths(w http.ResponseWriter, r *http.Request) { | |||||
| 101 | if r.Method != http.MethodHead { | 110 | if r.Method != http.MethodHead { | ||
| 102 | _, _ = w.Write(body) | 111 | _, _ = w.Write(body) | ||
| 103 | } | 112 | } | ||
| 104 | - | return | 113 | + | return nil |
| 105 | } | 114 | } | ||
| 106 | w.WriteHeader(http.StatusOK) | 115 | w.WriteHeader(http.StatusOK) | ||
| 107 | if r.Method != http.MethodHead { | 116 | if r.Method != http.MethodHead { | ||
| 108 | _, _ = io.Copy(w, plain) | 117 | _, _ = io.Copy(w, plain) | ||
| 109 | } | 118 | } | ||
| 119 | + | return nil | |||
| 110 | } | 120 | } | ||
| 111 | 121 | ||||
| 112 | // buildPaths lists the paths of a commit and returns them as gzipped JSON. | 122 | // buildPaths lists the paths of a commit and returns them as gzipped JSON. | ||
| @@ -131,7 +141,9 @@ func (s *Server) buildPaths(ctx context.Context, repo, sha string) ([]byte, erro | |||||
| 131 | if err != nil { | 141 | if err != nil { | ||
| 132 | return nil, err | 142 | return nil, err | ||
| 133 | } | 143 | } | ||
| 134 | - | if err := json.NewEncoder(zw).Encode(pathList{Paths: paths, Truncated: res.Msg.GetTruncated()}); err != nil { | 144 | + | enc := json.NewEncoder(zw) |
| 145 | + | enc.SetEscapeHTML(false) // a path is not HTML; "<", ">" and "&" stay as they are | |||
| 146 | + | if err := enc.Encode(pathList{Paths: paths, Truncated: res.Msg.GetTruncated()}); err != nil { | |||
| 135 | return nil, err | 147 | return nil, err | ||
| 136 | } | 148 | } | ||
| 137 | if err := zw.Close(); err != nil { | 149 | if err := zw.Close(); err != nil { | ||
M internal/adapters/web/paths_test.go +21 -1
| @@ -3,6 +3,7 @@ | |||
| 3 | 3 | package web | |
| 4 | 4 | ||
| 5 | 5 | import ( | |
| 6 | + | "bytes" | |
| 6 | 7 | "compress/gzip" | |
| 7 | 8 | "context" | |
| 8 | 9 | "io" | |
| @@ -25,6 +26,7 @@ import ( | |||
| 25 | 26 | type pathsGit struct { | |
| 26 | 27 | ermiteconnect.GitServiceClient | |
| 27 | 28 | lists atomic.Int32 | |
| 29 | + | paths []string // the list; a.go and b/c.go when nil | |
| 28 | 30 | } | |
| 29 | 31 | ||
| 30 | 32 | func (g *pathsGit) ResolveRevision(_ context.Context, req *connect.Request[pb.ResolveRevisionRequest]) (*connect.Response[pb.ResolveRevisionResponse], error) { | |
| @@ -33,7 +35,11 @@ func (g *pathsGit) ResolveRevision(_ context.Context, req *connect.Request[pb.Re | |||
| 33 | 35 | ||
| 34 | 36 | func (g *pathsGit) ListPaths(_ context.Context, req *connect.Request[pb.ListPathsRequest]) (*connect.Response[pb.ListPathsResponse], error) { | |
| 35 | 37 | g.lists.Add(1) | |
| 36 | - | return connect.NewResponse(&pb.ListPathsResponse{CommitId: req.Msg.GetRevision(), Paths: []string{"a.go", "b/c.go"}}), nil | |
| 38 | + | paths := g.paths | |
| 39 | + | if paths == nil { | |
| 40 | + | paths = []string{"a.go", "b/c.go"} | |
| 41 | + | } | |
| 42 | + | return connect.NewResponse(&pb.ListPathsResponse{CommitId: req.Msg.GetRevision(), Paths: paths}), nil | |
| 37 | 43 | } | |
| 38 | 44 | ||
| 39 | 45 | // A revalidation is answered after the access check and before the list is | |
| @@ -151,3 +157,17 @@ func TestEtagMatches(t *testing.T) { | |||
| 151 | 157 | assert.Equal(t, tt.want, etagMatches(tt.header, tag), "%q", tt.header) | |
| 152 | 158 | } | |
| 153 | 159 | } | |
| 160 | + | ||
| 161 | + | // A path is not HTML: "<", ">" and "&" are not escaped in the JSON. | |
| 162 | + | func TestBuildPathsDoesNotEscapeHTML(t *testing.T) { | |
| 163 | + | git := &pathsGit{paths: []string{"a&b/<c>.go"}} | |
| 164 | + | srv, err := New(Config{Repos: failing{}, Git: git, PublicURL: "https://forge.example"}) | |
| 165 | + | require.NoError(t, err) | |
| 166 | + | body, err := srv.buildPaths(context.Background(), "r", strings.Repeat("a", 40)) | |
| 167 | + | require.NoError(t, err) | |
| 168 | + | zr, err := gzip.NewReader(bytes.NewReader(body)) | |
| 169 | + | require.NoError(t, err) | |
| 170 | + | out, err := io.ReadAll(zr) | |
| 171 | + | require.NoError(t, err) | |
| 172 | + | assert.Contains(t, string(out), `"a&b/<c>.go"`) | |
| 173 | + | } | |
| @@ -3,6 +3,7 @@ | |||||
| 3 | package web | 3 | package web | ||
| 4 | 4 | ||||
| 5 | import ( | 5 | import ( | ||
| 6 | + | "bytes" | |||
| 6 | "compress/gzip" | 7 | "compress/gzip" | ||
| 7 | "context" | 8 | "context" | ||
| 8 | "io" | 9 | "io" | ||
| @@ -25,6 +26,7 @@ import ( | |||||
| 25 | type pathsGit struct { | 26 | type pathsGit struct { | ||
| 26 | ermiteconnect.GitServiceClient | 27 | ermiteconnect.GitServiceClient | ||
| 27 | lists atomic.Int32 | 28 | lists atomic.Int32 | ||
| 29 | + | paths []string // the list; a.go and b/c.go when nil | |||
| 28 | } | 30 | } | ||
| 29 | 31 | ||||
| 30 | func (g *pathsGit) ResolveRevision(_ context.Context, req *connect.Request[pb.ResolveRevisionRequest]) (*connect.Response[pb.ResolveRevisionResponse], error) { | 32 | func (g *pathsGit) ResolveRevision(_ context.Context, req *connect.Request[pb.ResolveRevisionRequest]) (*connect.Response[pb.ResolveRevisionResponse], error) { | ||
| @@ -33,7 +35,11 @@ func (g *pathsGit) ResolveRevision(_ context.Context, req *connect.Request[pb.Re | |||||
| 33 | 35 | ||||
| 34 | func (g *pathsGit) ListPaths(_ context.Context, req *connect.Request[pb.ListPathsRequest]) (*connect.Response[pb.ListPathsResponse], error) { | 36 | func (g *pathsGit) ListPaths(_ context.Context, req *connect.Request[pb.ListPathsRequest]) (*connect.Response[pb.ListPathsResponse], error) { | ||
| 35 | g.lists.Add(1) | 37 | g.lists.Add(1) | ||
| 36 | - | return connect.NewResponse(&pb.ListPathsResponse{CommitId: req.Msg.GetRevision(), Paths: []string{"a.go", "b/c.go"}}), nil | 38 | + | paths := g.paths |
| 39 | + | if paths == nil { | |||
| 40 | + | paths = []string{"a.go", "b/c.go"} | |||
| 41 | + | } | |||
| 42 | + | return connect.NewResponse(&pb.ListPathsResponse{CommitId: req.Msg.GetRevision(), Paths: paths}), nil | |||
| 37 | } | 43 | } | ||
| 38 | 44 | ||||
| 39 | // A revalidation is answered after the access check and before the list is | 45 | // A revalidation is answered after the access check and before the list is | ||
| @@ -151,3 +157,17 @@ func TestEtagMatches(t *testing.T) { | |||||
| 151 | assert.Equal(t, tt.want, etagMatches(tt.header, tag), "%q", tt.header) | 157 | assert.Equal(t, tt.want, etagMatches(tt.header, tag), "%q", tt.header) | ||
| 152 | } | 158 | } | ||
| 153 | } | 159 | } | ||
| 160 | + | ||||
| 161 | + | // A path is not HTML: "<", ">" and "&" are not escaped in the JSON. | |||
| 162 | + | func TestBuildPathsDoesNotEscapeHTML(t *testing.T) { | |||
| 163 | + | git := &pathsGit{paths: []string{"a&b/<c>.go"}} | |||
| 164 | + | srv, err := New(Config{Repos: failing{}, Git: git, PublicURL: "https://forge.example"}) | |||
| 165 | + | require.NoError(t, err) | |||
| 166 | + | body, err := srv.buildPaths(context.Background(), "r", strings.Repeat("a", 40)) | |||
| 167 | + | require.NoError(t, err) | |||
| 168 | + | zr, err := gzip.NewReader(bytes.NewReader(body)) | |||
| 169 | + | require.NoError(t, err) | |||
| 170 | + | out, err := io.ReadAll(zr) | |||
| 171 | + | require.NoError(t, err) | |||
| 172 | + | assert.Contains(t, string(out), `"a&b/<c>.go"`) | |||
| 173 | + | } | |||
M internal/adapters/web/raw.go +1 -1
| @@ -60,7 +60,7 @@ func (s *Server) raw(w http.ResponseWriter, r *http.Request) { | |||
| 60 | 60 | // private, and a shared cache must not keep serving its files (AC2). The | |
| 61 | 61 | // ETag keeps the revalidation cheap. | |
| 62 | 62 | cacheHeaders(ctx, h, "no-cache", sessionCache) | |
| 63 | - | if match := r.Header.Get("If-None-Match"); match != "" && h.Get("ETag") != "" && match == h.Get("ETag") { | |
| 63 | + | if etag := h.Get("ETag"); etag != "" && etagMatches(r.Header.Get("If-None-Match"), etag) { | |
| 64 | 64 | w.WriteHeader(http.StatusNotModified) | |
| 65 | 65 | return | |
| 66 | 66 | } | |
| @@ -60,7 +60,7 @@ func (s *Server) raw(w http.ResponseWriter, r *http.Request) { | |||||
| 60 | // private, and a shared cache must not keep serving its files (AC2). The | 60 | // private, and a shared cache must not keep serving its files (AC2). The | ||
| 61 | // ETag keeps the revalidation cheap. | 61 | // ETag keeps the revalidation cheap. | ||
| 62 | cacheHeaders(ctx, h, "no-cache", sessionCache) | 62 | cacheHeaders(ctx, h, "no-cache", sessionCache) | ||
| 63 | - | if match := r.Header.Get("If-None-Match"); match != "" && h.Get("ETag") != "" && match == h.Get("ETag") { | 63 | + | if etag := h.Get("ETag"); etag != "" && etagMatches(r.Header.Get("If-None-Match"), etag) { |
| 64 | w.WriteHeader(http.StatusNotModified) | 64 | w.WriteHeader(http.StatusNotModified) | ||
| 65 | return | 65 | return | ||
| 66 | } | 66 | } | ||