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

Remove all runtime Javascript dependencies #501

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matthieusieben
Copy link

@matthieusieben matthieusieben commented Nov 24, 2024

  1. Most of package.json dependencies are actually devDependencies
  2. The dependency on node-range can be easily removed

Zero runtime deps makes this package less susceptible to dependency attacks.

Copy link
Collaborator

@lyrixx lyrixx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

LGTM, but I prefer a review from @FGRibreau before merging

@lyrixx lyrixx requested a review from FGRibreau November 29, 2024 10:32
@FGRibreau
Copy link
Owner

@lyrixx I'm sorry, I currently don't have a lot of free time, can anyone else review this so we're sure we don't have any regressions?

@YasharF
Copy link
Contributor

YasharF commented Jan 16, 2025

... I'm sorry, I currently don't have a lot of free time, can anyone else review this so we're sure we don't have any regressions?

@FGRibreau Has your schedule got any better to look into this, or do you still need help from the community for this review? I noticed some glob deprecation notice for versions prior to v9, but I think it might be worth moving it to a devdependency before upgrading it.

@YasharF
Copy link
Contributor

YasharF commented Jan 16, 2025

Here is my comment to help out:
The isBlacklisted in the PR does a memory/performance trade-off when we have to go thru the domain string. It would use less memory because unlike the range approach we are not creating the entire list, but moves the algorithm from O(n) to O(n^2). I don't know if the trade-off would be a good idea or not. If we don't want to make the memory trade-off, we can just change allDomainSuffixes and keep isBlacklisted as is with something like:

function allDomainSuffixes(email) {
  var domainComponents = email.split('@')[1].split('.');
  var domainSuffixes = [];
  for (var i = 0; i < domainComponents.length; i++) {
    domainSuffixes.push(domainComponents.slice(i).join('.'));
  }
  return domainSuffixes;
}

With that said, that brings up a question of if we have a potential vulnerability where a malicious actor may send the server a bogus email address with a large chain of subdomains to ddos (high cpu) or crash the server (out of memory). I don't know if we check for that somewhere else in the code but if we don't, I think RFC 1034/1035 define a 63 octet/253 char limit for domain names. That can still create quite a few iterations so I think we may want to have a default limit that is more reasonable like 5 or 10 with an optional parameter for the users of the module to increase it if they really have a valid usecase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants