Fix CORS middleware hot-swapping

[adamsachs]

Closes PROD-1419

https://www.loom.com/share/c38df34b38194d76aa9b91ae9c32ba33

Description Of Changes

Fixes the broken CORS allow_origins update functionality. Basically, before we were trying to mutate the allow_origins attribute on the existing CORSMiddleware instance "in-place", which wasn't effective in actually updating middleware's functionality, as it requires some reinitialization.

To fix this, we can instead remove the existing CORSMiddleware instance and add a new CORSMiddleware instance (with the updated allows_origins values) using the proper fastAPI app method.

Some additional notes:

• as before, the complete set of cors_origins specified via API (i.e. via admin UI) will be the complete set of allow_origins in the server's CORS middleware. So any cors_origins set via "traditional" config mechanisms (i.e. fides.toml or env var) will be ignored. that being said, the cors_origin_regex (a different property) can only be set via "traditional" config mechanisms, and is not settable via API - as a security best practice. the cors_origin_regex set via traditional config mechanisms is still used/respected with this change, as it has always been.
• there's not much precedent for doing an operation like this that i can find - either the previous implementation or the new one proposed here, i.e. 'replacing' an existing middleware on a running app. from what i can tell, i don't see any major risk, but there's some inherent risk in not following a well established paradigm/precedent

Code Changes

• properly remove and replace any existing CORS middleware with an updated CORS middleware that's got its allow_origins values determined by config proxy, i.e. it prefers any API-set values and falls back to "traditional" config set values
• remove the addition of a CORS middleware based purely on "traditional" config values that was part of the core app setup, as it's no longer needed.
  • we don't need this because we now properly initialize the CORS middleware based on the config proxy (which falls back to the "traditional" config values if needed) in the run_database_startup startup hook, which comes a bit later in the startup sequence, once we've got a db session to work with (which the config proxy needs)
• update automated tests to actually evaluate the cors middleware functionality, i.e. closer to a real "integration" test!
  • these rewritten tests were (expectedly) failing with the previous app code, i.e. they expose the previous bug 👍

Steps to Confirm

• see loom and associated jira ticket for steps for repro'ing locally. i can lend a hand if needed!

Pre-Merge Checklist

• All CI Pipelines Succeeded
• Documentation:
  • documentation complete, PR opened in fidesdocs
  • documentation issue created in fidesdocs
• Issue Requirements are Met
• Relevant Follow-Up Issues Created
• Update CHANGELOG.md
• For API changes, the Postman collection has been updated

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
fides-plus-nightly	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 26, 2024 2:49pm

[cypress]

Passing run #6078 ↗︎

0	4	0	0	0
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.

Details:

Merge 80debf3 into 13a040e...
Project: fides	Commit: bcce1611e9 ℹ️
Status: Passed	Duration: 00:35 💡
Started: Jan 26, 2024 2:58 PM	Ended: Jan 26, 2024 2:59 PM

Review all test suite changes for PR #4570 ↗︎

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (13a040e) 86.79% compared to head (80debf3) 86.81%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4570      +/-   ##
==========================================
+ Coverage   86.79%   86.81%   +0.02%     
==========================================
  Files         329      330       +1     
  Lines       19816    19839      +23     
  Branches     2546     2545       -1     
==========================================
+ Hits        17199    17223      +24     
  Misses       2150     2150              
+ Partials      467      466       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[galvana]

Quick turnaround here! I only have one nit and one usability concern with the slash on the cors_origin values. According to this, the Origin values won't contain the slash (path) so we should make sure to strip the slashes or raise a validation error to prevent these "incorrect" values from being stored. And by incorrect I mean that our CORSMiddleware will try to match the Origin value as-is without trying to trim the slashes.

https://fetch.spec.whatwg.org/#origin-header

[galvana]

What's the reason for returning the old CORSMiddleware?

[adamsachs]

good catch! i think this is just a vestige from a previous iteration/refactor, i think i can remove as it's a potential cause for confusion 👍

[adamsachs]

Quick turnaround here! I only have one nit and one usability concern with the slash on the cors_origin values. According to this, the Origin values won't contain the slash (path) so we should make sure to strip the slashes or raise a validation error to prevent these "incorrect" values from being stored. And by incorrect I mean that our CORSMiddleware will try to match the Origin value as-is without trying to trim the slashes.

https://fetch.spec.whatwg.org/#origin-header

Thanks for highlighting/pushing on this @galvana! i happened to chat a bit about it with @NevilleS too, who mentioned that misconfiguring CORS origin settings with values that have trailing slashes and paths is actually a common hiccup/pain point, so your intuition to provide some more guard rails here seems spot on.

i've gone a step further than the trailing slash and i have a validator now that rejects any URL with any type of path. reading the MDN spec on origin headers, this is consistent -- origin URLs can only have a scheme, a hostname, and a port

See loom below for the integrated UX with the validation now. we could still probably use a bit of polish on the error message, but I think it's clear enough for this iteration 👍
https://www.loom.com/share/c0e9166119144b16bd5daf4481458321