Allow for empty corsExposedHeaders

[perrin4869]

Upgrading from v4 to v5 broke a lot of my tests regarding cors due to the upgrade to hapi@18. For some reason now, there are corsExposedHeaders set by default. Maybe an empty string, or empty array should be the default? I don't see why serverless-offline should have a an opinion on which headers should be exposed by default.
Another problem that I didn't address here is that now, returning access-control-allow-origin headers from a lambda gets ignored, and overwritten by hapi into access-control-allow-origin: event.headers.Origin, while before the headers returned by the lambda would get respected.

[dherault]

@dnalborczyk should we merge?

[dherault]

Thanks @perrin4869 for the PR, it's under consideration!

[dnalborczyk]

@dherault I actually wanted to ask you about this topic. I would need to look into this more deeply, what hapi does by default and/or the settings, and the AWS side of it. Without it I can't tell. you might have to decide.

on that matter, I was wondering what those flags are used for:

--corsAllowHeaders          Used as default Access-Control-Allow-Headers header value for responses. Delimit multiple values with commas. Default: 'accept,content-type,x-api-key'
--corsAllowOrigin           Used as default Access-Control-Allow-Origin header value for responses. Delimit multiple values with commas. Default: '*'
--corsDisallowCredentials   When provided, the default Access-Control-Allow-Credentials header value will be passed as 'false'. Default: true
--corsExposedHeaders        Used as additional Access-Control-Exposed-Headers header value for responses. Delimit multiple values with commas. Default: 'WWW-Authenticate,Server-Authorization'

I'm wondering if those should be rather removed in a next major release?

[dnalborczyk]

@perrin4869

Upgrading from v4 to v5 broke a lot of my tests regarding cors due to the upgrade to hapi@18. For some reason now, there are corsExposedHeaders set by default. Maybe an empty string, or empty array should be the default? I don't see why serverless-offline should have a an opinion on which headers should be exposed by default.

this was undoubtedly something which was not intended and slipped through the cracks with the update to hapi v18.

is this what you are talking about?
https://hapijs.com/api#route.options.cors

exposedHeaders - a strings array of exposed headers ('Access-Control-Expose-Headers'). Defaults to ['WWW-Authenticate', 'Server-Authorization']

could you elaborate more with some examples the what it does now (v5), maybe what it did do (v4), and even better, what it should do in your opinion.

Another problem that I didn't address here is that now, returning access-control-allow-origin headers from a lambda gets ignored, and overwritten by hapi into access-control-allow-origin: event.headers.Origin, while before the headers returned by the lambda would get respected.

same as before, could you also elaborate more with some examples?

[dherault]

I'm wondering if those should be rather removed in a next major release?

I'm not sure. If you set cors: true they are used (often as their default value) to allow CORS. So I'd say we keep them. Why would you remove them?

[perrin4869]

Hm... maybe the discussion regarding CORS in v4 vs v5 should be moved into a separate issue, seeing as this PR is nothing more than a very simple bugfix/feature (just enables you to pass an empty string to corsExposedHeaders as an option, which would throw an error before). I'll open an issue regarding which tests broke in the migration

[dnalborczyk]

@dherault

I'm wondering if those should be rather removed in a next major release?

I'm not sure. If you set cors: true they are used (often as their default value) to allow CORS. So I'd say we keep them. Why would you remove them?

just trying to understand what their use case is about. I never had the need for those when I used CORS.

[dnalborczyk]

@perrin4869

Hm... maybe the discussion regarding CORS in v4 vs v5 should be moved into a separate issue

this is not a discussion. to give any valuable feedback on the issue I'd like to understand the full picture, even though it might look like it has nothing to do with your issue in particular .

your PR might fix an issue just for your use case with corsExposedHeaders. what about corsAllowOrigin? what about corsAllowHeaders? are they facing the same issue?

this.options.corsAllowOrigin = this.options.corsAllowOrigin
  .replace(/\s/g, '')
  .split(',');

this.options.corsAllowHeaders = this.options.corsAllowHeaders
  .replace(/\s/g, '')
  .split(',');

this.options.corsExposedHeaders = this.options.corsExposedHeaders
  .replace(/\s/g, '')
  .split(',')
  .filter(header => header !== '');