From 3e69a1764408b62da3859e049b37e26fbb3a0c3b Mon Sep 17 00:00:00 2001 From: Marcel Wege Date: Mon, 3 Aug 2026 14:25:40 +0200 Subject: [PATCH] fix: accept {n,} patterns and localize setup-field hints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects in the setup-field validation shipped in #599, both found by writing the first real plugin manifest against it. Neither was visible from inside this repo. 1. The pattern allowlist refused `{n,}` while allowing `+`, which is the same thing. The first realistic pattern anyone wrote against the feature — `[A-Za-z]{2,}` for an email TLD — was rejected for no safety reason and had to be respelled. Shape is now judged purely by the group-content rules that already govern `+` and `*`, so every spelling of a quantifier is treated alike. Each hostile pattern in the existing table is now also asserted in its `{n,}` spelling, so the counted form cannot open a door the `+` form keeps shut. The size cap had to be extended to `{n,}`'s minimum, which nothing needed to bound while the construct was illegal — otherwise `a{100000,}` would have become legal the moment the shape check stopped rejecting it. The client carried a hand-mirrored copy of the same rule, and there the bug was worse: a rejected pattern means no native `pattern=` attribute and a check that fails open, so the operator would have typed a bad value, seen nothing, and hit a 400 from a validation the client had silently opted out of. Both halves are fixed together. 2. Every `400 runtime.setup_field_invalid` returned the English hint whatever the UI language, because the locale parameter was never passed. English help text in a German UI is one of the named contributing factors of the finding this feature exists to prevent. The client now resolves the hint from the field it already holds, keyed on the violation's field, and falls back to the server string when the field is unknown. No API change: `hint` stays the English fallback for clients that have no manifest. The unused locale parameter is gone rather than left implying a threading that does not exist. Middleware has no locale plumbing at all — no Accept-Language read anywhere, and the locale cookie never leaves the Next.js layer. Threading one would mean the server picking a language for a client it cannot see, which is the same untranslatable-string-through-the-API mistake in a different costume. --- middleware/src/plugins/installService.ts | 5 + middleware/src/plugins/setupFieldPattern.ts | 85 ++++++-- middleware/src/routes/runtime.ts | 10 +- .../test/setupFieldPatternValidation.test.ts | 183 +++++++++++++++++- .../_components/store/CredentialsEditor.tsx | 69 +++++-- .../app/_components/store/InstallButton.tsx | 20 +- .../app/_components/store/RequiresWizard.tsx | 12 +- .../CredentialsEditorPassword.test.tsx | 87 +++++++++ .../__tests__/setupFormPatternHint.test.tsx | 100 ++++++++++ web-ui/app/_components/store/setupForm.tsx | 36 +++- .../_lib/__tests__/setupFieldPattern.test.ts | 119 ++++++++++++ web-ui/app/_lib/setupFieldPattern.ts | 62 +++++- 12 files changed, 741 insertions(+), 47 deletions(-) create mode 100644 web-ui/app/_components/store/__tests__/setupFormPatternHint.test.tsx diff --git a/middleware/src/plugins/installService.ts b/middleware/src/plugins/installService.ts index 6dcb700e..d4bd654e 100644 --- a/middleware/src/plugins/installService.ts +++ b/middleware/src/plugins/installService.ts @@ -768,6 +768,11 @@ async function validateValues( // The manifest's own hint when it declared one, otherwise the // pre-existing generic message (kept byte-identical so existing // install-flow assertions and operator muscle memory still hold). + // + // The hint is ENGLISH — this process has no request locale. The + // wizard's `FieldRow` re-resolves it from `field.pattern_hint` in the + // active locale, keyed on this entry's `code`, so a German operator + // reads German. See `setupFieldPattern.ts` → `PatternViolation.hint`. message: violation.hint ?? `"${field.label}" entspricht nicht dem erwarteten Muster.`, diff --git a/middleware/src/plugins/setupFieldPattern.ts b/middleware/src/plugins/setupFieldPattern.ts index 2ec4d828..26a9461e 100644 --- a/middleware/src/plugins/setupFieldPattern.ts +++ b/middleware/src/plugins/setupFieldPattern.ts @@ -35,8 +35,10 @@ * (b) ALLOWLIST GRAMMAR — {@link screenPatternSource} parses the pattern * source and accepts only shapes that cannot blow up: no backreferences, * no quantifier applied to a group that contains alternation or another - * quantifier, no quantified lookaround, bounded group nesting, and no - * open-ended or huge counted repetition. Applied at manifest LOAD time. + * quantifier, no quantified lookaround, bounded group nesting, and a cap + * on how large a counted repetition may be. Applied at manifest LOAD + * time. Counted repetition (`{n}` / `{n,}` / `{n,m}`) is governed by + * exactly the same rules as `*` and `+` — see {@link checkCountedBounds}. * * (b) is deliberately conservative and WILL reject legitimate-looking patterns * (`^[a-z]+(-[a-z]+)*$` is a real catastrophic-backtracking shape even though a @@ -90,7 +92,23 @@ export const PATTERN_MATCH_BUDGET_MS = 50; /** Deepest group nesting the allowlist accepts (root counts as depth 0). */ const MAX_GROUP_DEPTH = 2; -/** Largest explicit repetition count the allowlist accepts. */ +/** + * Largest explicit repetition count the allowlist accepts, applied to BOTH + * bounds of a counted quantifier (`{n}`, `{n,}`, `{n,m}`). + * + * This is defence in depth, not the safety floor. Measured on node 22: + * `^[a-z]{1,100000}[a-z]{1,100000}$` against an 8191-char non-matching subject + * takes 71 ms, versus 39 ms for `^[a-z]+[a-z]+$` — the same polynomial class as + * the `+` form the allowlist has always accepted, not a new one. (V8 compiles + * counted repetition with a counter rather than unrolling it, so a huge bound + * is not a compile-time blowup either: `^a{100000,}$` compiles AND matches a + * 100k subject in 0.43 ms.) The load-bearing bound is the 50 ms worker budget. + * + * The cap is kept because it is free and it keeps an untrusted manifest from + * naming an arbitrary number, and it is kept at 100 rather than raised because + * 100 covers every credential shape this feature exists for: DNS label ≤ 63, + * TLD 2-63, SHA-256 hex 64, UUID segments, PIN/OTP lengths. + */ const MAX_COUNTED_REPETITION = 100; /** How long to wait for a freshly spawned worker to come online before giving @@ -106,6 +124,8 @@ interface QuantifierToken { readonly length: number; /** True for `{n}` / `{n,}` / `{n,m}` — the counted forms. */ readonly counted: boolean; + /** Lower bound for a counted form. */ + readonly min?: number; /** Upper bound for a counted form; `undefined` means open-ended (`{n,}`). */ readonly max?: number; } @@ -133,15 +153,26 @@ function parseQuantifier(src: string, i: number): QuantifierToken | null { ? undefined : Number(maxRaw); return max === undefined - ? { length, counted: true } - : { length, counted: true, max }; + ? { length, counted: true, min } + : { length, counted: true, min, max }; } +/** + * Size check for a counted quantifier. SHAPE is not this function's business: + * `{n,}` is exactly `{1,}`-style open-ended repetition, i.e. the same thing `+` + * and `*` express, and it is screened by the same group-content rules those go + * through (see the `)` branch of {@link screenPatternSource}). Refusing `{n,}` + * while accepting `+` bought no safety at all — it only forced manifest authors + * to write `[A-Za-z][A-Za-z]+` where they meant `[A-Za-z]{2,}`, which is the + * identical language spelled worse. All that is left here is the numeric cap. + * + * Both bounds are capped. For `{n,}` the only number an author supplies is the + * MINIMUM, so leaving `min` unchecked would have handed an untrusted manifest + * an unbounded knob the moment `{n,}` became legal. + */ function checkCountedBounds(q: QuantifierToken): string | null { - if (q.max === undefined) { - return 'open-ended counted repetition `{n,}` is not allowed'; - } - if (q.max > MAX_COUNTED_REPETITION) { + const largest = Math.max(q.min ?? 0, q.max ?? 0); + if (largest > MAX_COUNTED_REPETITION) { return `counted repetition above ${MAX_COUNTED_REPETITION} is not allowed`; } return null; @@ -166,10 +197,17 @@ interface GroupFrame { * - lookaround containing a quantifier — same blowup, hidden behind `(?=)` * - group nesting > 2 — bounds what the two rules above * have to reason about - * - `{n,}` / `{n,m}` with a huge m — bounded but arbitrarily large work + * - a counted repetition above 100 — see {@link MAX_COUNTED_REPETITION} * * Alternation and quantifiers are PROPAGATED to the enclosing frame on close, * so wrapping a hostile shape in another group cannot launder it. + * + * The rules deliberately do NOT distinguish quantifier SPELLINGS. `+`, `*`, + * `{2,}` and `{2,63}` are all "a quantifier": each is refused on a group that + * contains alternation or another quantifier, and each is accepted on a simple + * atom or character class. An earlier revision refused `{n,}` outright while + * accepting `+` — logically the same construct — which bought no safety and + * made `^[^@\s]+@[^@\s]+\.[A-Za-z]{2,}$` unwritable. */ export function screenPatternSource(source: string): string | null { const stack: GroupFrame[] = [ @@ -599,12 +637,28 @@ export interface PatternViolation { /** The setup-field key that failed. */ field: string; /** - * The manifest's own explanation of the expected shape, when it declared one. + * The manifest's own explanation of the expected shape, when it declared one, + * resolved to ENGLISH. * * DELIBERATELY OPTIONAL and never server-generated: the web-ui owns all * user-facing copy (`messages/{en,de}.json`) and renders its own localized * fallback when this is absent. A generated English or German sentence here * would be an untranslatable string smuggled in through the API. + * + * WHY ENGLISH, ALWAYS — and why that is not a localization bug. The middleware + * has no notion of a request locale: nothing reads `Accept-Language`, no + * locale cookie reaches it, and the web-ui's `NEXT_LOCALE` never leaves the + * Next.js layer. Manufacturing one just for this field would be the same + * "untranslatable string smuggled in through the API" mistake in a different + * costume — the server would be picking a language for a client it cannot see. + * + * So this stays the documented fallback for API clients that have no manifest + * of their own (curl, the install CLI, third-party integrations). Anything + * that HOLDS the manifest — i.e. the web-ui, which renders + * `field.pattern_hint` next to the input already — must resolve the localized + * map itself, keyed on {@link PatternViolation.field}, and use this only when + * the key matches no field it knows about. See + * `web-ui/app/_lib/setupFieldPattern.ts` → `resolveSetupFieldHint`. */ hint?: string; } @@ -613,6 +667,10 @@ export interface PatternViolation { * Pick the best hint string out of a `{ locale: text }` map. Mirrors the * web-ui's `pickLocalized`: preferred locale, then `en`, then `de`, then * anything. Kept local so this module stays dependency-free. + * + * `locale` exists for callers that genuinely have one. The middleware does not + * (see {@link PatternViolation.hint}), so every production call resolves to + * English by default and the CLIENT does the localized pick. */ export function pickPatternHint( map: Record | undefined, @@ -648,12 +706,13 @@ export async function checkSetupFieldPattern( field: PatternCheckableField, value: string, context = field.key, - locale = 'en', ): Promise { if (!field.pattern) return null; if (value.length === 0) return null; - const hint = pickPatternHint(field.pattern_hint, locale); + // English on purpose, and no `locale` parameter to imply otherwise: there is + // no request locale on this side of the wire. See `PatternViolation.hint`. + const hint = pickPatternHint(field.pattern_hint); const violation: PatternViolation = hint ? { field: field.key, hint } : { field: field.key }; diff --git a/middleware/src/routes/runtime.ts b/middleware/src/routes/runtime.ts index 322c1136..db0d8ea5 100644 --- a/middleware/src/routes/runtime.ts +++ b/middleware/src/routes/runtime.ts @@ -501,9 +501,13 @@ export function createRuntimeRouter(deps: RuntimeDeps): Router { code: 'runtime.setup_field_invalid', message: `value for '${violation.field}' does not match the expected format`, field: violation.field, - // Only ever the manifest's own localized hint. When the manifest - // declared none, `hint` is absent and the UI renders its own - // localized copy — the API never invents user-facing prose. + // Only ever the manifest's own hint, resolved to ENGLISH — this + // process has no request locale, and guessing one would smuggle an + // untranslatable string through the API. It is the fallback for + // clients without a manifest; the web-ui resolves `pattern_hint` + // itself from `field`. When the manifest declared no hint, this is + // absent and the UI renders its own localized copy. + // See `setupFieldPattern.ts` → `PatternViolation.hint`. ...(violation.hint !== undefined ? { hint: violation.hint } : {}), }); return; diff --git a/middleware/test/setupFieldPatternValidation.test.ts b/middleware/test/setupFieldPatternValidation.test.ts index e4e9eb2c..9ef4c19a 100644 --- a/middleware/test/setupFieldPatternValidation.test.ts +++ b/middleware/test/setupFieldPatternValidation.test.ts @@ -257,6 +257,27 @@ describe('OM-17 — compileSetupPattern safety screen', () => { assert.equal(violation?.field, 'k'); }); + it('`hint` is the ENGLISH entry, and that is the documented contract', async () => { + // The middleware has no request locale — nothing reads Accept-Language and + // `NEXT_LOCALE` never leaves the Next.js layer — so it must not pretend to + // pick one. English is the fallback for API clients with no manifest; a + // client that HOLDS the manifest resolves `pattern_hint` itself, keyed on + // `violation.field`. Pinned so nobody "fixes" this into a guessed locale. + const violation = await checkSetupFieldPattern( + { + key: 'gw_sa_client_email', + pattern: SA_EMAIL_PATTERN, + pattern_hint: { + en: 'expects …@….iam.gserviceaccount.com', + de: 'erwartet …@….iam.gserviceaccount.com', + }, + }, + 'tester@customer-company.de', + ); + assert.equal(violation?.field, 'gw_sa_client_email'); + assert.equal(violation?.hint, 'expects …@….iam.gserviceaccount.com'); + }); + it('omits `hint` when the manifest declared no pattern_hint', async () => { // The API must never invent user-facing prose; the web-ui owns that copy. const violation = await checkSetupFieldPattern( @@ -359,8 +380,22 @@ const REDOS_ALREADY_BLOCKED = [ '^(a{1,10}){1,10}b$', ]; -/** The two shapes this whole feature exists for. These MUST keep working. */ -const REALISTIC_PATTERNS = [SA_EMAIL_PATTERN, '^-----BEGIN [A-Z ]*PRIVATE KEY-----']; +/** + * Every pattern in the FIRST real manifest written against this feature + * (byte5ai/omadia-google-workspace#1). These MUST keep working — the feature is + * worthless if the manifest it exists for cannot express what it needs. + * + * `^…\.[A-Za-z]{2,}$` is here because the allowlist used to refuse `{n,}` while + * accepting `+`, which is the same construct. The manifest author had to write + * `[A-Za-z][A-Za-z]+` — identical language, worse to read — to get it past the + * screen. See {@link screenPatternSource}. + */ +const REALISTIC_PATTERNS = [ + SA_EMAIL_PATTERN, + '^-----BEGIN [A-Z ]*PRIVATE KEY-----', + '^[^@\\s]+@[^@\\s]+\\.[A-Za-z]{2,}$', + '^[^@\\s]+@[^@\\s]+\\.[A-Za-z]{2,63}$', +]; describe('OM-17 / F1 — allowlist grammar replaces the bypassable blacklist', () => { beforeEach(() => { @@ -401,9 +436,8 @@ describe('OM-17 / F1 — allowlist grammar replaces the bypassable blacklist', ( assert.notEqual(screenPatternSource('^(?=.*a+)b$'), null); }); - it('rejects group nesting deeper than 2 and open-ended/large {n,m}', () => { + it('rejects group nesting deeper than 2 and oversized {n,m}', () => { assert.notEqual(screenPatternSource('^(((a)))b$'), null); - assert.notEqual(screenPatternSource('a{2,}'), null); assert.notEqual(screenPatternSource('a{1,5000}'), null); assert.equal(screenPatternSource('^\\d{3}-\\d{4}$'), null); }); @@ -413,6 +447,147 @@ describe('OM-17 / F1 — allowlist grammar replaces the bypassable blacklist', ( }); }); +// --------------------------------------------------------------------------- +// F5 — the allowlist refused `{n,}` while accepting `+`, which IS `{1,}` +// --------------------------------------------------------------------------- + +/** + * The rule bought no safety and only cost manifest authors: the very first + * realistic pattern written against this feature — an email TLD, + * `^[^@\s]+@[^@\s]+\.[A-Za-z]{2,}$` — was refused and had to ship as + * `[A-Za-z][A-Za-z]+`, which is the identical language spelled worse. + * + * Counted quantifiers are now screened by exactly the rules `*` and `+` go + * through: refused on a group containing alternation or another quantifier, + * accepted on a simple atom or character class, with a numeric cap on both + * bounds. The `REDOS_BYPASSES` table above is the other half of this change — + * every hostile shape there must still be rejected, and each of those shapes + * would also be rejected written as `{n,}` (see below). + */ +describe('OM-17 / F5 — `{n,}` is screened exactly like the `+` it is equal to', () => { + beforeEach(() => { + resetSetupPatternCache(); + }); + + const ACCEPTED: ReadonlyArray = [ + ['^[A-Za-z]{2,}$', 'open-ended counted repetition on a character class'], + ['^a{2,}$', 'open-ended counted repetition on a literal'], + ['^[A-Za-z]{2,63}$', 'the bounded form of the same thing'], + ['^\\d{4}$', 'an exact count'], + ['^[a-z]{0,}$', '`{0,}` — i.e. `*`'], + ['^[a-z]{2,}?$', 'the lazy form'], + ['^a{100}$', 'exactly at the counted-repetition cap'], + ['^a{100,}$', 'the cap applied to the MINIMUM of an open-ended form'], + ]; + + for (const [pattern, why] of ACCEPTED) { + it(`accepts ${pattern} (${why})`, () => { + assert.equal( + screenPatternSource(pattern), + null, + `${pattern} must be accepted — it is exactly what \`+\`/\`*\` express`, + ); + assert.ok(compileSetupPattern(pattern, 'test') instanceof RegExp); + }); + } + + const REJECTED: ReadonlyArray = [ + ['^a{101}$', 'one above the counted-repetition cap'], + ['^a{101,}$', 'the MINIMUM of an open-ended form is capped too — without ' + + 'that, allowing `{n,}` would hand a manifest an unbounded knob'], + ['^a{1,101}$', 'upper bound above the cap'], + ['^a{100000,}$', 'an absurd open-ended minimum'], + // The hostile shapes from REDOS_BYPASSES, rewritten with `{n,}`. Allowing + // the counted spelling must not open a door the `+` spelling keeps shut. + ['^(a|a){1,}$', '`^(a|a)+$` in counted clothing — quantified alternation'], + ['^(a{1,})+$', '`^(a+)+$` in counted clothing — nested quantifier'], + ['^(a{1,}){1,}$', 'both halves counted'], + ['^((a{2,})){2,}$', '`^((a+))+$` in counted clothing — laundering by nesting'], + ['^(?:a|a){2,}$', 'non-capturing group does not launder it either'], + // No `.*` here on purpose — the counted quantifier must be the ONLY thing + // that trips the lookaround rule, otherwise the case proves nothing. + ['^(?=a{1,})b$', 'lookaround containing an open-ended counted repetition'], + ]; + + for (const [pattern, why] of REJECTED) { + it(`still rejects ${pattern} (${why})`, () => { + assert.notEqual( + screenPatternSource(pattern), + null, + `${pattern} was accepted by the screen`, + ); + assert.equal(compileSetupPattern(pattern, 'test'), null); + }); + } + + it('an accepted `{n,}` pattern MATCHES correctly end to end', async () => { + // Compiling is not the bar — the pattern has to do its job. This is the + // literal OM-17 confusion, on the field the real manifest declares with + // `{2,}`: a plausible-looking address must pass and a password must not. + const field = { + key: 'gw_impersonated_user', + pattern: '^[^@\\s]+@[^@\\s]+\\.[A-Za-z]{2,}$', + }; + assert.equal( + await checkSetupFieldPattern(field, 'tester@customer-company.de'), + null, + ); + assert.equal(await checkSetupFieldPattern(field, 'admin@byte5.io'), null); + // `{2,}` really is open-ended: a long TLD must pass, where `{2}` would not. + assert.equal( + await checkSetupFieldPattern(field, 'ops@example.technology'), + null, + ); + // …and it really is a MINIMUM of two: a 1-char TLD must fail. + assert.equal( + (await checkSetupFieldPattern(field, 'ops@example.x'))?.field, + 'gw_impersonated_user', + ); + // What the tester actually typed into a field like this. + assert.equal( + (await checkSetupFieldPattern(field, 'hunter2'))?.field, + 'gw_impersonated_user', + ); + }); + + it('the bounded `{2,63}` form matches the same way, and enforces its cap', async () => { + const field = { key: 'email', pattern: '^[^@\\s]+@[^@\\s]+\\.[A-Za-z]{2,63}$' }; + assert.equal(await checkSetupFieldPattern(field, 'a@b.de'), null); + assert.equal( + (await checkSetupFieldPattern(field, `a@b.${'x'.repeat(64)}`))?.field, + 'email', + ); + }); + + it('a whole realistic manifest field set loads with every pattern intact', async () => { + // Mirrors the field set of the first real manifest written against this + // feature. Inlined rather than read from that repo: the assertion is about + // OUR screen, and a test must not depend on a sibling checkout existing. + const plugin = adaptManifestV1({ + schema_version: '1', + identity: { id: 'gw', name: 'Google Workspace', version: '1.0.0' }, + setup: { + fields: REALISTIC_PATTERNS.map((pattern, idx) => ({ + key: `f${String(idx)}`, + type: 'secret', + pattern, + pattern_hint: { en: 'en hint', de: 'de hint' }, + })), + }, + }); + assert.equal(plugin?.setup_fields.length, REALISTIC_PATTERNS.length); + for (const f of plugin?.setup_fields ?? []) { + assert.equal( + f.pattern_unavailable, + undefined, + `${String(f.pattern)} came back pattern_unavailable`, + ); + assert.ok(f.pattern, `${f.key} lost its pattern`); + } + assert.deepEqual(getPatternProblems(), []); + }); +}); + describe('OM-17 / F1 — hard execution bound on the match itself', () => { after(async () => { await shutdownPatternWorker(); diff --git a/web-ui/app/_components/store/CredentialsEditor.tsx b/web-ui/app/_components/store/CredentialsEditor.tsx index 95b93483..772fdb7c 100644 --- a/web-ui/app/_components/store/CredentialsEditor.tsx +++ b/web-ui/app/_components/store/CredentialsEditor.tsx @@ -35,7 +35,10 @@ import { type SetupOption, } from '../../_lib/api'; import { pickLocalized } from '../../_lib/localized'; -import { violatesSetupPattern } from '../../_lib/setupFieldPattern'; +import { + resolveSetupFieldHint, + violatesSetupPattern, +} from '../../_lib/setupFieldPattern'; import type { PluginSetupField } from '../../_lib/storeTypes'; import { Button } from '@/app/_components/ui/Button'; @@ -192,11 +195,19 @@ export function CredentialsEditor({ ); setSavedAt(Date.now()); } catch (err) { - setError(humanizeError(err)); + setError(humanizeSecretsPatchError(err, setupFields, locale)); } finally { setSaving(false); } - }, [saving, dirtyCount, invalidKeys, setupFields, fieldStates, pluginId]); + }, [ + saving, + dirtyCount, + invalidKeys, + setupFields, + fieldStates, + pluginId, + locale, + ]); if (setupFields.length === 0) { return ( @@ -763,15 +774,7 @@ function humanizeError(err: unknown): string { const body = JSON.parse(err.body) as { code?: string; message?: string; - field?: string; - hint?: string; }; - // OM-17 — the server's field-level rejection. Prefer the manifest's own - // hint ("expects …@….iam.gserviceaccount.com") over the generic - // `code: message` line, which tells the operator nothing actionable. - if (body.code === 'runtime.setup_field_invalid' && body.hint) { - return body.field ? `${body.field}: ${body.hint}` : body.hint; - } if (body.code && body.message) return `${body.code}: ${body.message}`; if (body.message) return body.message; } catch { @@ -782,3 +785,47 @@ function humanizeError(err: unknown): string { if (err instanceof Error) return err.message; return String(err); } + +/** + * {@link humanizeError} plus the OM-17 field-level rejection, which only the + * secrets PATCH can return. + * + * Prefers the manifest's own hint ("expects …@….iam.gserviceaccount.com") over + * the generic `code: message` line, which tells the operator nothing + * actionable — and resolves it from OUR copy of `pattern_hint`, not from + * `body.hint`. The middleware has no request locale, so its hint is always + * English and a German operator would read an English sentence: exactly the + * English-in-a-German-UI confusion that was a named contributing factor of + * OM-17. `body.hint` remains the fallback for a key we do not know about. + * + * @param setupFields the manifest fields this editor renders — the source of + * the localized `pattern_hint` map + * @param locale the active UI locale + */ +function humanizeSecretsPatchError( + err: unknown, + setupFields: ReadonlyArray, + locale: string, +): string { + if (err instanceof ApiError) { + try { + const body = JSON.parse(err.body) as { + code?: string; + field?: string; + hint?: string; + }; + if (body.code === 'runtime.setup_field_invalid') { + const hint = resolveSetupFieldHint( + setupFields, + body.field, + body.hint, + locale, + ); + if (hint) return body.field ? `${body.field}: ${hint}` : hint; + } + } catch { + // fall through to the generic handling + } + } + return humanizeError(err); +} diff --git a/web-ui/app/_components/store/InstallButton.tsx b/web-ui/app/_components/store/InstallButton.tsx index a888c806..d31fe039 100644 --- a/web-ui/app/_components/store/InstallButton.tsx +++ b/web-ui/app/_components/store/InstallButton.tsx @@ -36,7 +36,11 @@ import type { import { PostInstallNextSteps } from './PostInstallNextSteps'; import { pickLocalized } from '../../_lib/localized'; import { RequiresWizard } from './RequiresWizard'; -import { FieldRow, extractValues } from './setupForm'; +import { + FieldRow, + extractValues, + type SetupFieldError, +} from './setupForm'; import { Markdown } from '../Markdown'; import { Button } from '@/app/_components/ui/Button'; @@ -93,8 +97,11 @@ export function InstallButton({ const locale = useLocale(); const setupGuideText = pickLocalized(setupGuide, locale); const [phase, setPhase] = useState({ kind: 'idle' }); + // OM-17 — the whole validation entry, not just its `message`: `FieldRow` + // needs the `code` to recognise a `pattern_mismatch` and swap the server's + // English hint for the localized one out of the manifest. const [fieldErrors, setFieldErrors] = useState< - Record + Record >({}); const drawerOpen = @@ -314,7 +321,7 @@ export function InstallButton({ function applyDetails(details: unknown): void { if (!Array.isArray(details)) return; - const next: Record = {}; + const next: Record = {}; for (const entry of details as InstallValidationError[]) { if ( entry && @@ -322,7 +329,10 @@ export function InstallButton({ typeof entry.key === 'string' && typeof entry.message === 'string' ) { - next[entry.key] = entry.message; + next[entry.key] = + typeof entry.code === 'string' + ? { code: entry.code, message: entry.message } + : { message: entry.message }; } } setFieldErrors(next); @@ -611,7 +621,7 @@ function InstalledPanel({ interface InstallDrawerProps { phase: Phase; pluginName: string; - fieldErrors: Record; + fieldErrors: Record; onClose: () => void; onSubmit: (values: Record) => void | Promise; /** Markdown setup guide rendered above the fields. */ diff --git a/web-ui/app/_components/store/RequiresWizard.tsx b/web-ui/app/_components/store/RequiresWizard.tsx index 2525f8f7..43222fc9 100644 --- a/web-ui/app/_components/store/RequiresWizard.tsx +++ b/web-ui/app/_components/store/RequiresWizard.tsx @@ -19,7 +19,11 @@ import type { UnresolvedCapabilityEntry, } from '../../_lib/storeTypes'; import { Chip } from './Chip'; -import { FieldRow, extractValues } from './setupForm'; +import { + FieldRow, + extractValues, + type SetupFieldError, +} from './setupForm'; import { Button } from '@/app/_components/ui/Button'; /** @@ -95,7 +99,9 @@ export function RequiresWizard({ initialSelections, ); const [phase, setPhase] = useState({ kind: 'review' }); - const [fieldErrors, setFieldErrors] = useState>({}); + const [fieldErrors, setFieldErrors] = useState< + Record + >({}); const formRef = useRef(null); // Promise-resolver for the inline pause: when a provider needs setup // input, the install-loop awaits this before continuing. @@ -479,7 +485,7 @@ function InstallingBody({ onFormSubmit, }: { phase: Extract; - fieldErrors: Record; + fieldErrors: Record; formRef: React.MutableRefObject; onFormSubmit: (values: Record) => void; }): React.ReactElement { diff --git a/web-ui/app/_components/store/__tests__/CredentialsEditorPassword.test.tsx b/web-ui/app/_components/store/__tests__/CredentialsEditorPassword.test.tsx index 8ca713a8..1e5889b8 100644 --- a/web-ui/app/_components/store/__tests__/CredentialsEditorPassword.test.tsx +++ b/web-ui/app/_components/store/__tests__/CredentialsEditorPassword.test.tsx @@ -1,6 +1,7 @@ import { fireEvent, screen, waitFor } from '@testing-library/react'; import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { ApiError } from '../../../_lib/api'; import { renderWithIntl } from '../../../_lib/test-utils'; import { CredentialsEditor } from '../CredentialsEditor'; import type { PluginSetupField } from '../../../_lib/storeTypes'; @@ -153,6 +154,92 @@ describe(' — OM-17 password misuse guard', () => { }); }); + it('surfaces the GERMAN hint when the SERVER rejects the value', async () => { + // The server 400 is the fallback path (the client blocks save first), but + // it is reachable — install wizard, API clients, any route where the + // client check is bypassed. The middleware has no request locale, so its + // `hint` is always English; a German operator must still read German. + // "English field labels and help texts in a German UI" was itself a named + // contributing factor of OM-17. + const EN = 'expects a service account address, not a person'; + const DE = 'erwartet eine Dienstkonto-Adresse, kein Personenkonto'; + mockPatchSecrets.mockRejectedValueOnce( + new ApiError( + 400, + 'Bad Request', + JSON.stringify({ + code: 'runtime.setup_field_invalid', + message: "value for 'gw_sa_client_email' does not match the expected format", + field: 'gw_sa_client_email', + hint: EN, + }), + ), + ); + + renderWithIntl( + , + { locale: 'de' }, + ); + + const input = await screen.findByRole('textbox'); + fireEvent.change(input, { target: { value: 'tester@customer-company.de' } }); + + const save = await screen.findByRole('button', { name: /Speichern/i }); + await waitFor(() => { + expect((save as HTMLButtonElement).disabled).toBe(false); + }); + fireEvent.click(save); + + await waitFor(() => { + expect(screen.getByText(new RegExp(DE))).toBeTruthy(); + }); + // …and NOT the English sentence the server actually sent. + expect(screen.queryByText(new RegExp(EN))).toBeNull(); + }); + + it('falls back to the SERVER hint for a field it does not know', async () => { + // A manifest newer than this page: an English sentence beats none at all. + const EN = 'expects a service account address'; + mockPatchSecrets.mockRejectedValueOnce( + new ApiError( + 400, + 'Bad Request', + JSON.stringify({ + code: 'runtime.setup_field_invalid', + field: 'a_key_this_page_never_rendered', + hint: EN, + }), + ), + ); + + renderWithIntl( + , + { locale: 'de' }, + ); + + const input = await screen.findByRole('textbox'); + fireEvent.change(input, { target: { value: 'anything' } }); + fireEvent.click(await screen.findByRole('button', { name: /Speichern/i })); + + await waitFor(() => { + expect(screen.getByText(new RegExp(EN))).toBeTruthy(); + }); + }); + it('shows the manifest placeholder when nothing is stored', async () => { // The manifest already declared `placeholder`; both renderers threw it away // and showed state-derived text (or a row of bullets) instead — hiding the diff --git a/web-ui/app/_components/store/__tests__/setupFormPatternHint.test.tsx b/web-ui/app/_components/store/__tests__/setupFormPatternHint.test.tsx new file mode 100644 index 00000000..a25b78c8 --- /dev/null +++ b/web-ui/app/_components/store/__tests__/setupFormPatternHint.test.tsx @@ -0,0 +1,100 @@ +import { screen } from '@testing-library/react'; +import { describe, expect, it } from 'vitest'; + +import { renderWithIntl } from '../../../_lib/test-utils'; +import { FieldRow } from '../setupForm'; +import type { InstallSetupField } from '../../../_lib/storeTypes'; + +/** + * OM-17 follow-up — the INSTALL WIZARD half of "the server can only ever send + * an English pattern_hint". + * + * `installService` rejects a mismatching value with + * `{ key, code: 'pattern_mismatch', message }`, where `message` IS the + * manifest's `pattern_hint` resolved to English: the middleware has no request + * locale, so it cannot resolve anything else. A German operator installing a + * plugin therefore read an English sentence in the one place that was supposed + * to stop them typing their Google account password — and + * "English field labels and help texts in a German UI" was itself a named + * contributing factor of OM-17. + * + * `FieldRow` holds the whole `{ locale: text }` map (it renders it under the + * input already), so it does the locale pick itself. No API change. + */ + +const EN = 'expects a service account address, not a person'; +const DE = 'erwartet eine Dienstkonto-Adresse, kein Personenkonto'; + +function field(over: Partial = {}): InstallSetupField { + return { + key: 'gw_sa_client_email', + label: 'Service account email', + type: 'string', + required: true, + pattern_hint: { en: EN, de: DE }, + ...over, + } as InstallSetupField; +} + +describe(' — a pattern rejection is shown in the ACTIVE locale', () => { + it('renders the German hint as the error for a German operator', () => { + renderWithIntl( + , + { locale: 'de' }, + ); + + // The error slot specifically — the static hint under the input also + // carries this text, so asserting "somewhere in the DOM" would prove + // nothing about the rejection. + expect(screen.getByRole('alert').textContent).toBe(DE); + // And the English sentence the server actually sent is nowhere on screen. + expect(screen.queryByText(EN)).toBeNull(); + }); + + it('renders the English hint as the error for an English operator', () => { + renderWithIntl( + , + { locale: 'en' }, + ); + + expect(screen.getByRole('alert').textContent).toBe(EN); + }); + + it('keeps the server message when the manifest declared no pattern_hint', () => { + // `installService` falls back to its own generic sentence in that case; + // the client has nothing better and must not swallow it. + const generic = '"Service account email" entspricht nicht dem erwarteten Muster.'; + renderWithIntl( + , + { locale: 'de' }, + ); + + expect(screen.getByRole('alert').textContent).toBe(generic); + }); + + it('leaves every OTHER error code untouched', () => { + // Only the pattern code carries manifest-owned prose. A `required` or + // `wrong_type` message must be rendered verbatim, hint or no hint. + const required = 'Feld "Service account email" ist erforderlich.'; + renderWithIntl( + , + { locale: 'de' }, + ); + + expect(screen.getByRole('alert').textContent).toBe(required); + }); + + it('renders no error slot at all when there is no error', () => { + renderWithIntl(, { locale: 'de' }); + expect(screen.queryByRole('alert')).toBeNull(); + }); +}); diff --git a/web-ui/app/_components/store/setupForm.tsx b/web-ui/app/_components/store/setupForm.tsx index 6fa621ee..af77585c 100644 --- a/web-ui/app/_components/store/setupForm.tsx +++ b/web-ui/app/_components/store/setupForm.tsx @@ -15,19 +15,44 @@ import type { InstallSetupField } from '../../_lib/storeTypes'; * coercion rules, same secret/url/integer/enum/boolean handling. */ +/** + * One server-side validation failure for one field, as the install API reports + * it (`details: [{ key, code, message }]`). + * + * The `code` is carried through — rather than flattening to the message string + * on arrival — so this component can tell a `pattern_mismatch` apart from the + * other codes. It has to: for a pattern mismatch the server's `message` IS the + * manifest's `pattern_hint`, resolved to English because the middleware has no + * request locale. We hold the whole localized map and render it under this very + * input, so we can do better. See `resolveSetupFieldHint`. + */ +export interface SetupFieldError { + code?: string; + message: string; +} + export function FieldRow({ field, error, idPrefix = 'install-field', }: { field: InstallSetupField; - error?: string; + error?: SetupFieldError; idPrefix?: string; }): React.ReactElement { const t = useTranslations('store.setupForm'); const locale = useLocale(); const id = `${idPrefix}-${field.key}`; const patternHint = pickLocalized(field.pattern_hint, locale); + // OM-17 — a German operator must not read an English rejection. Only the + // pattern code is overridden: every other install error is either already a + // catalog string or a value-shape message the manifest cannot explain. + const errorText = + error === undefined + ? undefined + : error.code === 'pattern_mismatch' && patternHint + ? patternHint + : error.message; // OM-17 — honour the manifest placeholder. The hardcoded `••••••••` told the // operator only "this is masked", which is exactly the signal that reads as // "type your password here". A manifest that says what shape it wants gets to @@ -195,9 +220,12 @@ export function FieldRow({ {field.help}

) : null} - {error ? ( -

- {error} + {errorText ? ( +

+ {errorText}

) : null} diff --git a/web-ui/app/_lib/__tests__/setupFieldPattern.test.ts b/web-ui/app/_lib/__tests__/setupFieldPattern.test.ts index ca86234d..19e282f6 100644 --- a/web-ui/app/_lib/__tests__/setupFieldPattern.test.ts +++ b/web-ui/app/_lib/__tests__/setupFieldPattern.test.ts @@ -4,6 +4,7 @@ import { anchorPatternSource, isPatternUsable, nativePatternAttribute, + resolveSetupFieldHint, screenPatternSource, violatesSetupPattern, } from '../setupFieldPattern'; @@ -108,3 +109,121 @@ describe('F4 — anchoring agrees with the server and with HTML `pattern=`', () expect(nativePatternAttribute(undefined)).toBeUndefined(); }); }); + +/** + * F5 — the screen refused `{n,}` while accepting `+`, which IS `{1,}`. + * + * This half matters as much as the server's: when only the server accepted + * `{2,}`, the client would call the pattern unusable, emit no native + * `pattern=` attribute, and let `violatesSetupPattern` fail OPEN — the operator + * would type a bad value, see no error, hit Save, and get a 400 from a check + * the client had silently opted out of. Keep the two grammars identical. + */ +describe('F5 — counted repetition is screened exactly like `+` and `*`', () => { + const EMAIL_TLD = '^[^@\\s]+@[^@\\s]+\\.[A-Za-z]{2,}$'; + + it.each([ + '^[A-Za-z]{2,}$', + '^[A-Za-z]{2,63}$', + '^a{2,}$', + '^[a-z]{0,}$', + '^a{100,}$', + EMAIL_TLD, + ])('accepts %s', (pattern) => { + expect(screenPatternSource(pattern)).toBeNull(); + expect(isPatternUsable(pattern)).toBe(true); + }); + + it.each([ + '^a{101}$', + '^a{101,}$', + '^a{1,101}$', + // The hostile shapes, respelled with `{n,}` — the counted spelling must not + // open a door the `+` spelling keeps shut. + '^(a|a){1,}$', + '^(a{1,})+$', + '^(a{1,}){1,}$', + '^(?:a|a){2,}$', + ])('still rejects %s', (pattern) => { + expect(screenPatternSource(pattern)).not.toBeNull(); + expect(isPatternUsable(pattern)).toBe(false); + }); + + it('an accepted `{n,}` pattern MATCHES correctly, it does not merely compile', () => { + // Compiling is not the bar. A pattern that is accepted but fails open is + // worse than one that is refused, because nothing tells the operator. + expect(violatesSetupPattern({ pattern: EMAIL_TLD }, 'tester@customer-company.de')).toBe(false); + expect(violatesSetupPattern({ pattern: EMAIL_TLD }, 'ops@example.technology')).toBe(false); + // `{2,}` really is a minimum of two… + expect(violatesSetupPattern({ pattern: EMAIL_TLD }, 'ops@example.x')).toBe(true); + // …and this is the value the OM-17 tester actually typed. + expect(violatesSetupPattern({ pattern: EMAIL_TLD }, 'hunter2')).toBe(true); + // A usable pattern also reaches the browser as a native attribute. + expect(nativePatternAttribute(EMAIL_TLD)).toBe(EMAIL_TLD); + }); +}); + +/** + * OM-17 follow-up — the server can only ever send an ENGLISH `pattern_hint`. + * + * The middleware has no request locale (nothing reads `Accept-Language`, + * `NEXT_LOCALE` never leaves the Next.js layer), so a German operator hitting + * the server check read an English sentence. "English field labels and help + * texts in a German UI" was itself a named contributing factor of OM-17. + */ +describe('resolveSetupFieldHint — the CLIENT owns the locale pick', () => { + const FIELDS = [ + { + key: 'gw_sa_client_email', + pattern_hint: { + en: 'expects …@….iam.gserviceaccount.com', + de: 'erwartet …@….iam.gserviceaccount.com', + }, + }, + ]; + + it('prefers the German hint over the English one the server sent', () => { + expect( + resolveSetupFieldHint( + FIELDS, + 'gw_sa_client_email', + 'expects …@….iam.gserviceaccount.com', + 'de', + ), + ).toBe('erwartet …@….iam.gserviceaccount.com'); + }); + + it('returns the English hint for an English operator', () => { + expect( + resolveSetupFieldHint(FIELDS, 'gw_sa_client_email', 'ignored', 'en'), + ).toBe('expects …@….iam.gserviceaccount.com'); + }); + + it('falls back to the server hint for a key it does not know', () => { + // A field the client never rendered (a manifest newer than this page). + // The English sentence beats no sentence at all. + expect( + resolveSetupFieldHint(FIELDS, 'some_other_key', 'server says this', 'de'), + ).toBe('server says this'); + expect( + resolveSetupFieldHint(FIELDS, undefined, 'server says this', 'de'), + ).toBe('server says this'); + }); + + it('returns undefined when neither side has a hint', () => { + expect( + resolveSetupFieldHint([{ key: 'k' }], 'k', undefined, 'de'), + ).toBeUndefined(); + }); + + it('falls back across locales when the manifest omits the active one', () => { + expect( + resolveSetupFieldHint( + [{ key: 'k', pattern_hint: { en: 'only english' } }], + 'k', + undefined, + 'de', + ), + ).toBe('only english'); + }); +}); diff --git a/web-ui/app/_lib/setupFieldPattern.ts b/web-ui/app/_lib/setupFieldPattern.ts index 3285e0e2..9373c8eb 100644 --- a/web-ui/app/_lib/setupFieldPattern.ts +++ b/web-ui/app/_lib/setupFieldPattern.ts @@ -25,6 +25,7 @@ * and has been replaced. See the server module header for the full rationale. */ +import { pickLocalized } from './localized'; import type { PluginSetupField } from './storeTypes'; /** Mirrors the server's `MAX_PATTERN_SOURCE_LENGTH`. */ @@ -42,6 +43,7 @@ const MAX_COUNTED_REPETITION = 100; interface QuantifierToken { readonly length: number; readonly counted: boolean; + readonly min?: number; readonly max?: number; } @@ -65,13 +67,18 @@ function parseQuantifier(src: string, i: number): QuantifierToken | null { ? undefined : Number(maxRaw); return max === undefined - ? { length, counted: true } - : { length, counted: true, max }; + ? { length, counted: true, min } + : { length, counted: true, min, max }; } +/** + * Size cap only — shape is the group-content rules' job, and they treat `{n,}` + * exactly like the `+` it is equivalent to. Both bounds are capped because for + * `{n,}` the minimum is the only number the manifest supplies. + */ function checkCountedBounds(q: QuantifierToken): string | null { - if (q.max === undefined) return 'open-ended counted repetition'; - if (q.max > MAX_COUNTED_REPETITION) return 'counted repetition too large'; + const largest = Math.max(q.min ?? 0, q.max ?? 0); + if (largest > MAX_COUNTED_REPETITION) return 'counted repetition too large'; return null; } @@ -304,3 +311,50 @@ export function violatesSetupPattern( regex.lastIndex = 0; return !regex.test(value); } + +// --------------------------------------------------------------------------- +// Localizing the server's pattern rejection +// --------------------------------------------------------------------------- + +/** The subset of a setup field the hint resolution needs. Structurally shared by + * `PluginSetupField` (post-install editor) and `InstallSetupField` (wizard). */ +interface HintableField { + key: string; + pattern_hint?: Record | undefined; +} + +/** + * Resolve the operator-facing text for a server-side pattern rejection, in the + * ACTIVE locale. + * + * WHY THIS EXISTS. The middleware has no request locale — nothing there reads + * `Accept-Language` and `NEXT_LOCALE` never leaves the Next.js layer — so its + * `hint` is always the English entry of the manifest's `pattern_hint` map. A + * German operator hitting the server check (install wizard, an API client, any + * route where the client-side check is bypassed) got an English sentence. + * "English field labels and help texts in a German UI" was itself one of the + * named contributing factors of OM-17, so shipping the OM-17 fix that way would + * have been an own goal. + * + * The fix needs no API change: we are holding the whole `{ locale: text }` map + * already — the editor and the wizard both render it under the input — so we + * pick from it ourselves, keyed on the `field`/`key` the server named. The + * server's English `hint` stays the fallback for the one case where that cannot + * work: a key matching no field this client knows about. + * + * @param fields the manifest setup fields this view is rendering + * @param fieldKey the offending key as named by the server (may be unknown) + * @param serverHint the server's English hint, used only as a fallback + * @param locale the active UI locale + */ +export function resolveSetupFieldHint( + fields: ReadonlyArray, + fieldKey: string | undefined, + serverHint: string | undefined, + locale: string, +): string | undefined { + const field = fieldKey + ? fields.find((f) => f.key === fieldKey) + : undefined; + return pickLocalized(field?.pattern_hint, locale) ?? serverHint; +}