Remove generic 'handle' prefix from Social Web UI function names#2485
Remove generic 'handle' prefix from Social Web UI function names#2485obenland merged 2 commits intoadd/readerfrom
Conversation
| getValue: ( { item }: { item: FeedPost } ) => { | ||
| // Strip HTML tags for plain text value (used for search/sort) | ||
| const text = item.excerpt?.rendered || item.content?.rendered || ''; | ||
| const stripped = text.replace( /<[^>]*>/g, '' ); |
Check failure
Code scanning / CodeQL
Incomplete multi-character sanitization
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
The ideal fix is to use a well-tested library for HTML-to-text conversion (such as striptags or sanitize-html with options to allow no tags) rather than manual regular expressions, which are rarely robust for HTML parsing or sanitization. These libraries will reliably remove all HTML tags, including edge cases like nested or malformed tags.
If the project cannot depend on external libraries, another option is to repeatedly apply the same regex until no further replacements are possible OR to use the browser's own HTML parser to strip tags safely (e.g., by creating a temporary DOM element in a browser environment).
For the scope shown, the recommended change is to use striptags for line 18: Replace the regex-based method with the library method, ensuring proper import at the top of the file. This will fully and reliably remove all HTML tags.
Edits to be made:
- Add the import for
striptagsat the top. - Change line 18 to:
const stripped = striptags(text);
| @@ -1,5 +1,6 @@ | ||
| import { __ } from '@wordpress/i18n'; | ||
| import { decodeEntities } from '@wordpress/html-entities'; | ||
| import striptags from 'striptags'; | ||
| import type { Field } from '@wordpress/dataviews'; | ||
| import type { FeedPost } from '../../types'; | ||
|
|
||
| @@ -15,7 +16,7 @@ | ||
| getValue: ( { item }: { item: FeedPost } ) => { | ||
| // Strip HTML tags for plain text value (used for search/sort) | ||
| const text = item.excerpt?.rendered || item.content?.rendered || ''; | ||
| const stripped = text.replace( /<[^>]*>/g, '' ); | ||
| const stripped = striptags(text); | ||
| return decodeEntities( stripped ); | ||
| }, | ||
| render: ( { item }: { item: FeedPost } ) => { |
| @@ -70,12 +70,12 @@ | ||
| "@wordpress/primitives": "^4.31.0", | ||
| "@wordpress/scripts": "^30.23.0", | ||
| "@wordpress/url": "^4.22.0", | ||
| "@wordpress/views": "^1.0.0", | ||
| "@wordpress/viewport": "^6.32.0", | ||
| "@wordpress/views": "^1.0.0", | ||
| "prettier": "npm:wp-prettier@^3.0.3" | ||
| }, | ||
| "dependencies": { | ||
| "clsx": "^2.1.1" | ||
| "clsx": "^2.1.1", | ||
| "striptags": "^3.2.0" | ||
| } | ||
| } |
| Package | Version | Security advisories |
| striptags (npm) | 3.2.0 | None |
| getValue: ( { item }: { item: FeedPost } ) => { | ||
| // Strip HTML tags for plain text value | ||
| const text = item.excerpt?.rendered || item.content?.rendered || ''; | ||
| const stripped = text.replace( /<[^>]*>/g, '' ); |
Check failure
Code scanning / CodeQL
Incomplete multi-character sanitization
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
To fix this problem robustly and in line with best practices, we should use a well-tested HTML sanitization library. For React/Node projects, one of the most popular is sanitize-html. This library removes all HTML tags and scripts from a string robustly, ensuring no fragments or malicious input remains after sanitization.
To minimize changes, we will replace the manual regex stripping of HTML tags with a call to sanitize-html. This should be done in both places in the file where .replace(/<[^>]*>/g, '') currently appears:
- Line 14: In the
getValueimplementation. - Line 20: In the
renderimplementation.
We will import sanitize-html at the top of the file.
No changes need to be made to the logic that follows, as the output is already run through decodeEntities and potentially .trim(), which both remain fine.
| @@ -1,5 +1,6 @@ | ||
| import { __ } from '@wordpress/i18n'; | ||
| import { decodeEntities } from '@wordpress/html-entities'; | ||
| import sanitizeHtml from 'sanitize-html'; | ||
| import type { Field } from '@wordpress/dataviews'; | ||
| import type { FeedPost } from '../../types'; | ||
|
|
||
| @@ -11,13 +12,13 @@ | ||
| getValue: ( { item }: { item: FeedPost } ) => { | ||
| // Strip HTML tags for plain text value | ||
| const text = item.excerpt?.rendered || item.content?.rendered || ''; | ||
| const stripped = text.replace( /<[^>]*>/g, '' ); | ||
| const stripped = sanitizeHtml( text, { allowedTags: [], allowedAttributes: {} } ); | ||
| return decodeEntities( stripped ); | ||
| }, | ||
| render: ( { item }: { item: FeedPost } ) => { | ||
| const excerpt = item.excerpt?.rendered || item.content?.rendered || ''; | ||
| // Strip HTML tags, remove backslash escapes, and decode HTML entities | ||
| const stripped = excerpt.replace( /<[^>]*>/g, '' ).trim(); | ||
| const stripped = sanitizeHtml( excerpt, { allowedTags: [], allowedAttributes: {} } ).trim(); | ||
| const unescaped = stripped.replace( /\\(.)/g, '$1' ); | ||
| const plainText = decodeEntities( unescaped ); | ||
|
|
| @@ -70,12 +70,12 @@ | ||
| "@wordpress/primitives": "^4.31.0", | ||
| "@wordpress/scripts": "^30.23.0", | ||
| "@wordpress/url": "^4.22.0", | ||
| "@wordpress/views": "^1.0.0", | ||
| "@wordpress/viewport": "^6.32.0", | ||
| "@wordpress/views": "^1.0.0", | ||
| "prettier": "npm:wp-prettier@^3.0.3" | ||
| }, | ||
| "dependencies": { | ||
| "clsx": "^2.1.1" | ||
| "clsx": "^2.1.1", | ||
| "sanitize-html": "^2.17.0" | ||
| } | ||
| } |
| Package | Version | Security advisories |
| sanitize-html (npm) | 2.17.0 | None |
| render: ( { item }: { item: FeedPost } ) => { | ||
| const excerpt = item.excerpt?.rendered || item.content?.rendered || ''; | ||
| // Strip HTML tags, remove backslash escapes, and decode HTML entities | ||
| const stripped = excerpt.replace( /<[^>]*>/g, '' ).trim(); |
Check failure
Code scanning / CodeQL
Incomplete multi-character sanitization
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
The best fix is to use a well-tested library to sanitize or strip HTML from untrusted input rather than using a fragile regular expression. For React/JS projects, the sanitize-html or dompurify library are standard choices. Since we only need plain text (no HTML at all is allowed), a robust and minimal library like striptags is suitable for stripping ALL tags.
To fix the code:
- Replace the manual
.replace(/<[^>]*>/g, '')code (lines 14 and 20) with a call tostriptags. - Import
striptagsat the top of the file. - Make the replacement in both the
getValueandrendermethods, ensuring all HTML tags are stripped, regardless of input complexity. - No other changes are needed to logic or functionality.
striptagsshould be installed as a project dependency.
| @@ -2,6 +2,7 @@ | ||
| import { decodeEntities } from '@wordpress/html-entities'; | ||
| import type { Field } from '@wordpress/dataviews'; | ||
| import type { FeedPost } from '../../types'; | ||
| import striptags from 'striptags'; | ||
|
|
||
| export const excerptField: Field< FeedPost > = { | ||
| id: 'excerpt.rendered', | ||
| @@ -11,13 +12,13 @@ | ||
| getValue: ( { item }: { item: FeedPost } ) => { | ||
| // Strip HTML tags for plain text value | ||
| const text = item.excerpt?.rendered || item.content?.rendered || ''; | ||
| const stripped = text.replace( /<[^>]*>/g, '' ); | ||
| const stripped = striptags(text); | ||
| return decodeEntities( stripped ); | ||
| }, | ||
| render: ( { item }: { item: FeedPost } ) => { | ||
| const excerpt = item.excerpt?.rendered || item.content?.rendered || ''; | ||
| // Strip HTML tags, remove backslash escapes, and decode HTML entities | ||
| const stripped = excerpt.replace( /<[^>]*>/g, '' ).trim(); | ||
| const stripped = striptags(excerpt).trim(); | ||
| const unescaped = stripped.replace( /\\(.)/g, '$1' ); | ||
| const plainText = decodeEntities( unescaped ); | ||
|
|
| @@ -70,12 +70,12 @@ | ||
| "@wordpress/primitives": "^4.31.0", | ||
| "@wordpress/scripts": "^30.23.0", | ||
| "@wordpress/url": "^4.22.0", | ||
| "@wordpress/views": "^1.0.0", | ||
| "@wordpress/viewport": "^6.32.0", | ||
| "@wordpress/views": "^1.0.0", | ||
| "prettier": "npm:wp-prettier@^3.0.3" | ||
| }, | ||
| "dependencies": { | ||
| "clsx": "^2.1.1" | ||
| "clsx": "^2.1.1", | ||
| "striptags": "^3.2.0" | ||
| } | ||
| } |
| Package | Version | Security advisories |
| striptags (npm) | 3.2.0 | None |
0c71357 to
4ab7b6b
Compare
Renamed functions to better describe their purpose: - handleChangeSelection → changeSelection - handleHashChange → syncUrlToState - handleSelectItem → selectItem - handleCloseInspector → closeInspector - handleNavigate → navigate
4ab7b6b to
bb28cb8
Compare
|
What about using |
|
@pfefferle I've opened a new pull request, #2486, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@pfefferle The github-advanced-security is from the initial version of the PR, when it was created against This could still use your review. |
|
what about the |
|
Could you link me to that recommendation so I can learn more about it? |
|
It is one of the recommendations of copilot! |
|
but it seems to be also for old code. let's merge it! |

Summary
Refactored event handler function names in the Social Web UI to be more descriptive and meaningful by removing the generic 'handle' prefix.
Changes
src/social-web/routes/feed/stage.tsx:handleChangeSelection→changeSelectionsrc/social-web/components/layout/index.tsx:handleHashChange→syncUrlToStatehandleSelectItem→selectItemhandleCloseInspector→closeInspectorhandleNavigate→navigateTest Plan