Skip to content

Commit 85d2d10

Browse files
JeanMecheatscott
authored andcommitted
fix(forms): harden FormGroup control lookups against prototype shadowing
Guard FormGroup control-map presence checks with safe own-property checks to avoid inherited/prototype collisions from reserved keys such as hasOwnProperty and toString. This prevents: - crashes from shadowed hasOwnProperty access paths - incorrect early-return and existence behavior for prototype-named controls Adds regression tests for prototype-shadowed keys covering: - register/add with toString - contains/get with hasOwnProperty - setControl/removeControl with toString - FormRecord behavior with hasOwnProperty (cherry picked from commit f06b96d)
1 parent 6e3d51d commit 85d2d10

5 files changed

Lines changed: 86 additions & 16 deletions

File tree

packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -673,6 +673,7 @@
673673
"hasModelInput",
674674
"hasOnDestroy",
675675
"hasOutput",
676+
"hasOwnControl",
676677
"hasParentInjector",
677678
"hasStyleInput",
678679
"hasStylingInputShadow",

packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -668,6 +668,7 @@
668668
"hasModelInput",
669669
"hasOnDestroy",
670670
"hasOutput",
671+
"hasOwnControl",
671672
"hasParentInjector",
672673
"hasStyleInput",
673674
"hasStylingInputShadow",

packages/forms/src/model/abstract_model.ts

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,12 @@
77
*/
88

99
import {
10+
computed,
1011
EventEmitter,
11-
signal,
1212
ɵRuntimeError as RuntimeError,
13-
ɵWritable as Writable,
13+
signal,
1414
untracked,
15-
computed,
15+
ɵWritable as Writable,
1616
} from '@angular/core';
1717
import {Observable, Subject} from 'rxjs';
1818

@@ -265,15 +265,15 @@ export function isOptionsObj(
265265
}
266266

267267
export function assertControlPresent(parent: any, isGroup: boolean, key: string | number): void {
268-
const controls = parent.controls as {[key: string | number]: unknown};
268+
const controls = parent.controls as {[key: string | number]: AbstractControl<any>};
269269
const collection = isGroup ? Object.keys(controls) : controls;
270270
if (!collection.length) {
271271
throw new RuntimeError(
272272
RuntimeErrorCode.NO_CONTROLS,
273273
typeof ngDevMode === 'undefined' || ngDevMode ? noControlsError(isGroup) : '',
274274
);
275275
}
276-
if (!controls[key]) {
276+
if (!hasOwnControl(controls, key)) {
277277
throw new RuntimeError(
278278
RuntimeErrorCode.MISSING_CONTROL,
279279
typeof ngDevMode === 'undefined' || ngDevMode ? missingControlError(isGroup, key) : '',
@@ -1807,3 +1807,10 @@ export abstract class AbstractControl<
18071807
untracked(() => this._hasRequired.set(this.hasValidator(Validators.required)));
18081808
}
18091809
}
1810+
1811+
export function hasOwnControl(
1812+
controls: {[key: string]: AbstractControl<any>},
1813+
name: string | number | symbol,
1814+
): boolean {
1815+
return Object.hasOwn(controls, name);
1816+
}

packages/forms/src/model/form_group.ts

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import {
1616
assertAllValuesPresent,
1717
assertControlPresent,
1818
FormResetEvent,
19+
hasOwnControl,
1920
pickAsyncValidators,
2021
pickValidators,
2122
ɵRawValue,
@@ -245,7 +246,8 @@ export class FormGroup<
245246
): AbstractControl<any>;
246247

247248
registerControl<K extends string & keyof TControl>(name: K, control: TControl[K]): TControl[K] {
248-
if (this.controls[name]) return (this.controls as any)[name];
249+
const existingControl = this._find(name as string);
250+
if (existingControl) return existingControl as TControl[K];
249251
this.controls[name] = control;
250252
control.setParent(this as FormGroup);
251253
control._registerOnCollectionChange(this._onCollectionChange);
@@ -322,8 +324,8 @@ export class FormGroup<
322324
* removed. When false, no events are emitted.
323325
*/
324326
removeControl(name: string, options: {emitEvent?: boolean} = {}): void {
325-
if ((this.controls as any)[name])
326-
(this.controls as any)[name]._registerOnCollectionChange(() => {});
327+
const existingControl = this._find(name);
328+
if (existingControl) existingControl._registerOnCollectionChange(() => {});
327329
delete (this.controls as any)[name];
328330
this.updateValueAndValidity({emitEvent: options.emitEvent});
329331
this._onCollectionChange();
@@ -364,7 +366,8 @@ export class FormGroup<
364366
emitEvent?: boolean;
365367
} = {},
366368
): void {
367-
if (this.controls[name]) this.controls[name]._registerOnCollectionChange(() => {});
369+
const existingControl = this._find(name as string);
370+
if (existingControl) existingControl._registerOnCollectionChange(() => {});
368371
delete this.controls[name];
369372
if (control) this.registerControl(name, control);
370373
this.updateValueAndValidity({emitEvent: options.emitEvent});
@@ -385,7 +388,7 @@ export class FormGroup<
385388
contains(this: FormGroup<{[key: string]: AbstractControl<any>}>, controlName: string): boolean;
386389

387390
contains<K extends string & keyof TControl>(controlName: K): boolean {
388-
return this.controls.hasOwnProperty(controlName) && this.controls[controlName].enabled;
391+
return this._find(controlName)?.enabled === true;
389392
}
390393

391394
/**
@@ -487,11 +490,9 @@ export class FormGroup<
487490
// `undefined` as a value.
488491
if (value == null /* both `null` and `undefined` */) return;
489492
(Object.keys(value) as Array<keyof TControl>).forEach((name) => {
490-
// The compiler cannot see through the uninstantiated conditional type of `this.controls`, so
491-
// `as any` is required.
492-
const control = (this.controls as any)[name];
493-
if (control) {
494-
control.patchValue(
493+
const existingControl = this._find(name as string);
494+
if (existingControl) {
495+
existingControl.patchValue(
495496
/* Guaranteed to be present, due to the outer forEach. */ value[
496497
name as keyof ɵFormGroupValue<TControl>
497498
]!,
@@ -664,7 +665,7 @@ export class FormGroup<
664665

665666
/** @internal */
666667
override _find(name: string | number): AbstractControl | null {
667-
return this.controls.hasOwnProperty(name as string)
668+
return hasOwnControl(this.controls, name as string)
668669
? (this.controls as any)[name as keyof TControl]
669670
: null;
670671
}

packages/forms/test/form_group_spec.ts

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import {
1717
FormArray,
1818
FormControl,
1919
FormGroup,
20+
FormRecord,
2021
NG_VALUE_ACCESSOR,
2122
ReactiveFormsModule,
2223
ValidationErrors,
@@ -950,6 +951,65 @@ import {
950951
});
951952
});
952953

954+
describe('prototype-shadowed control names', () => {
955+
it('should register a control named toString', () => {
956+
const group: FormGroup = new FormGroup({});
957+
958+
group.addControl('toString', new FormControl('', Validators.required));
959+
960+
expect(group.contains('toString')).toBe(true);
961+
expect(group.get('toString')).toBe(group.controls['toString']);
962+
expect(group.valid).toBe(false);
963+
});
964+
965+
it('should support contains and get for hasOwnProperty controls', () => {
966+
const group = new FormGroup({'hasOwnProperty': new FormControl('value')});
967+
968+
expect(group.contains('hasOwnProperty')).toBe(true);
969+
expect(group.get('hasOwnProperty')).toBe(group.controls['hasOwnProperty']);
970+
});
971+
972+
it('should support setControl and removeControl for toString', () => {
973+
const group: FormGroup = new FormGroup({});
974+
975+
group.setControl('toString', new FormControl('value'));
976+
expect((group.get('toString') as FormControl).value).toBe('value');
977+
978+
group.removeControl('toString');
979+
expect(group.get('toString')).toBe(null);
980+
});
981+
982+
it('should support hasOwnProperty controls in FormRecord', () => {
983+
const record = new FormRecord<FormControl<string | null>>({});
984+
985+
record.addControl('hasOwnProperty', new FormControl('value'));
986+
987+
expect(record.contains('hasOwnProperty')).toBe(true);
988+
expect(record.get('hasOwnProperty')?.value).toBe('value');
989+
});
990+
991+
it('should not crash when patching a shadowed control name', () => {
992+
const group = new FormGroup({});
993+
expect(() => group.patchValue({toString: 'value'})).not.toThrow();
994+
expect(group.get('toString')).toBe(null);
995+
});
996+
997+
it('should throw no-controls error when setting value on an empty group with a shadowed key', () => {
998+
const group = new FormGroup({});
999+
expect(() => group.setValue({toString: 'value'})).toThrowError(
1000+
new RegExp(`no form controls registered with this group`),
1001+
);
1002+
});
1003+
1004+
it('should throw missing-control error (not crash) for shadowed key in setValue', () => {
1005+
const group = new FormGroup({'one': new FormControl('')});
1006+
1007+
expect(() => group.setValue({one: 'v', toString: 'value'} as any)).toThrowError(
1008+
new RegExp(`NG01001: Cannot find form control with name: 'toString'`),
1009+
);
1010+
});
1011+
});
1012+
9531013
describe('statusChanges', () => {
9541014
let control: FormControl;
9551015
let group: FormGroup;

0 commit comments

Comments
 (0)