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

fix(#124): resolve all *.localhost to localhost, and fix ipv6 issue #1114

Merged
merged 2 commits into from
Dec 4, 2023

Conversation

BrandonGillis
Copy link
Contributor

@BrandonGillis BrandonGillis commented Dec 1, 2023

Description

This PR fix #124, by correctly handling :

This fix is heavily inspired by how PostmanRuntime is handling it (https:/postmanlabs/postman-runtime/blob/develop/lib/requester/core.js#L433), their handling is even more complex by overriding DNS lookup of node (https:/postmanlabs/postman-runtime/blob/develop/lib/requester/core.js#L285).

I did some tests on windows & linux, and this PR correctly fix subdomains, or ipv6 issues.

If you have any comments on this PR let me know !

Contribution Checklist:

  • The pull request only addresses one issue or adds one feature.
  • The pull request does not introduce any breaking changes
  • I have added screenshots or gifs to help explain the change if applicable.
  • I have read the contribution guidelines.
  • Create an issue and link to the pull request.

@BrandonGillis BrandonGillis changed the title Merge branch 'main' of https:/usebruno/bruno fix(#124): resolve all *.localhost to localhost Dec 1, 2023
@BrandonGillis BrandonGillis changed the title fix(#124): resolve all *.localhost to localhost fix(#124): resolve all *.localhost to localhost, and fix ipv6 issue Dec 2, 2023
@BrandonGillis
Copy link
Contributor Author

After some more thinking, I should probably do a dns lookup first and use the returned ip for localhost, and fallback to the ipv6 and ipv4 check if we can't connect to it, what do you think? @helloanoop

@helloanoop helloanoop merged commit e0969d6 into usebruno:main Dec 4, 2023
@helloanoop
Copy link
Contributor

Thanks for this fix @BrandonGillis !

@helloanoop
Copy link
Contributor

After some more thinking, I should probably do a dns lookup first and use the returned ip for localhost, and fallback to the ipv6 and ipv4 check if we can't connect to it, what do you think?

@BrandonGillis Based on the research you have done, you clearly are more knowledgable on this issue than me :). Based on my reading socket.connect(port, host) does not perform a dns connect internally, So I think it would make sense to do a dns look before we perform a socket.connect(port, host) call. This would also address edge cases where localhost entry in dns could be set to resolve to a custom host.

I would also recommend caching the check in a variable

const connectionCache = new Map(); // Cache to store checkConnection() results

const checkConnection = (host, port) =>
  new Promise((resolve) => {
    const key = `${host}:${port}`;
    const cachedResult = connectionCache.get(key);

    if (cachedResult !== undefined) {
      resolve(cachedResult);
    } else {
      const socket = new Socket();

      socket.once('connect', () => {
        socket.end();
        connectionCache.set(key, true); // Cache successful connection
        resolve(true);
      });

      socket.once('error', () => {
        connectionCache.set(key, false); // Cache failed connection
        resolve(false);
      });

      // Try to connect to the host and port
      socket.connect(port, host);
    }
  });

helloanoop added a commit that referenced this pull request Dec 4, 2023
@helloanoop
Copy link
Contributor

@BrandonGillis I had to temporarily disable this fix as it is breaking "localhost" resolution that used to work in my system. I am getting this error

cause: Error: getaddrinfo ENOTFOUND [::1]
      at GetAddrInfoReqWrap.onlookup [as oncomplete] (node:dns:71:26) {
    errno: -3008,
    code: 'ENOTFOUND',
    syscall: 'getaddrinfo',
    hostname: '[::1]'
  }

curl http://[::1]:6000/ping works on my system.

DNS resolution logs on my system

Resolving localhost

const dns = require('dns');

dns.lookup('localhost', (err, address, family) => {
  console.log('address: %j family: IPv%s', address, family);
});

// console: address: "::1" family: IPv6

Resolving::1
const dns = require('dns');

dns.lookup('[::1]', (err, address, family) => {
  if (err) {
    console.log(err);
    return;
  }

  console.log('address: %j family: IPv%s', address, family);
});

// console
// Error: getaddrinfo ENOTFOUND [::1]
//     at GetAddrInfoReqWrap.onlookup [as oncomplete] (node:dns:107:26) {
//   errno: -3008,
//   code: 'ENOTFOUND',
//   syscall: 'getaddrinfo',
//   hostname: '[::1]'
// }

Axios debugging

const axios = require('axios');

axios.get('http://[::1]:6000/ping')
  .then(function (response) {
    console.log(response.data);
  })
  .catch(function (error) {
    console.log(error);
  });

Above axios code when I run it independently fails on my system with the same -3008 error. I am assuming this code would work on your system @BrandonGillis

@helloanoop
Copy link
Contributor

@BrandonGillis I'd be willing to get on a call and fix this together. Let me know if this is possible, else no worries - we can continue to share findings here and get to a fix.

@BrandonGillis
Copy link
Contributor Author

Hello @helloanoop, I could be available at around 12h00 UTC+1 on the discord if you're available at that time, so we can take a look at the issue together. But I agree with you, I think we should first use a dns lookup, check the tcp connection and fallback to the fix I did if we can't reach it.

Regarding your issue I had it too on Linux with [::1], can't remember how as it disappeared after, I'll have to dig it out

@BrandonGillis
Copy link
Contributor Author

BrandonGillis commented Dec 4, 2023

Regarding your test with dns lookup

dns.lookup('[::1]', (err, address, family) => {
  if (err) {
    console.log(err);
    return;
  }

  console.log('address: %j family: IPv%s', address, family);
});

// console
// Error: getaddrinfo ENOTFOUND [::1]
//     at GetAddrInfoReqWrap.onlookup [as oncomplete] (node:dns:107:26) {
//   errno: -3008,
//   code: 'ENOTFOUND',
//   syscall: 'getaddrinfo',
//   hostname: '[::1]'
// }

this is normal, as it should be a hostname that's why it fail on [::1]

@BrandonGillis
Copy link
Contributor Author

After a quick search the issue your encountering on your machine is probably related to axios/axios#5333

@BrandonGillis
Copy link
Contributor Author

After some digging, it looks like there's a different behaviour from dns resolution / axios / nodejs on windows & linux :

Without my fix, here's the result on windows when using [::1] :

image

And now, without my fix on Linux with [::1] :
image

@helloanoop can you confirm that using http://[::1]:6000/ping that is working with curl on your machine, doesn't work on Bruno release v1.3.1, which have the fix disabled ?

@helloanoop
Copy link
Contributor

helloanoop commented Dec 4, 2023

@BrandonGillis Yes, I confirm http://[::1]:6000/ping gives me the error (v1.3.1 where your fix is disabled). I am testing on a Mac.

So it looks like below code would work in windows, but will error on Mac and Linux
@BrandonGillis Can you check if below code works on windows ?

const axios = require('axios');
const dns = require('dns');

axios.get('http://[::1]:6000/ping')
  .then(function (response) {
    console.log(response.data);
  })
  .catch(function (error) {
    console.log(error);
  });

dns.lookup('[::1]', (err, address, family) => {
  if (err) {
    console.log(err);
    return;
  }

  console.log('address: %j family: IPv%s', address, family);
});

Below are the console logs for above code on Mac

anoop:bruno-testbench/ (main✗) $ node test.js                                 [17:13:48]
AxiosError: getaddrinfo ENOTFOUND [::1]
    at AxiosError.from (/Users/anoop/bruno/bruno-testbench/node_modules/axios/dist/node/axios.cjs:837:14)
    at RedirectableRequest.handleRequestError (/Users/anoop/bruno/bruno-testbench/node_modules/axios/dist/node/axios.cjs:3016:25)
    at RedirectableRequest.emit (node:events:514:28)
    at eventHandlers.<computed> (/Users/anoop/bruno/bruno-testbench/node_modules/follow-redirects/index.js:14:24)
    at ClientRequest.emit (node:events:514:28)
    at Socket.socketErrorListener (node:_http_client:495:9)
    at Socket.emit (node:events:514:28)
    at emitErrorNT (node:internal/streams/destroy:151:8)
    at emitErrorCloseNT (node:internal/streams/destroy:116:3)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
  hostname: '[::1]',
  syscall: 'getaddrinfo',
  code: 'ENOTFOUND',
  errno: -3008,
  cause: Error: getaddrinfo ENOTFOUND [::1]
      at GetAddrInfoReqWrap.onlookupall [as oncomplete] (node:dns:118:26) {
    errno: -3008,
    code: 'ENOTFOUND',
    syscall: 'getaddrinfo',
    hostname: '[::1]'
  }
}
Error: getaddrinfo ENOTFOUND [::1]
    at GetAddrInfoReqWrap.onlookup [as oncomplete] (node:dns:107:26) {
  errno: -3008,
  code: 'ENOTFOUND',
  syscall: 'getaddrinfo',
  hostname: '[::1]'
}

@BrandonGillis
Copy link
Contributor Author

BrandonGillis commented Dec 4, 2023

I did test the code you provided on windows, and it works as expected :

address: "::1" family: IPv6
received Host : [::1]:6000

EDIT: I opened a new PR that should handle all cases correctly, and work as expected on windows & linux

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.

Localhost
2 participants