bugfix:308 | remove ungated overrideConfig spread#6279
Conversation
…wData The spread allowed API callers to inject arbitrary keys into the $flow.* template namespace — bypassing the per-parameter gating that replaceInputsWithConfig() enforces. Properly gated override paths (replaceInputsWithConfig, $vars) are unaffected. Closes FLOWISE-308
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves security by removing the spread of overrideConfig (mass assignment) in buildAgentflow.ts, buildChatflow.ts, and index.ts. A review comment points out that in packages/server/src/utils/index.ts, the resulting object is now missing apiMessageId and chatflowId, which are necessary for consistent variable resolution across different flow types.
| chatflowid, | ||
| chatId, | ||
| sessionId, | ||
| chatHistory, | ||
| ...overrideConfig | ||
| chatHistory |
There was a problem hiding this comment.
While removing the overrideConfig spread is correct for security, this flowData object is now missing apiMessageId and chatflowId (camelCase), which are included in the equivalent objects in buildChatflow.ts and buildAgentflow.ts. Adding them here ensures consistency for $flow.* variable resolution across different flow types and execution phases.
chatflowid,
chatflowId: chatflowid,
chatId,
sessionId,
chatHistory,
apiMessageId,References
- Avoid mass assignment from request bodies to entities. Instead of using a generic assignment like Object.assign(entity, body), explicitly map allowed properties.
There was a problem hiding this comment.
flowData in index.ts never had chatflowId or apiMessageId, even in the original PR #3075 that introduced it. The ...overrideConfig spread was masking this since it could coincidentally include those keys.
I can add chatflowId and apiMessageId if needed
| runtimeChatHistoryLength: Math.max(0, runtimeChatHistory.length - 1), | ||
| state: updatedState, | ||
| ...overrideConfig | ||
| state: updatedState |
There was a problem hiding this comment.
although the ticket recommended to just remove the ...overrideConfig.
It is kind of in conflict with this PR #3075
That PR purposely allowed any $flow.* variables to be injected, with this change it won't be allowed anymore.
If we still want that to be allowed we can do something like ...(apiOverrideStatus ? incomingInput.overrideConfig : {})
so when they enable apiOverrideStatus it will still work as it did before.
But I do believe removing ...overrideConfig is more correct and is better to rely on replaceInputsWithConfig()
jchui-wd
changed the title
2 participants
Changes
...overrideConfigspread fromflowConfig/flowDataobjects inbuildChatflow.ts, buildAgentflow.ts, and util.index.ts$flow.*template variable namespace without any authorization check, bypassing the per-parameter gating thatreplaceInputsWithConfig()enforces$flow.*variables (chatId, sessionId, chatHistory, etc.) continue to resolve because they are explicit fields on the config objectsFixes: Flowise 308
Video Demos
Test Videos Demo
Chatflow-Before-injected.mp4
Chatflow-Before-injected.mp4
Chatflow-After-not-injected.mp4
Chatflow-After-not-injected.mp4
Agentflow-BEFORE-ap...still-can-override.mp4
Agentflow-BEFORE-apiOverride-off-still-can-override.mp4
Agentflow-AFTER-Nee...Override-To-Work.mp4
Agentflow-AFTER-Need-apiOverride-To-Work.mp4