Skip to content
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

Only capture hostname when the request is not blocked because of SSRF #381

Closed
wants to merge 4 commits into from

Conversation

timokoessler
Copy link
Member

@timokoessler timokoessler commented Sep 19, 2024

This change also results in a different behavior when saving hostnames after a redirect using Undici / Fetch. Previously they were ignored, while redirects using the internal http or https module were already added to the list of outgoing hostnames.

Copy link

codecov bot commented Sep 19, 2024

Codecov Report

Attention: Patch coverage is 85.33333% with 11 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
library/sinks/HTTPRequest.ts 44.44% 5 Missing ⚠️
library/sinks/Undici.ts 68.75% 5 Missing ⚠️
library/sinks/undici/wrapDispatch.ts 90.90% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@timokoessler timokoessler marked this pull request as draft September 19, 2024 10:47
@timokoessler timokoessler marked this pull request as ready for review September 19, 2024 11:55
);
agent.start([new HTTPRequest()]);

t.same(agent.getHostnames().asArray(), []);
Copy link
Member

Choose a reason for hiding this comment

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

A bit weird to have an assertion outside a test? Let's move that line?

@@ -166,6 +175,9 @@ function wrapDNSLookupCallback(
}

if (!found) {
// Add to list of hostnames that the application is connecting to
agent.onConnectHostname(hostname, port);
Copy link
Member

Choose a reason for hiding this comment

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

Looking for a way to avoid the sprinkled onConnectHostname

Easy to forget when adding a new if statement here or there, right?

undici & fetch

const response = await fetch("url")

When response is returned and the fetch call didn't throw, we know the request succeeded, can we maybe hook into the return value and call agent.onConnectHostname(hostname, port); there?

http/https module

We could wrap the on("response", (...) => {}) handler but we should NEVER add it when no response handler is (See #346)

So I think we have no choice to use the DNS lookup indeed, instead of adding the logic in inspectDNSLookupCalls, can we add a separate wrapper for this?

optionObj.lookup = inspectDNSLookupCalls(
      nativeLookup,
      agent,
      module,
      `${module}.request`,
      url,
      stackTraceCallingLocation
    ) as NonNullable<RequestOptions["lookup"]>;

becomes

optionObj.lookup = addHostnameToAgent(
     agent,
     inspectDNSLookupCalls(
      nativeLookup,
      agent,
      module,
      `${module}.request`,
      url,
      stackTraceCallingLocation
    )
);

function addHostnameToAgent(agent: Agent, lookup: Function) {
  return function addHostnameToAgent(...args: unknown[]) {
    // see if DNS lookup succeeded
    
    if (!error) {
      agent.onConnectHostname(hostname, port);
    }
  }
}

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