Nugine / s3s

S3 Service Adapter
Apache License 2.0
132 stars 33 forks source link

CORS / OPTIONS pre-flight not implemented #144

Closed St4NNi closed 1 month ago

St4NNi commented 5 months ago

While playing around with browser based clients we encountered a problem with CORS / OPTIONS pre-flight requests.

The problem is that resolve_route does not support the OPTIONS HTTP method, only HEAD / GET / POST / PUT and DELETE are supported resulting in a not_implemented response when issuing a request using the OPTIONS method.

https://github.com/Nugine/s3s/blob/64cfef9d4d503ccab1f244ac4e672f009e71b50c/crates/s3s/src/ops/generated.rs#L5872-L5876

The question would be how should we handle pre-flight requests ?

I think it would be too cluttered to include them in all methods of the S3 Trait and instead it would be good to have a separate function (similar to check_access) that catches all requests using theOPTIONS method and allows user to return an appropriate response / set response headers to indicated allowed methods / headers etc.

The official clients (like aws-sdk-js) seem to use options pre-flight requests with the x-id query parameter to indicate the specific operation, e.g. http://my_bucket.example.com/foo.bar?x-id=PutObject

A short section that describes the official AWS behavior can be found here: https://docs.aws.amazon.com/AmazonS3/latest/API/RESTOPTIONSobject.html

For everyone else with this problem: As a workaround, we are currently using a wrapping service that catches all requests that use the OPTIONS method and return appropriate responses early before the call gets handled by the inner s3s service.

Nugine commented 5 months ago

Actually I encountered this problem when testing #137 against browser clients. I'm not sure whether S3Service should handle pre-flight requests and how to handle. A CORS middleware may be more suitable for this problem, instead of integrating CORS in S3Service. (FYI tower_web has CORS middleware and service)

S3 trait only describes the behavior of S3 api. We may need a more general builder/trait for configuring the behavior of S3 service.

seddonm1 commented 5 months ago

I have solved this problem via Tower middleware: https://github.com/seddonm1/s3ite/blob/main/src/main.rs#L172

We need a new release to support Hyper 1.0

seddonm1 commented 1 month ago

@Nugine Have you had any luck trying to wrap the S3Service (which is a Tower 1.0 service) with any of the tower-http middleware?

Nugine commented 1 month ago

Sorry for the delay. I was busy recently. s3s v0.10.1 will release in a few days, including your PR.

seddonm1 commented 1 month ago

Thanks @Nugine.

Nugine commented 1 month ago

s3s v0.10.1 is released https://github.com/Nugine/s3s/compare/v0.10.0...v0.10.1