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

Handle absent 'crypto' as per nodejs docs #19100

Closed
mihailik opened this issue Oct 11, 2017 · 5 comments
Closed

Handle absent 'crypto' as per nodejs docs #19100

mihailik opened this issue Oct 11, 2017 · 5 comments
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue Good First Issue Well scoped, documented and has the green light Help Wanted You can do this

Comments

@mihailik
Copy link
Contributor

TypeScript Version: 2.4.0 / nightly (2.5.0-dev.201xxxxx)

TypeScript compiler relies on node's 'crypto' module unconditionally:

https://github.com/Microsoft/TypeScript/blob/6997e9b7316b081f8731d131139c7d7f1bba9092/src/compiler/sys.ts#L132

Whereas node's documentation for crypto module explicitly states:

Determining if crypto support is unavailable

It is possible for Node.js to be built without including support for the crypto module. In such cases, calling require('crypto') will result in an error being thrown.
let crypto;
try {
  crypto = require('crypto');
} catch (err) {
  console.log('crypto support is disabled!');
}

That means on a reduced node environment the compiler crashes unnecessarily. By unnecessarily I mean the support for crypto is already optional in the compiler:

https://github.com/Microsoft/TypeScript/blob/6997e9b7316b081f8731d131139c7d7f1bba9092/src/compiler/sys.ts#L67

https://github.com/Microsoft/TypeScript/blob/d7269f1386bb1ad4351a6f6d5f749e89cb14c2b7/src/compiler/watch.ts#L635

https://github.com/Microsoft/TypeScript/blob/67a6a9477f90d61955f92fb33e7b11491dbd17a5/src/compiler/program.ts#L165

Expected behavior:

TSC to work without 'crypto'

Actual behavior:

TSC fails without 'crypto'

@mihailik
Copy link
Contributor Author

Note that one place in the code actually does rely on createHash unconditionally. Which should be a bug, considering that it's declared as optional in sys.ts line 67 (see above).

https://github.com/Microsoft/TypeScript/blob/a7fa187fb2ac58892593e8936ac728d61a57eacb/src/compiler/program.ts#L135

@mhegazy
Copy link
Contributor

mhegazy commented Oct 11, 2017

That means on a reduced node environment the compiler crashes unnecessarily.

Can you elaborate on what scenarios require node without crypto?

@mihailik
Copy link
Contributor Author

mihailik commented Oct 11, 2017

@mhegazy three scenarios [UPDATED]:

  • running non-standard node in a resource- or security-constrained environment (see node#5611 for their explicit support of this scenario)
  • running in emulated environment (browserify, webpack etc.)
  • building node from source and omitting openssl/crypto for random reason (see StackOverflow question or another nodejs post)

Here's a very relevant piece in node's lib/internal/util.js L100:

const noCrypto = !process.versions.openssl;
exports.assertCrypto = function(exports) {
  if (noCrypto)
    throw new Error('Node.js is not compiled with openssl crypto support');
};

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Oct 18, 2017

We're going to need some sort of hashing mechanism at some point, we should just use that as a fallback for createHash. In the mean time, I'm not sure if there's any harm in being precautious.

@DanielRosenwasser DanielRosenwasser added the Bug A bug in TypeScript label Oct 18, 2017
@DanielRosenwasser DanielRosenwasser added this to the Future milestone Oct 18, 2017
@DanielRosenwasser DanielRosenwasser added Good First Issue Well scoped, documented and has the green light Help Wanted You can do this labels Oct 18, 2017
@mhegazy mhegazy modified the milestones: Future, TypeScript 2.8 Jan 26, 2018
@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Jan 26, 2018
@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue Good First Issue Well scoped, documented and has the green light Help Wanted You can do this
Projects
None yet
Development

No branches or pull requests

4 participants