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 FIPS regression in 4.x #6110

Closed
wants to merge 1 commit into from
Closed

fix FIPS regression in 4.x #6110

wants to merge 1 commit into from

Conversation

sneak
Copy link

@sneak sneak commented Apr 8, 2016

Checklist
Affected core subsystem(s)

tiny change to lib/_tls_wrap.js

Description of change

This fixes a regression introduced in 4.2.4 where under certain circumstances (I believe mocha test framework, but didn't isolate) the code in getDefaultSessionIdContext() would throw an exception because process.config.variables was undefined and thus doesn't have attributes to access.

Why the project's libs in use were mucking about in process.config like that we'll never know, but this defensive patch makes it work again (like in 4.2.3).

#3755 (comment)

process.config.variables &&
'openssl_fips' in process.config.variables &&
process.config.variables.openssl_fips
) {
Copy link
Member

Choose a reason for hiding this comment

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

this can simply be:

if (process.config && process.config.variables && process.config.variables.openssl_fips)

Using the in syntax is not common in core.

Copy link
Author

Choose a reason for hiding this comment

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

thanks for the feedback! will update now.

FWIW I was guarding against the case where they are truthy but perhaps not an object (lacking the required attribute).

@jasnell
Copy link
Member

jasnell commented Apr 8, 2016

Small nit but generally this is ok. My apologies, however, in that I wasn't clear on my first note. Fixes to v4.x need to be opened as pull requests to the v4.x-staging branch rather than directly against v4.x. We only land commits into v4.x when we're actually cutting a release. Doing so allows us to stage out what actually lands in any particular v4 release. Can I ask you to please make the change I suggest above, but close this PR and open a new one against v4.x-staging?

@sneak
Copy link
Author

sneak commented Apr 8, 2016

Per request, created new PRs against v4.x-staging and v5.x (couldn't find a v5.x-staging).

@sneak sneak closed this Apr 8, 2016
@jasnell
Copy link
Member

jasnell commented Apr 8, 2016

Thank you @sneak ... I appreciate it. Just an FYI, We use the staging branches for LTS branches only (v4.x-staging, v0.12-staging and v0.10-staging). Since v5 is not an LTS stream, there is no staging branch for it.

@sneak
Copy link
Author

sneak commented Apr 8, 2016

Makes perfect sense. Glad to help - may my hours lost yesterday not bite anyone else using mocha/supertest/whatever broke it. (I know the people upstream of the app I am using are going to hit this same bug when they upgrade their node version.)

@sneak sneak deleted the patch-2 branch April 8, 2016 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants