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

Handle URL changes in latest Apple Platforms #148

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

cham-s
Copy link

@cham-s cham-s commented Feb 2, 2024

The latest Apple platforms automatically escape invalid URL characters - https://developer.apple.com/documentation/foundation/url/3126806-init

This aligns the behaviour across platform versions and Linux. Also updates a number of warnings in the tests

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.

Thanks!

Copy link

codecov bot commented Mar 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.55%. Comparing base (a935b63) to head (d145fc2).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #148      +/-   ##
==========================================
- Coverage   82.85%   82.55%   -0.30%     
==========================================
  Files           6        6              
  Lines         659      642      -17     
==========================================
- Hits          546      530      -16     
+ Misses        113      112       -1     
Files Coverage Δ
Sources/WebSocketKit/WebSocket+Connect.swift 98.86% <100.00%> (+0.27%) ⬆️

... and 5 files with indirect coverage changes

@0xTim
Copy link
Member

0xTim commented Apr 9, 2024

@cham-s just taking a look at this to see why CI is failing - it's failing to build on latest macOS for me because ossl_ssize_t differs in size. I also have some test failures once that's fixed and a couple more deprecations in the async tests if you want to take a look

@cham-s
Copy link
Author

cham-s commented Apr 15, 2024

@0xTim
I ran the tests indeed they fail with a message similar to the one shown inside the CI logs.
However the failures were already there prior to the PR.

func testBadURLInWebsocketConnect() async throws {
XCTAssertThrowsError(try WebSocket.connect(to: "%w", on: self.elg, onUpgrade: { _ in }).wait()) {
guard case .invalidURL = $0 as? WebSocketClient.Error else {
return XCTFail("Expected .invalidURL but got \(String(reflecting: $0))")
}
}
}

func testBadURLInWebsocketConnect() async throws {
do {
try await WebSocket.connect(to: "%w", on: self.elg, onUpgrade: { _ async in })
XCTAssertThrowsError({}())
} catch {
XCTAssertThrowsError(try { throw error }()) {
guard case .invalidURL = $0 as? WebSocketClient.Error else {
return XCTFail("Expected .invalidURL but got \(String(reflecting: $0))")
}
}
}
}

The tests expect "%w" to fail because it's not a valid URI.
But it doesn't fail because the logic inside the code relies on URL(string: String) initializer to perform the validation
URL(string: String) has no notion of WebSocket so it only performs a general check on the URL structure.

public static func connect(
to url: String,
headers: HTTPHeaders = [:],
configuration: WebSocketClient.Configuration = .init(),
on eventLoopGroup: EventLoopGroup,
onUpgrade: @Sendable @escaping (WebSocket) -> ()
) -> EventLoopFuture<Void> {
guard let url = URL(string: url) else {
return eventLoopGroup.any().makeFailedFuture(WebSocketClient.Error.invalidURL)
}
return self.connect(
to: url,
headers: headers,
configuration: configuration,
on: eventLoopGroup,
onUpgrade: onUpgrade
)
}

I verified it with a repl session:

swift repl

Welcome to Apple Swift version 5.10 (swiftlang-5.10.0.13 clang-1500.3.9.4).
Type :help for assistance.
  1> import Foundation
  2>  
  3> let strangeWebSocketString = "%w"
strangeWebSocketString: String = "%w"
  4> let url = URL(string: strangeWebSocketString)
url: Foundation.URL? = "%25w"
  5> url == nil
$R0: Bool = false

As a fix I allowed myself to try to strengthen the guard statement with a check for string with an explicit prefix of ws:// or wss://.

    public static func connect(
        to url: String,
        headers: HTTPHeaders = [:],
        configuration: WebSocketClient.Configuration = .init(),
        on eventLoopGroup: EventLoopGroup,
        onUpgrade: @Sendable @escaping (WebSocket) -> ()
    ) -> EventLoopFuture<Void> {
        guard
            url.hasPrefix("ws://") || url.hasPrefix("wss://"),
            let url = URL(string: url)
        else {
            return eventLoopGroup.any().makeFailedFuture(WebSocketClient.Error.invalidURL)
        }
        return self.connect(
            to: url,
            headers: headers,
            configuration: configuration,
            on: eventLoopGroup,
            onUpgrade: onUpgrade
        )
    }

I don't know if this is the best solution but it makes sure the caller first, inputs a string with a correct WebSocket scheme then let URL(string: String) perform additional checks.

@0xTim
Copy link
Member

0xTim commented Apr 19, 2024

Ok this is a change in behaviour in Swift - see https://developer.apple.com/documentation/foundation/url/3126806-init for an explanation

So I think the answer to this is use the new API if available https://developer.apple.com/documentation/foundation/url/4191020-init otherwise default to the old one. Another option if find a URL that still fails and update the test

@cham-s
Copy link
Author

cham-s commented Apr 22, 2024

@0xTim thanks for the links, I wasn't aware of this change. The code has been updated accordingly.

@0xTim 0xTim added the semver-patch Internal changes only label Apr 23, 2024
@0xTim 0xTim changed the title Update SSLTestHelpers, remove deprecation warnings in tests Handle URL changes in latest Apple Platforms Apr 23, 2024
@0xTim
Copy link
Member

0xTim commented Apr 23, 2024

Original description:

This is not much but after building the package a couple of warnings and errors appeared the following changes made them disappear.

  • Update the SSLTestHelpers.swift file with the updated version of NIOSSLTestHelpers.swift from the swift-nio-ssl package Tests directory.

  • Remove yellow warnings caused by deprecation of invoking the onPong closure with a single argument by using underscore to ignore the data argument.

// Before
ws.onPong {
    $0.close(promise: closePromise)
    promise.succeed()
}

// After
ws.onPong { webSocket, _ in
    webSocket.close(promise: closePromise)
    promise.succeed()
}

Update SSLTestHelpers, remove deprecation warnings

@0xTim
Copy link
Member

0xTim commented Apr 23, 2024

@cham-s I think we can remove the visionOS check - I don't mind the change of behaviour on that platform!

@cham-s
Copy link
Author

cham-s commented Apr 23, 2024

Ok, done

@cham-s
Copy link
Author

cham-s commented Apr 23, 2024

@0xTim tests are passing except for test / unit-tests / macos-unit (macos-13, ~14.3) .
I'm not sure what macos-13, ~14.3 means, I tried to guess and lowered the minimum version to macOS 14.3 but it still fails.

In any case it says extra argument 'encodingInvalidCharacters' meaning it passes the if statement

if #available(iOS 17.0, macOS 14.3, tvOS 17.0, watchOS 10.0, *) {
    optionalURL = URL(string: url, encodingInvalidCharacters: false)
} 

but doesn't recognise the API.

@gwynne
Copy link
Member

gwynne commented Apr 23, 2024

macos-13, ~14.3 means latest macOS 13 (Ventura) with Xcode 14.3 and its associated toolchains. A #available for macOS 14 should not be passing in that environment.

@gwynne
Copy link
Member

gwynne commented Apr 23, 2024

In any event, these URL changes are not correct; the result will be (even more) inconsistent behavior between platforms and platform versions, which is the opposite of what we want.

I withdraw this comment, it looks like I was confused as to what was actually happening here. Apologies for the noise!

@cham-s
Copy link
Author

cham-s commented Apr 24, 2024

@gwynne #148 (comment) thanks for the info.
In this case it's strange that it passes the if #available and doesn't resort to the else branch to handle the url using the macOS 13 API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Internal changes only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants