GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test: Verify request payload is uploaded consistently by awwright · Pull Request #34066 · nodejs/node

Conversation

@awwright

@awwright awwright commented Jun 26, 2020 •
edited by aduh95
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

This adds several HTTP tests testing for the behavior identified in #27880:

  • test-http-27880-bug.js — shows how, if you make a DELETE request with a payload, the payload will actually be written to the server where a second HTTP/1.1 request is expected; if the payload is a valid HTTP message, it will be interpreted as a second request by the server. (But the first request is sent with Connection: close so shouldn't the server close the connection by this point?)
  • test-http-27880-chunked.js — tests that the default behavior for ClientRequest#write should be a chunked encoding.
  • test-http-27880-content-length.js — tests that the default behavior for ClientRequest#end should be a standard message, with an automatically computed Content-Length header.
  • test-http-27880-empty.js — tests that the default behavior for ClientRequest#end should have no payload headers if there's no response body

Naming for the test files will probably need to be reconsidered; suggestions welcome.

Refs: #27880

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ronag and bricss reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to Node.js core tests and test infrastructure. label Jun 26, 2020
@awwright awwright mentioned this pull request Jun 26, 2020
No content length on DELETE and OPTIONS #27880
Open
@awwright
awwright force-pushed the issue-27880 branch from b4ce665 to 08a2d5d Compare June 26, 2020 06:43
@ronag ronag added the http Issues and PRs related to the http subsystem. label Jun 26, 2020
@awwright
awwright force-pushed the issue-27880 branch 2 times, most recently from 3a9b24a to 02d39b8 Compare June 26, 2020 08:02
jasnell
jasnell approved these changes Jun 26, 2020
View reviewed changes
@aduh95

aduh95 commented Oct 14, 2020

Copy link
Copy Markdown
Contributor

@awwright there are some failing tests which keep this from landing. Do you still want to work on this?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95 aduh95 added the stalled Issues and PRs manually marked as stalled and scheduled for automatic closure. label Oct 14, 2020
@github-actions

github-actions Bot commented Oct 14, 2020

Copy link
Copy Markdown
Contributor

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@awwright

awwright commented Oct 14, 2020

Copy link
Copy Markdown
Contributor Author

@aduh95 Those failed tests are bugs in the HTTP implementation that need to be fixed. However I'm not sure what the process is for doing this, exactly.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95 aduh95 removed the stalled Issues and PRs manually marked as stalled and scheduled for automatic closure. label Oct 14, 2020
@aduh95

aduh95 commented Oct 14, 2020

Copy link
Copy Markdown
Contributor

I think the process is to move those tests to test/known_issues.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@awwright
awwright force-pushed the issue-27880 branch from 02d39b8 to c722a16 Compare October 15, 2020 01:08
@awwright
awwright requested a review from jasnell October 15, 2020 01:11
@awwright

awwright commented Oct 15, 2020

Copy link
Copy Markdown
Contributor Author

@aduh95 I rebased and moved the tests, I'm hoping that is sufficient?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95

aduh95 commented Oct 15, 2020

Copy link
Copy Markdown
Contributor

Regarding the naming of the file, what about:

  • test-http-request-method-delete-payload.js
  • test-http-clientrequest-write-chunked.js
  • test-http-clientrequest-end-contentlength.js
  • test-http-clientrequest-end-empty-response-body.js
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@awwright

awwright commented Oct 15, 2020

Copy link
Copy Markdown
Contributor Author

That seems reasonable.

It's not obvious in how the files are written, but I'm trying to test the behavior in both directions, too. I actually forget off-hand if the bug is in requests only, or both request and responses.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95

aduh95 commented Nov 8, 2020

Copy link
Copy Markdown
Contributor

@awwright Do you want to rename those files so we can move forward with this?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95 aduh95 added the stalled Issues and PRs manually marked as stalled and scheduled for automatic closure. label Nov 8, 2020
@github-actions

github-actions Bot commented Nov 8, 2020

Copy link
Copy Markdown
Contributor

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions

github-actions Bot commented Jan 9, 2021

Copy link
Copy Markdown
Contributor

Closing this because it has stalled. Feel free to reopen if this PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot closed this Jan 9, 2021
@ronag

ronag commented Apr 25, 2021

Copy link
Copy Markdown
Member

@aduh95 @dnlup @awwright any chance of revival?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag

ronag commented Apr 25, 2021

Copy link
Copy Markdown
Member

@nodejs/http
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag ronag reopened this Apr 25, 2021
@ronag ronag added the review wanted PRs that need review. label Apr 25, 2021
@dnlup

dnlup commented Apr 26, 2021

Copy link
Copy Markdown
Contributor

I am a little swamped atm. I don't think I am going to be able to work on it now. Sorry.
ronag reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott Trott removed the stalled Issues and PRs manually marked as stalled and scheduled for automatic closure. label Dec 15, 2021
@98lenvi 98lenvi mentioned this pull request Sep 26, 2022
test: Verify request payload is uploaded consistently #44801
Closed
@awwright

awwright commented Oct 3, 2022

Copy link
Copy Markdown
Contributor Author

What else exactly does this need? Can we not merge a test that's showing an actual, live bug?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott
Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49
@awwright awwright mentioned this pull request Nov 30, 2023
HTTP requests with no Host header accepted #39033
Closed
@aduh95
aduh95 force-pushed the issue-27880 branch 2 times, most recently from 7cb53b7 to b753797 Compare May 11, 2024 17:20
@awwright @98lenvi @aduh95
test: verify request payload is uploaded consistently
1f659d4 
Node.js seems to change how it is uploaded based on the method,
but HTTP doesn't make any distinction.

Co-authored-by: Austin Wright <[email protected]>
Co-authored-by: Lenvin Gonsalves <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
@aduh95
aduh95 force-pushed the issue-27880 branch from b753797 to 1f659d4 Compare May 11, 2024 17:31
@aduh95 aduh95 added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label May 11, 2024
@nodejs-github-bot

nodejs-github-bot commented May 11, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59125/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented May 12, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59142/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented May 12, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59176/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95
aduh95 merged commit d4e365f into nodejs:main May 12, 2024
@aduh95

aduh95 commented May 12, 2024

Copy link
Copy Markdown
Contributor

Landed in d4e365f
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
targos pushed a commit that referenced this pull request May 12, 2024
@awwright @98lenvi
@aduh95 @targos
test: verify request payload is uploaded consistently
578868d 
Node.js seems to change how it is uploaded based on the method,
but HTTP doesn't make any distinction.

Co-authored-by: Austin Wright <[email protected]>
Co-authored-by: Lenvin Gonsalves <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
PR-URL: #34066
Refs: #27880
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request May 13, 2024
v22.2.0 release proposal #52971
Merged
@marco-ippolito marco-ippolito mentioned this pull request Jun 17, 2024
v20.15.0 proposal #53486
Merged
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@awwright @98lenvi
@aduh95 @marco-ippolito
test: verify request payload is uploaded consistently
1d01325 
Node.js seems to change how it is uploaded based on the method,
but HTTP doesn't make any distinction.

Co-authored-by: Austin Wright <[email protected]>
Co-authored-by: Lenvin Gonsalves <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
PR-URL: #34066
Refs: #27880
Reviewed-By: James M Snell <[email protected]>
soophoo pushed a commit to soophoo/node that referenced this pull request Jun 20, 2024
@awwright @98lenvi
@aduh95 @soophoo
test: verify request payload is uploaded consistently
760dd94 
Node.js seems to change how it is uploaded based on the method,
but HTTP doesn't make any distinction.

Co-authored-by: Austin Wright <[email protected]>
Co-authored-by: Lenvin Gonsalves <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
PR-URL: nodejs#34066
Refs: nodejs#27880
Reviewed-By: James M Snell <[email protected]>
@zeexzeex zeexzeex mentioned this pull request Aug 24, 2026
http: frame client requests that carry a payload #65512
Open
zeexzeex added a commit to zeexzeex/node that referenced this pull request Sep 3, 2026
@zeexzeex
http: frame client requests that carry a payload
124e439 
A client request whose method defaults to no chunked encoding (GET,
HEAD, DELETE, OPTIONS, TRACE) skipped both Content-Length and
Transfer-Encoding even when a payload was written. The payload was
still sent, so the peer read those bytes as the start of the next
request, which allows request smuggling.

Frame such requests like any other: use Content-Length when the length
is known and a chunked Transfer-Encoding when it is not. Server
responses and requests with no payload keep the previous behaviour,
and CONNECT is left untouched because its payload is tunnelled data
that must not be re-framed.

Headers can be rendered before anything is written, for example when
the headers are passed as an array or an Expect header is set. Track
whether a payload write actually happened so that case is not mistaken
for a body of unknown length.

The two known_issues tests covering this now pass and move to
parallel. They also asserted a response body for HEAD, which cannot
have one, and compared buffers with strictEqual, so both are corrected
in the move.

Fixes: nodejs#27880
Refs: nodejs#34066
Refs: nodejs#60718
Signed-off-by: Avocado <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell Awaiting requested review from jasnell

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. http Issues and PRs related to the http subsystem. review wanted PRs that need review. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@awwright @aduh95 @ronag @dnlup @nodejs-github-bot @jasnell @Trott
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.