Fix Mass Assignment in Dataset and DatasetRow Operations#6051
Fix Mass Assignment in Dataset and DatasetRow Operations#6051christopherholland-workday wants to merge 2 commits intomainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical security vulnerability related to mass assignment in dataset and dataset row operations. By replacing generic Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors dataset and dataset row creation and update operations to use explicit property assignments instead of Object.assign, aiming to prevent mass assignment vulnerabilities. However, the review highlights several critical and high-severity security vulnerabilities related to Insecure Direct Object Reference (IDOR) and missing authorization checks. Specifically, in updateDatasetRow, the authorization check occurs after the update, allowing unauthorized modifications. In createDataset, passing workspaceId directly from the request body is identified as a potential IDOR risk. Furthermore, in addDatasetRow, a missing authorization check before row creation could allow attackers to add data to unauthorized datasets.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the creation and update logic for datasets and dataset rows. It moves from using Object.assign with the entire request body to explicitly assigning specific properties such as name, description, input, output, workspaceId, and datasetId. This change improves data integrity and security by preventing unintended property assignments. Additionally, the PR introduces workspaceId checks for addDatasetRow and updateDatasetRow operations, enhancing access control by ensuring dataset rows are only manipulated within their designated workspaces. A review comment suggests improving the error message in updateDatasetRow to more accurately reflect that a dataset might not be found due to workspace access issues, rather than the row itself being missing.
| id: item.datasetId, | ||
| workspaceId: body.workspaceId | ||
| }) | ||
| if (!dataset) throw new InternalFlowiseError(StatusCodes.NOT_FOUND, `Dataset Row ${id} not found`) |
There was a problem hiding this comment.
The error message here (Dataset Row ${id} not found) might be misleading. If dataset is null, it means the dataset associated with the row (item.datasetId) was not found for the provided workspaceId. This indicates an access issue or a data inconsistency, not that the Dataset Row ${id} itself is missing (as item was successfully retrieved earlier). Consider a more precise message like Dataset ${item.datasetId} not found for workspace or Access denied to Dataset Row ${id}.
Flowise-323 (See testing section for Dataset and DatasetRow)