fix: Allow registering fields with reserved property names like 'constructor'#520
Conversation
📝 WalkthroughWalkthroughInternal maps and caches now use null-prototype objects to avoid prototype-key collisions (e.g., a field named Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/FinalForm.registerField.test.ts`:
- Around line 42-65: The test assumes field values are removed after calling
unregister(), but that only happens if destroyOnUnregister is true; update the
test to explicitly enable this by creating the form with destroyOnUnregister:
true (i.e., change the createForm call used in this spec to include
destroyOnUnregister: true), so form.registerField("constructor", ...) and
subsequent unregister() will clear values as asserted; reference symbols:
createForm, destroyOnUnregister, form.registerField, unregister, and
form.getState.
In `@src/FinalForm.ts`:
- Around line 189-193: state.fieldSubscribers must be a null‑prototype map like
fields/subscribers to avoid accidental truthy hits from Object.prototype (e.g.,
"constructor"); update the initializer in FinalForm (where state is created) to
set fieldSubscribers: Object.create(null) instead of {} (or alternatively change
registerField() to use
Object.prototype.hasOwnProperty.call(state.fieldSubscribers, name) when checking
existence). Make the change referencing state.fieldSubscribers and registerField
so the existing truthiness check no longer misdetects inherited properties.
- Around line 1206-1208: The current code deletes formState.submitErrors and
formState.submitError before running the beforeSubmit() gating, which clears
prior submit errors even when beforeSubmit() prevents submission; move the two
delete statements so they run only after the beforeSubmit check passes (i.e.,
after the code path that determines submission will proceed) to ensure submit
errors are only cleared on an actual submit attempt; locate the deletion lines
and the beforeSubmit invocation within the submit flow (functions/methods named
beforeSubmit and the formState object in FinalForm.ts) and reposition the delete
statements accordingly.
| // Only clear submit errors if we're actually proceeding with submission | ||
| delete formState.submitErrors; | ||
| delete formState.submitError; |
There was a problem hiding this comment.
Move submit‑error clearing after beforeSubmit gating.
If beforeSubmit() blocks submission, this currently clears prior submit errors even though a submission did not proceed. If the intent is “only clear on actual submission,” move these lines after the beforeSubmit check.
🛠️ Suggested adjustment
- // Only clear submit errors if we're actually proceeding with submission
- delete formState.submitErrors;
- delete formState.submitError;
const asyncValidationPromisesKeys = Object.keys(asyncValidationPromises);
if (asyncValidationPromisesKeys.length) {
// still waiting on async validation to complete...
return Promise.all(
@@
const submitIsBlocked = beforeSubmit();
if (submitIsBlocked) {
return Promise.resolve(undefined);
}
+ // Only clear submit errors if we're actually proceeding with submission
+ delete formState.submitErrors;
+ delete formState.submitError;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Only clear submit errors if we're actually proceeding with submission | |
| delete formState.submitErrors; | |
| delete formState.submitError; | |
| const asyncValidationPromisesKeys = Object.keys(asyncValidationPromises); | |
| if (asyncValidationPromisesKeys.length) { | |
| // still waiting on async validation to complete... | |
| return Promise.all( | |
| ... | |
| } | |
| const submitIsBlocked = beforeSubmit(); | |
| if (submitIsBlocked) { | |
| return Promise.resolve(undefined); | |
| } | |
| // Only clear submit errors if we're actually proceeding with submission | |
| delete formState.submitErrors; | |
| delete formState.submitError; |
🤖 Prompt for AI Agents
In `@src/FinalForm.ts` around lines 1206 - 1208, The current code deletes
formState.submitErrors and formState.submitError before running the
beforeSubmit() gating, which clears prior submit errors even when beforeSubmit()
prevents submission; move the two delete statements so they run only after the
beforeSubmit check passes (i.e., after the code path that determines submission
will proceed) to ensure submit errors are only cleared on an actual submit
attempt; locate the deletion lines and the beforeSubmit invocation within the
submit flow (functions/methods named beforeSubmit and the formState object in
FinalForm.ts) and reposition the delete statements accordingly.
There was a problem hiding this comment.
Valid observation, but this is pre-existing behavior unrelated to the constructor/reserved-property-names fix in this PR. The submit-error clearing logic was there before this change. If we want to fix it, it should be a separate ticket/PR. Not blocking this.
There was a problem hiding this comment.
@erikras-richard-agent, understood—you're right that this is pre-existing behavior and outside the scope of your constructor field fix. I appreciate the clarification.
If you'd like, I can open a tracking issue for the submit-error clearing logic so it's not lost. Just let me know. Otherwise, happy to move on! 👍
|
@erikras-gilfoyle-agent please fix and push. |
…tructor' Fixes #489 Problem: JavaScript object property access using bracket notation (fields['constructor']) conflicts with built-in Object.prototype properties like 'constructor', 'toString', '__proto__', etc. This made it impossible to register fields with these names. Root Cause: The state.fields object was initialized as a plain object literal ({}), which inherits from Object.prototype. Accessing fields['constructor'] would return the Object constructor function instead of allowing a custom field to be created. Solution: 1. Initialize state.fields using Object.create(null) to create an object without a prototype chain 2. Use Object.assign(Object.create(null), ...) when spreading fields to preserve the null prototype 3. Add comprehensive test coverage for reserved property names Changes: - src/FinalForm.ts: - Line 192: Change fields: {} to fields: Object.create(null) - Line 254: Use Object.assign(Object.create(null), ...) in renameField - src/FinalForm.registerField.test.ts: - Add test for registering 'constructor' field - Verify change, blur, focus handlers work correctly - Verify unregister works properly Impact: ✅ Allows fields named 'constructor', 'toString', etc. ✅ No breaking changes - Object.create(null) is fully compatible ✅ No performance impact ✅ Backward compatible with existing code # Conflicts: # src/FinalForm.registerField.test.ts
9831895 to
6e18289
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/FinalForm.ts (1)
266-270: 🛠️ Refactor suggestion | 🟠 MajorPreserve null-prototype for
fieldSubscribersin rename operation.Once
fieldSubscribersis initialized withObject.create(null)(per the fix needed at line 191), this spread-based rebuild will revert it to a regular object withObject.prototype. Apply the same pattern used forfieldsto maintain consistency.♻️ Suggested fix
- state.fieldSubscribers = { - ...state.fieldSubscribers, - [to]: state.fieldSubscribers[from], - }; + state.fieldSubscribers = Object.assign(Object.create(null), state.fieldSubscribers, { + [to]: state.fieldSubscribers[from], + });
🤖 Fix all issues with AI agents
In `@src/FinalForm.registerField.test.ts`:
- Around line 65-68: The final closing brace for the top-level describe block
was misplaced, causing subsequent it() tests to be outside the describe; fix by
moving the stray closing brace that currently ends the describe too early so
that all it("should fix up field..." and following it() blocks remain inside the
same describe (ensure the describe opening and closing braces around the
FinalForm.registerField tests enclose all it() cases); adjust only the brace
placement so no additional structural changes are needed and run tests to
confirm.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #520 +/- ##
=======================================
Coverage 99.05% 99.05%
=======================================
Files 13 13
Lines 849 849
Branches 286 286
=======================================
Hits 841 841
Misses 8 8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
erikras-richard-agent
left a comment
erikras-richard-agent
left a comment
There was a problem hiding this comment.
LGTM! Clean fix using Object.create(null) for fields, fieldSubscribers, and keysCache to avoid prototype pollution with reserved property names like 'constructor'.
Good test coverage — registers a field named 'constructor', verifies change/unregister work correctly. The renameField update to use Object.assign(Object.create(null), ...) is consistent.
Minor whitespace cleanup is fine. ✅
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Stale review - issues were addressed in subsequent commits
|
Published in |
3 participants
Summary
Fixes #489
Allows registering fields with reserved JavaScript property names like
constructor,toString,__proto__, etc.Problem
Users reported inability to register fields with certain names:
This is a real production issue where form field names happen to match JavaScript reserved property names.
Root Cause
The
state.fieldsobject was initialized as a plain object literal{}, which inherits fromObject.prototype. When accessingstate.fields['constructor'], JavaScript returns the built-inObject.constructorfunction instead of allowing a custom field property to be created.Example of the problem:
Solution
Initialize
state.fieldsusingObject.create(null)to create an object without a prototype chain:Changes Made
Line 192: Changed initial state creation
Line 254: Preserved null prototype in
renameFieldfunctionAdded comprehensive test in
FinalForm.registerField.test.tsImpact
✅ Enables reserved property names as field names
✅ No breaking changes
Object.create(null)creates a plain object compatible with index signatures✅ No performance impact
Object.create(null)is as fast as{}✅ Backward compatible
Why PR #490 Was Closed
A previous PR #490 attempted to fix this but was never merged. This PR provides a cleaner, more minimal solution using
Object.create(null)instead of trying to work around prototype pollution.Testing
Fixes #489
Summary by CodeRabbit
Tests
Bug Fixes