node-miniget icon indicating copy to clipboard operation
node-miniget copied to clipboard

validate http libs are present

Open TimeForANinja opened this issue 3 years ago • 5 comments

i am really thinking about renaming this https://github.com/fent/node-miniget/blob/master/src/index.ts#L165

or adding a check after https://github.com/fent/node-miniget/blob/master/src/index.ts#L11

just to make sure that people don't screw up when replacing them e.g. when using in a browser

TimeForANinja avatar Aug 04 '21 19:08 TimeForANinja

like placing a check for parsed.protocol in httpLibs?

fent avatar Jan 08 '22 13:01 fent

instead of doing

httpLib = httpLibs[String(parsed.protocol)];
if (!httpLib) {
  stream.emit('error', new Miniget.MinigetError(`Invalid URL: ${url}`));
  return;
}

we could do sth like

httpLib = httpLibs[String(parsed.protocol)];
if (!httpLibs.hasOwnProperty(parsed.protocol)) {
  stream.emit('error', new Miniget.MinigetError(`Invalid URL protocol: ${url}`));
  return;
} else if (!httpLib) {
  stream.emit('error', new Miniget.MinigetError(`unable to access http(s) library`));
  return;
}

TimeForANinja avatar Jan 12 '22 08:01 TimeForANinja

can you give an example of when the above would give the 2nd error?

fent avatar Jan 16 '22 11:01 fent

from what i understood an invalid polyfill would result in the require in this line https://github.com/fent/node-miniget/blob/master/src/index.ts#L9 sth. like null or undefined the usual !httpLib does not differentiate between httpLibs['https:'] being defined or being null means a !httpLib only tells us that it is not available atm (but not why) a hasOwnProperty would also tell us if it was ever assigned - even if undefined

short example: image

TimeForANinja avatar Jan 25 '22 08:01 TimeForANinja

ah i see what you mean now, this results in better error reporting. i support this

fent avatar Jan 25 '22 09:01 fent