From a4cf6ed9a28027f97758f1c0891a5f0c8b2f2f1d Mon Sep 17 00:00:00 2001 From: aaldersondev Date: Sat, 8 Aug 2026 04:37:53 +0200 Subject: [PATCH] fix(panel): a rule that reads as the strictest line in a template now enforces something MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `parseRules` split the rule string on `|` before it looked for `regex:`, so `required|regex:/^(paper|purpur)$/` was torn into `regex:/^(paper` and `purpur)$/`. The first would not compile, and the catch around it returned **true** — accepting every value. The second became an unrecognised rule name and was dropped. The strictest-looking line in a template therefore enforced nothing, and said nothing about it. Pterodactyl eggs write alternations constantly, so every imported template carrying one has been running unvalidated. The file's own comment said `regex:` "keeps its argument whole: it contains commas, pipes and colons that must not be split" — true of commas and colons, false of the pipe, which is the character an alternation is made of. Both halves are fixed, because either alone leaves a hole. The rule string is now scanned rather than split: a `regex:/` is consumed through its matching closing delimiter and its flags, honouring backslash escapes and character classes, where a `/` or a `|` inside `[...]` is an ordinary character. And an expression that will not compile refuses the value instead of accepting it, blaming the template rather than the value, with an error log naming the variable and the rule. The refusal fires even when there is no value to refuse. A variable that is normally left empty would otherwise keep its broken rule secret until the day somebody finally filled it in. Two consequences worth stating rather than discovering: A delimited regex containing a pipe now *works*, which is a tightening — anyone who imported an egg with an alternation has had that variable unvalidated, and a value saved earlier may be refused on the next save. That is the intended direction and the one visible break. An uppercase flag is no longer swallowed as a flag. Taking it would keep the rule whole and have the check read `/^(a|b)$/I` as a bare pattern — which compiles, matches almost nothing, and refuses every value while blaming the value. Stopping at the delimiter lets the fragment fail to compile, which is the loud answer. One case is knowingly left: an unterminated `regex:/` that borrows a `/` from a later rule, where the region between them happens to compile, is indistinguishable from a correctly delimited expression. Catching it would mean refusing any expression spanning a `|` followed by something that looks like a rule name, which breaks the legitimate `regex:/^(a|max:5)$/` to rescue a template that is already malformed. The bundled catalogue is unaffected: every regex it ships is a pipe-free character class, written that way to work around this very bug. Factorio's comment explaining the workaround is corrected, so nobody keeps coding around a bug that is gone. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QL3QL3ReEa9Sk68mxJW6Fu --- .../src/modules/startup/startup.service.ts | 5 +- .../modules/startup/variable-rules.spec.ts | 451 +++++++++++++++++- .../src/modules/startup/variable-rules.ts | 302 +++++++++++- docs/templates.md | 18 + packages/templates/src/catalog.spec.ts | 52 +- packages/templates/src/catalog/factorio.ts | 17 +- 6 files changed, 785 insertions(+), 60 deletions(-) diff --git a/apps/panel/src/modules/startup/startup.service.ts b/apps/panel/src/modules/startup/startup.service.ts index a5d9381..8914458 100644 --- a/apps/panel/src/modules/startup/startup.service.ts +++ b/apps/panel/src/modules/startup/startup.service.ts @@ -137,7 +137,10 @@ export class StartupService { continue; } - const violations = validateValue(value, variable.rules); + // The environment name and not the display one: a malformed rule is + // logged under it, and that is the string an administrator will grep the + // template for. + const violations = validateValue(value, variable.rules, variable.envVariable); if (violations.length > 0) { errors.push(`${variable.name}: ${violations.map((one) => one.message).join(' ')}`); diff --git a/apps/panel/src/modules/startup/variable-rules.spec.ts b/apps/panel/src/modules/startup/variable-rules.spec.ts index 05db6ef..382f6b9 100644 --- a/apps/panel/src/modules/startup/variable-rules.spec.ts +++ b/apps/panel/src/modules/startup/variable-rules.spec.ts @@ -1,6 +1,30 @@ -import { describe, expect, it } from 'vitest'; +import { Logger } from '@nestjs/common'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { MAX_VARIABLE_LENGTH, parseRules, validateValue } from './variable-rules.js'; +/** + * A malformed rule is reported at error level, which would otherwise paint this + * suite red for the cases that are deliberately provoking one. Silenced here + * and asserted on where it is the point of the test. + * + * Through a factory so the spy keeps its own type: annotating the variable with + * `ReturnType` widens it to `any` and the assertions below stop + * being checked at all. + */ +function silenceLogger() { + return vi.spyOn(Logger.prototype, 'error').mockImplementation(() => undefined); +} + +let logged: ReturnType; + +beforeEach(() => { + logged = silenceLogger(); +}); + +afterEach(() => { + logged.mockRestore(); +}); + /** True if the value passes every rule. */ function accepts(value: string, rules: string): boolean { return validateValue(value, rules).length === 0; @@ -36,6 +60,214 @@ describe('parseRules', () => { { name: 'string', args: [] }, ]); }); + + /** + * The pipe, which is what an alternation is made of. + * + * Splitting the rule string before looking for `regex:` tore this into + * `regex:/^(paper` and `purpur|spigot)$/`: the first no longer compiled, the + * rest was read as rules nobody recognises and dropped. Eggs write + * alternations constantly, so this shape is not an edge case — it is the + * common one. + */ + describe('a delimited regular expression', () => { + it('survives an alternation', () => { + expect(parseRules('required|string|regex:/^(paper|purpur|spigot)$/')).toEqual([ + { name: 'required', args: [] }, + { name: 'string', args: [] }, + { name: 'regex', args: ['/^(paper|purpur|spigot)$/'] }, + ]); + }); + + // The rules written after the expression have to come back too: an egg + // does not always put its regex last, and swallowing the remainder would + // drop them as silently as the split dropped the fragments. + it('does not swallow the rules that follow it', () => { + expect(parseRules('required|regex:/^(a|b)$/|max:10')).toEqual([ + { name: 'required', args: [] }, + { name: 'regex', args: ['/^(a|b)$/'] }, + { name: 'max', args: ['10'] }, + ]); + }); + + /** + * The flags belong to the rule, so the rule cannot end before them. + * + * The alternation earns its place in the input: with no pipe inside the + * expression, a scan that ended at the delimiter and left `i` behind would + * fall back to cutting on the pipe and land on the very same boundary, and + * the test would pass without the flags having been read at all. + */ + it('ends after its flags', () => { + expect(parseRules('regex:/^(a|b)$/i|max:5')).toEqual([ + { name: 'regex', args: ['/^(a|b)$/i'] }, + { name: 'max', args: ['5'] }, + ]); + }); + + /** + * A capital is not a flag JavaScript knows, so the scan stops at the + * delimiter and the rule falls to the pipe. + * + * The forgiving reading is the worse one, which is why this is pinned. + * Swallowing the `I` would keep the rule whole, and the check would then + * read `/^(a|b)$/I` as a *bare* pattern — which compiles, matches almost + * nothing, and refuses every value while blaming the value. Torn instead, + * the fragment does not compile and the operator is told their template is + * at fault. + */ + it('does not mistake a capital for a flag', () => { + expect(parseRules('regex:/^(a|b)$/I|max:5')).toEqual([ + { name: 'regex', args: ['/^(a'] }, + // Lower-cased like every rule name, which is why it reads `/i` here. + { name: 'b)$/i', args: [] }, + { name: 'max', args: ['5'] }, + ]); + }); + + // Inside a class a slash is a character, not the closing delimiter — as it + // is in JavaScript's own regex literals — and a pipe is not an alternation. + it('reads a character class holding a slash and a pipe', () => { + expect(parseRules('regex:/^[a-z/|]+$/|max:8')).toEqual([ + { name: 'regex', args: ['/^[a-z/|]+$/'] }, + { name: 'max', args: ['8'] }, + ]); + }); + + /** + * A `\/` is a slash the pattern matches, not the closing delimiter. + * + * The alternation behind it is again what gives the test teeth: an + * expression holding no pipe is cut in the same place whether or not the + * escape is understood, so `regex:/^a\/b$/|string` would pass either way. + */ + it('reads an escaped delimiter', () => { + expect(parseRules(String.raw`regex:/^a\/|b$/|string`)).toEqual([ + { name: 'regex', args: [String.raw`/^a\/|b$/`] }, + { name: 'string', args: [] }, + ]); + }); + + // An escape swallows exactly one character and no more. Here the escaped + // slash stands against the real delimiter — the shape any expression + // matching a URL path ends in — and a scan skipping two would eat the + // delimiter as well, leaving an expression that never closes. + it('reads an escaped delimiter standing against the closing one', () => { + expect(parseRules(String.raw`regex:/^(a|b)\//|string`)).toEqual([ + { name: 'regex', args: [String.raw`/^(a|b)\//`] }, + { name: 'string', args: [] }, + ]); + }); + + // Spacing is the template writer's business, not the scanner's: + // `required| regex:…` is the same template as `required|regex:…`, and an + // opening the scan walks past is an alternation torn in half. + it('recognises an opening standing behind a space', () => { + expect(parseRules('required| regex:/^(a|b)$/|max:5')).toEqual([ + { name: 'required', args: [] }, + { name: 'regex', args: ['/^(a|b)$/'] }, + { name: 'max', args: ['5'] }, + ]); + }); + + // And on the other side, where a blank must not be mistaken for the + // leftover that means the scan has misread the rule. + it('allows a space between the expression and the pipe', () => { + expect(parseRules('required|regex:/^(a|b)$/ |max:5')).toEqual([ + { name: 'required', args: [] }, + { name: 'regex', args: ['/^(a|b)$/'] }, + { name: 'max', args: ['5'] }, + ]); + }); + + // Rule names are matched case-insensitively everywhere else, and the scan + // has to agree or a shouted rule stops being a regex halfway through the + // parse. The expression itself keeps its case: folding that would change + // what it matches. + it('recognises an opening in capitals', () => { + expect(parseRules('REGEX:/^(A|B)$/|max:5')).toEqual([ + { name: 'regex', args: ['/^(A|B)$/'] }, + { name: 'max', args: ['5'] }, + ]); + }); + }); + + /** + * A `regex:/` whose expression never closes, and the delimiter the scan + * borrows for it. + * + * Nothing in the rule string announces that the expression was left open, so + * the scan runs on to the first `/` it can find — which may belong to a rule + * further along, and swallow every rule in between. Two things catch that: + * anything but a blank standing between the supposed end and the next pipe, + * and an enclosed expression that will not compile. Both give up on the scan + * and cut on the pipe instead, which is what an undelimited `regex:` gets and + * is never wider than what the old split accepted. + */ + describe('a delimited regular expression that never closes', () => { + it('does not close on a delimiter with a leftover behind it', () => { + expect(parseRules('regex:/^(a|b)$|in:x/y,z')).toEqual([ + { name: 'regex', args: ['/^(a'] }, + { name: 'b)$', args: [] }, + { name: 'in', args: ['x/y', 'z'] }, + ]); + }); + + it('does not close on a delimiter that leaves an expression it cannot compile', () => { + expect(parseRules('regex:/^(a|b$|in:x/|max:5')).toEqual([ + { name: 'regex', args: ['/^(a'] }, + { name: 'b$', args: [] }, + { name: 'in', args: ['x/'] }, + { name: 'max', args: ['5'] }, + ]); + }); + + // Flags are part of what has to compile, because they are part of what + // `checkRegex` will be handed: the scan judges the very string the check + // will judge, so the two cannot disagree about what is applicable. + it('does not close on a delimiter whose flags will not compile', () => { + expect(parseRules('regex:/^(a|b)$/z|max:5')).toEqual([ + { name: 'regex', args: ['/^(a'] }, + { name: 'b)$/z', args: [] }, + { name: 'max', args: ['5'] }, + ]); + }); + + // And where there is no delimiter to borrow either — here the only `/` + // left sits inside a character class — the scan simply finds no end, and + // the pipe has to be what closes the rule. + it('gives up when there is no closing delimiter to be found', () => { + expect(parseRules('regex:/^[a/]$|max:5')).toEqual([ + { name: 'regex', args: ['/^[a/]$'] }, + { name: 'max', args: ['5'] }, + ]); + }); + }); + + /** + * A `regex:` with no delimiters is cut on the pipe like anything else. + * + * Pterodactyl always delimits, so a bare one is hand-written and genuinely + * ambiguous: nothing in `regex:^(a|b)$|max:5` tells the alternation's pipe + * from the one introducing `max`. Cutting it keeps every rule that was + * written, and errs the safe way round: what survives is the expression up to + * its first pipe, either one branch of the alternation — a subset of what the + * whole would have matched — or a fragment the cut left unbalanced, which + * does not compile and refuses the value loudly. Neither is wider than + * intended. + */ + describe('an undelimited regular expression', () => { + it('still splits on the pipe', () => { + expect(parseRules('regex:^[a-z]+$|max:5')).toEqual([ + { name: 'regex', args: ['^[a-z]+$'] }, + { name: 'max', args: ['5'] }, + ]); + }); + + it('keeps the rules written after it, at the cost of the alternation', () => { + expect(parseRules('regex:^(a|b)$|max:5')).toContainEqual({ name: 'max', args: ['5'] }); + }); + }); }); describe('validateValue', () => { @@ -128,11 +360,156 @@ describe('validateValue', () => { expect(accepts('PAPER', 'regex:/^paper$/i')).toBe(true); }); - // An unreadable expression is the template's mistake, not the user's: - // blocking them on an error they cannot fix would make their server - // unconfigurable. - it('lets it through when the expression is invalid', () => { - expect(accepts('peu importe', 'regex:/[/')).toBe(true); + /** + * The shape an imported egg actually contains. + * + * This is the check the bug hid: split first and the expression became + * `/^(paper`, which fails to compile, and a failure to compile used to be + * read as the template's problem and accept every value. A rule that reads + * as the strictest line in the template enforced nothing at all. + */ + describe('an alternation, as the eggs write it', () => { + const RULES = 'required|string|regex:/^(paper|purpur|spigot)$/'; + + it.each(['paper', 'purpur', 'spigot'])('accepts "%s"', (value) => { + expect(accepts(value, RULES)).toBe(true); + }); + + it.each(['vanilla', 'paper2', '(paper', 'purpur)', ''])('refuses "%s"', (value) => { + expect(accepts(value, RULES)).toBe(false); + }); + }); + + // The regex in the middle of the list, not at its end: what follows has to + // be applied, or the scan has merely moved the silence one rule along. + it('applies the rules written after the expression', () => { + expect(accepts('paper', 'regex:/^(paper|purpur)$/|max:5')).toBe(true); + expect(accepts('purpur', 'regex:/^(paper|purpur)$/|max:5')).toBe(false); + }); + + it('applies a class containing a slash', () => { + expect(accepts('a/b', 'regex:/^[a-z/]+$/')).toBe(true); + expect(accepts('A/B', 'regex:/^[a-z/]+$/')).toBe(false); + }); + + it('applies a class containing a pipe', () => { + expect(accepts('a|b', 'regex:/^[a|b]+$/')).toBe(true); + expect(accepts('c', 'regex:/^[a|b]+$/')).toBe(false); + }); + + it('applies an escaped delimiter', () => { + expect(accepts('a/b', String.raw`regex:/^a\/b$/`)).toBe(true); + expect(accepts('ab', String.raw`regex:/^a\/b$/`)).toBe(false); + }); + + it('honours the flags with a rule behind them', () => { + expect(accepts('PAPER', 'regex:/^paper$/i|max:20')).toBe(true); + expect(accepts('SPIGOT', 'regex:/^paper$/i|max:20')).toBe(false); + }); + + /** + * An expression that cannot be compiled refuses the value. + * + * It used to accept it, on the grounds that the template is at fault and + * the user cannot fix it — and that is what turned the splitting bug above + * into silence, because the split manufactured uncompilable expressions and + * this branch swallowed every one. A rule that fails open is not a rule. + * The cost is admitted: the variable cannot be set until an administrator + * edits the template. It is paid once, visibly, by the first person to + * touch the variable. + */ + describe('an expression that will not compile', () => { + it('refuses the value', () => { + expect(accepts('whatever', 'regex:/[/')).toBe(false); + expect(accepts('whatever', 'required|regex:/^(unclosed$/')).toBe(false); + }); + + // Blaming the value would send somebody looking for what is wrong with + // `server.jar`, which is nothing. + it('blames the template rather than the value', () => { + const violation = validateValue('whatever', 'regex:/[/')[0]; + + expect(violation?.rule).toBe('regex'); + expect(violation?.message).toMatch(/template/i); + }); + + // Nobody else is going to report this: the user sees a field they cannot + // fill, and has no idea the template is the reason. + it('logs the variable and the rule at error level', () => { + validateValue('whatever', 'required|regex:/[/', 'SERVER_JARFILE'); + + expect(logged).toHaveBeenCalledWith(expect.stringContaining('SERVER_JARFILE')); + expect(logged).toHaveBeenCalledWith(expect.stringContaining('regex:/[/')); + }); + + /** + * An empty value is the one that gets through, and it is still logged. + * + * No rule but `required` is ever applied to an empty value — that is what + * makes `nullable` mean anything — so a broken expression cannot refuse + * one. Without the log the operator of a variable normally left empty + * would hear nothing at all, and would find out the day somebody first + * needs to fill it in. + */ + it('reports itself even when there is no value to refuse', () => { + expect(accepts('', 'nullable|regex:/[/')).toBe(true); + expect(logged).toHaveBeenCalledWith(expect.stringContaining('regex:/[/')); + }); + + it('reports itself before required has its say', () => { + expect(validateValue('', 'required|regex:/[/')[0]?.rule).toBe('required'); + expect(logged).toHaveBeenCalledWith(expect.stringContaining('regex:/[/')); + }); + + // The ambiguous case, torn on its pipes by design. What matters is that + // it lands here, loudly, rather than accepting everything in silence. + it('catches an undelimited alternation', () => { + expect(accepts('a', 'regex:^(a|b)$')).toBe(false); + expect(logged).toHaveBeenCalled(); + }); + + /** + * The rules written after it survive, which is the point of giving up on + * the scan rather than trusting a borrowed delimiter. + * + * Read as one expression — `/^(a|b$|in:x/`, closed on the slash belonging + * to `in:` — the two rules behind it would vanish, and the only complaint + * would be about the regex. Cut on the pipe, all three are applied. + */ + it('keeps the rules standing behind a borrowed delimiter', () => { + const violations = validateValue('123456', 'regex:/^(a|b$|in:x/|max:5'); + + expect(violations.map((violation) => violation.rule).sort()).toEqual([ + 'in', + 'max', + 'regex', + ]); + }); + }); + + /** + * A torn alternation is not always audible, and does not have to be. + * + * `regex:^a|b$` cuts to `^a`, which compiles perfectly well: it accepts + * `apple`, logs nothing, and quietly turns away the `xxxb` its author meant + * to allow. That is the ambiguity's real cost — not a refusal somebody + * hears about, but an expression narrower than the one written. Narrower is + * the direction that can be lived with in a file whose job is to narrow; + * the way to avoid it altogether is to write the delimiters. + */ + it('narrows an undelimited alternation to its first branch, in silence', () => { + expect(accepts('apple', 'regex:^a|b$|max:5')).toBe(true); + expect(accepts('xxxb', 'regex:^a|b$|max:5')).toBe(false); + expect(logged).not.toHaveBeenCalled(); + }); + + // The report an empty value triggers is for `regex:` rules alone. Reading + // any rule's first argument as an expression would have `in:*,none` + // denounced as a broken template, which is an alarm about nothing — and an + // operator who has been woken by one stops reading the next. + it('says nothing about a rule that is not an expression at all', () => { + expect(accepts('', 'nullable|in:*,none')).toBe(true); + expect(logged).not.toHaveBeenCalled(); }); }); @@ -195,3 +572,65 @@ describe('the templates .jar file rule', () => { expect(accepts(value, RULES)).toBe(false); }); }); + +/** + * The rules the shipped catalogue already carries, unchanged by the scan. + * + * None of them holds a pipe inside its expression — the catalogue was written + * around the bug, in character classes — so every one of them was cut correctly + * by the old split and has to keep behaving identically. This is what would + * notice if the scan had moved a boundary by one character. + */ +describe('the shipped catalogue rules', () => { + const SAVE_NAME = 'required|string|regex:/^[A-Za-z0-9_-]{1,64}$/'; + const FACTORIO_VERSION = 'required|string|regex:/^[0-9a-z][0-9a-z.]{0,19}$/'; + + it.each(['gamesave', 'my_world-2', 'a'.repeat(64)])('accepts "%s" as a save name', (value) => { + expect(accepts(value, SAVE_NAME)).toBe(true); + }); + + it.each(['../../etc/passwd', 'save.zip', 'a b', 'a'.repeat(65), ''])( + 'rejects "%s" as a save name', + (value) => { + expect(accepts(value, SAVE_NAME)).toBe(false); + }, + ); + + it.each(['stable', 'experimental', '2.0.28'])('accepts "%s" as a version', (value) => { + expect(accepts(value, FACTORIO_VERSION)).toBe(true); + }); + + it.each(['../1.1.110', 'stable/../../secrets', '..', 'http://elsewhere.test/x'])( + 'rejects "%s" as a version', + (value) => { + expect(accepts(value, FACTORIO_VERSION)).toBe(false); + }, + ); + + /** + * Not one of them is malformed, so not one of them changes behaviour. + * + * Worth asserting rather than assuming: a malformed rule no longer passes + * everything, it refuses everything, and a catalogue rule that did that would + * make a shipped template's variable impossible to set. `catalog.spec.ts` + * holds the same guard over the real catalogue; this one covers the strings + * as this file reads them. + */ + it('compiles every one of them without complaint', () => { + const catalogue = [ + 'required|string|max:20', + 'required|string|max:30', + String.raw`required|string|max:100|regex:/^[A-Za-z0-9._-]+\.jar$/`, + FACTORIO_VERSION, + SAVE_NAME, + 'required|in:0,1', + 'nullable|string', + ]; + + for (const rules of catalogue) { + validateValue('server.jar', rules, 'CATALOGUE'); + } + + expect(logged).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/panel/src/modules/startup/variable-rules.ts b/apps/panel/src/modules/startup/variable-rules.ts index 69c25be..5f7fcc6 100644 --- a/apps/panel/src/modules/startup/variable-rules.ts +++ b/apps/panel/src/modules/startup/variable-rules.ts @@ -1,3 +1,5 @@ +import { Logger } from '@nestjs/common'; + /** * Validating template variables. * @@ -26,7 +28,14 @@ * min:n / max:n length for text, value for a number * between:a,b both at once * in:a,b,c a closed list of values - * regex:/…/ a regular expression, applied as is + * regex:/…/ a regular expression, delimiters included, applied as is + * + * A `regex:` is read whole: the rule string is **scanned**, not split, so the + * `|` of an alternation belongs to the expression and not to the rule list. An + * expression that cannot be compiled refuses the value rather than accepting + * it, and is reported in the log even when there is no value to refuse; + * `checkRegex` below says why that is the only safe direction for a file whose + * job is to narrow what reaches a startup command. */ /** @@ -39,6 +48,9 @@ */ export const MAX_VARIABLE_LENGTH = 2048; +/** A malformed rule is the operator's fault, and nobody else will report it. */ +const logger = new Logger('VariableRules'); + export interface RuleViolation { rule: string; message: string; @@ -49,9 +61,29 @@ interface ParsedRule { args: string[]; } +/** The rule name, and so how far past its start the expression begins. */ +const REGEX_RULE = 'regex:'; + +/** What opens a region the pipe does not divide. Lower case: the scan folds. */ +const DELIMITED_REGEX = `${REGEX_RULE}/`; + +/** + * Cuts a rule string into its rules. + * + * `|` separates rules, **except** inside a delimited `regex:` — and that + * exception is the whole reason this scans rather than calling `split('|')`. + * Eggs write alternations constantly, `regex:/^(paper|purpur)$/`, and splitting + * first tore that into `regex:/^(paper` and `purpur)$/`: the first no longer + * compiled, the second was read as a rule nobody recognises and dropped. The + * strictest-looking line in a template enforced nothing whatever, and said + * nothing about it either. + * + * So a rule opening with `regex:/` is consumed through its closing `/` and its + * flags, and only then may a `|` end it. Everything else is still cut on the + * pipe, exactly as before. + */ export function parseRules(raw: string): ParsedRule[] { - return raw - .split('|') + return splitRules(raw) .map((part) => part.trim()) .filter((part) => part !== '') .map((part) => { @@ -64,18 +96,181 @@ export function parseRules(raw: string): ParsedRule[] { const name = part.slice(0, separator).toLowerCase(); const rest = part.slice(separator + 1); - // `regex:` keeps its argument whole: it contains commas, pipes and - // colons that must not be split. + // `regex:` keeps its argument whole: it contains commas and colons that + // must not be split. Its pipes survived one step earlier, in the scan. return { name, args: name === 'regex' ? [rest] : rest.split(',') }; }); } +function splitRules(raw: string): string[] { + const parts: string[] = []; + let start = 0; + + // A pipe closing the last rule leaves nothing but emptiness behind it, so + // stopping at the end of the string drops no rule. + while (start < raw.length) { + const end = endOfRule(raw, start); + + parts.push(raw.slice(start, end)); + start = end + 1; + } + + return parts; +} + +/** + * Index of the `|` closing the rule that begins at `start`, or the end of the + * string. + * + * A `regex:` **not** followed by `/` is cut on the pipe like any other rule. + * Pterodactyl always delimits, so a bare one is hand-written, and it is + * genuinely ambiguous: in `regex:^(a|b)$|max:5` nothing tells the alternation's + * pipe from the one introducing `max`. Cutting keeps every rule that was + * written, and errs the safe way round. What the cut leaves is the expression + * up to its first pipe: either one branch of the alternation, which matches a + * subset of what the whole would have matched — `regex:^a|b$` becomes `^a`, + * which no longer admits the `b` branch — or a fragment the cut left unbalanced + * or dangling, which does not compile and refuses the value loudly. Neither can + * accept more than the template's author asked for. The other reading, + * swallowing the rest of the string, would silently drop every rule written + * after the regex, which is the same silent under-validation this file has just + * stopped doing, moved one rule to the right. + * + * The narrowing is not always audible: a torn fragment often compiles, and its + * author then finds their expression stricter than they wrote it with nothing + * in the log to explain why. That is the cost of the ambiguity; it is the + * cheaper direction, and delimiting the expression avoids it entirely. + */ +function endOfRule(raw: string, start: number): number { + const head = skipSpaces(raw, start); + + if (raw.slice(head, head + DELIMITED_REGEX.length).toLowerCase() === DELIMITED_REGEX) { + const closing = endOfExpression(raw, head + DELIMITED_REGEX.length); + + if (closing !== -1) { + const next = skipSpaces(raw, closing); + + // Two tests that the `/` the scan stopped on really was the author's + // closing delimiter, because on an unterminated expression the scan will + // happily borrow one from a rule further along. Nothing but blanks may + // stand between the expression and the pipe: any other leftover means we + // have not understood the rule. And what the scan enclosed has to + // compile — the borrowed delimiter usually makes nonsense of the + // expression, and there is no reading under which an expression this + // file cannot apply is the one worth keeping. It is the very string + // `checkRegex` will be handed, so the two always agree. + // + // Failing either test falls back to the pipe below, which is how the + // whole rule string was cut before there was a scan at all: never wider + // than what this file already accepted. + // + // One case is left uncaught, knowingly. If a `regex:/` is never closed + // and a later rule lends it a `/`, and the region between them happens to + // compile, nothing in the string distinguishes that from a correctly + // delimited expression — `regex:/^(a|b)$|in:x/|max:5` reads as one valid + // regex and quietly swallows `in:x`. Catching it would mean refusing an + // expression that spans a `|` followed by something that looks like a + // rule name, which breaks the legitimate `regex:/^(a|max:5)$/` to rescue + // a template that is already malformed. A widening confined to broken + // templates is the better of the two, so it stays and is written down. + const enclosed = raw.slice(head + REGEX_RULE.length, closing); + + if ((next === raw.length || raw[next] === '|') && compileExpression(enclosed) !== null) { + return next; + } + } + } + + const pipe = raw.indexOf('|', start); + + return pipe === -1 ? raw.length : pipe; +} + +/** + * Scans a delimited expression from the first character of its body, and + * returns the index just past its closing `/` and flags — or -1 if it never + * closes. + * + * Two slashes are not the end of anything: a `\/`, which is a slash the pattern + * matches, and a `/` inside a `[…]` class, which JavaScript's own regex-literal + * grammar also reads as ordinary. Stopping at either cuts a working expression + * in half, and what is left is no longer the expression its author wrote: one + * that will not compile, if they are lucky and hear about it, and otherwise one + * that quietly matches something else entirely. + */ +function endOfExpression(raw: string, from: number): number { + let inClass = false; + + for (let index = from; index < raw.length; index += 1) { + const character = raw[index]; + + if (character === '\\') { + // The escape swallows whatever follows, delimiter included. + index += 1; + continue; + } + + if (inClass) { + inClass = character !== ']'; + continue; + } + + if (character === '[') { + inClass = true; + continue; + } + + if (character === '/') { + // Lower case only, because every flag JavaScript accepts is lower case. + // + // Taking capitals too looks more forgiving and is worse. `/^(a|b)$/I` is + // a broken expression either way, but swallowing the `I` keeps the rule + // whole, and `checkRegex` then reads the whole thing as a *bare* pattern + // — which compiles, matches almost nothing, and refuses every value + // while blaming the value. Stopping at the `/` instead lets the rule fall + // to the pipe, where the fragment fails to compile and the operator gets + // the logged "your template's rule is malformed" they can act on. + // + // This only decides where the rule ends; whether a run of lower-case + // letters is a flag set a regular expression will accept is + // `checkRegex`'s business. + let end = index + 1; + + while (end < raw.length && /[a-z]/.test(raw[end] ?? '')) { + end += 1; + } + + return end; + } + } + + return -1; +} + +function skipSpaces(raw: string, from: number): number { + let index = from; + + while (index < raw.length && /\s/.test(raw[index] ?? '')) { + index += 1; + } + + return index; +} + /** * Checks a value against a set of rules. * + * @param subject the variable the rules belong to, used only to name it in the + * log when one of them turns out to be malformed. Optional because a rule + * string is checkable on its own; a log line saying which rule broke without + * saying where it lives is still worth having. * @returns the list of rules broken, empty if the value is acceptable. */ -export function validateValue(value: string, raw: string): RuleViolation[] { +export function validateValue( + value: string, + raw: string, + subject = 'an unnamed variable', +): RuleViolation[] { if (value.length > MAX_VARIABLE_LENGTH) { return [ { @@ -90,6 +285,18 @@ export function validateValue(value: string, raw: string): RuleViolation[] { const empty = value.trim() === ''; if (empty) { + // The verdict is deliberately thrown away — what an empty value is worth is + // decided just below, not by an expression — but the call is kept for the + // log line inside it. A non-empty value produces that same line *and* a + // violation the operator sees; an empty one produces neither unless we ask, + // so a variable normally left empty would keep its broken rule secret until + // the day somebody finally fills it in. + for (const rule of rules) { + if (rule.name === 'regex') { + checkRegex(rule.args[0] ?? '', value, subject); + } + } + if (rules.some((rule) => rule.name === 'required')) { return [{ rule: 'required', message: 'This value is required.' }]; } @@ -100,7 +307,7 @@ export function validateValue(value: string, raw: string): RuleViolation[] { } for (const rule of rules) { - const violation = check(rule, value); + const violation = check(rule, value, subject); if (violation) { violations.push(violation); @@ -110,7 +317,7 @@ export function validateValue(value: string, raw: string): RuleViolation[] { return violations; } -function check(rule: ParsedRule, value: string): RuleViolation | null { +function check(rule: ParsedRule, value: string, subject: string): RuleViolation | null { const numeric = Number(value); switch (rule.name) { @@ -166,9 +373,7 @@ function check(rule: ParsedRule, value: string): RuleViolation | null { : { rule: 'in', message: `Accepted values: ${rule.args.join(', ')}.` }; case 'regex': - return matchesRegex(rule.args[0] ?? '', value) - ? null - : { rule: 'regex', message: 'This value is not in the expected format.' }; + return checkRegex(rule.args[0] ?? '', value, subject); default: // An unknown rule is **not** the user's mistake: it is the template @@ -210,19 +415,76 @@ function compare( return { rule: kind, message: `${subject} has to be ${limit} ${bound}.` }; } -function matchesRegex(pattern: string, value: string): boolean { +/** + * Applies a template's expression, refusing the value when the expression + * cannot be read. + * + * This used to accept the value instead, reasoning that an unreadable + * expression is the template's fault and that blocking a user on a mistake they + * cannot fix helps nobody. That reasoning is what turned the splitting bug + * above into silence: the split manufactured uncompilable expressions by the + * dozen, this branch swallowed every one, and a `regex:` guarding a file name + * or a download URL accepted anything at all with nothing anywhere saying so. A + * validation rule that fails open is not a validation rule, and the more a + * particular rule matters the more expensive its silence is. + * + * So the value is refused, and the message blames the template rather than the + * value: without that, somebody spends an afternoon working out what is wrong + * with `server.jar`. The log line exists for the same reason as the message — + * this is an operator-facing fault, and the user hitting it has no way to + * report anything useful about it. + * + * What it trades away is real. Until an administrator edits the template, that + * variable can hold nothing but an empty value — and not even that where the + * template says `required` — so a user who only wanted to rename their jar is + * stuck behind a broken template. That is the right way round: the block is + * visible, it lands on the first person to touch the variable, and one edit + * clears it — whereas failing open is a hole nobody is ever told about, on + * precisely the variables somebody thought worth constraining. Since the scan + * above, few expressions reach this branch at all: the split was manufacturing + * most of the failures, so what is left is a genuinely broken template. + */ +function checkRegex(pattern: string, value: string, subject: string): RuleViolation | null { + const expression = compileExpression(pattern); + + if (!expression) { + // Precise about the one value that still gets through, because an operator + // told "no value is accepted" who then watches an empty one save will trust + // nothing else the line says. + logger.error( + `Template variable ${subject} carries a rule that is not a valid regular expression ` + + `(regex:${pattern}). Every value will be refused for that variable until the template ` + + `is corrected, bar an empty one where no required rule stands beside it.`, + ); + + return { + rule: 'regex', + message: + "This variable's template rule is malformed and cannot be applied; " + + 'an administrator has to correct the template.', + }; + } + + return expression.test(value) + ? null + : { rule: 'regex', message: 'This value is not in the expected format.' }; +} + +/** + * Reads a `regex:` argument, `/pattern/flags` as the eggs write it or a bare + * pattern, and returns null when it cannot be read. + * + * The scan above uses this too, on the region it thinks is an expression, so + * that what it decides to keep whole and what this file can actually apply are + * the same judgement made once. + */ +function compileExpression(pattern: string): RegExp | null { // The `/pattern/flags` form, as it appears in eggs. const delimited = /^\/(.*)\/([a-z]*)$/s.exec(pattern); try { - const expression = delimited - ? new RegExp(delimited[1] ?? '', delimited[2]) - : new RegExp(pattern); - - return expression.test(value); + return delimited ? new RegExp(delimited[1] ?? '', delimited[2]) : new RegExp(pattern); } catch { - // An unreadable expression: the template is at fault, not the value. - // Refusing it would block the user on an error they cannot fix. - return true; + return null; } } diff --git a/docs/templates.md b/docs/templates.md index 35362b6..be8ba70 100644 --- a/docs/templates.md +++ b/docs/templates.md @@ -115,6 +115,24 @@ A `userEditable` variable feeds the startup command: it is user input that influ runs. The default is therefore "not editable", and every exception has to be a conscious choice — with rules narrow enough to accept only what makes sense. +**Write a `regex:` with its delimiters**, `regex:/^(paper|purpur)$/`, as the eggs do. The rule string +is scanned rather than split, so the pipes of an alternation belong to the expression and the rules +written after it are still read; flags, escaped delimiters and a `/` inside a character class are all +handled. A `regex:` without delimiters — or one whose closing delimiter is missing — is cut on the +pipe like any other rule, because nothing can tell an alternation's pipe from the one introducing the +next rule. An undelimited alternation therefore does not survive: what is left of it is its first +branch, which accepts less than was written, or a fragment that does not compile at all. Only the +second of those says anything in the log, so the delimiters are worth the two characters. + +**A rule that is not a valid regular expression refuses every value it is applied to**, with a +message saying the template is at fault and an error in the panel's log naming the variable. The +exception is an empty value: no rule but `required` is ever applied to one, so it is still accepted — +but the log fires all the same, which is how a broken rule on a variable normally left empty gets +noticed at all. A malformed rule used to accept every value instead, which meant a typo left the +variable unvalidated with nothing anywhere saying so. The trade is deliberate: the variable cannot be +given a value until the template is corrected, and that is a block somebody sees and fixes rather +than a hole nobody hears about. + ### Startup detection A server that is up is not a server that is ready. The template says how to tell the difference, and diff --git a/packages/templates/src/catalog.spec.ts b/packages/templates/src/catalog.spec.ts index 5e856c1..3ea81ab 100644 --- a/packages/templates/src/catalog.spec.ts +++ b/packages/templates/src/catalog.spec.ts @@ -57,6 +57,32 @@ describe('catalogue de templates', () => { } expect(template.stopCommand).toMatch(/^(command:.+|signal:SIG(TERM|INT|KILL))$/); + + /** + * Every `regex:` rule is delimited, comes last, and compiles. + * + * All three matter to the panel's reading of the rule string. It scans + * a `regex:/…/` whole, so an alternation inside one is safe — but only + * a delimited one; a bare `regex:` is still cut on the pipe, because + * nothing can tell an alternation's pipe from the next rule's. And a + * rule that will not compile no longer passes every value: it refuses + * them all, so a typo shipped here would make the variable impossible + * to set rather than merely unguarded. Last in the list is this + * catalogue's own convention, and what `ruleExpression` below reads. + */ + for (const variable of template.variables) { + if (!variable.rules.includes('regex:')) { + continue; + } + + const delimited = /regex:\/(.*)\/([a-z]*)$/.exec(variable.rules); + + expect( + delimited, + `${variable.envVariable}: its regex rule is not delimited, or is not last`, + ).not.toBeNull(); + expect(() => new RegExp(delimited?.[1] ?? '', delimited?.[2])).not.toThrow(); + } }, ); }); @@ -337,32 +363,6 @@ describe('catalogue de templates', () => { return new RegExp(source!); }; - /** - * The rule string is split on `|` **before** anything looks for `regex:`. - * - * An alternation inside one is therefore torn into fragments: the first - * becomes an unterminated expression, which fails to compile, and a - * pattern that fails to compile is treated as the template's mistake and - * accepts every value; the remaining fragments are read as rules nobody - * recognises and ignored. The result reads as the strictest line in the - * file and enforces nothing at all — so the expressions here are written - * as character classes, and this is what keeps them that way. - */ - it('writes its regular expressions without an alternation', () => { - for (const variable of factorio?.variables ?? []) { - const index = variable.rules.indexOf('regex:'); - - if (index === -1) { - continue; - } - - expect( - variable.rules.slice(index), - `${variable.envVariable} splits on its own rule`, - ).not.toContain('|'); - } - }); - // This value becomes a segment of the download URL, and curl resolves a // path before it sends it. it.each(['stable', 'experimental', 'latest', '2.0.28', '1.1.110'])( diff --git a/packages/templates/src/catalog/factorio.ts b/packages/templates/src/catalog/factorio.ts index 2002b80..2066607 100644 --- a/packages/templates/src/catalog/factorio.ts +++ b/packages/templates/src/catalog/factorio.ts @@ -538,13 +538,16 @@ export const factorio: TemplateDefinition = { * * Written as a character class and not as an alternation of the three * channel words and a dotted version, which is what it would like to be. - * The rule string is split on `|` *before* anything looks at `regex:`, so - * an alternation is torn into fragments: the first becomes an unterminated - * expression that fails to compile — and an expression that fails to - * compile is treated as the template's fault and passes every value — and - * the rest become unknown rules, which are ignored. The check would read - * as the strictest one in the file and enforce nothing at all. A class - * survives the split because it contains no `|`. + * That was forced: the panel used to split the rule string on `|` before + * looking for `regex:`, which tore an alternation into fragments the + * first of which no longer compiled — and an expression that would not + * compile was treated as the template's fault and passed every value. The + * class survived only because it contains no pipe. The panel now scans + * for the delimiters instead of splitting, so an alternation here would + * hold, and a rule that will not compile refuses the value rather than + * waving it through. The class is kept because it is correct and in use; + * narrowing it to an alternation is a separate decision about what a + * version may be, not a workaround any more. * * What it admits: `stable`, `experimental`, `latest`, and `2.0.28`. What * it excludes is what matters — no `/`, `%`, `:`, `@`, `?` or `\`, so