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; +}