diff --git a/context/github-sync-invariants.md b/context/github-sync-invariants.md index 02ef3c98b..bb5ca5ec8 100644 --- a/context/github-sync-invariants.md +++ b/context/github-sync-invariants.md @@ -742,7 +742,13 @@ app to an endpoint its credential cannot use, which fails even though the token chain "correctly" falls back to the PAT. - Private `user-attachments` reads are the exception to app-token-first reads: GitHub returns 404 to installation tokens, so the repo-scoped image proxy must - use the user's PAT/`gh` chain (`internal/github/client.go::GetMarkdownImage`). + use the user's PAT/`gh` chain (`internal/github/markdown_images.go::GetMarkdownImage`). +- Repository-file markdown images (`blob`/`raw` web URLs, `raw.githubusercontent.com`) + are proxied only for the route's own repository, use the normal read chain, and are + type-sniffed because the contents raw media type hides the file type; web URLs do not + delimit ref from path, so ref splits are tried shortest-first past 404s. They are + marked mutable because the ref is usually a branch. The frontend must apply the + same URL rules (`internal/github/markdown_images.go::getRepositoryFileImage`). Config may carry multiple `[[github_apps]]` rows for one host, but those rows represent distinct app credentials. Management commands must target one row by app owner/installation account or app id, and duplicate installation accounts on diff --git a/context/platform-sync-invariants.md b/context/platform-sync-invariants.md index 595ec759e..00a229b74 100644 --- a/context/platform-sync-invariants.md +++ b/context/platform-sync-invariants.md @@ -414,6 +414,11 @@ GitLab private Markdown upload web URLs do not accept API-token authentication. Translate only repo-scoped upload URLs to the authenticated project-upload API; never proxy arbitrary provider URLs. (`internal/platform/gitlab/markdown_images.go::GetMarkdownImage`) +The markdown image cache is keyed by stable repository identity, never the owner/name +route, so a replacement occupant of a reused route cannot receive the previous +repository's bytes; providers mark ref-addressed sources `Mutable` and the server then +caches them for minutes instead of a year (`internal/server/markdown_images.go::markdownImageCacheKey`). + GitLab merge request and issue `iid` values are repo-scoped numbers. Persist provider object ids separately from user-visible numbers, and scope events by provider identity so equal GitHub/GitLab ids do not collide. diff --git a/frontend/src/lib/utils/markdown.test.ts b/frontend/src/lib/utils/markdown.test.ts index 18ab85eee..5a6d154a7 100644 --- a/frontend/src/lib/utils/markdown.test.ts +++ b/frontend/src/lib/utils/markdown.test.ts @@ -43,6 +43,51 @@ describe("renderMarkdown task lists", () => { } }); + it("proxies images committed to the repository through the repo-scoped API", async () => { + const repo = { + provider: "github", + platformHost: "github.com", + owner: "acme", + name: "widgets", + repoPath: "acme/widgets", + }; + const sources = [ + "https://github.com/acme/widgets/blob/feat/search-controls/docs/images/search.png?raw=true", + "https://github.com/acme/widgets/raw/main/docs/images/search.png", + "https://raw.githubusercontent.com/Acme/Widgets/main/docs/images/search.png", + ]; + + for (const source of sources) { + const html = await renderMarkdown(`![Search options](${source})`, repo); + expect(html).toContain( + `src="/api/v1/repo/github/acme/widgets/markdown-image?source=${encodeURIComponent(source)}"`, + ); + } + }); + + it("leaves GitHub images outside the repository on direct loading", async () => { + const repo = { + provider: "github", + platformHost: "github.com", + owner: "acme", + name: "widgets", + repoPath: "acme/widgets", + }; + const sources = [ + "https://github.com/acme/other/blob/main/docs/images/search.png?raw=true", + "https://raw.githubusercontent.com/acme/other/main/docs/images/search.png", + "https://github.com/acme/widgets/tree/main/docs/images/search.png", + "https://github.com/acme/widgets/blob/main", + "https://gist.githubusercontent.com/acme/widgets/main/docs/images/search.png", + ]; + + for (const source of sources) { + const html = await renderMarkdown(`![Search options](${source})`, repo); + expect(html).toContain(`src="${source}"`); + expect(html).not.toContain("markdown-image"); + } + }); + it("proxies private GitLab upload images through the repo-scoped API", async () => { const source = "/uploads/0123456789abcdef/private-image.png"; const canonicalSource = "https://gitlab.example.com/group/project/uploads/0123456789abcdef/private-image.png"; diff --git a/frontend/src/lib/utils/markdown.ts b/frontend/src/lib/utils/markdown.ts index 8ee19b8e5..80098671f 100644 --- a/frontend/src/lib/utils/markdown.ts +++ b/frontend/src/lib/utils/markdown.ts @@ -426,12 +426,12 @@ function proxiedMarkdownImageSource(source: string, repo: RepoContext): string | provider === "gitlab" ? normalizedGitLabMarkdownImageSource(source, host, repo.repoPath) : source; if (!normalizedSource) return null; const url = new URL(normalizedSource); - if (url.protocol !== "https:" || url.host.toLowerCase() !== host.toLowerCase()) return null; - if (provider === "github" && !url.pathname.startsWith("/user-attachments/assets/")) return null; + if (url.protocol !== "https:") return null; + if (provider === "github" && !isProxiedGitHubImage(url, host, repo.repoPath)) return null; if ( provider === "gitlab" && - !url.pathname.startsWith(`/${repo.repoPath}/uploads/`) && - !/^\/-\/project\/\d+\/uploads\//.test(url.pathname) + (url.host.toLowerCase() !== host.toLowerCase() || + (!url.pathname.startsWith(`/${repo.repoPath}/uploads/`) && !/^\/-\/project\/\d+\/uploads\//.test(url.pathname))) ) return null; return providerRepoResourceURL(repo, "/markdown-image", { source: url.toString() }); @@ -440,6 +440,26 @@ function proxiedMarkdownImageSource(source: string, repo: RepoContext): string | } } +// GitHub images that need the repository credential: private user-attachments +// uploads, and files committed to this repository referenced through blob or +// raw web URLs. Other repositories' files stay on direct loading because the +// route's credential is only known to reach this repository. The server +// applies the same rules (internal/github/markdown_images.go). +function isProxiedGitHubImage(url: URL, host: string, repoPath: string): boolean { + const urlHost = url.host.toLowerCase(); + const platformHost = host.toLowerCase(); + const segments = url.pathname.split("/").filter(Boolean); + const inRepository = segments.slice(0, 2).join("/").toLowerCase() === repoPath.toLowerCase(); + if (urlHost === platformHost) { + if (url.pathname.startsWith("/user-attachments/assets/")) return true; + return inRepository && (segments[2] === "blob" || segments[2] === "raw") && segments.length >= 5; + } + if (urlHost === "raw.githubusercontent.com" && platformHost === "github.com") { + return inRepository && segments.length >= 4; + } + return false; +} + function normalizedGitLabMarkdownImageSource(source: string, host: string, repoPath: string): string | null { if (/^https:\/\//i.test(source)) return source; const uploadPath = source.replace(/^\//, ""); diff --git a/internal/github/client.go b/internal/github/client.go index eaabd3f4c..32b68341f 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -7,11 +7,8 @@ import ( "encoding/json" "errors" "fmt" - "io" "log/slog" - "mime" "net/http" - "net/url" "sort" "strconv" "strings" @@ -183,95 +180,6 @@ type repoUserClient interface { ) (*gh.User, error) } -// markdownImageClient carries the full repository identity even though the -// attachment URL is host-scoped: credential selection is per repository, so a -// repo-scoped route must be able to pick its own token for the fetch. -type markdownImageClient interface { - GetMarkdownImage( - ctx context.Context, owner, repo, sourceURL string, - ) (platform.MarkdownImage, error) -} - -const maxMarkdownImageBytes = 25 << 20 - -var allowedMarkdownImageTypes = map[string]struct{}{ - "image/avif": {}, - "image/bmp": {}, - "image/gif": {}, - "image/jpeg": {}, - "image/png": {}, - "image/webp": {}, -} - -func (c *liveClient) GetMarkdownImage( - ctx context.Context, - owner, _, sourceURL string, -) (platform.MarkdownImage, error) { - parsed, err := url.Parse(sourceURL) - if err != nil || parsed.Scheme != "https" || parsed.User != nil || - !strings.EqualFold(parsed.Host, c.platformHost) || - !strings.HasPrefix(parsed.EscapedPath(), "/user-attachments/assets/") { - return platform.MarkdownImage{}, &platform.Error{ - Code: platform.ErrCodeInvalidArgument, Provider: platform.KindGitHub, - PlatformHost: c.platformHost, Field: "source", Err: errors.New("unsupported markdown image URL"), - } - } - - // github.com/user-attachments accepts the user's credential but returns - // 404 for installation tokens, even when the app can read the repository. - authCtx := tokenauth.WithMutationAuth(tokenauth.WithGitHubOwner(ctx, owner)) - if c.source == nil { - return platform.MarkdownImage{}, errors.New("GitHub markdown image token source is unavailable") - } - token, err := c.source.Token(authCtx) - if err != nil { - return platform.MarkdownImage{}, err - } - req, err := http.NewRequestWithContext(authCtx, http.MethodGet, parsed.String(), nil) - if err != nil { - return platform.MarkdownImage{}, err - } - req.Header.Set("Accept", "application/octet-stream") - req.Header.Set("Authorization", "Bearer "+token) - client := c.markdownImageHTTPClient - if client == nil { - client = http.DefaultClient - } - resp, err := client.Do(req) - if err != nil { - return platform.MarkdownImage{}, err - } - defer resp.Body.Close() - if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden { - return platform.MarkdownImage{}, platform.PermissionDenied(platform.KindGitHub, c.platformHost, errors.New(resp.Status)) - } - if resp.StatusCode == http.StatusNotFound { - return platform.MarkdownImage{}, &platform.Error{Code: platform.ErrCodeNotFound, Provider: platform.KindGitHub, PlatformHost: c.platformHost} - } - if resp.StatusCode < 200 || resp.StatusCode >= 300 { - return platform.MarkdownImage{}, fmt.Errorf("fetch GitHub markdown image: %s", resp.Status) - } - - contentType, _, err := mime.ParseMediaType(resp.Header.Get("Content-Type")) - if err != nil { - return platform.MarkdownImage{}, fmt.Errorf("parse GitHub markdown image content type: %w", err) - } - if _, ok := allowedMarkdownImageTypes[contentType]; !ok { - return platform.MarkdownImage{}, &platform.Error{ - Code: platform.ErrCodeInvalidArgument, Provider: platform.KindGitHub, - PlatformHost: c.platformHost, Field: "source", Err: fmt.Errorf("unsupported image content type %q", contentType), - } - } - content, err := io.ReadAll(io.LimitReader(resp.Body, maxMarkdownImageBytes+1)) - if err != nil { - return platform.MarkdownImage{}, err - } - if len(content) > maxMarkdownImageBytes { - return platform.MarkdownImage{}, fmt.Errorf("GitHub markdown image exceeds %d bytes", maxMarkdownImageBytes) - } - return platform.MarkdownImage{Content: content, ContentType: contentType}, nil -} - type conditionalPullRequestGetter interface { GetPullRequestIfChanged( ctx context.Context, diff --git a/internal/github/markdown_images.go b/internal/github/markdown_images.go new file mode 100644 index 000000000..a0276d6e1 --- /dev/null +++ b/internal/github/markdown_images.go @@ -0,0 +1,266 @@ +package github + +import ( + "bytes" + "context" + "errors" + "fmt" + "io" + "mime" + "net/http" + "net/url" + "path" + "strings" + + "go.kenn.io/forge/internal/platform" + "go.kenn.io/forge/internal/tokenauth" +) + +// markdownImageClient carries the full repository identity even though an +// attachment URL is host-scoped: credential selection is per repository, so a +// repo-scoped route must be able to pick its own token for the fetch. +type markdownImageClient interface { + GetMarkdownImage( + ctx context.Context, owner, repo, sourceURL string, + ) (platform.MarkdownImage, error) +} + +const maxMarkdownImageBytes = 25 << 20 + +var allowedMarkdownImageTypes = map[string]struct{}{ + "image/avif": {}, + "image/bmp": {}, + "image/gif": {}, + "image/jpeg": {}, + "image/png": {}, + "image/webp": {}, +} + +var errMarkdownImageTooLarge = fmt.Errorf("GitHub markdown image exceeds %d bytes", maxMarkdownImageBytes) + +// GetMarkdownImage fetches a provider-hosted image with the repository's +// credential. Two source shapes are supported: private user-attachments +// uploads on the platform host, and files committed to the repository +// itself, referenced through blob or raw web URLs (or raw.githubusercontent.com +// on github.com). Anything else is rejected so the proxy cannot be pointed at +// arbitrary hosts. +func (c *liveClient) GetMarkdownImage( + ctx context.Context, + owner, repo, sourceURL string, +) (platform.MarkdownImage, error) { + parsed, err := url.Parse(sourceURL) + if err != nil || parsed.Scheme != "https" || parsed.User != nil { + return platform.MarkdownImage{}, c.invalidMarkdownImageSource(errors.New("unsupported markdown image URL")) + } + if segments, ok := markdownRepositoryFileSegments(parsed, c.platformHost, owner, repo); ok { + return c.getRepositoryFileImage(ctx, owner, repo, segments) + } + if !strings.EqualFold(parsed.Host, c.platformHost) || + !strings.HasPrefix(parsed.EscapedPath(), "/user-attachments/assets/") { + return platform.MarkdownImage{}, c.invalidMarkdownImageSource(errors.New("unsupported markdown image URL")) + } + return c.getAttachmentImage(ctx, owner, parsed) +} + +func (c *liveClient) invalidMarkdownImageSource(err error) error { + return &platform.Error{ + Code: platform.ErrCodeInvalidArgument, Provider: platform.KindGitHub, + PlatformHost: c.platformHost, Field: "source", Err: err, + } +} + +// markdownRepositoryFileSegments recognizes image URLs that point at a file in +// the route's own repository and returns the ref-and-path segments that follow +// the repository name. Files in other repositories are not proxied: a public +// repository's file loads directly in the browser, and this route's credential +// is only known to be valid for the route's repository. +func markdownRepositoryFileSegments(parsed *url.URL, platformHost, owner, repo string) ([]string, bool) { + segments := strings.Split(strings.Trim(parsed.Path, "/"), "/") + if len(segments) < 2 || !strings.EqualFold(segments[0], owner) || !strings.EqualFold(segments[1], repo) { + return nil, false + } + var rest []string + switch { + case strings.EqualFold(parsed.Host, platformHost): + if len(segments) < 3 || (segments[2] != "blob" && segments[2] != "raw") { + return nil, false + } + rest = segments[3:] + case platformHost == "github.com" && strings.EqualFold(parsed.Host, "raw.githubusercontent.com"): + rest = segments[2:] + default: + return nil, false + } + if len(rest) < 2 { + return nil, false + } + for _, segment := range rest { + if segment == "" || segment == "." || segment == ".." { + return nil, false + } + } + return rest, true +} + +// getRepositoryFileImage reads a committed file through the contents API. Web +// URLs do not delimit the ref from the file path, and branch names may contain +// slashes, so the split is resolved the way GitHub does: the shortest ref that +// yields the file wins, and 404s move on to the next candidate. The ref is +// usually a branch, so the result is marked mutable. +func (c *liveClient) getRepositoryFileImage( + ctx context.Context, + owner, repo string, + segments []string, +) (platform.MarkdownImage, error) { + for split := 1; split < len(segments); split++ { + ref := strings.Join(segments[:split], "/") + filePath := segments[split:] + content, err := c.fetchRepositoryFile(ctx, owner, repo, ref, filePath) + if err != nil { + if githubStatusCode(err) == http.StatusNotFound { + continue + } + return platform.MarkdownImage{}, c.repositoryFileError(err) + } + contentType, err := c.repositoryImageContentType(content, filePath[len(filePath)-1]) + if err != nil { + return platform.MarkdownImage{}, err + } + return platform.MarkdownImage{Content: content, ContentType: contentType, Mutable: true}, nil + } + return platform.MarkdownImage{}, &platform.Error{ + Code: platform.ErrCodeNotFound, Provider: platform.KindGitHub, PlatformHost: c.platformHost, + } +} + +func (c *liveClient) fetchRepositoryFile( + ctx context.Context, + owner, repo, ref string, + filePath []string, +) ([]byte, error) { + escaped := make([]string, len(filePath)) + for i, segment := range filePath { + escaped[i] = url.PathEscape(segment) + } + u := fmt.Sprintf( + "repos/%s/%s/contents/%s?ref=%s", + url.PathEscape(owner), url.PathEscape(repo), strings.Join(escaped, "/"), url.QueryEscape(ref), + ) + req, err := c.gh.NewRequest(ctx, http.MethodGet, u, nil) + if err != nil { + return nil, err + } + req.Header.Set("Accept", "application/vnd.github.raw+json") + buf := &boundedBuffer{limit: maxMarkdownImageBytes} + resp, err := c.gh.Do(req, buf) + c.trackRate(resp) + if err != nil { + return nil, err + } + return buf.Bytes(), nil +} + +func (c *liveClient) repositoryFileError(err error) error { + if errors.Is(err, errMarkdownImageTooLarge) { + return errMarkdownImageTooLarge + } + switch githubStatusCode(err) { + case http.StatusUnauthorized, http.StatusForbidden: + return platform.PermissionDenied(platform.KindGitHub, c.platformHost, err) + } + return fmt.Errorf("fetch GitHub repository image: %w", err) +} + +// repositoryImageContentType derives the image type from the bytes because the +// contents API labels every raw response with its own media type rather than +// the file's. The extension only decides formats sniffing cannot recognize. +func (c *liveClient) repositoryImageContentType(content []byte, fileName string) (string, error) { + contentType, _, _ := mime.ParseMediaType(http.DetectContentType(content)) + if _, ok := allowedMarkdownImageTypes[contentType]; ok { + return contentType, nil + } + if contentType == "application/octet-stream" { + byExtension, _, _ := mime.ParseMediaType(mime.TypeByExtension(strings.ToLower(path.Ext(fileName)))) + if _, ok := allowedMarkdownImageTypes[byExtension]; ok { + return byExtension, nil + } + } + return "", c.invalidMarkdownImageSource(fmt.Errorf("unsupported image content type %q", contentType)) +} + +// boundedBuffer stops a contents download once it passes the proxy's size +// limit instead of buffering a file the route would reject anyway. +// The buffer is a field rather than embedded so io.Copy cannot bypass Write +// through bytes.Buffer's ReadFrom. +type boundedBuffer struct { + buf bytes.Buffer + limit int +} + +func (b *boundedBuffer) Write(p []byte) (int, error) { + if b.buf.Len()+len(p) > b.limit { + return 0, errMarkdownImageTooLarge + } + return b.buf.Write(p) +} + +func (b *boundedBuffer) Bytes() []byte { + return b.buf.Bytes() +} + +func (c *liveClient) getAttachmentImage( + ctx context.Context, + owner string, + parsed *url.URL, +) (platform.MarkdownImage, error) { + // github.com/user-attachments accepts the user's credential but returns + // 404 for installation tokens, even when the app can read the repository. + authCtx := tokenauth.WithMutationAuth(tokenauth.WithGitHubOwner(ctx, owner)) + if c.source == nil { + return platform.MarkdownImage{}, errors.New("GitHub markdown image token source is unavailable") + } + token, err := c.source.Token(authCtx) + if err != nil { + return platform.MarkdownImage{}, err + } + req, err := http.NewRequestWithContext(authCtx, http.MethodGet, parsed.String(), nil) + if err != nil { + return platform.MarkdownImage{}, err + } + req.Header.Set("Accept", "application/octet-stream") + req.Header.Set("Authorization", "Bearer "+token) + client := c.markdownImageHTTPClient + if client == nil { + client = http.DefaultClient + } + resp, err := client.Do(req) + if err != nil { + return platform.MarkdownImage{}, err + } + defer resp.Body.Close() + if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden { + return platform.MarkdownImage{}, platform.PermissionDenied(platform.KindGitHub, c.platformHost, errors.New(resp.Status)) + } + if resp.StatusCode == http.StatusNotFound { + return platform.MarkdownImage{}, &platform.Error{Code: platform.ErrCodeNotFound, Provider: platform.KindGitHub, PlatformHost: c.platformHost} + } + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + return platform.MarkdownImage{}, fmt.Errorf("fetch GitHub markdown image: %s", resp.Status) + } + + contentType, _, err := mime.ParseMediaType(resp.Header.Get("Content-Type")) + if err != nil { + return platform.MarkdownImage{}, fmt.Errorf("parse GitHub markdown image content type: %w", err) + } + if _, ok := allowedMarkdownImageTypes[contentType]; !ok { + return platform.MarkdownImage{}, c.invalidMarkdownImageSource(fmt.Errorf("unsupported image content type %q", contentType)) + } + content, err := io.ReadAll(io.LimitReader(resp.Body, maxMarkdownImageBytes+1)) + if err != nil { + return platform.MarkdownImage{}, err + } + if len(content) > maxMarkdownImageBytes { + return platform.MarkdownImage{}, errMarkdownImageTooLarge + } + return platform.MarkdownImage{Content: content, ContentType: contentType}, nil +} diff --git a/internal/github/markdown_images_test.go b/internal/github/markdown_images_test.go new file mode 100644 index 000000000..66ad082a5 --- /dev/null +++ b/internal/github/markdown_images_test.go @@ -0,0 +1,152 @@ +package github + +import ( + "net/http" + "net/http/httptest" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.kenn.io/forge/internal/platform" +) + +var pngBytes = []byte("\x89PNG\r\n\x1a\n\x00\x00\x00\rIHDR") + +type contentsRequest struct { + Path string + Ref string + Accept string +} + +// contentsServer serves the contents API for one repository file that lives on +// a branch whose name contains a slash, the case a web URL cannot delimit. +func contentsServer(t *testing.T, body []byte) (*httptest.Server, func() []contentsRequest) { + t.Helper() + var mu sync.Mutex + var requests []contentsRequest + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + requests = append(requests, contentsRequest{ + Path: r.URL.Path, Ref: r.URL.Query().Get("ref"), Accept: r.Header.Get("Accept"), + }) + mu.Unlock() + if r.URL.Path != "/api/v3/repos/acme/widgets/contents/docs/images/search.png" || + r.URL.Query().Get("ref") != "feat/search-controls" { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message":"Not Found"}`)) + return + } + w.Header().Set("Content-Type", "application/vnd.github.raw; charset=utf-8") + _, _ = w.Write(body) + })) + t.Cleanup(server.Close) + return server, func() []contentsRequest { + mu.Lock() + defer mu.Unlock() + return append([]contentsRequest(nil), requests...) + } +} + +func newMarkdownImageTestClient(t *testing.T, server *httptest.Server) *liveClient { + t.Helper() + client, err := NewClient(testTokenSource("token"), "github.com", nil, nil, WithBaseURLForTesting(server.URL)) + require.NoError(t, err) + return client.(*liveClient) +} + +func TestGetMarkdownImageResolvesRepositoryFileAcrossSlashedBranch(t *testing.T) { + assert := assert.New(t) + require := require.New(t) + server, requests := contentsServer(t, pngBytes) + client := newMarkdownImageTestClient(t, server) + + image, err := client.GetMarkdownImage(t.Context(), "acme", "widgets", + "https://github.com/acme/widgets/blob/feat/search-controls/docs/images/search.png?raw=true") + require.NoError(err) + assert.Equal("image/png", image.ContentType) + assert.Equal(pngBytes, image.Content) + assert.True(image.Mutable, "branch-addressed files change under the same URL") + + got := requests() + require.Len(got, 2, "the shortest ref candidate misses before the slashed branch resolves") + assert.Equal(contentsRequest{ + Path: "/api/v3/repos/acme/widgets/contents/search-controls/docs/images/search.png", + Ref: "feat", + Accept: "application/vnd.github.raw+json", + }, got[0]) + assert.Equal(contentsRequest{ + Path: "/api/v3/repos/acme/widgets/contents/docs/images/search.png", + Ref: "feat/search-controls", + Accept: "application/vnd.github.raw+json", + }, got[1]) +} + +func TestGetMarkdownImageAcceptsRawAndRawHostRepositoryURLs(t *testing.T) { + server, _ := contentsServer(t, pngBytes) + client := newMarkdownImageTestClient(t, server) + + for _, source := range []string{ + "https://github.com/acme/widgets/raw/feat/search-controls/docs/images/search.png", + "https://raw.githubusercontent.com/acme/widgets/feat/search-controls/docs/images/search.png", + "https://raw.githubusercontent.com/Acme/Widgets/feat/search-controls/docs/images/search.png", + } { + image, err := client.GetMarkdownImage(t.Context(), "acme", "widgets", source) + require.NoError(t, err, source) + assert.Equal(t, "image/png", image.ContentType, source) + } +} + +func TestGetMarkdownImageRejectsFilesOutsideRouteRepository(t *testing.T) { + server, requests := contentsServer(t, pngBytes) + client := newMarkdownImageTestClient(t, server) + + for _, source := range []string{ + "https://github.com/acme/other/blob/main/docs/images/search.png?raw=true", + "https://raw.githubusercontent.com/acme/other/main/docs/images/search.png", + "https://github.com/acme/widgets/tree/main/docs", + "https://github.com/acme/widgets/blob/main", + "https://github.com/acme/widgets/blob/main/../secret.png", + "https://gist.githubusercontent.com/acme/widgets/main/docs/images/search.png", + } { + _, err := client.GetMarkdownImage(t.Context(), "acme", "widgets", source) + var providerErr *platform.Error + require.ErrorAs(t, err, &providerErr, source) + assert.Equal(t, platform.ErrCodeInvalidArgument, providerErr.Code, source) + } + assert.Empty(t, requests(), "rejected sources must not reach the provider") +} + +func TestGetMarkdownImageReportsMissingRepositoryFile(t *testing.T) { + server, requests := contentsServer(t, pngBytes) + client := newMarkdownImageTestClient(t, server) + + _, err := client.GetMarkdownImage(t.Context(), "acme", "widgets", + "https://github.com/acme/widgets/blob/main/docs/images/missing.png?raw=true") + var providerErr *platform.Error + require.ErrorAs(t, err, &providerErr) + assert.Equal(t, platform.ErrCodeNotFound, providerErr.Code) + assert.Len(t, requests(), 3, "every ref split is tried before reporting not found") +} + +func TestGetMarkdownImageRejectsNonImageRepositoryFile(t *testing.T) { + server, _ := contentsServer(t, []byte("")) + client := newMarkdownImageTestClient(t, server) + + _, err := client.GetMarkdownImage(t.Context(), "acme", "widgets", + "https://github.com/acme/widgets/blob/feat/search-controls/docs/images/search.png?raw=true") + var providerErr *platform.Error + require.ErrorAs(t, err, &providerErr) + assert.Equal(t, platform.ErrCodeInvalidArgument, providerErr.Code) +} + +func TestGetMarkdownImageRejectsOversizedRepositoryFile(t *testing.T) { + oversized := make([]byte, maxMarkdownImageBytes+1) + copy(oversized, pngBytes) + server, _ := contentsServer(t, oversized) + client := newMarkdownImageTestClient(t, server) + + _, err := client.GetMarkdownImage(t.Context(), "acme", "widgets", + "https://github.com/acme/widgets/blob/feat/search-controls/docs/images/search.png?raw=true") + require.ErrorIs(t, err, errMarkdownImageTooLarge) +} diff --git a/internal/platform/client.go b/internal/platform/client.go index c846c4a2f..d7ee82f8b 100644 --- a/internal/platform/client.go +++ b/internal/platform/client.go @@ -43,6 +43,10 @@ type RepositoryReader interface { type MarkdownImage struct { Content []byte ContentType string + // Mutable marks a source whose bytes can change under the same URL, such + // as a file read through a branch or tag ref. Callers must cache it + // briefly instead of treating the URL as content-addressed. + Mutable bool } // MarkdownImageReader fetches provider-hosted images embedded in repository diff --git a/internal/server/markdown_image_cache.go b/internal/server/markdown_image_cache.go index d93b311ee..2e085ecc0 100644 --- a/internal/server/markdown_image_cache.go +++ b/internal/server/markdown_image_cache.go @@ -20,9 +20,12 @@ import ( const ( markdownImageCacheTTL = 14 * 24 * time.Hour + markdownImageMutableTTL = 5 * time.Minute markdownImageFetchTimeout = 30 * time.Second markdownImageCacheMaxBytes = int64(512 << 20) - markdownImageCacheMagic = "kenn-forge-markdown-image-v1\n" + markdownImageCacheMagic = "kenn-forge-markdown-image-v2\n" + markdownImageMutableFlag = "mutable" + markdownImageImmutableFlag = "immutable" ) type markdownImageCache struct { @@ -53,24 +56,66 @@ func (c *markdownImageCache) get(key string) (platform.MarkdownImage, bool) { } path := c.path(key) info, err := os.Stat(path) - if err != nil || time.Since(info.ModTime()) > markdownImageCacheTTL { - if err == nil { - _ = os.Remove(path) - } + if err != nil { return platform.MarkdownImage{}, false } data, err := os.ReadFile(path) - if err != nil || !bytes.HasPrefix(data, []byte(markdownImageCacheMagic)) { + if err != nil { + return platform.MarkdownImage{}, false + } + image, ok := decodeMarkdownImageCacheEntry(data) + if !ok { + _ = os.Remove(path) + return platform.MarkdownImage{}, false + } + ttl := markdownImageCacheTTL + if image.Mutable { + ttl = markdownImageMutableTTL + } + if time.Since(info.ModTime()) > ttl { + _ = os.Remove(path) + return platform.MarkdownImage{}, false + } + return image, true +} + +// Cache entries are the magic line, the content type line, a mutability flag +// line, then the raw bytes. +func encodeMarkdownImageCacheHeader(image platform.MarkdownImage) string { + flag := markdownImageImmutableFlag + if image.Mutable { + flag = markdownImageMutableFlag + } + return markdownImageCacheMagic + image.ContentType + "\n" + flag + "\n" +} + +func decodeMarkdownImageCacheEntry(data []byte) (platform.MarkdownImage, bool) { + if !bytes.HasPrefix(data, []byte(markdownImageCacheMagic)) { return platform.MarkdownImage{}, false } payload := data[len(markdownImageCacheMagic):] - separator := bytes.IndexByte(payload, '\n') - if separator <= 0 { + typeEnd := bytes.IndexByte(payload, '\n') + if typeEnd <= 0 { + return platform.MarkdownImage{}, false + } + rest := payload[typeEnd+1:] + flagEnd := bytes.IndexByte(rest, '\n') + if flagEnd <= 0 { + return platform.MarkdownImage{}, false + } + var mutable bool + switch string(rest[:flagEnd]) { + case markdownImageMutableFlag: + mutable = true + case markdownImageImmutableFlag: + mutable = false + default: return platform.MarkdownImage{}, false } return platform.MarkdownImage{ - ContentType: string(payload[:separator]), - Content: payload[separator+1:], + ContentType: string(payload[:typeEnd]), + Content: rest[flagEnd+1:], + Mutable: mutable, }, true } @@ -123,7 +168,7 @@ func (c *markdownImageCache) set(key string, image platform.MarkdownImage) error } tempPath := temp.Name() defer func() { _ = os.Remove(tempPath) }() - if _, err = temp.WriteString(markdownImageCacheMagic + image.ContentType + "\n"); err == nil { + if _, err = temp.WriteString(encodeMarkdownImageCacheHeader(image)); err == nil { _, err = temp.Write(image.Content) } closeErr := temp.Close() diff --git a/internal/server/markdown_images.go b/internal/server/markdown_images.go index 4a740883a..18df830cb 100644 --- a/internal/server/markdown_images.go +++ b/internal/server/markdown_images.go @@ -58,8 +58,7 @@ func (s *Server) getMarkdownImageFor( return nil, markdownImageError(ctx, err, kind, host) } ref := httpapi.PlatformRepoRef(*repo) - cacheKey := string(ref.Platform) + "\x00" + ref.Host + "\x00" + ref.RepoPath + "\x00" + source - image, err := s.markdownImages.load(ctx, cacheKey, func(fetchCtx context.Context) (platform.MarkdownImage, error) { + image, err := s.markdownImages.load(ctx, markdownImageCacheKey(ref, source), func(fetchCtx context.Context) (platform.MarkdownImage, error) { return reader.GetMarkdownImage(fetchCtx, ref, source) }) if err != nil { @@ -67,13 +66,27 @@ func (s *Server) getMarkdownImageFor( } return &markdownImageOutput{ ContentType: image.ContentType, - CacheControl: "private, max-age=31536000, immutable", + CacheControl: markdownImageCacheControl(image), ContentLength: strconv.Itoa(len(image.Content)), ContentTypeOptions: "nosniff", Body: image.Content, }, nil } +// markdownImageCacheKey uses the stable provider identity, not the owner/name +// route: a replacement repository at a reused route must never be served the +// previous occupant's private bytes. +func markdownImageCacheKey(ref platform.RepoRef, source string) string { + return string(ref.Platform) + "\x00" + ref.Host + "\x00" + ref.PlatformExternalID + "\x00" + source +} + +func markdownImageCacheControl(image platform.MarkdownImage) string { + if image.Mutable { + return "private, max-age=" + strconv.Itoa(int(markdownImageMutableTTL.Seconds())) + } + return "private, max-age=31536000, immutable" +} + func markdownImageError(ctx context.Context, err error, kind platform.Kind, host string) error { if ctxErr := ctx.Err(); ctxErr != nil { return ctxErr diff --git a/internal/server/markdown_images_test.go b/internal/server/markdown_images_test.go index dee5c6836..8211aa83b 100644 --- a/internal/server/markdown_images_test.go +++ b/internal/server/markdown_images_test.go @@ -2,10 +2,12 @@ package server import ( "context" + "fmt" "net/http" "net/http/httptest" "net/url" "os" + "path/filepath" "testing" "time" @@ -263,3 +265,87 @@ func TestMarkdownImageRouteResolvesOpaqueGitLabProjectID(t *testing.T) { "/api/v4/projects/42/uploads/secret/private.png", }, paths) } + +// Owner/name is a mutable route. When a different repository takes over the +// route, the cache must not hand it the previous occupant's private bytes. +func TestMarkdownImageCacheDoesNotFollowRouteReuse(t *testing.T) { + require := require.New(t) + assert := assert.New(t) + const source = "https://github.com/acme/widget/blob/main/docs/images/search.png?raw=true" + fetches := 0 + mock := &mockGH{getMarkdownImageFn: func( + context.Context, string, string, string, + ) (platform.MarkdownImage, error) { + fetches++ + return platform.MarkdownImage{ + Content: []byte(fmt.Sprintf("bytes-%d", fetches)), ContentType: "image/png", + }, nil + }} + srv, database := setupTestServerWithMock(t, mock) + srv.markdownImages = newMarkdownImageCache(t.TempDir()) + _, err := database.UpsertRepo(t.Context(), verifiedGitHubRepoIdentity("github.com", "acme", "widget")) + require.NoError(err) + target := "/api/v1/repo/github/acme/widget/markdown-image?source=" + url.QueryEscape(source) + + first := repoBrowserRequest(t, srv, http.MethodGet, target) + require.Equal(http.StatusOK, first.Code, first.Body.String()) + assert.Equal("bytes-1", first.Body.String()) + + replacement := db.GitHubRepoIdentity("github.com", "acme", "widget") + replacement.PlatformRepoID = "repo-acme-widget-replacement" + _, _, err = database.ReconcileRepositoryObservation(t.Context(), replacement, time.Now().UTC().Add(time.Second)) + require.NoError(err) + + second := repoBrowserRequest(t, srv, http.MethodGet, target) + require.Equal(http.StatusOK, second.Code, second.Body.String()) + assert.Equal("bytes-2", second.Body.String(), "the replacement repository must fetch its own image") + assert.Equal(2, fetches) +} + +// Branch-addressed files change under the same URL, so the browser and the +// disk cache must both revalidate them soon; attachments stay immutable. +func TestMarkdownImageRouteCachesMutableImagesBriefly(t *testing.T) { + require := require.New(t) + assert := assert.New(t) + const mutableSource = "https://github.com/acme/widget/blob/main/docs/images/search.png?raw=true" + const immutableSource = "https://github.com/user-attachments/assets/11111111-2222-3333-4444-555555555555" + fetches := map[string]int{} + mock := &mockGH{getMarkdownImageFn: func( + _ context.Context, _, _ string, sourceURL string, + ) (platform.MarkdownImage, error) { + fetches[sourceURL]++ + return platform.MarkdownImage{ + Content: []byte("png-bytes"), + ContentType: "image/png", + Mutable: sourceURL == mutableSource, + }, nil + }} + srv, database := setupTestServerWithMock(t, mock) + srv.markdownImages = newMarkdownImageCache(t.TempDir()) + _, err := database.UpsertRepo(t.Context(), verifiedGitHubRepoIdentity("github.com", "acme", "widget")) + require.NoError(err) + request := func(source string) *httptest.ResponseRecorder { + return repoBrowserRequest(t, srv, http.MethodGet, + "/api/v1/repo/github/acme/widget/markdown-image?source="+url.QueryEscape(source)) + } + + mutable := request(mutableSource) + require.Equal(http.StatusOK, mutable.Code, mutable.Body.String()) + assert.Equal("private, max-age=300", mutable.Header().Get("Cache-Control")) + immutable := request(immutableSource) + require.Equal(http.StatusOK, immutable.Code, immutable.Body.String()) + assert.Equal("private, max-age=31536000, immutable", immutable.Header().Get("Cache-Control")) + + entries, err := os.ReadDir(srv.markdownImages.root) + require.NoError(err) + require.Len(entries, 2) + aged := time.Now().Add(-markdownImageMutableTTL - time.Minute) + for _, entry := range entries { + require.NoError(os.Chtimes(filepath.Join(srv.markdownImages.root, entry.Name()), aged, aged)) + } + + require.Equal(http.StatusOK, request(mutableSource).Code) + require.Equal(http.StatusOK, request(immutableSource).Code) + assert.Equal(2, fetches[mutableSource], "a mutable entry past its short TTL is fetched again") + assert.Equal(1, fetches[immutableSource], "an immutable entry is still served from disk") +}