Regression of PR #141 AWS API Gateway support

[cblackcom]

Just writing to let you know that the "fix" in PR #141 which I authored has regressed and the library no longer works behind AWS API Gateway. If this is important to you please advise and I will work out and submit a PR accordingly. Cheers!

[rs]

Can you please add more context one what fails and how you believe we should fix it?

[cblackcom]

Hey there— and thank you as always for your work ongoing on this library.

So API Gateway does this strange thing where it takes the Access-Control-Request-Headers on an inbound request and, if there are multiple values, splits them up into their own individual lines before passing the request on.

Go's http.header.Values() picks all of these up correctly, so I believe that AWS's way of doing this, as odd as it might seem, adheres to a real spec somewhere. Prior to PR #141, http.header.Get() was being used, and in that case, it would only pick up the first line's worth, i.e. only the first value specified by the client, not all of them.

As of v1.10.1 now seeing the same behavior.

Happy to assist however I can. Thank you!

[jub0bs]

@cblackcom

the "fix" in PR https://github.com/rs/cors/pull/141 which I authored has regressed

I think that your changes were "erased" by https://github.com/rs/cors/pull/171, which was a fix for https://github.com/rs/cors/issues/170.

API Gateway does this strange thing where it takes the Access-Control-Request-Headers on an inbound request and, if there are multiple values, splits them up into their own individual lines before passing the request on.

I haven't verified that AWS API Gateway does this but, if it does,

• it violates HTTP standards (unless I missed something, nothing in RFC 9110 allows a proxy to split a list-based field into multiple field lines), and
• it breaks the Fetch standard's guarantee that browsers include at most one Access-Control-Request-Headers header in preflight requests, a guarantee which rs/cors's current implementation relies on.

Accordingly, I believe that the onus to fix this is on AWS. It would be worth pointing it out to them.

Edit: see follow-up comment.

[jub0bs]

After consulting some W3C and WHATWG people, I must admit I was wrong. A proxy is allowed to split a list-based field like Access-Control-Request-Headers as long as it doesn't change the order of the values listed in that field. See https://matrixlogs.bakkot.com/WHATWG/2024-08-16#L4 and https://matrixlogs.bakkot.com/WHATWG/2024-08-16#L5.

So it seems that AWS API Gateway are within their rights after all. Accordingly, I'm going to implement a fix in jub0bs/cors within the next few days. If @rs agrees, I'll port my fix to this library.

[rs]

Thanks @jub0bs

[cblackcom]

@jub0bs Great info, thanks, and certainly glad to hear both of these libraries are going to be able to work out of the box with API Gateway! Please let me know if there's anything I can do at all to assist.

[leemeichin]

I think we're getting stung by this - perhaps because we're using {proxy+} for routing in API Gateway (using SAM...)

Short of just spelling out all those routes by hand in the template again, is there any other workaround?

[jub0bs]

@leemeichin I'm working on a fix (at least for jub0bs/cors, at the minute). Out of curiosity, could you please describe (in a follow-up comment) what your proxy is doing to the Access-Control-Request-Headers header? Exact same behaviour as that described by @cblackcom?

[jub0bs]

@rs This library used to (as recently as v1.10.1) tolerate arbitrarily long optional whitespace around the values listed in the Access-Control-Request-Headers header. This behaviour is more or less in accordance with RFC 9110 (although it trimmed more than the required SP and HTAB bytes).

However, being this liberal exposes CORS middleware to adversarial preflight requests in a way reminiscent of (but less serious than) issue #170. For instance, adversaries could spoof preflight requests containing the following header:

Access-Control-Request-Headers: some-allowed-header,{lots of whitespace}some-token

In my initial attempt at a fix, I tried to adhere to RFC 9110 and allow arbitrary OWS, but I observed that processing such a malicious preflight request took an order of magnitude (a few ms) longer than in v1.11.0 (less than a μs); I took care to avoid any unnecessary allocations, though, in order to avoid a regression of #170.

Would this approach (tolerating arbitrary long OWS around elements) be acceptable, in your opinion? FWIW, for my own library, I'm considering limiting the amount of OWS that I tolerate, in order to maintain performance at the cost of some interoperability.

[rs]

I agree, let's allow a reasonable amount of dup whitespaces for interoperability but not so much that it would expose it to abuse.

[leemeichin]

@jub0bs doing nothing intentional as far as I'm aware.. we're just using net/http with the new functionality in go 1.22, namely the ability to specific the method name and path variables in a route. A middleware just wraps the serveMux instance like this:

func WithCORS(next http.Handler) http.Handler {
    return cors.New(cors.Options{
        AllowedOrigins:       []string{"*"},
        AllowedMethods:       []string{"HEAD", "GET", "POST", "PUT", "PATCH", "DELETE", "OPTIONS"},
        AllowedHeaders:       []string{"Content-Type,X-Amz-Date,Authorization,X-Api-Key,X-Amz-Security-Token"},
        OptionsSuccessStatus: 200,
        Debug:                true,
    }).Handler(next)
}

Funnily enough, this works fine except when there are multiple handlers on the same route, for different methods. This is just using the updates in go 1.22 where you can define a route like http.HandleFunc("GET/path") and http.HandleFunc("POST /path").

I believe the fact that I have no issue except when using the same path with handlers for two separate HTTP methods means there's something going on with the cors library here. My only workaround so far has been to get rid of {proxy+} and just list every endpoint by hand in our SAM template.

[jub0bs]

@leemeichin Am I understanding correctly that the problem doesn't manifest itself if you use method-less patterns (e.g. "/path" instead of "GET /path") throughout? If so, I suspect you're hitting a different problem (explained elsewhere in the context of a different CORS library, but my point applies even to rs/cors). Long story short: you have to be careful with method-full patterns.

[leemeichin]

@jub0bs we're using method-full patterns exclusively, it just happens that most of them are unique (not fully REST-ful as it were), so it's only in the instance where there are two method-full handlers on the same path, but with two different methods, that it seems to happen. Would not be surprised if it's because of how the go team defined route specificity for this when most libraries use the order you define routes.

I think we're talking about a separate issue here and not the API Gateway issue though now, so happy to move this elsewhere or try some other approaches before reporting back.

[jub0bs]

@leemeichin I do suspect your issue is unrelated to this one. Feel free to open a new issue with a minimal reproducible example.

[cblackcom]

I think we're getting stung by this - perhaps because we're using {proxy+} for routing in API Gateway (using SAM...)

Short of just spelling out all those routes by hand in the template again, is there any other workaround?

@leemeichin To confirm, I'm also using {proxy+} route in API Gateway but I'm not using method-full patterns with net/http... my issues go away with rs/cors and jub0bs/cors once the multiple Access-Control-Request-Headers lines split out by API Gateway are handled correctly by the middleware.

https://github.com/rs/cors/pull/141/commits/405181998d9b46dce68f0ec8b14b2f6de6b10a03 https://github.com/pndlm/jub0bs-cors/commit/eb2bf0ee690b034a5ec3cf02521946cc2372f535

[jub0bs]

@cblackcom Thanks for the insight! So maybe @leemeichin's is related to yours after all...

Good that you have a temporary fix, but I perceive joining the multiple ACRH field lines as problematic, since an adversary could send loads of empty ACRH field lines in order to trigger many heap allocations. See #170. I have implemented (only in jub0bs/cors so far) a fix that obviates this problem; I just need to find the time to release it. Hopefully, this weekend; if not, early next week. I'll open a PR for a similar fix in rs/cors soon after.

[cblackcom]

@jub0bs Yeah, I sincerely appreciate the efforts you are undertaking on making the libraries as resilient as possible. I should have clarified that my hacks were simply to make it work for now, and for demonstrating the issue, and that I don't consider them ideal by any stretch!

[jub0bs]

@cblackcom No problem; that's how I understood it. Thanks again for bringing up this issue. I wouldn't have realised the problem otherwise. As for my contributions to rs/cors, I'm responsible for reversing (as part of #171) your changes from #141, so I feel responsible for fixing the mess I caused there.

[jub0bs]

@leemeichin Out of curiosity, did upgrading to rs/cors v1.11.1 solve your issue?

[leemeichin]

@jub0bs it did indeed; thank you for the fix ❤️

[jub0bs]

@leemeichin Great! Thanks for confirming.