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

Add dnsLookup option to allow custom blacklist/whitelist logic #1

Merged
merged 5 commits into from
Nov 7, 2022

Conversation

jonhaddow
Copy link

@jonhaddow jonhaddow commented Nov 7, 2022

As described in this upstream issue, this PR introduces a dnsLookup configuration option which allows for custom logic to whitelist or blacklist requests to specific hostnames.

To test the change, start the server locally and check that hostnames starting with loopback or site local IPs are blocked.

@@ -268,6 +286,9 @@ function getHandler(options, proxy) {
setHeaders: {}, // Set these request headers.
corsMaxAge: 0, // If set, an Access-Control-Max-Age header with this value (in seconds) will be added.
helpFile: __dirname + '/help.txt',
dnsLookup: function(hostname, callback) {
dns.lookup(hostname, {hints: dns.ADDRCONFIG}, callback);

Choose a reason for hiding this comment

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

Does this not need to include the

    if (hostname.startsWith('169.254.')) {
      throw new Error();
    }

logic? Also, we need to exclude loopback addresses (IPv4 addresses starting with 127. or IPv6 addresses starting with ::1) and site local addresses (IPv4 addresses starting with 10., 172.16. or 192.168., you might have to ask Google what the IPv6 equivalents are...)

Copy link
Author

@jonhaddow jonhaddow Nov 7, 2022

Choose a reason for hiding this comment

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

Yeah, I can include that in this PR. Was wondering if it should be a separate PR specific to our use case.

I guess any PR submitted upstream can have it separated.

Copy link

@AndyNeale AndyNeale Nov 7, 2022

Choose a reason for hiding this comment

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

Marc's request was

Before we can make the corsproxy available to customers again we need to ensure it won't respond to any requests that are address.isLoopbackAddress() || address.isSiteLocalAddress() or 169.254.

so it should be fine to tackle all of those use cases in a single PR (and you're right, we could always tidy up the code in future by adding separate isLoopbackAddress and isLocalAddress methods)...

@jonhaddow jonhaddow assigned AndyNeale and unassigned jonhaddow Nov 7, 2022
Copy link

@AndyNeale AndyNeale left a comment

Choose a reason for hiding this comment

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

Looks good to me...

@jonhaddow jonhaddow merged commit c1e2487 into master Nov 7, 2022
@jonhaddow jonhaddow deleted the feature/dns_lookup branch November 7, 2022 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants