soto-project / soto-core

Core framework of Soto the Swift SDK for AWS
https://soto.codes
Apache License 2.0
111 stars 51 forks source link

AWSSigner processURL does not handle query parameters with no equal sign or value correctly #523

Closed mildm8nnered closed 2 years ago

mildm8nnered commented 2 years ago

Describe the bug

So I think there's an issue with the implementation of processURL in AWSSigner. Consider a request to begin a multipart upload, with bucket "MyBucket", the path according to Amazon should be "MyBucket?uploads".

But for signing, Amazon considers the encoded query string to be uploads=, and the = is important. Amazon will fail signatures that do not use that form as the second line of the canonical request string constructed during signing.

The issue is that AWSSigner.processURL() does not add the = in this case. At or around line 65, I think

        let urlQueryString = urlComponents.queryItems?
            .sorted {
                if $0.name < $1.name { return true }
                if $0.name > $1.name { return false }
                guard let value1 = $0.value, let value2 = $1.value else { return false }
                return value1 < value2
            }
            .map { item in item.value.map { "\(item.name)=\($0.uriEncode())" } ?? item.name }
            .joined(separator: "&")

really wants to be

        let urlQueryString = urlComponents.queryItems?
            .sorted {
                if $0.name < $1.name { return true }
                if $0.name > $1.name { return false }
                guard let value1 = $0.value, let value2 = $1.value else { return false }
                return value1 < value2
            }
            .map { item in item.value.map { "\(item.name)=\($0.uriEncode())" } ?? "\(item.name)=" }
            .joined(separator: "&")

Assuming I'm right, it's really surprising to me that this hasn't come up before. I grabbed soto-s3-file-transfer, but looking there, the begin multipart request gets passed through another very similar piece of code, in AWSRequest.swift (around line 285), that performs an almost identical operation, but taking care of nil values correctly:

        // Set query params. Percent encode these ourselves as Foundation and AWS disagree on what should be percent encoded in the query values
        // Also the signer doesn't percent encode the queries so they need to be encoded here
        if queryParams.count > 0 {
            let urlQueryString = queryParams
                .map { (key: $0.key, value: "\($0.value)") }
                .sorted {
                    // sort by key. if key are equal then sort by value
                    if $0.key < $1.key { return true }
                    if $0.key > $1.key { return false }
                    return $0.value < $1.value
                }
                .map { "\($0.key)=\(Self.urlEncodeQueryParam($0.value))" }
                .joined(separator: "&")
            urlComponents.percentEncodedQuery = urlQueryString
        }

To Reproduce

Steps to reproduce the behavior:

Just pass a URL with path MyBucket?uploads to AWSSigner.processURL() to see that it does not add the =

Expected behavior

processed URLs should be in the correct format for Amazon canonical request string

Setup (please complete the following information):

Additional context Add any other context about the problem here.

adam-fowler commented 2 years ago

processURL isn't actually used by SotoCore service operations. It is there just to provide support for signing URLs. That is why this hasn't been caught earlier. If you want to add a PR for this that would be most helpful.

mildm8nnered commented 2 years ago

https://github.com/soto-project/soto-core/pull/524