23 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 11.2-ready-to-share-summercms-io-website-and-newsletter-plugin | 2026-10-01T14:47:34Z | standard | 71 |
|
|
issues_found |
Phase 11.2: Code Review Report
Reviewed: 2026-10-01T14:47:34Z Depth: standard Files Reviewed: 71 Status: issues_found
Summary
The review covered the framework's site_url/site_label change (D-41/D-46), the PostgreSQL 15 doc edits and the phase gate script in summercms.go. In the three new site repositories it covered the static file handler and routes, the build, smoke and deploy-check scripts, the nginx and supervisor configs, DEPLOY.md, the D-40 terminal check and the Nuxt site.
The core Go code holds up under adversarial tracing:
- Path traversal. The static handler (
static.go) cannot traverse paths: it only looks names up in a preloaded in-memory map, afterpath.Cleanand a dot-segment refusal. - Open redirect. The extension-less 301 cannot become an open redirect. Its
Locationis built from the mount and a cleaned name, and only for a file that exists in the map, so a//hostvalue is impossible. - Cache headers and ETags. These match D-07. ETags still revalidate behind nginx gzip, because nginx turns them weak and Go's
If-None-Matchuses weak comparison. site_urlvalidation (checkSiteURL). It is strict: it accepts only http(s) URLs with a host and no user info, or a path with a single leading/.html/templateescapes the header output in both the attribute and text contexts.
The defects are concentrated in the deploy procedure and the check tooling:
- One security flaw in the server layout: root runs file operations inside a directory the service account owns.
- Missing HSTS.
- A certbot renewal path that may break.
- A terminal check that hides a PATH dependency the page copy has.
- A site test gate that only works on one Node major line.
- A smoke script that can test the wrong server.
Critical Issues
CR-01: The install root is owned by the service account, so root's deploy steps can be redirected through symlinks
File: /media/nvme/dev/golem15/summercms.io/summercms/sm-summercmsio-app/DEPLOY.md:74-83 (also :123-124, :135-137)
Issue: adduser --system --group --home /srv/summercms-io ... summercms creates /srv/summercms-io as the account's home, owned by summercms. Line 82 then makes bin/ and config/ root-owned, and the comment claims "the service cannot rewrite its own binary". That claim does not hold.
-
The claim is false. The owner of the parent directory can rename or delete any entry in it. A compromised
summercmsprocess canmv bin bin.x && mkdir binand drop its ownsummercms-io, which supervisor then starts on the next restart. -
Root can be redirected through symlinks. The service can also replace
binorconfigwith a symlink to a directory of its choosing. Every later deploy step runs as root inside that user-controlled directory:rsync --rsync-path="sudo rsync" ... rome:/srv/summercms-io/bin/summercms-io.newrsync --rsync-path="sudo rsync" config/ rome:/srv/summercms-io/config/sudo cp -p ...sudo mv bin/summercms-io.new bin/summercms-io
Each of these follows the symlink and writes root-owned files into an arbitrary directory (CWE-59). The procedure is documented to be run verbatim on every deploy, so the privilege boundary D-32 asks for does not exist.
Fix: Keep the install root root-owned. Give only storage/ (and .env read access) to the service:
sudo adduser --system --group --no-create-home --home /nonexistent --shell /usr/sbin/nologin summercms
sudo install -d -o root -g root -m 0755 /srv/summercms-io /srv/summercms-io/bin /srv/summercms-io/config
sudo install -d -o summercms -g summercms -m 0750 /srv/summercms-io/storage
# .env: root-owned, group-readable by the service, not writable by it
sudo install -o root -g summercms -m 0640 /dev/null /srv/summercms-io/.env
Then confirm that serve writes nothing outside storage/ (the uploads bucket is ./storage/app/uploads with create_dir=true).
Warnings
WR-01: The HTTPS server block sends no Strict-Transport-Security header
File: /media/nvme/dev/golem15/summercms.io/summercms/sm-summercmsio-app/deploy/nginx/summercms.io.conf:33-66
Issue: The app sets only X-Content-Type-Options and Referrer-Policy (static.go:218-221), and nginx adds nothing. With no HSTS, a first visit or a typed summercms.io goes over plain HTTP before the 301 and can be stripped by an attacker on the network. The focus area for this phase lists "security headers" on the nginx config, and this one is missing.
Fix: Add the header to the apex block, and to the www HTTPS block as well:
add_header Strict-Transport-Security "max-age=31536000; includeSubDomains" always;
Start with a short max-age if subdomains on rome are not all on HTTPS yet. Extend the post-cutover verification in DEPLOY.md to check for the header.
WR-02: The HTTP block redirects ACME challenges, and the app 404s /.well-known/
File: /media/nvme/dev/golem15/summercms.io/summercms/sm-summercmsio-app/deploy/nginx/summercms.io.conf:10-16
Issue: The port-80 server returns 301 https://summercms.io$request_uri for every path, including /.well-known/acme-challenge/*. Let's Encrypt follows that redirect to the HTTPS block, which proxies everything to the binary, and the binary refuses any dot-segment with a 404 (static.go:164-169).
- Webroot authenticator. If the existing certbot lineage on rome uses the webroot authenticator, certificate renewal fails silently about 60 days after cutover.
- nginx authenticator. Renewal survives only because certbot injects its own
rewrite ... break.
DEPLOY.md's cutover copies the certificate lines but never checks the renewal method.
Fix: Serve challenges before the redirect, and add a renewal dry run to the cutover checklist:
server {
listen 80; listen [::]:80;
server_name summercms.io www.summercms.io;
location ^~ /.well-known/acme-challenge/ { root /var/www/letsencrypt; } # match the lineage's webroot
location / { return 301 https://summercms.io$request_uri; }
}
sudo grep authenticator /etc/letsencrypt/renewal/summercms.io.conf
sudo certbot renew --cert-name summercms.io --dry-run
WR-03: The D-40 terminal check puts GOBIN on PATH, so it does not prove the page's commands work in a fresh shell
File: /media/nvme/dev/golem15/summercms.io/summercms/sm-summercmsio-app/terminal_check_test.go:68-88,107-109 (copy: vue-summercmsio-app/app/data/terminal.json:9,13)
Issue: The page tells visitors to run go install ./cmd/summer and then summer build. go install writes to $(go env GOPATH)/bin (~/go/bin). The official Linux install instructions only add /usr/local/go/bin to PATH, so in a common fresh shell summer build fails with summer: command not found.
terminalEnv prepends the temp GOBIN to PATH, so TestTerminalCommands passes regardless. D-39 promises the card "works when pasted into a fresh shell", and the check does not prove it. That contradicts D-40's goal.
Fix: D-40 says the copy is not changed without asking, so raise this with the user. Two options:
- Change the check. Set
GOBINbut leave PATH alone, so the check fails today and documents the gap. - Change the copy, with approval. Make it PATH-independent, for example
go install ./cmd/summerfollowed byexport PATH="$(go env GOPATH)/bin:$PATH", or call"$(go env GOPATH)/bin/summer" build.
WR-04: The site test gate and the pnpm test script only work on Node 22.18 to 22.x
File: /media/nvme/dev/golem15/summercms.io/summercms/sm-summercmsio-app/vue-summercmsio-app/package.json:11,14-15; /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/scripts/check-phase11.2.sh:190-198
Issue: The script is node --test tests/*.test.ts and engines says >=22.6.
- Older Node 22. Node 22.6 to 22.17 need
--experimental-strip-typesto run.tsfiles, so the tests fail to load there. - Node 23 and later. The default test reporter became
speceven when output is not a TTY.run_sitethen seesℹ pass Nlines instead of TAP# pass Nlines. Thegrep -E '^# (tests|...)'on line 194 exits 1 underset -e, and the gate aborts with norefuse:message. On Node 24, the current LTS, the gate fails.
Fix: Pin the reporter and the engines range:
"test": "node --test --test-reporter=tap tests/*.test.ts",
"engines": { "node": ">=22.18" }
Also make the summary grep in run_site non-fatal (grep ... || true) so the explicit refuse checks report the failure.
WR-05: The smoke script can test a different server that already holds the port
File: /media/nvme/dev/golem15/summercms.io/summercms/sm-summercmsio-app/scripts/smoke.sh:18,81-95
Issue: The readiness loop curls $BASE/ before it checks kill -0 "$PID".
- If port 18095 is already taken (a leftover smoke or dev server, or anything else),
servefails to bind and exits. - The first
curlstill succeeds against the foreign process, soup=1. - Every later assertion runs against the wrong server. A leftover instance of an older binary passes and prints
smoke: okfor a binary that never started.
Fix: Before the readiness loop, refuse a busy port, for example with if curl -s -o /dev/null "$BASE/"; then fail "port $PORT already in use"; fi. Inside the loop, also require that $PID is still alive after the first successful curl, and check serve.log for the listening on line.
WR-06: A release build does not compile what its plugin tests checked, and does not require clean submodules
File: /media/nvme/dev/golem15/summercms.io/summercms/sm-summercmsio-app/scripts/build.sh:79,91-103
Issue: In release mode, step 3 runs go -C "$PLUG" test under the app's default go.work. That resolves the framework to ../summercms.go (the working tree), so the plugin tests compile against different framework code than the v0.1.0 export the binary is built from (line 103).
The drift check on line 91 also only covers main.go and plugins.gen.go. A release binary can therefore embed uncommitted plugin code and an uncommitted Nuxt site, because vue-summercmsio-app and plugins/golem15/summercms are not checked. That undercuts "Only deploy a release build" (DEPLOY.md:111) and D-42's "the page, the docs and the code agree".
Fix: Write the temp go.work before step 3 and run the plugin tests with GOWORK="$TMP/go.work". In release mode, refuse dirty submodules:
for d in "$ROOT" "$SITE" "$PLUG"; do
[[ -z "$(git -C "$d" status --porcelain --ignore-submodules=none -- . ':!public')" ]] || die "$d has uncommitted changes"
done
Info
IN-01: The extension-less docs 301 drops the query string
File: /media/nvme/dev/golem15/summercms.io/summercms/sm-summercmsio-app/plugins/golem15/summercms/static.go:178-182
Issue: http.Redirect(w, r, t.mount+name+".html", 301) discards r.URL.RawQuery, so /docs/x?utm_source=... loses its parameters. The fragment survives because the browser keeps it.
Fix: Append "?" + r.URL.RawQuery when it is non-empty. That is safe because the path part is still constrained to existing files.
IN-02: site_label in site.yaml cannot be paired with --site-url alone
File: /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/internal/docsite/load.go:128-131,278,300
Issue: ParseSite rejects a site_label without a site_url before the command-line overrides are applied. So site.yaml with only site_label: Acme, plus --site-url /, fails with "site_label needs site_url", although the merged result is valid. load() already performs the merged check on line 300.
Fix: Drop the pairing check from ParseSite (keep the oneLine validation) and rely on the merged check in load(). Alternatively, document that the label needs the URL in the same layer.
IN-03: The og:image alt text is not reactive, unlike the other SEO fields
File: /media/nvme/dev/golem15/summercms.io/summercms/sm-summercmsio-app/vue-summercmsio-app/app/pages/index.vue:17
Issue: alt: t('meta.ogImageAlt') is evaluated once. Every other field uses a getter (() => t(...)), and twitterImageAlt does too. When 11.3 adds pl, a client-side locale switch would leave og:image:alt in English, which goes against D-33's "without touching component markup".
Fix: Make ogImage a computed value: ogImage: () => ({ url: '/og-image.png', ..., alt: t('meta.ogImageAlt') }).
IN-04: Copy-button failures are silent and success is not announced
File: /media/nvme/dev/golem15/summercms.io/summercms/sm-summercmsio-app/vue-summercmsio-app/app/components/TerminalCard.vue:34-53,62
Issue: When both clipboard paths fail, copy() returns with no feedback. The "Copied" label change sits in a plain <button> with no live region, so screen readers do not announce it.
Fix: Add aria-live="polite" to a status element, and show a short "Copy failed" state on !ok.
IN-05: The gate's built stage can check one framework checkout and build from another
File: /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/scripts/check-phase11.2.sh:211-219
Issue: run_built picks release or dev mode by looking for v0.1.0 in ${SUMMERCMS_FRAMEWORK:-$ROOT}. build.sh defaults to $APP/../summercms.go. If PHASE11_2_ROOT or PHASE11_2_APP is set without SUMMERCMS_FRAMEWORK, the two can disagree, and the tag check passes on one tree while the release build fails on the other.
Fix: Export SUMMERCMS_FRAMEWORK="$fw" before calling build.sh.
IN-06: Pipelines in an assignment exit under set -e without a diagnostic
File: /media/nvme/dev/golem15/summercms.io/summercms/sm-summercmsio-app/scripts/smoke.sh:162-163,168; /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/scripts/check-phase11.2.sh:194
Issue: Under pipefail, JS="$(curl ... | grep -o ... | head -n1)" exits the script when grep finds nothing (or on a SIGPIPE race with head). The next line, [[ -n "$JS" ]] || fail "index.html references no /_nuxt/*.js", is never reached. The same applies to the cd .../_fonts && ls substitution and the summary grep in run_site. The checks still fail closed, but without a useful message.
Fix: Add || true inside the substitution and rely on the explicit emptiness checks that follow.
IN-07: The external link check follows redirects, so a login redirect would count as a pass
File: /media/nvme/dev/golem15/summercms.io/summercms/sm-summercmsio-app/plugins/golem15/summercms/links_test.go:297-312
Issue: http.Client follows redirects by default. If git.golem15.com ever gated anonymous access with a redirect to /user/login, the check would see the login page's 200 and pass. That is the exact condition D-38 asks this check to catch. Today the Source URL answers 200 directly.
Fix: Set CheckRedirect to return http.ErrUseLastResponse. Alternatively, assert that resp.Request.URL.String() == link after following.
IN-08: The scroll-spy section list is defined twice
File: /media/nvme/dev/golem15/summercms.io/summercms/sm-summercmsio-app/vue-summercmsio-app/app/components/SiteHeader.vue:7-12,19; app/utils/scrollSpy.ts:5
Issue: navItems and SPY_SECTIONS list the same four ids separately. If a section is added to one list but not the other, the header link exists but is never highlighted, or the reverse.
Fix: Derive navItems from SPY_SECTIONS, for example SPY_SECTIONS.map((id) => ({ id, key: \header.nav.${id}` }))`.
Reviewed: 2026-10-01T14:47:34Z Reviewer: Claude (gsd-code-reviewer) Depth: standard