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(tests): avoid installing package from untrusted registry in integration tests [SECURITY] #8776

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kgeorgiou
Copy link

Summary
I noticed one of the tests installs a package from registry hxxps://example-registry-doesnt-exist[.]com with the --registry flag, with the intent to test the case where the registry doesn't exist.

However someone could register that domain and serve a package with malicious lifecycle scripts (e.g. "preinstall")

(I managed to register and park that to domain to prevent such scenario.)

This PR makes the following improvements:

  • Replaces .com with the .invalid TLD (RFC 6761)
  • Passes --ignore-scripts since lifecycle scripts are not relevant for this test
  • Ensures the test fails if installing from the nonexistent registry doesn't throw an exception

Test plan
-

expect(stdoutOutput.toString()).toMatch(/getaddrinfo ENOTFOUND example-registry-doesnt-exist\.com/g);
}
const yarnAdd = runYarn(['add', 'is-array', '--registry', registry, '--ignore-scripts'], {cwd});
await expect(yarnAdd).rejects.toThrow('getaddrinfo ENOTFOUND example-registry-doesnt-exist.invalid');

Choose a reason for hiding this comment

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

Note some DNSs time out on .invalid domains throwing EAI_AGAIN.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @ylemkimon - I wasn't aware of that. Updated the test to expect either ENOTFOUND or EAI_AGAIN from getaddrinfo.

Let me know what you think.

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.

2 participants