diff --git a/CLAUDE.md b/CLAUDE.md index a0c754ed..15acf77a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -60,7 +60,7 @@ git push origin backend/vX.Y.Z GoReleaser must ignore `backend/*` tags in `.goreleaser.yml` so release notes and current-tag detection use only product tags. Docker image publishing stays separate and is handled by the existing Docker workflow. -For local artifact runs, install GoReleaser, Go 1.25, Node 24+, PNPM 10, and Perl, then use `make release`. The target runs a snapshot/no-publish GoReleaser build, leaves local artifacts and metadata in `dist/`, and cleans generated frontend embed files after GoReleaser exits. Do not run raw `goreleaser release` for local artifacts unless you also run `./scripts/cleanup-release-assets.sh` afterward. +For local artifact runs, install GoReleaser, Go 1.25, Node 24+ and PNPM 10, then use `make release`. The target runs a snapshot/no-publish GoReleaser build, leaves local artifacts and metadata in `dist/`, and cleans generated frontend embed files after GoReleaser exits. Do not run raw `goreleaser release` for local artifacts unless you also run `./scripts/cleanup-release-assets.sh` afterward. ## Milestones and Issue Labels diff --git a/Dockerfile b/Dockerfile index 6d338f85..ab8175fd 100644 --- a/Dockerfile +++ b/Dockerfile @@ -62,7 +62,6 @@ RUN apk --no-cache add gcc libc-dev ADD backend /build/backend # to embed the frontend files statically into Remark42 binary COPY --from=build-frontend /srv/frontend/apps/remark42/public/ /build/backend/app/cmd/web/ -RUN find /build/backend/app/cmd/web/ -regex '.*\.\(html\|js\|mjs\)$' -print -exec sed -i "s|{% REMARK_URL %}|http://127.0.0.1:8080|g" {} \; WORKDIR /build/backend RUN echo go version: `go version` diff --git a/backend/app/rest/api/rest.go b/backend/app/rest/api/rest.go index cac95b4a..83942744 100644 --- a/backend/app/rest/api/rest.go +++ b/backend/app/rest/api/rest.go @@ -394,7 +394,7 @@ func (s *Rest) routes() http.Handler { log.Printf("[WARN] no embedded frontend, serving built-in assets only: %v", err) embeddedFrontend = emptyFS{} } - addFileServer(router, embeddedFrontend, s.WebRoot, s.Version) + addFileServer(router, embeddedFrontend, s.WebRoot, s.Version, s.RemarkURL) return router } @@ -510,7 +510,7 @@ func (s *Rest) configCtrl(w http.ResponseWriter, r *http.Request) { // serves /web from the frontend build, falling back to the assets embedded in the binary for // names the build does not produce. the frontend build is read from webRoot on disk, or from the // copy embedded at app/cmd/web when that directory is absent. -func addFileServer(r *routegroup.Bundle, embeddedFrontend fs.FS, webRoot, version string) { +func addFileServer(r *routegroup.Bundle, embeddedFrontend fs.FS, webRoot, version, remarkURL string) { frontendFS := embeddedFrontend if _, err := os.Stat(webRoot); err == nil { @@ -520,12 +520,22 @@ func addFileServer(r *routegroup.Bundle, embeddedFrontend fs.FS, webRoot, versio log.Printf("[INFO] run file server, embedded") } - webFS := http.StripPrefix("/web", http.FileServer(http.FS(webFiles{frontend: frontendFS, embedded: webassets.FS}))) + // wrapped rather than substituted once at startup: the disk root can change under a running + // server, and the docker image has already substituted its copy, where this is a no-op + sources := templatedFS{ + fs: webFiles{frontend: frontendFS, embedded: webassets.FS}, + remarkURL: remarkURL, + } + webFS := http.StripPrefix("/web", http.FileServer(http.FS(sources))) r.HandleFunc("GET /web", http.RedirectHandler("/web/", http.StatusMovedPermanently).ServeHTTP) r.With(rateLimiter(20), R.Timeout(10*time.Second), - cacheControl(time.Hour, version), + // the served body now depends on remarkURL, so it has to be part of the validator. Without + // it an operator who corrects a wrong REMARK_URL and restarts the same binary keeps getting + // 304 on revalidation, and the client keeps a bundle addressed to the old host for good, + // since no-cache means it revalidates rather than aging out + cacheControl(time.Hour, version+":"+remarkURL), ).HandleFunc("GET /web/", func(w http.ResponseWriter, r *http.Request) { // don't show dirs, just serve files if strings.HasSuffix(r.URL.Path, "/") && len(r.URL.Path) > 1 && r.URL.Path != ("/web/") { diff --git a/backend/app/rest/api/rest_public_test.go b/backend/app/rest/api/rest_public_test.go index adff57f0..30fcbf88 100644 --- a/backend/app/rest/api/rest_public_test.go +++ b/backend/app/rest/api/rest_public_test.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/json" "fmt" + "image/png" "io" "mime/multipart" "net/http" @@ -986,13 +987,26 @@ func TestRest_QR(t *testing.T) { assert.Equal(t, "image/png", r.Header.Get("Content-Type")) assert.Equal(t, http.StatusOK, r.StatusCode) - // compare the image + // compare the decoded image rather than the encoded bytes: the pixels are what the endpoint + // promises, while the byte stream is whatever the toolchain's png encoder produces, and + // pinning that fails on a go release that changes it fh, err := os.Open("testdata/qr_test.png") defer func() { assert.NoError(t, fh.Close()) }() - assert.NoError(t, err) - img, err := io.ReadAll(fh) - assert.NoError(t, err) - assert.Equal(t, img, bdy) + require.NoError(t, err) + + want, err := png.Decode(fh) + require.NoError(t, err) + got, err := png.Decode(bytes.NewReader(bdy)) + require.NoError(t, err, "the endpoint did not return a decodable png") + + require.Equal(t, want.Bounds(), got.Bounds(), "the qr code is not the size it used to be") + for y := want.Bounds().Min.Y; y < want.Bounds().Max.Y; y++ { + for x := want.Bounds().Min.X; x < want.Bounds().Max.X; x++ { + if want.At(x, y) != got.At(x, y) { + t.Fatalf("the qr code differs at %d,%d: want %v, got %v", x, y, want.At(x, y), got.At(x, y)) + } + } + } } func TestRest_Info(t *testing.T) { diff --git a/backend/app/rest/api/rest_test.go b/backend/app/rest/api/rest_test.go index 53148231..7441a024 100644 --- a/backend/app/rest/api/rest_test.go +++ b/backend/app/rest/api/rest_test.go @@ -189,7 +189,7 @@ func TestRest_FileServerBackendAssets(t *testing.T) { func TestRest_FileServerEmbeddedFrontend(t *testing.T) { frontend := fstest.MapFS{"index.html": {Data: []byte("embedded frontend index")}} router := routegroup.New(http.NewServeMux()) - addFileServer(router, frontend, filepath.Join(t.TempDir(), "absent"), "test-version") + addFileServer(router, frontend, filepath.Join(t.TempDir(), "absent"), "test-version", "https://remark.example.com") ts := httptest.NewServer(router) defer ts.Close() diff --git a/backend/app/rest/api/webfiles.go b/backend/app/rest/api/webfiles.go index e5c3b120..69595e8e 100644 --- a/backend/app/rest/api/webfiles.go +++ b/backend/app/rest/api/webfiles.go @@ -1,7 +1,9 @@ package api import ( + "bytes" "errors" + "io" "io/fs" "path/filepath" "strings" @@ -68,3 +70,71 @@ type emptyFS struct{} func (emptyFS) Open(name string) (fs.File, error) { return nil, &fs.PathError{Op: "open", Path: name, Err: fs.ErrNotExist} } + +// remarkURLPlaceholder is what the frontend build carries wherever the instance URL belongs. The +// bundler cannot know that URL, so it emits this marker and every distribution fills it in: the +// docker image rewrites the files under the web root at container start, and the binary, which +// serves the build embedded in itself and has nothing to rewrite, does it here. +const remarkURLPlaceholder = "{% REMARK_URL %}" + +// templatedFS fills the instance URL into the files carrying the placeholder. Without it the +// binary serves whatever the build baked in, which is a host no visitor can reach, and the widget +// falls back to it whenever a page omits remark_config.host. +type templatedFS struct { + fs fs.FS + remarkURL string +} + +// Open substitutes in the file types the frontend templates, and hands everything else through +// untouched so images and stylesheets keep streaming from their original source +func (t templatedFS) Open(name string) (fs.File, error) { + f, err := t.fs.Open(name) + if err != nil || !templatedName(name) { + return f, err + } + + info, err := f.Stat() + if err != nil || info.IsDir() { + return f, err + } + + body, err := io.ReadAll(f) + if cerr := f.Close(); err == nil { + err = cerr + } + if err != nil { + return nil, err + } + + body = bytes.ReplaceAll(body, []byte(remarkURLPlaceholder), []byte(t.remarkURL)) + return &memFile{Reader: bytes.NewReader(body), info: sizedInfo{FileInfo: info, size: int64(len(body))}}, nil +} + +// templatedName reports whether the frontend templates this file type. It mirrors the set the +// docker image rewrites, so both distributions substitute in the same files +func templatedName(name string) bool { + switch filepath.Ext(name) { + case ".html", ".js", ".mjs": + return true + } + return false +} + +// memFile is a substituted file held in memory. The file server needs a seeker to answer range +// requests and to sniff a content type, which a substituted body no longer has on disk +type memFile struct { + *bytes.Reader + info fs.FileInfo +} + +func (f *memFile) Stat() (fs.FileInfo, error) { return f.info, nil } +func (f *memFile) Close() error { return nil } + +// sizedInfo reports the length after substitution. The file server writes Content-Length from it, +// so reporting the length on disk would truncate the response or leave the client waiting +type sizedInfo struct { + fs.FileInfo + size int64 +} + +func (i sizedInfo) Size() int64 { return i.size } diff --git a/backend/app/rest/api/webfiles_test.go b/backend/app/rest/api/webfiles_test.go index c4e994e5..402d2213 100644 --- a/backend/app/rest/api/webfiles_test.go +++ b/backend/app/rest/api/webfiles_test.go @@ -3,11 +3,15 @@ package api import ( "io" "io/fs" + "net/http" + "net/http/httptest" "os" "path/filepath" "testing" "testing/fstest" + "github.com/go-pkgz/routegroup" + "github.com/umputun/remark42/backend/app/webassets" "github.com/stretchr/testify/assert" @@ -218,3 +222,122 @@ func TestWebFiles_OpenUnreadableFrontendFile(t *testing.T) { _ = f.Close() } } + +func TestTemplatedFS_SubstitutesTheInstanceURL(t *testing.T) { + const placeholder = "host: '" + remarkURLPlaceholder + "'" + source := fstest.MapFS{ + "iframe.html": {Data: []byte(placeholder)}, + "embed.mjs": {Data: []byte(placeholder)}, + "embed.js": {Data: []byte(placeholder)}, + "remark.css": {Data: []byte(placeholder)}, + "nothing.html": {Data: []byte("no marker here")}, + } + tfs := templatedFS{fs: source, remarkURL: "https://remark.example.com"} + + tbl := []struct { + name string + want string + }{ + {"iframe.html", "host: 'https://remark.example.com'"}, + {"embed.mjs", "host: 'https://remark.example.com'"}, + {"embed.js", "host: 'https://remark.example.com'"}, + // the docker image rewrites html, js and mjs and nothing else, and a stylesheet carrying + // the marker would mean the frontend started templating a file type this does not cover + {"remark.css", placeholder}, + {"nothing.html", "no marker here"}, + } + + for _, tt := range tbl { + t.Run(tt.name, func(t *testing.T) { + f, err := tfs.Open(tt.name) + require.NoError(t, err) + defer func() { assert.NoError(t, f.Close()) }() + + body, err := io.ReadAll(f) + require.NoError(t, err) + assert.Equal(t, tt.want, string(body)) + + info, err := f.Stat() + require.NoError(t, err) + assert.Equal(t, int64(len(tt.want)), info.Size(), + "the size has to be the substituted one, or the response is truncated or left hanging") + assert.Equal(t, tt.name, info.Name()) + }) + } +} + +func TestTemplatedFS_PassesErrorsThrough(t *testing.T) { + tfs := templatedFS{fs: fstest.MapFS{}, remarkURL: "https://remark.example.com"} + + _, err := tfs.Open("absent.html") + assert.ErrorIs(t, err, fs.ErrNotExist) +} + +// TestRest_FileServerFillsInTheInstanceURL covers the reason templatedFS exists: the binary serves +// the frontend build embedded in itself, and nothing else fills the placeholder in for it. +func TestRest_FileServerFillsInTheInstanceURL(t *testing.T) { + frontend := fstest.MapFS{ + "embed.mjs": {Data: []byte("host=\"" + remarkURLPlaceholder + "\"")}, + "logo.svg": {Data: []byte(remarkURLPlaceholder)}, + "plain.html": {Data: []byte("nothing to fill in")}, + } + router := routegroup.New(http.NewServeMux()) + addFileServer(router, frontend, filepath.Join(t.TempDir(), "absent"), "test-version", "https://remark.example.com") + + ts := httptest.NewServer(router) + defer ts.Close() + + t.Run("the bundle carries the configured url", func(t *testing.T) { + body, code := get(t, ts.URL+"/web/embed.mjs") + assert.Equal(t, http.StatusOK, code) + assert.Equal(t, `host="https://remark.example.com"`, body) + }) + + t.Run("the legacy js name carries it too", func(t *testing.T) { + body, code := get(t, ts.URL+"/web/embed.js") + assert.Equal(t, http.StatusOK, code) + assert.Equal(t, `host="https://remark.example.com"`, body) + }) + + t.Run("other types are served untouched", func(t *testing.T) { + body, code := get(t, ts.URL+"/web/logo.svg") + assert.Equal(t, http.StatusOK, code) + assert.Equal(t, remarkURLPlaceholder, body) + }) + + t.Run("a file without the marker is unchanged", func(t *testing.T) { + body, code := get(t, ts.URL+"/web/plain.html") + assert.Equal(t, http.StatusOK, code) + assert.Equal(t, "nothing to fill in", body) + }) +} + +// TestRest_FileServerEtagVariesWithTheInstanceURL covers the case this substitution exists for. An +// operator who notices the widget is addressed to the wrong host corrects REMARK_URL and restarts, +// and the binary and so the version is unchanged. If the validator ignores remarkURL the client +// revalidates, gets 304 and keeps the bundle pointing at the old host. Cache-Control is no-cache, +// so it revalidates every time and never ages out of that state. +func TestRest_FileServerEtagVariesWithTheInstanceURL(t *testing.T) { + frontend := fstest.MapFS{"embed.mjs": {Data: []byte("host=\"" + remarkURLPlaceholder + "\"")}} + + etagFor := func(remarkURL string) string { + router := routegroup.New(http.NewServeMux()) + addFileServer(router, frontend, filepath.Join(t.TempDir(), "absent"), "test-version", remarkURL) + ts := httptest.NewServer(router) + defer ts.Close() + + resp, err := http.Get(ts.URL + "/web/embed.mjs") + require.NoError(t, err) + defer resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode) + return resp.Header.Get("Etag") + } + + first := etagFor("https://old.example.com") + second := etagFor("https://new.example.com") + + require.NotEmpty(t, first, "the file server has to send a validator at all") + assert.NotEqual(t, first, second, + "same version and same path, different instance url: the validator has to change or the "+ + "client keeps a bundle addressed to the old host") +} diff --git a/backend/app/store/image/image_test.go b/backend/app/store/image/image_test.go index 12eeb213..836c587c 100644 --- a/backend/app/store/image/image_test.go +++ b/backend/app/store/image/image_test.go @@ -41,13 +41,15 @@ func TestService_SaveAndLoad(t *testing.T) { assert.Equal(t, "test_id", store.LoadCalls()[0].ID) } +// the resized dimensions are what resize promises; the encoded length is whatever the compressor +// in the toolchain happens to produce, and pinning it fails on a go release that changes it func TestService_Resize(t *testing.T) { img, err := readAndValidateImage(gopherPNG(), 1500) assert.NoError(t, err) - assert.Equal(t, 1462, len(img)) + assert.NotEmpty(t, img) img = resize(img, 32, 32) - assert.Equal(t, 1135, len(img)) + assertImageFits(t, img, 32, 32) } func TestService_ResizeJpeg(t *testing.T) { @@ -57,10 +59,24 @@ func TestService_ResizeJpeg(t *testing.T) { img, err := readAndValidateImage(fh, 32000) assert.NoError(t, err) - assert.InDelta(t, 16756, len(img), 100) + assert.NotEmpty(t, img) img = resize(img, 400, 300) - assert.InDelta(t, 10913, len(img), 100) + assertImageFits(t, img, 400, 300) +} + +// assertImageFits decodes the image and checks it is inside the box resize was given, and that it +// touches one side of it, which is what fitting to a box rather than merely shrinking means +func assertImageFits(t *testing.T, data []byte, limitW, limitH int) { + t.Helper() + + cfg, _, err := image.DecodeConfig(bytes.NewReader(data)) + require.NoError(t, err, "the resized image does not decode") + + assert.LessOrEqual(t, cfg.Width, limitW, "wider than the box it was resized into") + assert.LessOrEqual(t, cfg.Height, limitH, "taller than the box it was resized into") + assert.True(t, cfg.Width == limitW || cfg.Height == limitH, + "%dx%d touches neither side of the %dx%d box, so it was not fitted to it", cfg.Width, cfg.Height, limitW, limitH) } func TestService_SaveTooLarge(t *testing.T) { diff --git a/e2e/assets_test.go b/e2e/assets_test.go new file mode 100644 index 00000000..c951db84 --- /dev/null +++ b/e2e/assets_test.go @@ -0,0 +1,83 @@ +//go:build e2e + +package e2e + +import ( + "fmt" + neturl "net/url" + "strings" + "testing" + + "github.com/mxschmitt/playwright-go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// remarkURLPlaceholder is what the frontend build emits wherever the instance url belongs. The +// bundler cannot know that url, so every distribution fills the marker in: the docker image +// rewrites its web root at container start, the binary substitutes as it serves. +const remarkURLPlaceholder = "{% REMARK_URL %}" + +// TestAssets_InstanceURLIsFilledIn covers the substitution, which nothing else here exercises. +// The demo pages set remark_config.host from location.origin themselves, so the compiled-in +// fallback is never read and the marker could survive into a release without a test noticing. +func TestAssets_InstanceURLIsFilledIn(t *testing.T) { + page := newPage(t) + + pauseForAuthLimit() + _, err := page.Goto(baseURL + "/web/") + require.NoError(t, err) + + // every file type the distributions rewrite, not only the one the widget happens to load + for _, name := range []string{"embed.mjs", "counter.mjs", "last-comments.mjs", "index.html"} { + status, body := pageFetch(t, page, "GET", baseURL+"/web/"+name, nil) + require.Equal(t, 200, status, name) + assert.NotContains(t, body, remarkURLPlaceholder, "%s still carries the marker", name) + } + + // and the value that replaced it is this instance, not some other host. only the bundles + // carry it; index.html sets the host from the page's own origin + status, body := pageFetch(t, page, "GET", baseURL+"/web/embed.mjs", nil) + require.Equal(t, 200, status) + assert.Contains(t, body, baseURL, "embed.mjs should fall back to this instance") +} + +// TestAssets_WidgetRunsOnTheCompiledInURL is the behavior the substitution exists for. +// +// The widget document carries no host: iframe.html builds remark_config out of its own query +// string, and the parent never passes one, so everything the bundle requests is addressed with +// the url the build was substituted with. A marker left in place leaves the widget asking for +// `{% REMARK_URL %}/api/v1/config` and rendering nothing. +// +// The demo pages cannot show this. Their loader builds the bundle's own script url from +// remark_config.host, so a page without one never gets as far as loading the widget. +func TestAssets_WidgetRunsOnTheCompiledInURL(t *testing.T) { + page := newPage(t) + + var configURL string + page.OnRequest(func(r playwright.Request) { + if configURL == "" && strings.Contains(r.URL(), "/api/v1/config") { + configURL = r.URL() + } + }) + + pauseForAuthLimit() + _, err := page.Goto(fmt.Sprintf("%s/web/iframe.html?site_id=remark&url=%s", + baseURL, neturl.QueryEscape(threadURL(t)))) + require.NoError(t, err) + + // the document really had no host of its own, or the fallback was never reached + host, err := page.Evaluate(`() => window.remark_config && window.remark_config.host`) + require.NoError(t, err) + require.Nil(t, host, "the widget document should carry no host") + + // it rendered, so the url it was addressing resolved + waitVisible(t, page.Locator(commentFormSel).First()) + + eventually(t, waitTimeout, "the widget never asked for its config", func() bool { + return configURL != "" + }) + assert.True(t, strings.HasPrefix(configURL, baseURL), + "the widget addressed %q rather than this instance", configURL) + assert.NotContains(t, configURL, remarkURLPlaceholder) +} diff --git a/scripts/prepare-release-assets.sh b/scripts/prepare-release-assets.sh index da838d3c..7cee64a9 100755 --- a/scripts/prepare-release-assets.sh +++ b/scripts/prepare-release-assets.sh @@ -7,7 +7,7 @@ PUBLIC_DIR="$APP_DIR/public" EMBED_DIR="$ROOT/backend/app/cmd/web" PREPARED_MARKER="$EMBED_DIR/.release-assets-prepared" -for cmd in git pnpm perl; do +for cmd in git pnpm; do if ! command -v "$cmd" >/dev/null 2>&1; then echo "error: $cmd is required to build release assets" >&2 exit 1 @@ -48,11 +48,11 @@ mkdir -p "$EMBED_DIR" cp -R "$PUBLIC_DIR"/. "$EMBED_DIR"/ -find "$EMBED_DIR" -type f \( -name '*.html' -o -name '*.js' -o -name '*.mjs' \) \ - -exec perl -pi -e 's|\{\% REMARK_URL \%\}|http://127.0.0.1:8080|g' {} + - -if grep -R "{% REMARK_URL %}" "$EMBED_DIR" >/dev/null; then - echo "error: unreplaced REMARK_URL placeholder in $EMBED_DIR" >&2 +# the placeholder stays in: the server fills it with its own REMARK_URL when it serves these files, +# which is the only chance the binary gets. substituting a value here would bake one instance's +# address into every copy of the release +if ! grep -Rq "{% REMARK_URL %}" "$EMBED_DIR"; then + echo "error: no REMARK_URL placeholder in $EMBED_DIR, the build stopped templating it" >&2 exit 1 fi