PERF: Do not set any caching headers on preflight requests

[xfalcox]

This will allow each app that uses this library to handle caching behavior of the preflight requests in a way that makes sense for their app on their own CORS middleware.

[benlangfeld]

https://github.com/discourse/message_bus/pull/246 and https://github.com/discourse/message_bus/pull/244 are relevant.

This is a breaking change and should be loudly called out in the changelog.

[SamSaffron]

Yeah I don't think we can do this, there are some headers we want to pass through.

[xfalcox]

1. Currently we are telling browsers to NOT cache preflight. Which causes this on Firefox:

   

2. Preflight cache has a specific header named Access-Control-Max-Age.

3. Currently MessageBus replies with an invalid Content-Type and response combo:

   

IMO, we should either do this properly or leave for downstream library consumers.

[SamSaffron]

I see... how about we then add:

  if @bus.extra_preflight_response_headers_lookup
      @bus.extra_preflight_response_headers_lookup.call(env).each do |k, v|
        pre_headers[k] = v
      end
    end

Then we are not leaving people with no way of injecting headers they need into preflight while making the change?

I thought @benlangfeld had a reason he wanted headers injected into preflight?

[xfalcox]

Then we are not leaving people with no way of injecting headers they need into preflight while making the change?

That's the thing, we already inject headers in the preflight responses at

https://github.com/discourse/discourse/blob/aee9fcd2571516ba3c852a1858bef2e4f605cc37/config/initializers/004-message_bus.rb#L34

And

https://github.com/discourse/discourse/blob/b301a6b3db288ef50fbbf736262de7eed71eb5ef/config/initializers/008-rack-cors.rb#L20

The problem is removing those. Which is way I'd like for the library to let downstream handle it on their middlewares.

[SamSaffron]

But we are injecting here:

https://github.com/discourse/discourse/blob/aee9fcd2571516ba3c852a1858bef2e4f605cc37/config/initializers/004-message_bus.rb#L78-L81

So will the change not break us?

MessageBus middlware is way up the stack prior to Discourse::Cors.

With:

if @bus.extra_preflight_response_headers_lookup
      @bus.extra_preflight_response_headers_lookup.call(env).each do |k, v|
        pre_headers[k] = v
      end
    end

We will have another place to put headers.

---

At a minimum a good fix would be to force content type after the current callback, cause we know it is not json.

[xfalcox]

But we are injecting here:

https://github.com/discourse/discourse/blob/aee9fcd2571516ba3c852a1858bef2e4f605cc37/config/initializers/004-message_bus.rb#L78-L81

So will the change not break us?

Yes, that is what my first link in the previous response was about.

It's easy to add headers when using MessageBus as a library. Shouldn't we let the apps using it as a library set whatever they see fit, instead of having to remove a bunch of unnecessary or even (in content-type case) incorrect headers?

[benlangfeld]

It's easy to add headers when using MessageBus as a library. Shouldn't we let the apps using it as a library set whatever they see fit, instead of having to remove a bunch of unnecessary or even (in content-type case) incorrect headers?

My objection to this change as it stands is that it takes the capability to add headers away from me unless I add extra middleware, thus using a different approach from adding headers to any other non-preflight response from MessageBus. It is therefore a breaking change.

[xfalcox]

My objection to this change as it stands is that it takes the capability to add headers away from me unless I add extra middleware, thus using a different approach from adding headers to any other non-preflight response from MessageBus. It is therefore a breaking change.

Ohhhhhhh now I get what you meant. Will fix.

[xfalcox]

How about now @benlangfeld ?

[benlangfeld]

How about now @benlangfeld ?

I think this could do with some test coverage. It's still a backward incompatible change, so would require a major version bump per semver, but at least it's not a significant inconvenience for users.