access-control-max-age will be 0 if you use Drupal's default CORS config in D10

Created on 12 March 2023, over 1 year ago
Updated 13 March 2023, over 1 year ago

Problem/Motivation

Our Cors middleware asm89/stack-cors was updated in #3306946: Update Composer dependencies in 10.0 and 9.5, and increase constraints to require latest minors β†’ from 1.x to 2.x. The middleware's interpretation of its configuration has changed slightly in 2.x, however the default configuration in default.settings.yml will result in an access-control-max-age of 0, instead of undefined. This may result in poorer performance for clients which will perform more CORS preflight requests than necessary.

This configuration option is explicitly compared to null in 2.x. to disable being set. In 1.x, it was tested for truthiness.

See change upstream at https://github.com/asm89/stack-cors/commit/df153ceda9c83f6fe497907f72ab4... and then https://github.com/asm89/stack-cors/blame/7a198ec737e926eab15d29368fc6ff....

See upstream documentation update PR: https://github.com/asm89/stack-cors/pull/101

Steps to reproduce

Use the default configuration value of false for maxAge in cors.config, and the max-age will be cast from false to 0.

Proposed resolution

Update documentation and also craft a change record/PSA that informs people of this change that wasn't identified in the 10.0.0 release cycle.

Remaining tasks

Maintainer review, decision on how to inform users.

User interface changes

None.

API changes

Default configuration change.

Data model changes

None.

Release notes snippet

TBD

πŸ› Bug report
Status

Needs work

Version

10.1 ✨

Component
BaseΒ  β†’

Last updated about 12 hours ago

Created by

πŸ‡ΊπŸ‡ΈUnited States bradjones1 Digital Nomad Life

Live updates comments and jobs are added and updated live.
  • Needs change record

    A change record needs to be drafted before an issue is committed. Note: Change records used to be called change notifications.

  • Needs release note

    The major change should have a special release note written to summarize the importance of the change. See Write a release note for an issue.

Sign in to follow issues

Comments & Activities

  • Issue created by @bradjones1
  • @bradjones1 opened merge request.
  • Status changed to Needs review over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States bradjones1 Digital Nomad Life
  • πŸ‡ΊπŸ‡Έ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 over 1 year ago
  • πŸ‡ΊπŸ‡Έ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.

Production build 0.71.5 2024