fix(editor): sanitize input spaces in data entry (BB-602)#1211
fix(editor): sanitize input spaces in data entry (BB-602)#1211AkshayJalageri wants to merge 1 commit intometabrainz:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements input sanitization for form data to prevent database entries with leading/trailing whitespace and inconsistent spacing. The changes introduce a sanitizeInput function that trims whitespace, normalizes multiple consecutive spaces (including fullwidth spaces), and recursively applies this sanitization to all form data before submission, with an intended exception for annotation fields to preserve Markdown formatting.
Key Changes
- Added
sanitizeInputfunction to normalize whitespace in text inputs - Implemented recursive
sanitizePayloadfunction to sanitize nested objects and arrays - Applied sanitization in all three submission paths:
postSubmission,postUFSubmission, andsubmitSingleEntity
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/client/helpers/utils.tsx | Adds the sanitizeInput utility function that normalizes whitespace and trims strings |
| src/client/entity-editor/submission-section/actions.ts | Implements sanitizePayload to recursively sanitize form data and integrates it into all submission workflows |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| if (_.isPlainObject(data)) { | ||
| return _.mapValues(data, (value, key) => { | ||
| if (key === 'annotation') { return value; } |
There was a problem hiding this comment.
The check for 'annotation' is incorrect. Based on the codebase structure, the actual key is 'annotationSection', not 'annotation'. The annotationSection object contains a 'content' property. This means annotation content will be sanitized, contradicting the stated intention to preserve formatting in annotations. The check should be 'annotationSection' instead of 'annotation'.
There was a problem hiding this comment.
Like copilot says, I think this is incorrect
| export function sanitizeInput(text: string): string { | ||
| if (!text || typeof text !== 'string') { | ||
| return ''; | ||
| } | ||
|
|
||
| return text | ||
| .replace(/\u3000/g, ' ') // Convert fullwidth space to normal space | ||
| .replace(/\s+/g, ' ') // Merge double/multiple spaces into one | ||
| .trim(); // Remove start/end spaces | ||
| } No newline at end of file |
There was a problem hiding this comment.
This function uses space indentation instead of tab indentation, which violates the codebase conventions defined in .editorconfig. According to .editorconfig, .tsx files should use tab indentation (indent_style = tab). Please reformat this function to use tabs for consistency with the rest of the file.
MonkeyDo
left a comment
There was a problem hiding this comment.
Hi, thanks for opening a PR !
All the unrelated auto-formatting changes are making it quite hard to review what are the actual changes, but here is some preliminary feedback from what I could clearly see.
| export const SET_SUBMITTED = 'SET_SUBMITTED'; | ||
|
|
||
| export type Action = { | ||
| type: string, |
There was a problem hiding this comment.
All those unrelated formatting changes are unwanted and should be removed.
| */ | ||
| export function sanitizeInput(text: string): string { | ||
| if (!text || typeof text !== 'string') { | ||
| return ''; |
There was a problem hiding this comment.
Should probably return text; instead?
| } | ||
| if (_.isPlainObject(data)) { | ||
| return _.mapValues(data, (value, key) => { | ||
| if (key === 'annotation') { return value; } |
There was a problem hiding this comment.
Like copilot says, I think this is incorrect
| * Recursively sanitizes data object strings. | ||
| * Skips 'annotation' field to preserve formatting. | ||
| */ | ||
| function sanitizePayload(data: any): any { |
There was a problem hiding this comment.
So we already have an existing method to sanitize text, which was just behind a simple search on the codebase...
Changes should be implemented there (I think only the trimming?) instead of creating a new method.
bookbrainz-site/src/common/helpers/utils.ts
Lines 99 to 132 in a16d1db
Additionally, the payload is already processed on the server-side, which future-proofs in case we ever want to allow submitting data through API.
bookbrainz-site/src/server/routes/entity/entity.tsx
Lines 1065 to 1092 in a16d1db
|
I will also add this: we require you disclose any use of LLM/AI agents, see our contribution guidelines |
Problem
As described in BB-602, user input in forms currently retains unnecessary whitespace. This leads to database entries with leading/trailing spaces or inconsistent double spacing in names and titles.
Solution
I have implemented a sanitization helper that processes form data before submission.
\u3000) into single spaces.annotationfield is explicitly skipped to preserve Markdown formatting.Ticket
https://tickets.metabrainz.org/browse/BB-602