search

ladjs / superagent

Ajax for Node.js and browsers (JS HTTP client). Maintained for @forwardemail, @ladjs, @spamscanner, @breejs, @cabinjs, and @lassjs.
https://ladjs.github.io/superagent/
MIT License
16.57k stars 1.33k forks source link

fix: replace deprecated `node:url` methods #1802

Closed alumni closed 2 months ago

alumni commented 2 months ago

Checklist

This PR replaces the deprecated node:url methods parse and resolve, which, as per NodeJS documentation , they are prone to security vulnerabilities.

They also have a non-standard behavior for which a workarounds are needed - e.g.: a workaround for escaping backticks was already implemented (and is being removed in this PR), but there is no similar workaround for single quotes.

To avoid the security vulnerabilities and the non-standard behavior, we can easily replace these methods with the standard-compliant URL Web API.

titanism commented 2 months ago

Hi there @alumni - it looks like npm test is not working as the linting fails, e.g. npx xo fails linting. Also there are other cases where node: prefix is not being used and old url approach is still there (e.g. tests folder). Can you fix all these? You may need to bump xo and eslint deps to fix linting warnings and update .xo-config.js.

alumni commented 2 months ago

I'll try to take a look. The multipart test times out for me even in the commit before the merge.

alumni commented 2 months ago

I managed to replace the deprecated parse in other places as well. make test-node seems to have identical output as the one in other recent commits on the master branch. I can submit a PR for that.

Regarding the linting problem - unless there's a known to be working npm lockfile, it will be quite difficult to figure out what's wrong - xo has changed completely since it was last upgraded, so upgrading it now seems to be a major task.

titanism commented 2 months ago

See https://github.com/ladjs/superagent/pull/1803#issuecomment-2079879220 @alumni