Skip to content

Commit 20c1fe1

Browse files
eliran-opseliran-micnadaverell
authored
fix(forms): validate Helm release name, ignored namespace, and port-forward port (#573)
* fix(forms): validate Helm release name, ignored namespace, and port-forward port Five reported form-validation bugs (SKY-821) addressed by introducing a shared, pure validators module and wiring it into the three offending dialogs: 24. Helm install dialog accepted release names like "Invalid Name With Spaces!" and proceeded to step 2; the install only failed server-side, leaving users with an opaque 422. 25. Audit Settings → Add Ignored Namespace accepted "INVALID_NAME!@#" with no validation; the audit pipeline then silently never matched the bogus entry, so the user thought the ignore filter "didn't work". 26. Port-Forward dialog: the local-port `<input type="number">` swallowed values like "0", "-1", and "70000" with no feedback — the typed value disappeared or truncated mid-typing. 27. The port input gave no rejection feedback at all; rejected values just vanished. 63. Audit Settings 'Save' button stayed enabled when the only pending edit was an invalid namespace string (would silently drop the user's entry on save). New module `packages/k8s-ui/src/utils/validators.ts` with: - `validateRFC1123Label` — k8s namespace / pod / service names - `validateRFC1123Subdomain` — dotted DNS subdomains - `validateHelmReleaseName` — Helm's 53-char limit on top of subdomain - `validatePort` — strict integer in [1, 65535]; rejects "0", "-1", "1.0", "1e3", "+1", "70000", whitespace-padded values, anything with non-digit characters 29 unit tests pin the canonical k8s / Helm rules and cover every fixture from SKY-821 verbatim, plus boundaries (63-char label limit, 53-char Helm limit) and adversarial inputs (decimals, scientific, truncation). Wiring: - `InstallWizard.tsx`: validates release name + namespace via memoized validators; inline red border + error text under each input; `canProceedFromInfo` now requires both validations to pass; preserves existing helper text when input is valid. - `AuditSettingsDialog.tsx`: validates the staged-but-unsubmitted namespace input; disables the Add button on invalid or duplicate input; shows an inline red error; disables Save when there's pending invalid input (with a tooltip explaining why). - `PortForwardButton.tsx`: tracks the raw input separately from the validated port so the user always sees the characters they typed (no more silent swallow); shows a red border + inline error when the input is rejected; falls back to the original remote port for the built kubectl command so the dialog always shows a runnable command. Linear: SKY-821 Made-with: Cursor * fix(helm): submit the trimmed releaseName/namespace we actually validated Cursor Bugbot caught: the form validates `releaseName.trim()` and `namespace.trim()` but `handleInstall` previously sent the raw untrimmed values to the server. A user typing "my-release " (trailing space) would pass green client-side validation and then get a confusing server-side rejection — the exact class of bug this PR exists to prevent. Trim once, locally, before the submit call (and reuse the trimmed strings in the success callback's namespace/release-name arguments). Made-with: Cursor * chore(InstallWizard): strip CLAUDE.md comment-rot trailers Made-with: Cursor * fix(validators): per-label 63-char check; drop trailing period Bugbot follow-ups on PR #573. 1. validateHelmReleaseName length error trailed with a period. The ValidationResult contract is 'no trailing period — call sites compose them into sentences', and InstallWizard does exactly that ('Release name {error}.'), producing '...limit..' in the UI when a user typed > 53 chars. 2. validateRFC1123Subdomain only checked total length (≤ 253) and the 'segments' regex, so a single 200-char label passed client-side and was rejected only server-side. K8s' IsDNS1123Subdomain enforces ≤ 63 PER label on top of the 253 total. Add the per-label check and pin it under 'each dot-separated label must be at most 63 characters'. Made-with: Cursor * fix(validators): reject dots in helm release names Helm itself permits dots in release names but generated resource names like `<release>-<chart>-<hash>` must be valid DNS-1123 labels (no dots). A dotted release like `my.app` passes Helm's validation, then K8s rejects the resource apply server-side with an opaque 422. Switch validateHelmReleaseName to delegate to validateRFC1123Label so the dot is caught with clear inline feedback before submit. Cursor bugbot finding (validators.ts:126). * review: gate Save on duplicate namespace; strip remaining ticket refs The audit dialog showed a red 'already in the list' error for duplicate namespace input but didn't disable Save — the user could click Save and silently lose the pending entry. Save now blocks on newNsDuplicate as well. Also strips SKY-821 / 'bug NN' / Bugbot references from validators, test descriptions, and the port-forward + audit comment blocks (commit 362a6bf only cleaned InstallWizard.tsx). --------- Co-authored-by: eliran-mic <eliran.mic@gmail.com> Co-authored-by: Nadav Erell <nadaverell@gmail.com>
1 parent 31f8118 commit 20c1fe1

6 files changed

Lines changed: 538 additions & 35 deletions

File tree

packages/k8s-ui/src/utils/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,3 +13,4 @@ export * from './skeleton-yaml'
1313
export * from './k8s-errors'
1414
export * from './parse-go-time'
1515
export * from './view-transition'
16+
export * from './validators'
Lines changed: 207 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,207 @@
1+
import { describe, it, expect } from 'vitest'
2+
import {
3+
validateRFC1123Label,
4+
validateRFC1123Subdomain,
5+
validateHelmReleaseName,
6+
validatePort,
7+
} from './validators'
8+
9+
// Validation in user-facing forms is a silent-correctness-or-loud-error
10+
// boundary: a regression here either lets bad input through to the API
11+
// (helm install fails with a server-side 422 the user can't connect to
12+
// their action) or rejects valid input (user is locked out of a
13+
// legitimate name). Pin the canonical k8s / Helm rules so any drift
14+
// fails the test instead of the user.
15+
16+
describe('validateRFC1123Label', () => {
17+
it('accepts simple lowercase labels', () => {
18+
expect(validateRFC1123Label('default').valid).toBe(true)
19+
expect(validateRFC1123Label('my-app').valid).toBe(true)
20+
expect(validateRFC1123Label('a').valid).toBe(true)
21+
expect(validateRFC1123Label('a1').valid).toBe(true)
22+
expect(validateRFC1123Label('1a').valid).toBe(true)
23+
})
24+
25+
it('rejects empty input', () => {
26+
const r = validateRFC1123Label('')
27+
expect(r.valid).toBe(false)
28+
if (!r.valid) expect(r.error).toMatch(/empty/)
29+
})
30+
31+
it('rejects uppercase characters and special chars', () => {
32+
const r = validateRFC1123Label('INVALID_NAME!@#')
33+
expect(r.valid).toBe(false)
34+
if (!r.valid) expect(r.error).toMatch(/lowercase/)
35+
})
36+
37+
it('rejects underscores, spaces, and other special characters', () => {
38+
expect(validateRFC1123Label('my_app').valid).toBe(false)
39+
expect(validateRFC1123Label('my app').valid).toBe(false)
40+
expect(validateRFC1123Label('my.app').valid).toBe(false)
41+
expect(validateRFC1123Label('my!app').valid).toBe(false)
42+
})
43+
44+
it('rejects names with leading or trailing hyphens', () => {
45+
expect(validateRFC1123Label('-app').valid).toBe(false)
46+
expect(validateRFC1123Label('app-').valid).toBe(false)
47+
})
48+
49+
it('rejects names longer than 63 characters', () => {
50+
const r = validateRFC1123Label('a'.repeat(64))
51+
expect(r.valid).toBe(false)
52+
if (!r.valid) expect(r.error).toMatch(/63/)
53+
})
54+
55+
it('accepts a name exactly 63 characters long (boundary)', () => {
56+
expect(validateRFC1123Label('a'.repeat(63)).valid).toBe(true)
57+
})
58+
})
59+
60+
describe('validateRFC1123Subdomain', () => {
61+
it('accepts dotted subdomains', () => {
62+
expect(validateRFC1123Subdomain('foo.bar').valid).toBe(true)
63+
expect(validateRFC1123Subdomain('foo.bar.baz').valid).toBe(true)
64+
expect(validateRFC1123Subdomain('foo').valid).toBe(true)
65+
})
66+
67+
it('rejects names with consecutive dots or trailing dots', () => {
68+
expect(validateRFC1123Subdomain('foo..bar').valid).toBe(false)
69+
expect(validateRFC1123Subdomain('foo.').valid).toBe(false)
70+
expect(validateRFC1123Subdomain('.foo').valid).toBe(false)
71+
})
72+
73+
it('rejects uppercase', () => {
74+
expect(validateRFC1123Subdomain('Foo.bar').valid).toBe(false)
75+
})
76+
77+
it('rejects names longer than 253 characters', () => {
78+
const seg = 'a'.repeat(60)
79+
const tooLong = `${seg}.${seg}.${seg}.${seg}.${seg}` // 60*5 + 4 dots = 304
80+
const r = validateRFC1123Subdomain(tooLong)
81+
expect(r.valid).toBe(false)
82+
if (!r.valid) expect(r.error).toMatch(/253/)
83+
})
84+
85+
it('rejects single labels longer than 63 chars even when total is under 253', () => {
86+
// K8s' IsDNS1123Subdomain enforces ≤ 63 chars per dot-
87+
// separated label on top of the 253-char total. Without the
88+
// per-label check, a single 200-char label slipped through
89+
// client-side and was rejected only server-side.
90+
const huge = 'a'.repeat(200)
91+
const r = validateRFC1123Subdomain(huge)
92+
expect(r.valid).toBe(false)
93+
if (!r.valid) expect(r.error).toMatch(/63/)
94+
})
95+
96+
it('accepts a 63-char label at the boundary', () => {
97+
expect(validateRFC1123Subdomain('a'.repeat(63)).valid).toBe(true)
98+
})
99+
100+
it('rejects only the offending label when other labels are fine', () => {
101+
const r = validateRFC1123Subdomain(`ok.${'a'.repeat(64)}.also-ok`)
102+
expect(r.valid).toBe(false)
103+
if (!r.valid) expect(r.error).toMatch(/63/)
104+
})
105+
})
106+
107+
describe('validateHelmReleaseName', () => {
108+
it('rejects names with spaces and special characters', () => {
109+
const r = validateHelmReleaseName('Invalid Name With Spaces!')
110+
expect(r.valid).toBe(false)
111+
})
112+
113+
it('accepts legal release names', () => {
114+
expect(validateHelmReleaseName('nginx').valid).toBe(true)
115+
expect(validateHelmReleaseName('my-nginx-1').valid).toBe(true)
116+
})
117+
118+
it('rejects release names longer than 53 characters', () => {
119+
const r = validateHelmReleaseName('a'.repeat(54))
120+
expect(r.valid).toBe(false)
121+
if (!r.valid) expect(r.error).toMatch(/53/)
122+
})
123+
124+
it('accepts a name exactly 53 characters long (boundary)', () => {
125+
expect(validateHelmReleaseName('a'.repeat(53)).valid).toBe(true)
126+
})
127+
128+
it('error string has no trailing period (call-site composition convention)', () => {
129+
// ValidationResult contract: error strings have no trailing
130+
// period — the caller composes them into sentences and adds
131+
// its own. InstallWizard renders `Release name {error}.` so a
132+
// trailing period in this error string produced "...limit.." in
133+
// the UI when a user exceeded the cap.
134+
const r = validateHelmReleaseName('a'.repeat(54))
135+
if (r.valid) throw new Error('expected invalid')
136+
expect(r.error.endsWith('.')).toBe(false)
137+
})
138+
139+
it('rejects dotted names (generated resource names must be DNS-1123 labels)', () => {
140+
// Helm itself permits dots, but generated resource names like
141+
// `<release>-<chart>-<hash>` must be valid labels. Catch the dot
142+
// up front instead of letting K8s reject the apply server-side.
143+
expect(validateHelmReleaseName('my.app').valid).toBe(false)
144+
expect(validateHelmReleaseName('foo.bar.baz').valid).toBe(false)
145+
})
146+
})
147+
148+
describe('validatePort', () => {
149+
it('accepts ports in [1, 65535]', () => {
150+
for (const p of [1, 80, 443, 8080, 65535]) {
151+
const r = validatePort(p)
152+
expect(r.valid).toBe(true)
153+
if (r.valid) expect(r.value).toBe(p)
154+
}
155+
})
156+
157+
it('accepts string ports', () => {
158+
const r = validatePort('8080')
159+
expect(r.valid).toBe(true)
160+
if (r.valid) expect(r.value).toBe(8080)
161+
})
162+
163+
it('rejects 0', () => {
164+
const r = validatePort('0')
165+
expect(r.valid).toBe(false)
166+
if (!r.valid) expect(r.error).toMatch(/0 is reserved|between 1/)
167+
})
168+
169+
it('rejects negative numbers', () => {
170+
const r = validatePort('-1')
171+
expect(r.valid).toBe(false)
172+
})
173+
174+
it('rejects ports above 65535', () => {
175+
const r = validatePort('70000')
176+
expect(r.valid).toBe(false)
177+
if (!r.valid) expect(r.error).toMatch(/65535|between/)
178+
})
179+
180+
it('rejects empty input', () => {
181+
expect(validatePort('').valid).toBe(false)
182+
})
183+
184+
it('rejects non-integer formats (decimals, scientific, signed, padded)', () => {
185+
expect(validatePort('1.0').valid).toBe(false)
186+
expect(validatePort('1e3').valid).toBe(false)
187+
expect(validatePort('+1').valid).toBe(false)
188+
expect(validatePort(' 80 ').valid).toBe(false)
189+
expect(validatePort('80abc').valid).toBe(false)
190+
})
191+
192+
it('does not silently truncate (the "70000 → 7000" maxlength bug)', () => {
193+
// The browser's number input was effectively dropping the 5th digit
194+
// on certain triple-select+type sequences. Our validator must NOT
195+
// accept the truncated value silently — it must reject the full
196+
// string the user typed.
197+
const r = validatePort('70000')
198+
expect(r.valid).toBe(false)
199+
// Make sure we don't accidentally massage the value into 7000 just
200+
// because that happens to be a valid port.
201+
if (!r.valid) {
202+
// No `value` field on the failure shape — guarantees we never
203+
// pretend the user typed something else.
204+
expect((r as unknown as { value?: number }).value).toBeUndefined()
205+
}
206+
})
207+
})
Lines changed: 178 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,178 @@
1+
// Pure validators for user-provided form inputs.
2+
//
3+
// Centralised here so the same rules apply to every form (Helm install
4+
// release name, Audit Settings ignored namespaces, Port Forward local
5+
// port, anything else that lands later). Reasons live next to the
6+
// constraints so reviewers can sanity-check us against the underlying
7+
// Kubernetes / Helm specs instead of folklore.
8+
9+
/**
10+
* Result shape for all validators. `valid: true` is success; otherwise
11+
* `error` is a short, user-readable explanation suitable for inline
12+
* form feedback (no leading capital, no trailing period — call sites
13+
* compose them into sentences).
14+
*/
15+
export type ValidationResult =
16+
| { valid: true }
17+
| { valid: false; error: string }
18+
19+
const RFC1123_LABEL_MAX = 63
20+
// Kubernetes namespace / pod / service names are RFC 1123 labels:
21+
// lowercase alphanumeric or '-', start and end alphanumeric, ≤63 chars.
22+
// Source: k8s.io/apimachinery/pkg/util/validation.IsDNS1123Label.
23+
const RFC1123_LABEL_RE = /^[a-z0-9]([-a-z0-9]*[a-z0-9])?$/
24+
25+
const RFC1123_SUBDOMAIN_MAX = 253
26+
// RFC 1123 subdomain = one or more labels joined by '.', total ≤253.
27+
const RFC1123_SUBDOMAIN_RE =
28+
/^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$/
29+
30+
const HELM_RELEASE_NAME_MAX = 53
31+
// Helm release names: DNS subdomain plus the additional limit of 53
32+
// chars so the generated resource names (`<release>-<chart>-<hash>`)
33+
// fit under the 63-char label cap. Source:
34+
// https://helm.sh/docs/chart_best_practices/conventions/#chart-names
35+
36+
/**
37+
* Validates a Kubernetes RFC 1123 label. Used for namespace names,
38+
* pod / service / configmap / secret names — anything Kubernetes
39+
* accepts as a single-segment identifier.
40+
*/
41+
export function validateRFC1123Label(input: string): ValidationResult {
42+
if (input.length === 0) {
43+
return { valid: false, error: 'must not be empty' }
44+
}
45+
if (input.length > RFC1123_LABEL_MAX) {
46+
return {
47+
valid: false,
48+
error: `must be at most ${RFC1123_LABEL_MAX} characters (got ${input.length})`,
49+
}
50+
}
51+
if (input !== input.toLowerCase()) {
52+
return {
53+
valid: false,
54+
error: 'must be lowercase (Kubernetes names are case-sensitive and only allow [a-z0-9-])',
55+
}
56+
}
57+
if (!RFC1123_LABEL_RE.test(input)) {
58+
return {
59+
valid: false,
60+
error: 'must contain only lowercase letters, numbers, and hyphens, and start/end with a letter or number',
61+
}
62+
}
63+
return { valid: true }
64+
}
65+
66+
/**
67+
* Validates a Kubernetes RFC 1123 subdomain (labels joined by dots).
68+
* Used for Helm chart names, image registries — anything that allows
69+
* dotted segments. NOT for pod/namespace names; use the label form.
70+
*/
71+
export function validateRFC1123Subdomain(input: string): ValidationResult {
72+
if (input.length === 0) {
73+
return { valid: false, error: 'must not be empty' }
74+
}
75+
if (input.length > RFC1123_SUBDOMAIN_MAX) {
76+
return {
77+
valid: false,
78+
error: `must be at most ${RFC1123_SUBDOMAIN_MAX} characters (got ${input.length})`,
79+
}
80+
}
81+
if (input !== input.toLowerCase()) {
82+
return {
83+
valid: false,
84+
error: 'must be lowercase',
85+
}
86+
}
87+
if (!RFC1123_SUBDOMAIN_RE.test(input)) {
88+
return {
89+
valid: false,
90+
error: 'must contain only lowercase letters, numbers, hyphens, and dots, and each segment must start/end with a letter or number',
91+
}
92+
}
93+
// Per-label cap: K8s' IsDNS1123Subdomain enforces ≤ 63 chars
94+
// PER dot-separated label on top of the 253-char total. The
95+
// regex above doesn't catch that; without this check a single
96+
// 200-char label passes here and only fails server-side.
97+
for (const label of input.split('.')) {
98+
if (label.length > RFC1123_LABEL_MAX) {
99+
return {
100+
valid: false,
101+
error: `each dot-separated label must be at most ${RFC1123_LABEL_MAX} characters (got ${label.length} for "${label}")`,
102+
}
103+
}
104+
}
105+
return { valid: true }
106+
}
107+
108+
/**
109+
* Validates a Helm release name. Same shape as an RFC 1123 label
110+
* (no dots) capped at 53 chars (Helm's hard limit; longer names
111+
* produce resources that exceed K8s' 63-char label limit).
112+
*
113+
* Why label and not subdomain: Helm itself permits dots in release
114+
* names, but most charts compose resource names as
115+
* `<release>-<chart>-<hash>` which must be valid DNS-1123 labels.
116+
* A dotted release name like `my.app` produces resources whose
117+
* names fail K8s label validation server-side. Reject the dot up
118+
* front so the user sees a clear inline error instead of a server
119+
* 422 after submit.
120+
*/
121+
export function validateHelmReleaseName(input: string): ValidationResult {
122+
if (input.length === 0) {
123+
return { valid: false, error: 'must not be empty' }
124+
}
125+
if (input.length > HELM_RELEASE_NAME_MAX) {
126+
return {
127+
valid: false,
128+
// No trailing period — call sites compose this into a sentence
129+
// and append their own.
130+
error: `must be at most ${HELM_RELEASE_NAME_MAX} characters (got ${input.length}); Helm caps release names so derived resource names fit under K8s' 63-char limit`,
131+
}
132+
}
133+
return validateRFC1123Label(input)
134+
}
135+
136+
/**
137+
* Validates a TCP/UDP port number provided as a free-text input.
138+
*
139+
* Accepts:
140+
* - a number-like string with no whitespace, no leading zeros, no '+'
141+
* - in [1, 65535]
142+
*
143+
* Rejects '0' (reserved), negative numbers, fractional numbers,
144+
* scientific notation, and anything containing non-digit characters.
145+
*
146+
* Returns the parsed integer alongside the validation result so call
147+
* sites don't have to re-parse.
148+
*/
149+
export type PortValidationResult =
150+
| { valid: true; value: number }
151+
| { valid: false; error: string }
152+
153+
export function validatePort(input: string | number): PortValidationResult {
154+
// Coerce a number argument to its decimal string form so the same
155+
// strict rules (no fractional, no NaN) apply regardless of how the
156+
// value reached us. We then re-parse to integer.
157+
const raw = typeof input === 'number' ? String(input) : input
158+
if (raw === '' || raw == null) {
159+
return { valid: false, error: 'port is required' }
160+
}
161+
// Reject anything other than digits — '+1', '1.0', '1e3', ' 80 '
162+
// all fail here. Catching them as parse failures gives clearer
163+
// feedback than silently truncating via Number().
164+
if (!/^\d+$/.test(raw)) {
165+
return {
166+
valid: false,
167+
error: 'port must be a whole number between 1 and 65535',
168+
}
169+
}
170+
const n = Number(raw)
171+
if (!Number.isInteger(n) || n < 1 || n > 65535) {
172+
return {
173+
valid: false,
174+
error: 'port must be between 1 and 65535 (0 is reserved)',
175+
}
176+
}
177+
return { valid: true, value: n }
178+
}

0 commit comments

Comments
 (0)