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

UnhandledPromiseRejectionWarning: TypeError [ERR_INVALID_URL]: Invalid URL #604

Closed
przemyslawpluta opened this issue Sep 7, 2018 · 1 comment
Labels
bug Something does not work as it should

Comments

@przemyslawpluta
Copy link

przemyslawpluta commented Sep 7, 2018

As per below example domain redirects 302 to invalid host name empty http:// this breaks got and throws UnhandledPromiseRejectionWarning regardless of any attempt to catch errors.

const got = require('got');

(async () => {
    try {
        const response = await got('fashion-bcom-prod.herokuapp.com');
        console.log(response.body);
    } catch (error) {
        console.log(error.response.body);
    }
})();
Possibly Unhandled Rejection at: Promise  Promise {
  <rejected> { TypeError [ERR_INVALID_URL]: Invalid URL: //
    at onParseError (internal/url.js:219:17)
    at parse (internal/url.js:228:3)
    at new URL (internal/url.js:311:5)
    at cacheableRequest (/wvp/node_modules/got/source/request-as-event-emitter.js:123:20)
    at ClientRequest.handler (/wvp/node_modules/cacheable-request/src/index.js:118:7)
    at Object.onceWrapper (events.js:315:30)
    at emitOne (events.js:121:20)
    at ClientRequest.emit (events.js:211:7)
    at ClientRequest.origin.emit.args [as emit] (/wvp/node_modules/@szmarczak/http-timer/source/index.js:36:11)
    at HTTPParser.parserOnIncomingClient (_http_client.js:551:21) input: '//' } }  reason:  { TypeError [ERR_INVALID_URL]: Invalid URL: //
    at onParseError (internal/url.js:219:17)
    at parse (internal/url.js:228:3)
    at new URL (internal/url.js:311:5)
    at cacheableRequest (/wvp/node_modules/got/source/request-as-event-emitter.js:123:20)
    at ClientRequest.handler (/wvp/node_modules/cacheable-request/src/index.js:118:7)
    at Object.onceWrapper (events.js:315:30)
    at emitOne (events.js:121:20)
    at ClientRequest.emit (events.js:211:7)
    at ClientRequest.origin.emit.args [as emit] (/wvp/node_modules/@szmarczak/http-timer/source/index.js:36:11)
    at HTTPParser.parserOnIncomingClient (_http_client.js:551:21) input: '//' }
@sindresorhus sindresorhus added the bug Something does not work as it should label Sep 7, 2018
@alextes
Copy link
Contributor

alextes commented Sep 7, 2018

Sadly I hit my time limit for this issue.

I have a fix, but sadly no test.
To clarify, got sends a request, a redirect comes back, the redirect neatly contains a Location: // header as it should, but the location is not a valid URL. Bad server 🙅‍♀️!

So got tries to helpfully redirect but in constructing the redirect URL blows up.
Fix is:

diff --git a/source/request-as-event-emitter.js b/source/request-as-event-emitter.js
index 3187657..6bb9454 100644
--- a/source/request-as-event-emitter.js
+++ b/source/request-as-event-emitter.js
@@ -120,7 +120,12 @@ module.exports = options => {
                                }

                                const bufferString = Buffer.from(response.headers.location, 'binary').toString();
-                               redirectUrl = (new URL(bufferString, urlLib.format(options))).toString();
+                               try {
+                                       redirectUrl = (new URL(bufferString, urlLib.format(options))).toString();
+                               } catch (error) {
+                                       emitter.emit('error', new RequestError(error, options));
+                                       return;
+                               }

                                try {
                                        decodeURI(redirectUrl);

Sadly, in trying to write the test for it I've gotten stuck on figuring out why the test gets stuck. The following hangs:

test.only('bad URL should not result in unhandled exception', async t => {
	await t.throwsAsync(() => got('//'));
})

Do the same in Node: got('//') and you immediately receive an unhandled rejection. If I were to guess I'd suppose ava has some protection in place to not blow up when a test blows up with an unhandled rejection, but because got violently explodes the returned promise by got never resolves.

For the one adding in a proper test here's what I have so far, it includes a quick curl exec to verify the new /invalid-redirect route is working.

Add this to the test/arguments.js server setup:

	s.on('/invalid-redirect', (_, response) => {
		response.writeHead(302, {
			Location: '//'
		});
		response.end();
	});

Test

const {exec} = require('child_process');

test('bad redirect should not result in unhandled exception', async t => {
	console.log(s.url)
	exec(`curl -sLD - ${s.url}/invalid-redirect`, (error, stdout) => {
		console.log('cURL ERR\n', error)
		console.log('cURL OUT\n', stdout);
	});
	await t.throwsAsync(() => got(`${s.url}/invalid-redirect`));
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something does not work as it should
Projects
None yet
Development

No branches or pull requests

3 participants