Sending Access-Control-Expose-Headers response header

[tlinhart]

Hi, setting the Access-Control-Expose-Headers response header with this middleware feels a bit wrong. There a couple of reasons.

At the moment the header is always set, even for non-CORS requests. This doesn't cause any harm but feels superfluous. It should probably be used only in context of a CORS protocol.

If you want to allow propagation of correlation ID from the downstream services/components, you have to handle CORS in its entirety, especially responding with Access-Control-Allow-Headers to preflight requests to actually allow sending request headers with correlation ID set. This is typically done with a specialized middleware like Starlette's CORS middleware which has its own means of setting the expose headers. If you use both middlewares together the Access-Control-Expose-Headers header is duplicated in the response. This again is strictly not a problem (according to RFC 9110) but again feels superfluous especially when you need to expose multiple headers and use CORS middleware to handle this.

What do you think @JonasKs @sondrelg ?

[sondrelg]

You're suggesting we just drop that part entirely?

I agree, it seems like specifying expose_headers in the cors middleware is the way to go if you want these headers attached to your response. For context, this middleware basically inherited that piece of logic from here.

This feels like major bump territory, right?

[tlinhart]

OK, thank you. I'll try to prepare something along these lines tomorrow.

[tlinhart]

@JonasKs @sondrelg

I've just updated the README, could you please take a look if it's complete from your point of view?

[sondrelg]

Looks great.

One final point of discussion before we release this. Since we're bumping major versions anyways, perhaps it would now make sense to change the behaviour of the newly added update_request_header parameter. Seems like defaulting to True would be most useful.

What do you think?

[tlinhart]

It's a different concept but I agree that this is a good opportunity to introduce the change. So I'm for if you ask me :-)

[JonasKs]

Agree!

[tlinhart]

@sondrelg
I created a PR (#67) but it fails the Python 3.7 test. Personally I doubt it's related to the changes and more likely relates to 5a7be63 given the DeprecationWarning in the workflow log. What do you think?

[tlinhart]

Yeah, I guess it's a problem with importlib-metadata package. If I compare the failing workflow run with the yesterday's run which passes, different versions of the package are being installed.

[sondrelg]

Ah ok, don't worry about it 👍 I'll take a look later and fix it separately if needed