-
Notifications
You must be signed in to change notification settings - Fork 2
Feat/dx 3070 map tag with react component #190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: development
Are you sure you want to change the base?
Feat/dx 3070 map tag with react component #190
Conversation
for (let i = 0; i < keys.length - 1; i++) { | ||
const key = keys[i]; | ||
if (!(key in current) || typeof current[key] !== 'object' || current[key] === null) { | ||
current[key] = {}; |
Check warning
Code scanning / CodeQL
Prototype-polluting assignment Medium
library input
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
The best fix is to add a safeguard in the setNestedProperty
function so that if any key in the path is a dangerous prototype-polluting property like "__proto__"
, "prototype"
, or "constructor"
, the function throws an error or silently skips assignment. This prevents both reading from and writing to sensitive properties on the prototype chain.
This can be achieved by adding a check within the function's main loop and before the actual assignment, ensuring that none of the segments in the path match any of these dangerous strings. If a dangerous key is found, the function should immediately return or throw.
The required change is:
- In
src/helper/react-utils.ts
, at/after line 173 (thesetNestedProperty
function), check each key (within the for loop and just before the assignment) against the blocked list. If any key is forbidden, abort the function or throw an error.
No external imports or dependencies are necessary; this is a simple safe-list implemented inline.
-
Copy modified lines R176-R177 -
Copy modified lines R180-R183 -
Copy modified lines R189-R195
@@ -173,16 +173,26 @@ | ||
export function setNestedProperty(obj: any, path: string, value: any): void { | ||
const keys = path.split('.'); | ||
let current = obj; | ||
|
||
const dangerousKeys = ['__proto__', 'constructor', 'prototype']; | ||
|
||
for (let i = 0; i < keys.length - 1; i++) { | ||
const key = keys[i]; | ||
if (dangerousKeys.includes(key)) { | ||
// Prevent prototype pollution by skipping dangerous keys | ||
return; | ||
} | ||
if (!(key in current) || typeof current[key] !== 'object' || current[key] === null) { | ||
current[key] = {}; | ||
} | ||
current = current[key]; | ||
} | ||
|
||
current[keys[keys.length - 1]] = value; | ||
|
||
const lastKey = keys[keys.length - 1]; | ||
if (dangerousKeys.includes(lastKey)) { | ||
// Prevent prototype pollution by skipping dangerous keys | ||
return; | ||
} | ||
current[lastKey] = value; | ||
} | ||
|
||
/** |
current = current[key]; | ||
} | ||
|
||
current[keys[keys.length - 1]] = value; |
Check warning
Code scanning / CodeQL
Prototype-polluting assignment Medium
library input
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
To fix the prototype pollution vulnerability, we need to prevent dangerous property names from being used in any part of the path
. Specifically, the keys '__proto__'
, 'constructor'
, and 'prototype'
must not be permitted in any segment. Best practice is to reject (throw an error) if these property names are encountered, since ignoring the assignment could silently break expected functionality.
The changes required:
- In
src/helper/react-utils.ts
, inside the functionsetNestedProperty
, before performing any property assignment/traversal, iterate throughkeys
and check if any are in the forbidden set. - If a forbidden key is present, throw an error or log and return (depending on preference—but throwing is safest).
- To do this, add a constant array of forbidden property names (
['__proto__', 'constructor', 'prototype']
) near the function or at module level.
No external dependencies are needed, as this is simple string comparison.
-
Copy modified lines R175-R181
@@ -172,6 +172,13 @@ | ||
*/ | ||
export function setNestedProperty(obj: any, path: string, value: any): void { | ||
const keys = path.split('.'); | ||
const forbidden = ['__proto__', 'constructor', 'prototype']; | ||
// Check for dangerous property names | ||
for (const k of keys) { | ||
if (forbidden.includes(k)) { | ||
throw new Error(`Refusing to set property "${k}" due to prototype pollution risk.`); | ||
} | ||
} | ||
let current = obj; | ||
|
||
for (let i = 0; i < keys.length - 1; i++) { |
current = current[key]; | ||
} | ||
|
||
current[keys[keys.length - 1]] = value; |
Check warning
Code scanning / CodeQL
Prototype-polluting function Medium
here
current
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
To prevent prototype pollution when setting nested properties using string paths, block assignments to the reserved property names __proto__
and constructor
in any position of the path. The best fix is to add a check inside the loop iterating over keys and before the final assignment, returning early or throwing an error if any path segment is "__proto__"
or "constructor"
. This should be implemented directly in setNestedProperty
inside src/helper/react-utils.ts between lines 173-186. No external library is needed for this fix.
-
Copy modified lines R179-R182 -
Copy modified lines R188-R193
@@ -176,13 +176,21 @@ | ||
|
||
for (let i = 0; i < keys.length - 1; i++) { | ||
const key = keys[i]; | ||
if (key === "__proto__" || key === "constructor") { | ||
// Prevent prototype pollution | ||
return; | ||
} | ||
if (!(key in current) || typeof current[key] !== 'object' || current[key] === null) { | ||
current[key] = {}; | ||
} | ||
current = current[key]; | ||
} | ||
|
||
current[keys[keys.length - 1]] = value; | ||
const finalKey = keys[keys.length - 1]; | ||
if (finalKey === "__proto__" || finalKey === "constructor") { | ||
// Prevent prototype pollution | ||
return; | ||
} | ||
current[finalKey] = value; | ||
} | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces React integration functionality to the Contentstack Utils library, allowing conversion of JSON RTE (Rich Text Editor) content to React elements. The implementation provides a flexible framework for rendering Contentstack JSON content as React components while maintaining attribute mapping and custom rendering options.
- Adds a comprehensive React integration module with default rendering options for all standard HTML elements
- Implements utility functions for React-specific operations like prop conversion, style parsing, and text formatting
- Provides comprehensive test coverage for various JSON RTE scenarios and edge cases
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
src/json-to-react.ts | Main converter function with default render options for all HTML node types |
src/index.ts | Exports React integration functions and types |
src/helper/react-utils.ts | React utility functions for prop conversion, styling, and element creation |
src/Models/react-types.ts | TypeScript type definitions for React integration |
test/json-to-react.test.ts | Comprehensive test suite covering various JSON RTE conversion scenarios |
.talismanrc | Security configuration updates for new files |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
const React = (globalThis as any).React || (typeof window !== 'undefined' ? (window as any).React : null); | ||
if (React && React.createElement) { | ||
defaultCreateElement = React.createElement; | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using globalThis
and window
with as any
bypasses TypeScript's type safety. Consider using proper type guards or feature detection to safely access these global objects.
const React = (globalThis as any).React || (typeof window !== 'undefined' ? (window as any).React : null); | |
if (React && React.createElement) { | |
defaultCreateElement = React.createElement; | |
} else { | |
let React: any = undefined; | |
if (typeof globalThis === 'object' && globalThis !== null && 'React' in globalThis) { | |
React = (globalThis as { React?: unknown }).React; | |
} else if (typeof window === 'object' && window !== null && 'React' in window) { | |
React = (window as { React?: unknown }).React; | |
} else { | |
React = null; | |
} | |
if ( | |
React && | |
typeof React === 'object' && | |
typeof (React as { createElement?: unknown }).createElement === 'function' | |
) { | |
defaultCreateElement = (React as { createElement: (...args: any[]) => ReactElement }).createElement; | |
} else { |
Copilot uses AI. Check for mistakes.
* Parse CSS style string into React style object | ||
*/ | ||
export function parseStyleString(styleStr: string): { [key: string]: string | number } { | ||
const styles: any = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using any
type defeats the purpose of TypeScript. Consider using a more specific type like Record<string, string | number>
or the return type directly.
const styles: any = {}; | |
const styles: { [key: string]: string | number } = {}; |
Copilot uses AI. Check for mistakes.
const colonIndex = rule.indexOf(':'); | ||
if (colonIndex === -1) return; | ||
|
||
const property = rule.substring(0, colonIndex).trim(); | ||
const value = rule.substring(colonIndex + 1).trim(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CSS parsing logic could be more robust. Consider handling edge cases like empty rules, multiple colons, or malformed CSS. Also, using split(':')
with proper validation might be clearer than manual substring operations.
const colonIndex = rule.indexOf(':'); | |
if (colonIndex === -1) return; | |
const property = rule.substring(0, colonIndex).trim(); | |
const value = rule.substring(colonIndex + 1).trim(); | |
if (!rule.trim()) return; // skip empty rules | |
const parts = rule.split(':', 2); | |
if (parts.length !== 2) return; | |
const property = parts[0].trim(); | |
const value = parts[1].trim(); |
Copilot uses AI. Check for mistakes.
findRenderContent(path, entryItem, (content: Document | Document[]) => { | ||
const reactElement = documentToReact(content, mergedRenderOptions, createElement); | ||
setNestedProperty(entryItem, path, reactElement); | ||
return content as any; // Return original to satisfy the callback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using as any
to bypass type checking is not recommended. Consider updating the callback signature in findRenderContent
to properly handle the return type, or use a more specific type assertion.
return content as any; // Return original to satisfy the callback |
Copilot uses AI. Check for mistakes.
export interface ReactNode { | ||
[key: string]: any; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ReactNode
interface is too permissive with [key: string]: any
. This should be more specific - React's actual ReactNode
type is ReactElement | string | number | ReactFragment | ReactPortal | boolean | null | undefined
. Consider aligning with React's actual type definition.
export interface ReactNode { | |
[key: string]: any; | |
} | |
// Local placeholder types to match React's actual ReactNode definition | |
export type ReactFragment = {} | ReactNode[]; | |
export interface ReactPortal {} | |
export type ReactNode = | |
| ReactElement | |
| string | |
| number | |
| ReactFragment | |
| ReactPortal | |
| boolean | |
| null | |
| undefined; |
Copilot uses AI. Check for mistakes.
No description provided.