-
Notifications
You must be signed in to change notification settings - Fork 8
Extract hooks #898
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: main
Are you sure you want to change the base?
Extract hooks #898
Conversation
if (key == "id" && shouldExtractId) { | ||
newVariables.id = value; | ||
} else { | ||
newVariables[action.modelApiIdentifier][key] = value; |
Check warning
Code scanning / CodeQL
Prototype-polluting assignment Medium
library input
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
To fix this prototype pollution risk, we need to prevent dangerous property names (e.g., __proto__
, constructor
, prototype
) from being set as object keys on plain objects. The standard approaches are:
- Sanitizing input keys: Check keys before assigning them, and reject or skip dangerous ones.
- Using a prototype-less object: Replace
{}
withObject.create(null)
, which has no prototype, so polluting assignments are safe. - Using a Map: However, if consumers expect
variables
to be a plain object, the safest is to use a prototype-less object.
In this code, the critical region is the construction of newVariables
(lines 279–291), especially the assignment of subkeys at line 289. Rather than {}
, we should use Object.create(null)
for newVariables[action.modelApiIdentifier]
, and ideally also for newVariables
itself (though its keys come from trusted sources except possibly key == 'id'
).
Implementation steps:
- On line 280, replace
{}
withObject.create(null)
. - Where keys are written (line 289), optionally add a check skipping dangerous keys, but using prototype-less objects is already sufficient.
- This change is wholly within the shown code region.
- No external imports are needed.
-
Copy modified line R280
@@ -277,7 +277,7 @@ | ||
newVariables = variables; | ||
} else { | ||
newVariables = { | ||
[action.modelApiIdentifier]: {}, | ||
[action.modelApiIdentifier]: Object.create(null), | ||
}; | ||
for (const [key, value] of Object.entries(variables)) { | ||
if (action.paramOnlyVariables?.includes(key)) { |
for (const validationError of invalidRecordError.validationErrors) { | ||
if (invalidRecordError.modelApiIdentifier) { | ||
result[invalidRecordError.modelApiIdentifier] ??= {}; | ||
result[invalidRecordError.modelApiIdentifier][validationError.apiIdentifier] = { message: validationError.message }; |
Check warning
Code scanning / CodeQL
Prototype-polluting assignment Medium
... a description that explains what, why, and how ...
PR Checklist