checkPlugin: flag absolute /static/plugins/ asset paths in templates (#5203)

[JohnMcLear]

Closes #5203.

Plugin templates that reference assets with absolute paths like
`/static/plugins/ep_foo/x.css` silently break any Etherpad instance
hosted behind a reverse proxy at a sub-path (e.g. https://example.com/etherpad/pad\) — the browser resolves the path against the domain root instead of the proxy prefix and the asset 404s. The correct form is `../static/plugins/...` (relative). [ep_embedmedia#4](https://github.com/ether/ep_embedmedia/pull/4/files) fixed one plugin manually; #5203 asked for a mechanical check.

Summary

• Walk each plugin's `templates/` and `static/` for `.ejs` / `.html` files.
• Scan for `/static/plugins/` occurrences that are not preceded by a URL scheme (`:`), a dot, a slash, or a word char — so \\"https://host/static/plugins/...\\" and already-correct `../static/plugins/...` don't match.
• Emit a `WARN` line describing the bad path and how to fix it.
• In `autofix` mode, rewrite them in place to `../static/plugins/`.

Test plan

• Regex verified against six cases (absolute bad, absolute in URL, already-relative, embedded in src/href, /static/ outside /plugins/, bare path).
• `pnpm run ts-check` clean.

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Add linting for absolute plugin asset paths in templates

✨ Enhancement

Walkthroughs

Description

• Add linting check for absolute /static/plugins/ paths in plugin templates
• Scan templates/ and static/ directories for .ejs and .html files
• Warn when absolute paths found; suggest relative ../static/plugins/ form
• Support autofix mode to rewrite paths in place

Diagram

flowchart LR
  A["Plugin templates/static dirs"] -- "scan .ejs/.html files" --> B["Regex check for /static/plugins/"]
  B -- "match found" --> C["Warn with fix suggestion"]
  C -- "autofix enabled" --> D["Rewrite to ../static/plugins/"]
  B -- "no match" --> E["Continue"]

Loading

File Changes

1. bin/plugins/checkPlugin.ts ✨ Enhancement +39/-0

Add absolute plugin asset path linting

• Added check for absolute /static/plugins/ asset paths in plugin template files
• Implements recursive directory walk to find .ejs and .html files in templates/ and static/
 directories
• Uses negative lookbehind regex to avoid matching URLs with schemes, relative paths, or word
 characters
• Logs warnings for problematic paths and provides autofix capability to rewrite them as relative
 paths

bin/plugins/checkPlugin.ts

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1) 📎 Requirement gaps (0)

1. Autofix breaks static HTML ☑ 🐞 Bug ≡ Correctness

Description

In autofix mode, the new scan rewrites "/static/plugins/..." to "../static/plugins/..." in HTML
files under a plugin’s static/ directory, but those files are served from
/static/plugins//static/... so the rewritten relative URL resolves to a non-existent path and will
404. The warning text also gives the same incorrect remediation guidance for static/*.html as for
templates.

Code

bin/plugins/checkPlugin.ts[R383-412]

+  const templateDirs = ['templates', 'static']; // scan both — static/*.html exists too
+  const STATIC_ABS = /(?<![./:\w])\/static\/plugins\//g;
+  for (const dir of templateDirs) {
+    const abs = `${pluginPath}/${dir}`;
+    if (!files.includes(dir)) continue;
+    const scanFiles: string[] = [];
+    const walk = async (d: string) => {
+      for (const entry of await fsp.readdir(d, {withFileTypes: true})) {
+        const full = `${d}/${entry.name}`;
+        if (entry.isDirectory()) {
+          if (entry.name === 'node_modules' || entry.name === '.git') continue;
+          await walk(full);
+        } else if (/\.(ejs|html)$/.test(entry.name)) {
+          scanFiles.push(full);
+        }
+      }
+    };
+    await walk(abs);
+    for (const fp of scanFiles) {
+      const src = await fsp.readFile(fp, 'utf8');
+      if (!STATIC_ABS.test(src)) continue;
+      STATIC_ABS.lastIndex = 0;
+      const rel = path.relative(pluginPath, fp);
+      logger.warn(`${rel} contains absolute '/static/plugins/...' asset paths; ` +
+                  'these break reverse-proxied Etherpad deployments. Use ' +
+                  "'../static/plugins/...' instead.");
+      if (autoFix) {
+        logger.info(`Autofixing absolute /static/plugins/ paths in ${rel}`);
+        await fsp.writeFile(fp, src.replace(STATIC_ABS, '../static/plugins/'));
+      }

Evidence

The new logic explicitly scans both templates and static and applies the same replacement
(../static/plugins/) to any match. Etherpad serves plugin static files via the /static/* handler
by rewriting plugins//static/... to the plugin’s on-disk static/... path, which means an HTML
file inside a plugin’s static/ directory is loaded from /static/plugins//static/...; from that
base URL, ../static/plugins/... points under /static/plugins//static/ (wrong). In contrast, core
client code uses ../static/plugins/... when referencing plugin assets from pad pages, which is the
context where this rewrite is correct.

bin/plugins/checkPlugin.ts[377-412]
src/node/utils/Minify.ts[168-195]
src/static/js/ace.ts[182-188]
doc/plugins.md[20-54]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`checkPlugin` currently applies the same autofix (`/static/plugins/...` -> `../static/plugins/...`) to both `templates/` and `static/` HTML/EJS files. This is correct for templates rendered under pad pages, but **incorrect for HTML served from a plugin’s `static/` directory**, because those pages are loaded from `/static/plugins/<plugin>/static/...` and need different relative paths.
### Issue Context
Etherpad serves plugin static files through the `/static/*` handler (see Minify path rewriting). A one-size-fits-all rewrite to `../static/plugins/` will generate broken URLs for static HTML.
### Fix Focus Areas
- bin/plugins/checkPlugin.ts[377-413]
### Suggested change
- Keep scanning `static/**/*.html` for *warnings*, but either:
- **Do not autofix** files under `static/`, and update the warning message to say “use a relative path appropriate for the file’s location (no leading '/')”, OR
- Implement a separate autofix for `static/**/*.html` that computes the correct relative prefix to `/static/plugins/` based on the file’s depth under the plugin’s `static/` directory.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. No test for STATIC_ABS 📘 Rule violation ☼ Reliability

Description

This PR adds new linting/autofix behavior for absolute /static/plugins/... paths but does not add
a regression test to ensure the warning and rewrite logic won’t regress. Without automated coverage,
future changes could silently reintroduce reverse-proxy-breaking paths or break the autofix.

Code

bin/plugins/checkPlugin.ts[R377-413]

+  // Check template files for absolute `/static/plugins/...` asset paths.
+  // Those break any Etherpad instance hosted behind a reverse proxy at a
+  // sub-path (e.g. https://example.com/etherpad/pad) because the browser
+  // resolves them against the domain root instead of the proxy prefix.
+  // Rewrite to the relative `../static/plugins/...` form (see #5203, and
+  // ep_embedmedia#4 for the original fix this check is modelled on).
+  const templateDirs = ['templates', 'static']; // scan both — static/*.html exists too
+  const STATIC_ABS = /(?<![./:\w])\/static\/plugins\//g;
+  for (const dir of templateDirs) {
+    const abs = `${pluginPath}/${dir}`;
+    if (!files.includes(dir)) continue;
+    const scanFiles: string[] = [];
+    const walk = async (d: string) => {
+      for (const entry of await fsp.readdir(d, {withFileTypes: true})) {
+        const full = `${d}/${entry.name}`;
+        if (entry.isDirectory()) {
+          if (entry.name === 'node_modules' || entry.name === '.git') continue;
+          await walk(full);
+        } else if (/\.(ejs|html)$/.test(entry.name)) {
+          scanFiles.push(full);
+        }
+      }
+    };
+    await walk(abs);
+    for (const fp of scanFiles) {
+      const src = await fsp.readFile(fp, 'utf8');
+      if (!STATIC_ABS.test(src)) continue;
+      STATIC_ABS.lastIndex = 0;
+      const rel = path.relative(pluginPath, fp);
+      logger.warn(`${rel} contains absolute '/static/plugins/...' asset paths; ` +
+                  'these break reverse-proxied Etherpad deployments. Use ' +
+                  "'../static/plugins/...' instead.");
+      if (autoFix) {
+        logger.info(`Autofixing absolute /static/plugins/ paths in ${rel}`);
+        await fsp.writeFile(fp, src.replace(STATIC_ABS, '../static/plugins/'));
+      }
+    }

Evidence

PR Compliance ID 3 requires a regression test when introducing a bug fix. The diff adds new
detection and autofix logic for absolute /static/plugins/... references, but no corresponding test
change is included alongside this new behavior.

bin/plugins/checkPlugin.ts[377-413]
Best Practice: Repository guidelines

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New `/static/plugins/` detection + autofix logic was added to `checkPlugin.ts`, but there is no automated regression test that would fail if the regex or rewrite behavior regresses.
## Issue Context
This check is intended to prevent reverse-proxy breakage caused by absolute asset paths in plugin templates; it should be protected by a test that verifies both detection (warn) and autofix rewrite behavior on representative sample inputs.
## Fix Focus Areas
- bin/plugins/checkPlugin.ts[377-413]
- src/tests/backend/specs/checkPlugin-static-paths.ts[1-200]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Directory scan may abort 🐞 Bug ☼ Reliability

Description

The scan only checks files.includes(dir) (names from readdir(pluginPath)) and then calls
fsp.readdir() on ${pluginPath}/${dir} without verifying it is a readable directory, so a
non-directory entry (or unreadable directory) will throw and abort the whole checkPlugin run. This
reduces the tool’s robustness and can prevent subsequent checks from running.

Code

bin/plugins/checkPlugin.ts[R386-401]

+    const abs = `${pluginPath}/${dir}`;
+    if (!files.includes(dir)) continue;
+    const scanFiles: string[] = [];
+    const walk = async (d: string) => {
+      for (const entry of await fsp.readdir(d, {withFileTypes: true})) {
+        const full = `${d}/${entry.name}`;
+        if (entry.isDirectory()) {
+          if (entry.name === 'node_modules' || entry.name === '.git') continue;
+          await walk(full);
+        } else if (/\.(ejs|html)$/.test(entry.name)) {
+          scanFiles.push(full);
+        }
+      }
+    };
+    await walk(abs);
+    for (const fp of scanFiles) {

Evidence

files is populated via fsp.readdir(pluginPath) which returns only entry names, not types. The
new walk() unconditionally calls fsp.readdir(d, {withFileTypes: true}) with no stat() guard or
error handling, so ENOTDIR/EACCES will terminate the process.

bin/plugins/checkPlugin.ts[173-174]
bin/plugins/checkPlugin.ts[385-401]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new template/static scanning logic assumes `${pluginPath}/${dir}` is a readable directory and will throw if it is not, aborting the whole `checkPlugin` run.
### Issue Context
`files` is derived from `readdir(pluginPath)` and does not provide file type information.
### Fix Focus Areas
- bin/plugins/checkPlugin.ts[173-174]
- bin/plugins/checkPlugin.ts[385-401]
### Suggested change
- Before `await walk(abs)`, `stat()`/`lstat()` `abs` and `continue` (with a warn) if it is not a directory.
- Wrap the initial `walk(abs)` (or the `readdir` inside `walk`) in a try/catch that logs the error and continues with the next directory/file instead of aborting the whole script.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more

[SamTV12345]

Nice addition