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.

Sign up for GitHub

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

Jump to bottom

Add support for various deflate compression levels #309

Merged
merged 4 commits into from
Oct 11, 2021
Merged

Add support for various deflate compression levels #309

merged 4 commits into from
Oct 11, 2021

Conversation

soulseekah
Copy link
Contributor

Different compression levels yield a specific second byte in the magic
header for the deflate encoding. All of them can be decoded by the same
functions without any issues. Let them through, as they've been seen in
the wild.

Fixes #301

@soulseekah soulseekah mentioned this pull request Feb 10, 2018
Closed
rmccue
rmccue previously requested changes Feb 12, 2018
View reviewed changes
Copy link
Collaborator

@rmccue rmccue left a comment

Choose a reason for hiding this comment

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

This also needs tests in the Encoding.php suite.

library/Requests.php Outdated
if (substr($data, 0, 2) !== "\x1f\x8b" && substr($data, 0, 2) !== "\x78\x9c") {
// All valid deflate, gzip header magic markers
$valid_magic = array("\x1f\x8b", "\x78\x01", "\x78\x5e", "\x78\x9c", "\x78\xda");
if (!in_array(substr($data, 0, 2), $valid_magic)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to use the $strict parameter to in_array, but otherwise looks good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in follow up commit. Tests included.

@codecov-io
Copy link

Codecov Report

Merging #309 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #309      +/-   ##
============================================
+ Coverage     92.11%   92.11%   +<.01%     
+ Complexity      760      759       -1     
============================================
  Files            21       21              
  Lines          1762     1763       +1     
============================================
+ Hits           1623     1624       +1     
  Misses          139      139
Impacted Files Coverage Δ Complexity Δ
library/Requests.php 75.67% <100%> (+0.08%) 117 <0> (-1) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4055bc4...6815ae6. Read the comment docs.

@soulseekah
Copy link
Contributor Author

soulseekah commented Feb 12, 2018 •
edited
Loading

Added strict in_array checking, added compression level tests (generated using zpipe.c from the zlib 1.2.11 using levels 1, 3 and 9, 5 was already being tested as the generic case). Also added Encoding.php to the test suite, it was not explicitly defined in phpunit.xml.dist and thus was not being run at all.

@jrfnl jrfnl changed the base branch from stable to develop June 18, 2021 09:55
@jrfnl
Copy link
Member

jrfnl commented Sep 12, 2021

@schlessera I've just been looking at this PR. This seems like a good change. What do you think ?

To get this ready to merge, IMO, the following actions are needed:

  • Rebase the PR.
    This should automatically remove the change to tests/phpunit.xml.dist. If not, that change needs to be removed as this is now fixed more thoroughly already.
  • Move the $valid_magic array to a class constant and document the source of the magic markers.
    Nice to have/micro-optimization: Use the markers as the keys in the array instead of as the values and use isset() instead of in_array().

@soulseekah @jrfnl
Add support for various deflate compression levels
a2c8660 
Different compression levels yield a specific second byte in the magic
header for the deflate encoding. All of them can be decoded by the same
functions without any issues. Let them through, as they've been seen in
the wild.

Fixes WordPress#301
@jrfnl jrfnl added this to the 2.0.0 milestone Sep 17, 2021
@jrfnl
Copy link
Member

jrfnl commented Sep 17, 2021

I've made the changes as per the above comment. This PR is now ready for review/merge.

@soulseekah @jrfnl
Add strict in_array checking, expand deflate tests
b0eea74 
Strict check magic zlib magic markers.
Add zlib compression level tests.
~~Enable encoding tests, these were not included in the test suite.~~
@jrfnl
Requests::decompress(): move magic markers to class property
c13e6bc 
... and document the source and what the marker is indicating.

Includes switching to using `isset()` instead of `in_array()` for a tiny performance boost.
@jrfnl jrfnl dismissed rmccue’s stale review October 4, 2021 22:44

Outdated/superseded

schlessera
schlessera approved these changes Oct 11, 2021
View reviewed changes
src/Requests.php Outdated Show resolved Hide resolved
soulseekah
soulseekah commented Oct 11, 2021
View reviewed changes
src/Requests.php
@@ -187,7 +187,7 @@ class Requests {
/**
* All (known) valid deflate, gzip header magic markers.
*
* These markers related to different compression levels.
* These markers relate to different compression levels.
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 :)

@schlessera schlessera merged commit 1d20025 into WordPress:develop Oct 11, 2021
@schlessera
Copy link
Member

Thanks for the PR, @soulseekah !

This was referenced Nov 5, 2021
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schlessera schlessera schlessera approved these changes

@rmccue rmccue rmccue left review comments

Assignees
No one assigned
Labels
Component: Core Status: triage Type: bug
Projects
None yet
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.

Requests::decompress falsely determines content is not compressed
5 participants
@soulseekah @codecov-io @jrfnl @schlessera @rmccue