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: replace deprecated node:url methods #1802

Merged
merged 1 commit into from
Apr 25, 2024
Merged

Conversation

alumni
Copy link
Contributor

@alumni alumni commented Apr 25, 2024

Checklist

  • I have ensured my pull request is not behind the main or master branch of the original repository.
  • I have rebased all commits where necessary so that reviewing this pull request can be done without having to merge it first.
  • I have written a commit message that passes commitlint linting.
  • I have ensured that my code changes pass linting tests.
  • I have ensured that my code changes pass unit tests.
  • I have described my pull request and the reasons for code changes along with context if necessary.

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 titanism merged commit e996382 into ladjs:master Apr 25, 2024
5 checks passed
@alumni alumni deleted the fix-url-parse branch April 25, 2024 19:02
@titanism
Copy link
Collaborator

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
Copy link
Contributor Author

alumni commented Apr 25, 2024

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

@alumni
Copy link
Contributor Author

alumni commented Apr 25, 2024

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
Copy link
Collaborator

See #1803 (comment) @alumni

@wesleytodd
Copy link

wesleytodd commented Jul 23, 2024

Hey, I am pretty sure this is a breaking change that snuck in. And unfortunately this means some tests which the send module use to ensure you cannot path escape are broken because of it. new URL will flatten out request paths with .. and . segments but node:url did not have this behavior (I could probably do more to prove this, but I am pretty sure).

In normal circumstances this is a good thing, but in this case it means we cannot use supertest to test this case anymore. I am not sure it makes sense really to try and revert this, and send is using 6.2.2 so we expected some breaking changes in trying to upgrade, but just wanted to flag that this is technically breaking in a patch.

Secondly, I am not sure how we can use supertest and superagent anymore in these tests now that it is modifying the intended test behavior. Any ideas on how we could do this kind of test and still upgrade? https://github.com/pillarjs/send/blob/master/test/send.js#L1406-L1410

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.

3 participants