Skip to content

Rack::Response should not generate invalid content-length header.#2219

Merged
ioquatix merged 1 commit into
rack:mainfrom
socketry:rack-response-content-length
Jul 11, 2024
Merged

Rack::Response should not generate invalid content-length header.#2219
ioquatix merged 1 commit into
rack:mainfrom
socketry:rack-response-content-length

Conversation

@ioquatix

@ioquatix ioquatix commented Jun 21, 2024

Copy link
Copy Markdown
Member

I'm experimenting with potential fixes for #2218.

@ioquatix ioquatix force-pushed the rack-response-content-length branch 6 times, most recently from 1ff018f to e0d6409 Compare July 10, 2024 06:10
@ioquatix

ioquatix commented Jul 10, 2024

Copy link
Copy Markdown
Member Author

The one Sinatra test failure that is affected by this part of the code:

  1) Failure:
StaticTest#test_handles_valid_byte_ranges_correctly_0 [test/static_test.rb:147]:
Incorrect Content-Length for bytes=0-0.
--- expected
+++ actual
@@ -1,3 +1 @@
-# encoding: US-ASCII
-#    valid: true
-"1"
+nil

However, the reason why this header is missing is because it is deleted by Sinatra's own code:

https://github.com/sinatra/sinatra/blob/main/lib/sinatra/base.rb#L298

It was being inadvertently added back in by Response#buffered_body! (invoked by MockResponse#initialize). In other words, it appears that the test is wrong, or Sinatra's code is wrong, and this wrongness was hidden by the previous implementation of MockResponse inadvertently adding the content-length header when none existed in the response itself.

cc @dentarg

@ioquatix ioquatix force-pushed the rack-response-content-length branch from e0d6409 to 2264cc4 Compare July 10, 2024 07:05
@ioquatix

ioquatix commented Jul 10, 2024

Copy link
Copy Markdown
Member Author

I'm removing the Sinatra tests from this PR, as I think it's almost good to go. I might have a go at adding some tests for the behaviour of buffered_body! not adding content-length if it's not present (would fail on 3-0-stable for example).

@ioquatix ioquatix mentioned this pull request Jul 10, 2024
Closed
@ioquatix ioquatix force-pushed the rack-response-content-length branch from 2264cc4 to fe3c95e Compare July 10, 2024 07:46
ioquatix
ioquatix commented Jul 10, 2024
View reviewed changes
Comment thread test/spec_response.rb
@ioquatix ioquatix force-pushed the rack-response-content-length branch from fe3c95e to 5a77e22 Compare July 10, 2024 08:10
@ioquatix ioquatix requested a review from jeremyevans July 10, 2024 08:28
@dentarg dentarg mentioned this pull request Jul 10, 2024
Closed
@ioquatix ioquatix force-pushed the rack-response-content-length branch 3 times, most recently from 7d98277 to f07b73c Compare July 10, 2024 09:23
jeremyevans
jeremyevans approved these changes Jul 10, 2024
View reviewed changes
Comment thread lib/rack/response.rb Outdated
dentarg added a commit to dentarg/sinatra that referenced this pull request Jul 10, 2024
@dentarg
Don't delete content-length header when Rack::Files is used
13f4dfb 
The class name changed in rack/rack@3936b90

We have been here before, for example with sinatra@1dfae3d

This was hidden until Rack 3.1, from Samuel Williams[1]:

> It was being inadvertently added back in by Response#buffered_body!
> (invoked by MockResponse#initialize). In other words, it appears that
> the test is wrong, or Sinatra's code is wrong, and this wrongness was
> hidden by the previous implementation of MockResponse inadvertently
> adding the content-length header when none existed in the response
> itself.

1: rack/rack#2219 (comment)
@ioquatix
Improve Rack::Response content-length header generation.
0ae201c 
- Generate the header when the length is zero.
- Don't generate invalid content-length headers.
@ioquatix ioquatix force-pushed the rack-response-content-length branch from f07b73c to 0ae201c Compare July 11, 2024 00:26
@ioquatix

ioquatix commented Jul 11, 2024

Copy link
Copy Markdown
Member Author

I did a final check with Sinatra external tests:

  1) Failure:
HelpersTest::TestLogger#test_logging_works_when_logging_is_enabled_0 [test/helpers_test.rb:1903]:
Expected false to be truthy.

  2) Failure:
StaticTest#test_handles_valid_byte_ranges_correctly_0 [test/static_test.rb:147]:
Incorrect Content-Length for bytes=0-0.
--- expected
+++ actual
@@ -1,3 +1 @@
-# encoding: US-ASCII
-#    valid: true
-"1"
+nil

These are expected failures, and should be fixed in Sinatra.

@ioquatix ioquatix merged commit 658081f into rack:main Jul 11, 2024
@ioquatix ioquatix deleted the rack-response-content-length branch July 11, 2024 00:40
ioquatix added a commit that referenced this pull request Jul 11, 2024
@ioquatix
Improve Rack::Response content-length header generation. (#2219)
f6f1510 
- Generate the header when the length is zero.
- Don't generate invalid content-length headers.
dentarg added a commit to dentarg/sinatra that referenced this pull request Jul 11, 2024
@dentarg
Don't delete content-length header when Rack::Files is used
b25a84e 
The class name changed in rack/rack@3936b90

We have been here before, for example with sinatra@1dfae3d

This was hidden until Rack 3.1, from Samuel Williams[1]:

> It was being inadvertently added back in by Response#buffered_body!
> (invoked by MockResponse#initialize). In other words, it appears that
> the test is wrong, or Sinatra's code is wrong, and this wrongness was
> hidden by the previous implementation of MockResponse inadvertently
> adding the content-length header when none existed in the response
> itself.

1: rack/rack#2219 (comment)
dentarg added a commit to sinatra/sinatra that referenced this pull request Jul 11, 2024
@dentarg
Don't delete content-length header when Rack::Files is used
a39c0c9 
The class name changed in rack/rack@3936b90

We have been here before, for example with 1dfae3d

This was hidden until Rack 3.1, from Samuel Williams[1]:

> It was being inadvertently added back in by Response#buffered_body!
> (invoked by MockResponse#initialize). In other words, it appears that
> the test is wrong, or Sinatra's code is wrong, and this wrongness was
> hidden by the previous implementation of MockResponse inadvertently
> adding the content-length header when none existed in the response
> itself.

1: rack/rack#2219 (comment)
zzak pushed a commit to zzak/sinatra that referenced this pull request May 23, 2025
@dentarg @zzak
Don't delete content-length header when Rack::Files is used
153f4ef 
The class name changed in rack/rack@3936b90

We have been here before, for example with sinatra@1dfae3d

This was hidden until Rack 3.1, from Samuel Williams[1]:

> It was being inadvertently added back in by Response#buffered_body!
> (invoked by MockResponse#initialize). In other words, it appears that
> the test is wrong, or Sinatra's code is wrong, and this wrongness was
> hidden by the previous implementation of MockResponse inadvertently
> adding the content-length header when none existed in the response
> itself.

1: rack/rack#2219 (comment)
zzak pushed a commit to sinatra/sinatra that referenced this pull request May 23, 2025
@dentarg @zzak
Don't delete content-length header when Rack::Files is used
adff0be 
The class name changed in rack/rack@3936b90

We have been here before, for example with 1dfae3d

This was hidden until Rack 3.1, from Samuel Williams[1]:

> It was being inadvertently added back in by Response#buffered_body!
> (invoked by MockResponse#initialize). In other words, it appears that
> the test is wrong, or Sinatra's code is wrong, and this wrongness was
> hidden by the previous implementation of MockResponse inadvertently
> adding the content-length header when none existed in the response
> itself.

1: rack/rack#2219 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeremyevans jeremyevans jeremyevans approved these changes

Assignees

No one assigned

Labels

Backport

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ioquatix @jeremyevans