Skip to content

Conversation

@christopherholland-workday
Copy link
Contributor

This code was using node-fetch instead of the custom-built secureFetch method. secureFetch has SSRF protection built in and should be the standard used in Flowise when making fetch calls.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request correctly replaces node-fetch with the custom secureFetch method in WebScraperTool to add SSRF protection, which is a good security improvement. My review includes a suggestion to simplify the call to secureFetch for better code clarity and to remove redundant options.

private async scrapeSingleUrl(url: string): Promise<Omit<ScrapedPageData, 'url'> & { foundLinks: string[] }> {
try {
const response = await fetch(url, { timeout: this.timeoutMs, redirect: 'follow', follow: 5 })
const response = await secureFetch(url, { timeout: this.timeoutMs, redirect: 'follow', follow: 5 })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The secureFetch function handles redirects manually and overrides the redirect option. The follow option is also not used by secureFetch's internal logic. To avoid confusion and improve clarity, these properties can be removed from the call. secureFetch defaults to a maximum of 5 redirects, which preserves the original behavior of follow: 5.

Suggested change
const response = await secureFetch(url, { timeout: this.timeoutMs, redirect: 'follow', follow: 5 })
const response = await secureFetch(url, { timeout: this.timeoutMs })

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants