- Issue created by @bradjones1
- @bradjones1 opened merge request.
- Status changed to Needs review
almost 2 years ago 10:42pm 12 March 2023 - πΊπΈUnited States bradjones1 Digital Nomad Life
I wonder if we shouldn't include a status report item that sanity-checks the CORS config and tells you that it's potentially wrong? Especially with the tie-ins with JSON:API in core. It could warn that when JSON:API is in mutable mode, that the headers are not explicitly set, e.g. content-type and content-disposition. That could be a follow-up, though.
At the very least we could catch the legacy false value and say, hey, consider setting it to null if you want the legacy behavior, or >= 0. But false is never valid.
- πΊπΈUnited States bradjones1 Digital Nomad Life
A self-review on the config documentation: The library now interprets '*' for the allowed headers list as an instruction to repeat back the headers as sent from the user-agent... so the note about listing them explicitly when credentials are enabled/included might be overkill. However, it's undoubtedly more secure to actually list the headers you want to allow. So I guess it depends on how conservative we want our documentation to be.
- Status changed to Needs work
almost 2 years ago 1:58am 13 March 2023 - πΊπΈUnited States bradjones1 Digital Nomad Life
Aha... the test failure points me to the applicable test (guess I was too lazy to look for it) but also... shows there is actually no test coverage for the max-age header. Which is why this went undiscovered.
Also the updated config sets the exposed headers setting to an array, since it's consumed as an array. So that actually adds to the...urgency? Import? of this issue because it's a second parameter that isn't properly defaulted. However it is tested by truthiness, and the only way that this would work without failing (PHP error on the explode() call on the array) is if you properly set the param... but still... it should be correct in the defaults.
NW for test update and new coverage for access-control-max-age header.
- π¬π§United Kingdom catch
At the very least we could catch the legacy false value and say, hey, consider setting it to null if you want the legacy behavior, or >= 0. But false is never valid.
Yes we should add something like that to system_requirements().
Tagging for a release note and change record, but we should probably backdate those to 10.0.0 with a note that the defaults for new installs are fixed in whichever release this is fixed in.
Also - what's the behaviour if a site doesn't have default.services.yml at all?
- πΊπΈUnited States bradjones1 Digital Nomad Life
Also - what's the behaviour if a site doesn't have default.services.yml at all?
The library's default value for max-age is 0, however that doesn't get applied because Drupal's default value is in core.services.yml which I noticed, thanks to your question, also needs to be updated.
https://git.drupalcode.org/project/drupal/-/blob/30a1e2325247fa6c6f45854...
This contains the same incorrect config so the max-age false value would cast to 0.
This all assumes CORS is enabled, of course, but that's not an uncommon configuration... and whenever you specify the cors.config value you need to include all the applicable keys, because they are not recursively merged.