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

http2: set origin name correctly when servername is empty #42838

Merged
merged 1 commit into from
May 25, 2022

Conversation

ofirbarak
Copy link
Contributor

Fixes: #39919
Refs: #39934

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Apr 23, 2022
Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -3119,7 +3119,7 @@ function initializeTLSOptions(options, servername) {
options.ALPNProtocols = ['h2'];
if (options.allowHTTP1 === true)
ArrayPrototypePush(options.ALPNProtocols, 'http/1.1');
if (servername !== undefined && options.servername === undefined)
if (servername !== undefined && !options.servername)
Copy link
Contributor

@austinkelleher austinkelleher Apr 23, 2022

Choose a reason for hiding this comment

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

It looks like the referenced PR had the same change and @lpinca had left a comment about the behavior. https://github.com/nodejs/node/pull/39934/files#r699529426

I'm not entirely sure what the desired behavior is here but it's probably worth discussing. In the comment, @lpinca calls out the following commit: 98e9de7

Looking at the latest https documentation, you can see that servername is still documented the same way for https.Agent:

* `servername` {string} the value of

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your reply, I understand the problem much better now, although I think it is still wired we return https://false.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any conclusion yet on what needs to be done for this?

@mcollina
Copy link
Member

Can you brebase the change? I think this can land but we need to get ci passing

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@mcollina mcollina added commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels May 25, 2022
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels May 25, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/42838
✔  Done loading data for nodejs/node/pull/42838
----------------------------------- PR info ------------------------------------
Title      http2: set origin name correctly when servername is empty (#42838)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     ofirbarak:fix-invalid-origin-set -> nodejs:master
Labels     http2
Commits    1
 - http2: set origin name correctly when servername is empty
Committers 1
 - ofir 
PR-URL: https://github.com/nodejs/node/pull/42838
Fixes: https://github.com/nodejs/node/issues/39919
Refs: https://github.com/nodejs/node/pull/39934
Reviewed-By: Paolo Insogna 
Reviewed-By: Rafael Gonzaga 
Reviewed-By: Mohammed Keyvanzadeh 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/42838
Fixes: https://github.com/nodejs/node/issues/39919
Refs: https://github.com/nodejs/node/pull/39934
Reviewed-By: Paolo Insogna 
Reviewed-By: Rafael Gonzaga 
Reviewed-By: Mohammed Keyvanzadeh 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - http2: set origin name correctly when servername is empty
   ℹ  This PR was created on Sat, 23 Apr 2022 13:13:21 GMT
   ✔  Approvals: 3
   ✔  - Paolo Insogna (@ShogunPanda): https://github.com/nodejs/node/pull/42838#pullrequestreview-950964984
   ✔  - Rafael Gonzaga (@RafaelGSS): https://github.com/nodejs/node/pull/42838#pullrequestreview-950965332
   ✔  - Mohammed Keyvanzadeh (@VoltrexMaster): https://github.com/nodejs/node/pull/42838#pullrequestreview-951014402
   ⚠  GitHub cannot link the author of 'http2: set origin name correctly when servername is empty' to their GitHub account.
   ⚠  Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-05-25T07:19:57Z: https://ci.nodejs.org/job/node-test-pull-request/44157/
- Querying data for job/node-test-pull-request/44157/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/2383813741

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels May 25, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 25, 2022
@nodejs-github-bot nodejs-github-bot merged commit f9f22b4 into nodejs:master May 25, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in f9f22b4

bengl pushed a commit that referenced this pull request May 30, 2022
Fixes: #39919
Refs: #39934

PR-URL: #42838
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@bengl bengl mentioned this pull request May 31, 2022
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
Fixes: #39919
Refs: #39934

PR-URL: #42838
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this pull request Jul 12, 2022
Fixes: #39919
Refs: #39934

PR-URL: #42838
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
Fixes: #39919
Refs: #39934

PR-URL: #42838
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
Fixes: nodejs/node#39919
Refs: nodejs/node#39934

PR-URL: nodejs/node#42838
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid HTTP/2 origin set when servername is empty
8 participants