Allow Access-Control-Allow-Credentials to be set for --cors mode

[bethesque]

See pact-foundation/pact-js#409

[bethesque]

Question - can Access-Control-Allow-Credentials be safely set for all requests when in --cors mode, or will this cause unintended side effects? That is, should we make a separate option to turn this on or off, or should it just be bundled as part of the existing --cors headers.

[mefellows]

It's probably generally a safe thing to just include it. Given that the CORS headers aren't serialised into the contract (because the mock service is artificially adding them) it's not going to give (any more) false positives. The extra header present shouldn't break existing tests because the presence of that header doesn't impact client behaviour unless the XMLHttpRequest request credentials flag is enabled (unless of course they are somehow using the absence of that header for a negative test - in which case, this is bad, because now they are relying on a header not present in the contract for a functional test).

An alternative that may make everyone happy:

1. have the --cors flag set all of the defaults, including Access-Control-Allow-Credentials
2. if/when people need to customise the behaviour, have them pass in the specific headers/values they need as a repeatable arg e.g. --cors-header Access-Control-Allow-Credentials=true --cors-header Access-Control-Allow-Origin=* type thing

I'd say go with (1) until we have a need for (2).

[bethesque]

@vandemark it should already add the credentials header, but it will only do it if there was an Authorization or Cookie header set https://github.com/pact-foundation/pact-mock_service/blob/master/lib/pact/mock_service/request_handlers/options.rb#L41-L43

[bethesque]

Sorry, only if it was requested in the Access-Control-Request-Headers.

[vandemark]

@mefellows I actually might have just identified a need for your option 2, that being another request we have that uses a "custom" header and the pact server is throwing this back at us: https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS/Errors/CORSInvalidAllowHeader

edit: just noticed that if cors=true then the Access-Control-Allow-Headers is set up correctly, so ignore this

[bethesque]

@vandemark can you provide an executable example of your issue in a github repository please? I won't be able to fix anything until I can recreate the issue. You can fork the pact-js repository and modify one of the examples in the https://github.com/pact-foundation/pact-js/tree/master/examples directory.

[vandemark]

@bethesque sure thing, I need to make sure I'm going through my company's proper channels but I will start putting something together as soon as possible

[vandemark]

@bethesque I updated one of the examples in the jest folder to replicate the issue here: https://github.com/vandemark/pact-js/tree/cors-example

I made two examples, both use withCredentials but only one has an options preflight request caused by a custom header.

[bethesque]

Thanks! I should be able to look at it by Thursday at the latest, as I have an OSS day then.

[vandemark]

Hi @bethesque , I can try to make this update if that's not an issue. Should just be a small change here: https://github.com/pact-foundation/pact-mock_service/blob/master/lib/pact/consumer/mock_service/cors_origin_header_middleware.rb

[bethesque]

Please do submit a PR. I'm sorry I wasn't able to get to it on my last OSS day.

[vandemark]

Hi @bethesque, I got the changes in my projects repo and everything is working as expected now! Thanks for the help.

[bethesque]

You're welcome. Thanks for your help.