Use secureFetch and secureAxiosRequest for more URLs#5886
Use secureFetch and secureAxiosRequest for more URLs#5886christopherholland-workday wants to merge 3 commits intomainfrom
Conversation
christopherholland-workday
changed the title
![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 aims to significantly improve security by consistently replacing axios and fetch calls with secureAxiosRequest and secureFetch wrappers, and by implementing support for custom CA certificates via agentOptions. While this is a positive step, the current implementation of these secure helpers is flawed: they fail to drop sensitive headers (like Authorization) during cross-origin redirects, potentially leading to credential leakage. Additionally, the MCP toolkit's validation pattern is insufficient, making it vulnerable to DNS rebinding and redirect-based SSRF. These critical security vulnerabilities must be addressed to ensure the intended security benefits are fully realized. A minor issue also noted is a leftover debugging statement that needs removal.
| while (redirectCount <= maxRedirects) { | ||
| const resolved = await resolveAndValidate(currentUrl) | ||
| const agent = createPinnedAgent(resolved) | ||
| const agent = createPinnedAgent(resolved, agentOptions) | ||
|
|
||
| const response = await fetch(currentUrl, { ...currentInit, agent: () => agent }) |
There was a problem hiding this comment.
The secureFetch function implements manual redirect handling but fails to remove sensitive headers like Authorization or Cookie when following a redirect to a different origin. This can lead to credential leakage if a trusted server redirects the request to a malicious third-party domain.
Remediation: Check if the redirect URL has a different origin than the current URL and, if so, strip sensitive headers from the request configuration.
There was a problem hiding this comment.
This may be better for another PR as it's not directly related to this change
|
For |
@HenryHengZJ Blocking local by default is the security approach we decided to take in other changes, e.g. #5862 Users can control what's blocked (and control the default) using the env vars |
4 participants
FLOWISE-285