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

Fix broken URI behaviors #3140

Merged
merged 10 commits into from Jan 22, 2024
Merged

Fix broken URI behaviors #3140

merged 10 commits into from Jan 22, 2024

Conversation

gwynne
Copy link
Member

@gwynne gwynne commented Jan 22, 2024

These changes are now available in 4.92.0

Numerous issues have arisen with the changes made to URI as a result of the fix for GHSA-r6r4-5pr8-gjcp. This update fixes all known issues and restores several changed URI behaviors (although, quite deliberately, not all of them), including new tests. Fixes #3133, #3135, #3137, and #3138.

Also addresses Sendable warnings in ContentEncoder, ContentDecoder, ContentContainer, PlaintextDecoder, PlaintextEncoder, URLQueryDecoder, URLQueryEncoder, URLQueryContainer, URLEncodedFormDecoder, and URLEncodedFormEncoder.

Shoutout to @weissi, @grahamburgsma, and @finestructure for their help tracking down the various problems, thank you all!

@gwynne gwynne added bug Something isn't working semver-minor Contains new API labels Jan 22, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jan 22, 2024

Codecov Report

Attention: 45 lines in your changes are missing coverage. Please review.

Comparison is base (0680f9f) 76.75% compared to head (c9d9c3c) 76.79%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3140      +/-   ##
==========================================
+ Coverage   76.75%   76.79%   +0.03%     
==========================================
  Files         211      211              
  Lines        8020     8084      +64     
==========================================
+ Hits         6156     6208      +52     
- Misses       1864     1876      +12     
Files Coverage Δ
...urces/Vapor/Content/ContainerGetPathExecutor.swift 94.11% <100.00%> (ø)
Sources/Vapor/Content/ContentCoders.swift 0.00% <ø> (ø)
Sources/Vapor/Content/JSONCoders+Content.swift 77.77% <ø> (ø)
Sources/Vapor/Content/PlaintextDecoder.swift 44.44% <100.00%> (ø)
Sources/Vapor/Content/URLQueryCoders.swift 0.00% <ø> (ø)
...rces/Vapor/Multipart/FormDataDecoder+Content.swift 77.77% <ø> (ø)
...s/Vapor/URLEncodedForm/URLEncodedFormEncoder.swift 68.34% <ø> (ø)
Sources/Vapor/Content/ContentContainer.swift 77.77% <0.00%> (ø)
Sources/Vapor/Content/URLQueryContainer.swift 76.47% <0.00%> (ø)
...rces/Vapor/Multipart/FormDataEncoder+Content.swift 77.77% <75.00%> (-3.48%) ⬇️
... and 3 more

... and 1 file with indirect coverage changes

@gwynne gwynne force-pushed the fix-broken-uri branch 2 times, most recently from ac91cbb to 63b7f60 Compare January 22, 2024 06:55
Copy link
Contributor

@MahdiBM MahdiBM left a comment

Choose a reason for hiding this comment

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

Looks fine to me but I'd wait for @0xTim too.

README.md Show resolved Hide resolved
Sources/Vapor/Utilities/URI.swift Show resolved Hide resolved
Tests/AsyncTests/AsyncRequestTests.swift Show resolved Hide resolved
Tests/VaporTests/URITests.swift Show resolved Hide resolved
Sources/Vapor/Utilities/URI.swift Outdated Show resolved Hide resolved
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

A few queries but overall looks good

README.md Outdated Show resolved Hide resolved
Sources/Vapor/Content/PlaintextEncoder.swift Show resolved Hide resolved
Sources/Vapor/Utilities/URI.swift Outdated Show resolved Hide resolved
@gwynne gwynne requested a review from 0xTim January 22, 2024 20:36
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

Just double checking we have tests that cover all the reported bugs. It looks like it but double checking

@gwynne
Copy link
Member Author

gwynne commented Jan 22, 2024

We do - there were three actual issues filed plus the one I opened as a central point of reference. All three original tickets have new individual tests marked with the issue numbers, plus the many additional tests of URI that were added in the original URI PR (and updated to be correct in this one).

@gwynne gwynne merged commit d5025b3 into main Jan 22, 2024
16 of 17 checks passed
@gwynne gwynne deleted the fix-broken-uri branch January 22, 2024 23:17
keniwhat pushed a commit to keniwhat/vapor that referenced this pull request Apr 27, 2024
* main:
  Patch configuration and log actual port on startup (vapor#3160)
  Update provider tests to 5.10 (vapor#3178)
  Migrate to Async NIOFileIO APIs (vapor#3167)
  Removed streamFile deprecation + deactivated advancedETagComparison by default (vapor#3177)
  Remove HeadResponder (vapor#3147)
  Advanced ETag Comparison now supported (vapor#3015)
  Enabled Request Decompression By Default (vapor#3175)
  HTTP2 Response Compression/Request Decompression (vapor#3126)
  Don't set ignore status for SIGTERM and SIGINT on Linux (vapor#3174)
  Fix typos across the codebase (vapor#3162)
  Fix some Sendable warnings on 5.10 (vapor#3158)
  Allow `HTTPServer`'s configuration to be dynamically updatable (vapor#3132)
  Fix issue when client disconnects midway through a stream (vapor#3102)
  Fix handling of "flag" URL query params (vapor#3151)
  Bump the dependencies group with 1 update (vapor#3148)
  Merge Async Tests (vapor#3141)
  Fix URI handling with multiple slashes and variable components. (vapor#3143)
  Fix broken URI behaviors (vapor#3140)

# Conflicts:
#	Package.swift
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working semver-minor Contains new API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Double encoded post requests after recent update (4.89.3 → 4.91.1)
4 participants