Deferred (CORS) headers not propagated

[dbaynard]

Following the CORS configuration in caddyserver/website#324 (comment) I see that my endpoint using caddy-s3-proxy doesn't defer the response headers. Could it be because this module doesn't call next.ServeHttp(w, r) in the success case? Edit: it's not, the code needs to call ResponseWriter.WriteHeaders.

This is blocking CORS requests from working, as the Access-Control-Allow-Origin header is not present on the response.

---

I've sanitized the following output (from httpie).

This is the non–caddy-s3-proxy endpoint.

> http -v -d 'https://api.example.com/vanilla' \
  'origin: https://origin.example.com' \
  'x-preflight: true'
GET /vanilla HTTP/1.1
Accept: */*
Accept-Encoding: identity
Connection: keep-alive
Host: api.example.com
User-Agent: HTTPie/3.2.2
origin: https://origin.example.com
x-preflight: true



HTTP/1.1 200 OK
Access-Control-Allow-Credentials: true
Access-Control-Allow-Origin: https://api.example.com
Access-Control-Expose-Headers: Content-Encoding, Content-Type, Date, Location, Server, Transfer-Encoding
Alt-Svc: h3=":443"; ma=2592000
Content-Type: <ELIDED>
Date: <ELIDED>
Server: Caddy, <ELIDED>
Transfer-Encoding: chunked

This is the caddy-s3-proxy one.

> http -v -d 'https://api.example.com/s3proxy' \
  'origin: https://origin.example.com' \
  'x-preflight: true'
GET /s3proxy HTTP/1.1
Accept: */*
Accept-Encoding: identity
Connection: keep-alive
Host: api.example.com
User-Agent: HTTPie/3.2.2
origin: https://origin.example.com
x-preflight: true



HTTP/1.1 200 OK
Alt-Svc: h3=":443"; ma=2592000
Content-Type: <ELIDED>
Date: Wed, 08 Nov 2023
Etag: "<ELIDED>"
Last-Modified: <ELIDED>
Server: Caddy
Transfer-Encoding: chunked

Note, in particular, the lack of the Access-Control-Allow-Origin header; this means that CORS requests do not work.

I've read #33 but this seems different, as the headers should be set already (due to the defer).

Does this sound like a feature that is missing? And if so, and you don't have an alternative that you recommend for CORS, would you consider this a bug?

In that case, I can dig through the code to try to fix this, but it might be an obvious solution to you — I've skimmed https://github.com/lindenlab/caddy-s3-proxy/blob/850db193cb7f48546439d236f2a6de7bd7436e2e/s3proxy.go and found the code calls return nil in ServeHttp.

caddy-s3-proxy/s3proxy.go

Lines 361 to 374 in 850db19

	switch r.Method {
	case http.MethodGet:
	err = p.GetHandler(w, r, fullPath)
	case http.MethodPut:
	err = p.PutHandler(w, r, fullPath)
	case http.MethodDelete:
	err = p.DeleteHandler(w, r, fullPath)
	default:
	err = caddyhttp.Error(http.StatusMethodNotAllowed, errors.New("method not allowed"))
	}
	if err == nil {
	// Success!
	return nil
	}

Should it be calling return next.ServeHttp(w, r), as it does in the error case?

caddy-s3-proxy/s3proxy.go

Lines 398 to 402 in 850db19

	// process errors directive
	doPassThrough, doS3ErrorPage, s3Key := p.determineErrorsAction(caddyErr.StatusCode)
	if doPassThrough {
	return next.ServeHTTP(w, r)
	}

[dbaynard]

I think my hypothesis was correct?

route @origin {
  header X-Before-Proxy true
  header >X-Before-Proxy-Deferred true
  s3proxy {
    bucket "bubblx-storage"
    region "eu-west-1"
  }
  header X-After-Proxy true
  header >X-After-Proxy-Deferred true
}

That resulted in this.

GET /s3proxy HTTP/1.1
Accept: */*
Accept-Encoding: identity
Connection: keep-alive
Host: api.example.com
User-Agent: HTTPie/3.2.2
origin: https://origin.example.com
x-preflight: true



HTTP/1.1 200 OK
Alt-Svc: h3=":443"; ma=2592000
Content-Type: audio/wav
Date: Wed, 08 Nov 2023 17:23:52 GMT
Etag: "<ELIDED>"
Last-Modified: <ELIDED>
Server: Caddy
Transfer-Encoding: chunked
X-Before-Proxy: true

So only the non-deferred header, works, and only before the call to s3Proxy, which is what I'd expect to see for the non-deferred headers if the middleware doesn't call the next middleware in the chain.
I suspect this is the issue.

What I don't understand is why it is return nil — as a result I'd be wary of changing it.

I suppose I could leave the default as is, and extend pass_through for the success case?

[dbaynard]

OK it's not that; it's that io.Copy doesn't add the deferred headers. This… this actually makes sense, though I don't quite understand it.

As described in http.ResponseWriter, the Write method calls WriteHeader, and the headers can't be changed afterwards — as in, writing to the ResponseWriter prevents any additional headers from being added.

This is what io.Copy does. But unlike for ResponseWriter.Write, it doesn't add the deferred headers.

Or, at least that's what I infer, as calling ResponseWriter.WriteHeader before the io.Copy results in the deferred headers appearing, and calling after, or replacing the return nil with return next.ServeHttp(w,r) does not. I think the io.Copy is necessary, as per go - How to stream an io.ReadCloser into a http.ResponseWriter - Stack Overflow, so I'll add the WriteHeader(http.StatusOk) on the line before (it doesn't cause issues with errors).

Plausibly it ought to return next.StatusHttp(w,r) as there could be subsequent middleware, but I shan't change that, for now.