Skip to content

Fix default ecdh curve for better compatibility#2159

Merged
astorije merged 1 commit intomasterfrom
xpaw/ecdh-auto-curves
Mar 7, 2018
Merged

Fix default ecdh curve for better compatibility#2159
astorije merged 1 commit intomasterfrom
xpaw/ecdh-auto-curves

Conversation

@xPaw
Copy link
Copy Markdown
Member

@xPaw xPaw commented Mar 6, 2018

nodejs/node#16196
nodejs/node#16853

This isn't exactly a problem with The Lounge, but as we support LTS versions, we increase compatibility by including a fix for these Node versions.

@xPaw xPaw added this to the 3.0.0 milestone Mar 6, 2018
@dgw
Copy link
Copy Markdown
Contributor

dgw commented Mar 6, 2018

The bug this change fixes manifests as previews failing to load for a small set of websites when The Lounge is run with certain NodeJS versions. TLS cipher negotiation fails because Node's default is not properly set (nodejs/node#16196).

One workaround is to downgrade Node to a version from before the change (v8.5.0). This code change (to auto-negotiate the TLS cipher by default) is the other known workaround, which fixes the issue when running The Lounge under Node v8.6+ and v9.

Node v10 will fix this at the source (nodejs/node#16853), but this will still be needed to support LTS versions until at least December 31, 2019 (when Node v8 support is planned to end).

Copy link
Copy Markdown
Contributor

@dgw dgw left a comment

Choose a reason for hiding this comment

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

As the reporter of this issue, and the one who tracked down the referenced NodeJS ticket, I have tested this change and found it to fix the problem I encountered with loading previews on certain domains. 👍

Copy link
Copy Markdown
Member

@AlMcKinlay AlMcKinlay left a comment

Choose a reason for hiding this comment

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

Oh goodness that's so dumb.

Copy link
Copy Markdown
Member

@astorije astorije left a comment

Choose a reason for hiding this comment

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

Oh boy.

Does this affect all versions of v6 and v8? If not, what are the v6/8 versions that are fixing this?

I don't think we have to support this forever or until v10 is the minimal version we support. Supporting LTS versions doesn't mean we have to support all of v6 and all of v8. It's pretty common to see packages that support v6 while specifying >=6.9.0 for example.
In fact, looking at https://nodejs.org/en/download/releases/, it seems that only >=6.9.0 and >= 8.9.0 are LTS versions...

@astorije astorije merged commit c3ed4eb into master Mar 7, 2018
@astorije astorije deleted the xpaw/ecdh-auto-curves branch March 7, 2018 00:53
@astorije astorije added the Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors. label Mar 7, 2018
@dgw
Copy link
Copy Markdown
Contributor

dgw commented Mar 7, 2018

@astorije This affects Node >= 8.6.0, so v8 LTS is included.

#2163 handles restricting the fix to only affected Node versions, so it should be all set until v10 drops.

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

Labels

Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants