fix: point field validation accepts a half-filled coordinate pair when a coordinate is 0 - #17599
Open
shuvamk wants to merge 1 commit into
Open
fix: point field validation accepts a half-filled coordinate pair when a coordinate is 0#17599shuvamk wants to merge 1 commit into
shuvamk wants to merge 1 commit into
Conversation
…n a coordinate is 0 The pairing check in the `point` validator tests coordinate presence with truthiness. `0` is a valid longitude (Greenwich) and a valid latitude (equator) but is falsy, so a `0` coordinate reads as "not provided" and the cross-indexed pairing check never fires. On an optional point field: [5, null] -> validation:invalidInput (correct) [0, null] -> true (should be invalidInput) [null, 0] -> true (should be invalidInput) ['', 0] -> true (should be invalidInput) [0, 'abc'] -> true (should be invalidInput) This is reachable from the admin UI: the Point field seeds its value as [null, null] and its onChange writes parseFloat(input) per coordinate, so typing `0` into Longitude and leaving Latitude blank produces [0, null]. Every other longitude is rejected; only `0` slips through, and the half-filled array then reaches the database adapter. Replace the truthiness presence test with an explicit null/undefined/'' check, and reuse it for the bounds guards below so the whole function agrees on what "provided" means. The bounds guards are unaffected behaviourally since `0` is in range on both axes. Required point fields already return validation:requiresTwoNumbers for these inputs, so this only changes optional fields. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
On an optional
pointfield, a coordinate pair where the entered coordinate is0and the other coordinate is missing or unparseable passes validation:main[5, null]validation:invalidInputvalidation:invalidInput[0, null]validation:invalidInputtrue[null, 0]validation:invalidInputtrue['', 0]validation:invalidInputtrue[0, 'abc']validation:invalidInputtrue['abc', 0]validation:invalidInputtrueEvery longitude except
0is rejected when the latitude is blank.0alone slips through.Payload's own pipeline produces this shape.
packages/payload/src/fields/hooks/beforeValidate/promise.ts:140normalises each coordinate before the validator runs — a string that is empty after trimming becomesnull, anything else goes throughparseFloat. So a REST or GraphQL submit of{ point: ['0', ''] }is canonicalised into exactly[0, null], which then validates. The admin UI reaches the same state:packages/ui/src/fields/Point/index.tsxseeds the value as[null, null]andhandleChangewritesparseFloat(e.target.value)for the edited coordinate only, so typing0into Longitude and leaving Latitude blank yields[0, null].I have not traced what the adapters do with the half-filled array once it is past validation, so I am not claiming a specific downstream symptom — everything below is scoped to the validator, which is what the added tests cover.
Why?
packages/payload/src/fields/validations.ts:The cross-indexing is deliberate and correct — "a latitude was provided, so the longitude must parse to a number", and vice versa. The defect is that presence is tested with truthiness.
0is a valid longitude (Greenwich) and a valid latitude (equator), but it is falsy, so a0coordinate is read as "not provided" and the pairing check never fires. The two bounds guards below use the same truthiness test.How?
Replace the truthiness presence test with an explicit
null/undefined/''check, and reuse it for the bounds guards so the whole function agrees on what "provided" means:The bounds guards are unchanged behaviourally —
0is in range on both axes — but leaving them onvalue[0] &&next tohasLngin the same function would be confusing.Scope: this only affects optional point fields.
required: truealready returnsvalidation:requiresTwoNumbersfor every input in the table above, and that path is untouched.Tests
Six cases added to the existing
describe('point')block inpackages/payload/src/fields/validations.spec.ts— four for the broken inputs, two pinning the other direction ([0, 0]and the['', '']default both still validate).I verified the new tests fail without the source change. Stashing only
validations.tsand keeping the spec:Restoring the fix:
All 18 pre-existing
pointtests pass unchanged.Full unit suite (
vitest run --project unit), same machine, same commit:main:2 failed | 1785 passed | 2 skipped (1789)2 failed | 1791 passed | 2 skipped (1795)+6, no change to the failures. Both pre-existing failures are local-environment artifacts of my checkout (tools/releaser/src/lib/publishList.spec.tsshells out topnpm turbo;packages/payload/src/bin/build.spec.tsasserts a resolvedvitebin path), pluspackages/typescript-plugin/src/__tests__/plugin.spec.tsfailing to collect. They are unrelated to this change and reproduce on a clean tree.ESLint on
validations.ts: 0 errors; the 4 remaining warnings are pre-existing and on lines 382/954/1019. Prettier clean.tsc --noEmitonpackages/payloadreports no errors other than the pre-existingTS6305unbuilt-project-reference noise.Note on #17538
#17538 edits this same line, but reads the cross-indexing as a typo and un-crosses it:
That removes the pairing check entirely and makes
[5, null]valid. It fails the existingshould prevent missing valuetest ([0.1]), which I confirmed by applying it locally:Flagging it so the two don't get merged in a way that silently reverts this. Happy to rebase around whichever lands first.
Alternative
The presence test could be narrowed further — e.g. treating any non-numeric coordinate as absent so
[0, 'abc']returnsrequiresTwoNumbersinstead ofinvalidInput. I keptinvalidInputbecause that is what['bad', 'input']already returns on an optional field, and changing it would alter messages for inputs that are currently handled correctly. Happy to switch if you'd prefer the other message.