Skip to content

Commit 94ce8b2

Browse files
authored
ci(js): lint the backend JavaScript, which nothing did (#855)
Closes #825. A file whose methods had landed outside their class passed the whole CI matrix — **71 green checks** — and the rating buttons it shipped would not have worked. PHPStan does not read JavaScript, the functional suite renders the template without executing the module, and no Playwright spec covers that view. A person reading the diff found it. ## `node --check` lost on evidence, not on taste It was the cheap candidate: no dependency, no config. Tested against the actual broken file, it **accepted** it — because it parses a `.js` file as a CommonJS *script*. ESLint parses it as an ES *module*, which is how the browser loads it, and reports the parse error on the right line. That also settles something left open in #842's review. I had written there that I could not reconcile `node --check` passing with a minimal reproduction failing, and would not dress it up. The answer is the parsing mode: the file was a syntax error all along, and `node --check` read it in the wrong one. ## My reason for not deciding this earlier was wrong I had said eslint "would bring a JS toolchain into a PHP extension that has none". One command disproves it: `package.json`, `package-lock.json`, three devDependencies including Playwright, and CI already runs `npm install` for the e2e job. ESLint is one more line in a file that exists. **No sibling extension lints JavaScript** — `rte_ckeditor_image` has a `package.json` for commitlint and husky only, `nr-vault` one for Playwright, `nr-repurpose` none at all. There was no house standard to copy, which is a reason to set one rather than to wait for it. ## The rule set is small on purpose `no-undef` is an **error**: it is the rule that catches the defect above. `no-unsanitized` is loaded but set to **warn**, and the reason is measured rather than cautious. As an error it reports 12 assignments, and **none of them is a hole** — the code escapes through `escapeHtml()`, visibly, with a comment saying so: ```js // SECURITY: Escape all external data to prevent XSS const safeProviderName = escapeHtml(providerInfo.suggestedName); ``` The rule recognises a fixed set of sanitizers and a helper of ours is not among them. Dropping the plugin was not an option either: two files already carry `eslint-disable-line no-unsanitized/property` — suppressions written for a rule that never ran, which become "rule not found" errors the moment linting is switched on. Those twelve are triaged in **#854** with the evidence and three options. A security question belongs where it can be argued and closed, not in a config comment. ## Verification | | | |---|---| | clean tree | 0 errors, 14 warnings, exit 0 | | #825's shape reintroduced (method after the class's closing brace) | parse error, exit 1 | | `npm ci` from the lockfile, then lint | exit 0 | Repo rules: no job is defined here — thin caller of the shared `script-check` reusable, `check-workflow-ownership.php` passes, and `drift_compare.py` against the org template reports no drift. `pre-command` installs the devDependencies the runner does not ship. ## One thing worth knowing beyond this repo This makes nr_llm the first Netresearch TYPO3 extension with a JavaScript gate. If it holds up, it belongs in the shared template rather than being rebuilt per repository — otherwise the next extension gets the same 71-green-checks surprise. _Assisted by claude-code:claude-opus-5 — [Session](https://claude.ai/code/session_01MNg1MysJVugv1xo2husknU)_
2 parents 45c1b00 + 633798d commit 94ce8b2

4 files changed

Lines changed: 1212 additions & 3 deletions

File tree

.github/workflows/js-lint.yml

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
name: JavaScript Lint
2+
3+
# Lints Resources/Public/JavaScript. Nothing did before #825: a file whose
4+
# methods had landed outside their class passed the whole CI matrix — 71 green
5+
# checks — because PHPStan does not read JavaScript, the functional suite
6+
# renders the template without executing the module, and no Playwright spec
7+
# covers that view. The defect was found by a person reading the diff.
8+
#
9+
# `node --check` was the cheaper candidate and lost on evidence: it parses a
10+
# `.js` file as a CommonJS script and accepted the broken file, while ESLint
11+
# parses it as an ES module — which is how the browser loads it — and reports
12+
# the parse error on the right line.
13+
#
14+
# Thin caller of the shared script-check reusable, so the checkout pin and
15+
# harden-runner stay maintained centrally. `pre-command` installs the npm
16+
# devDependencies the runner does not ship.
17+
18+
on:
19+
push:
20+
branches: [main]
21+
pull_request:
22+
merge_group:
23+
workflow_dispatch:
24+
25+
permissions: {}
26+
27+
jobs:
28+
js-lint:
29+
uses: netresearch/.github/.github/workflows/script-check.yml@main
30+
permissions:
31+
contents: read
32+
with:
33+
pre-command: npm ci --no-audit --no-fund
34+
check-cmd: npx eslint

eslint.config.mjs

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
// Lints the backend JavaScript. Nothing did before: a file whose methods had
2+
// landed outside their class passed the whole CI matrix — 71 green checks —
3+
// because PHPStan does not read JavaScript, the functional suite renders the
4+
// template without executing the module, and no Playwright spec covers that
5+
// view (#825).
6+
//
7+
// `node --check` was considered as a cheaper floor and rejected on evidence:
8+
// it parses a `.js` file as a CommonJS script and accepted the broken file,
9+
// while ESLint parses it as an ES module — which is how the browser loads it —
10+
// and reports the parse error on the right line.
11+
//
12+
// The rule set is deliberately small. This is the first JavaScript gate in any
13+
// of our TYPO3 extensions, and a large one would arrive as a backlog of
14+
// pre-existing violations that nobody asked for.
15+
import globals from 'globals';
16+
import noUnsanitized from 'eslint-plugin-no-unsanitized';
17+
18+
export default [
19+
{
20+
files: ['Resources/Public/JavaScript/**/*.js'],
21+
// Vendored libraries are shipped as-is; linting them would report
22+
// somebody else's code in our gate.
23+
ignores: ['Resources/Public/JavaScript/Vendor/**'],
24+
languageOptions: {
25+
ecmaVersion: 2022,
26+
sourceType: 'module',
27+
globals: {
28+
...globals.browser,
29+
// Provided by the TYPO3 backend at runtime, not importable.
30+
TYPO3: 'readonly',
31+
bootstrap: 'readonly',
32+
},
33+
},
34+
plugins: { 'no-unsanitized': noUnsanitized },
35+
rules: {
36+
// The rule that would have caught #825.
37+
'no-undef': 'error',
38+
'no-unused-vars': ['warn', { argsIgnorePattern: '^_' }],
39+
// Warning, not error, and the reason is measured rather than
40+
// cautious. Switched on as an error it reports 12 assignments —
41+
// and none of them is a hole: the code escapes first, visibly, at
42+
// `SetupWizard.js:471-474` ("SECURITY: Escape all external data")
43+
// through `escapeHtml()`. The rule cannot recognise that helper as
44+
// a sanitizer, which is a known shape for it.
45+
//
46+
// The plugin is still loaded, because two files carry
47+
// `eslint-disable-line no-unsanitized/property` — suppressions
48+
// written for a rule that never ran. Without the plugin those
49+
// comments become "rule not found" errors the moment linting is
50+
// switched on.
51+
//
52+
// Making them errors would land a backlog of twelve judgements
53+
// nobody asked for on a change whose job is #825. Leaving the rule
54+
// out entirely would drop a real check silently. The findings are
55+
// triaged in their own issue, with the escapeHtml evidence, so the
56+
// security question lives somewhere it can be argued rather than
57+
// in a config comment.
58+
'no-unsanitized/property': 'warn',
59+
'no-unsanitized/method': 'warn',
60+
},
61+
},
62+
];

0 commit comments

Comments
 (0)