From 5f4325706b5f8b9b9e6889f3c73c119023c4151e Mon Sep 17 00:00:00 2001 From: Evelyn Chen Date: Sat, 29 Aug 2026 23:48:39 +0000 Subject: [PATCH] fix: make CORS reconciliation recoverable --- README.md | 3 +- scripts/deploy.py | 90 ++++++++++++++++++++++++++++++++++-------- scripts/utils.py | 4 ++ tests/test_contract.py | 57 +++++++++++++++++++++++--- 4 files changed, 131 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index 8fa5ba8..cf534f8 100644 --- a/README.md +++ b/README.md @@ -155,7 +155,8 @@ authority is derived from its bucket; a site cannot expose an arbitrary backend. `cors_origins` is reconciled as a bucket policy on every split publication. Values are either `*` or HTTPS origins; browser access is limited to `GET` and `HEAD`. Omitting the field removes stale CORS from that bucket. Protected artifacts cannot allow wildcard CORS. All immutable objects and -all bucket CORS policies complete before mutable channels change. +all bucket CORS policies complete before mutable channels change; if any policy write fails, the +policies already attempted are restored to their pre-publication values. Each split route gets a bucket-specific `.web.sjc001.fritzlab.net` ExternalName Service annotated to disable pass-host-header and a separate Ingress. Route diff --git a/scripts/deploy.py b/scripts/deploy.py index 886b666..45e25bb 100644 --- a/scripts/deploy.py +++ b/scripts/deploy.py @@ -174,21 +174,28 @@ def configure_cors(bucket, origins, endpoint, aws_env): """Reconcile read-only browser access without exposing publication credentials.""" if origins is None: return - if not origins: + config = None + if origins: + config = { + "CORSRules": [{ + "AllowedOrigins": origins, + "AllowedMethods": ["GET", "HEAD"], + "AllowedHeaders": ["*"], + "ExposeHeaders": ["ETag"], + "MaxAgeSeconds": 3600, + }], + } + set_cors_configuration(bucket, config, endpoint, aws_env) + + +def set_cors_configuration(bucket, config, endpoint, aws_env): + """Apply an exact bucket CORS configuration, or remove it when absent.""" + if config is None: run([ "aws", "--endpoint-url", endpoint, "s3api", "delete-bucket-cors", "--bucket", bucket, ], env=aws_env) return - config = { - "CORSRules": [{ - "AllowedOrigins": origins, - "AllowedMethods": ["GET", "HEAD"], - "AllowedHeaders": ["*"], - "ExposeHeaders": ["ETag"], - "MaxAgeSeconds": 3600, - }], - } with tempfile.NamedTemporaryFile("w", suffix=".json", encoding="utf-8") as handle: json.dump(config, handle) handle.flush() @@ -198,11 +205,61 @@ def configure_cors(bucket, origins, endpoint, aws_env): ], env=aws_env) -def configure_artifact_cors(artifact, credential_env_names=None): - configure_cors( - artifact["bucket"], artifact["cors_origins"], artifact["s3_endpoint"], - publication_aws_env(artifact, credential_env_names), - ) +def get_cors_configuration(bucket, endpoint, aws_env): + """Read the exact bucket CORS configuration for rollback.""" + result = _aws_capture([ + "aws", "--endpoint-url", endpoint, "s3api", "get-bucket-cors", + "--bucket", bucket, "--output", "json", + ], aws_env) + if result.returncode == 0: + try: + config = json.loads(result.stdout) + except json.JSONDecodeError as error: + raise RuntimeError(f"get-bucket-cors returned invalid JSON for {bucket}") from error + if not isinstance(config, dict) or not isinstance(config.get("CORSRules"), list): + raise RuntimeError(f"get-bucket-cors returned an invalid policy for {bucket}") + return config + error = f"{result.stdout}\n{result.stderr}" + if "NoSuchCORSConfiguration" in error: + return None + raise RuntimeError(f"get-bucket-cors failed for {bucket}: {error.strip()}") + + +def reconcile_artifact_cors(artifacts, credential_env_names=None): + """Reconcile all policies, restoring the prior set if any write fails.""" + snapshots = [] + for artifact in artifacts: + if artifact["cors_origins"] is None: + continue + aws_env = publication_aws_env(artifact, credential_env_names) + snapshots.append(( + artifact, + aws_env, + get_cors_configuration(artifact["bucket"], artifact["s3_endpoint"], aws_env), + )) + + attempted = [] + try: + for artifact, aws_env, previous in snapshots: + attempted.append((artifact, aws_env, previous)) + configure_cors( + artifact["bucket"], artifact["cors_origins"], artifact["s3_endpoint"], aws_env, + ) + except Exception as error: + rollback_errors = [] + for artifact, aws_env, previous in reversed(attempted): + try: + set_cors_configuration( + artifact["bucket"], previous, artifact["s3_endpoint"], aws_env, + ) + except Exception as rollback_error: + rollback_errors.append(f"{artifact['bucket']}: {rollback_error}") + if rollback_errors: + raise RuntimeError( + f"CORS reconciliation failed ({error}); rollback also failed for " + f"{'; '.join(rollback_errors)}" + ) from error + raise def publish_route_immutables(artifact, route, site_dir, credential_env_names=None): @@ -499,8 +556,7 @@ def deploy_static(site_name, site_dir, action_dir, token, cfg): # Reconcile every browser-read policy before publishing mutable content. # A CORS failure therefore cannot leave a new channel pointing at a release # whose cross-origin assets browsers cannot consume. - for artifact in cfg["artifacts"]: - configure_artifact_cors(artifact, credential_env_names) + reconcile_artifact_cors(cfg["artifacts"], credential_env_names) for route in cfg["routes"]: s3_sync( artifact_by_name[route["artifact"]], route, site_dir, credential_env_names, diff --git a/scripts/utils.py b/scripts/utils.py index 6148aae..f499731 100644 --- a/scripts/utils.py +++ b/scripts/utils.py @@ -139,6 +139,10 @@ def _cors_origins(value, label): f"{label} must contain '*' or canonical HTTPS origins" ) from None else: + if getattr(address, "scope_id", None) is not None: + raise ConfigError( + f"{label} must contain '*' or canonical HTTPS origins" + ) hostname = f"[{address.compressed}]" if address.version == 6 else address.compressed canonical.append(f"https://{hostname}{f':{port}' if port not in (None, 443) else ''}") if len(canonical) != len(set(canonical)): diff --git a/tests/test_contract.py b/tests/test_contract.py index ef609f4..f09deb9 100644 --- a/tests/test_contract.py +++ b/tests/test_contract.py @@ -131,6 +131,8 @@ class ConfigContractTests(unittest.TestCase): "https://consumer.example:443", "https://consumer.example/", "https://consumer.example:invalid", + "https://[fe80::1%eth0]", + "https://[fe80::1%25eth0]", ): with self.subTest(origin=origin): self.assert_invalid( @@ -427,6 +429,51 @@ class PublishingTests(unittest.TestCase): ) self.assertIn("delete-bucket-cors", request.call_args.args[0]) + def test_cors_reconciliation_restores_prior_policies_on_failure(self): + artifacts = [ + { + "bucket": "first", "cors_origins": ["https://new.example"], + "s3_endpoint": "http://garage-s3.storage.svc:3900", "credentials": {}, + }, + { + "bucket": "second", "cors_origins": [], + "s3_endpoint": "http://garage-s3.storage.svc:3900", "credentials": {}, + }, + ] + prior = [ + {"CORSRules": [{"AllowedOrigins": ["https://old.example"]}]}, + None, + ] + writes = [] + + def write(bucket, config, *_args): + writes.append((bucket, config)) + if bucket == "second" and len(writes) == 2: + raise RuntimeError("write failed") + + with patch.object(deploy, "publication_aws_env", return_value={}), patch.object( + deploy, "get_cors_configuration", side_effect=prior, + ) as read, patch.object(deploy, "set_cors_configuration", side_effect=write), \ + self.assertRaisesRegex(RuntimeError, "write failed"): + deploy.reconcile_artifact_cors(artifacts) + + self.assertEqual(2, read.call_count) + self.assertEqual( + [ + ("first", {"CORSRules": [{ + "AllowedOrigins": ["https://new.example"], + "AllowedMethods": ["GET", "HEAD"], + "AllowedHeaders": ["*"], + "ExposeHeaders": ["ETag"], + "MaxAgeSeconds": 3600, + }]}), + ("second", None), + ("second", None), + ("first", prior[0]), + ], + writes, + ) + def test_apps_clone_never_places_token_in_argv_or_log(self): calls = [] secret = "clone-secret-must-not-appear" @@ -732,15 +779,15 @@ class PublishingTests(unittest.TestCase): deploy, "publish_route_immutables", side_effect=[None, RuntimeError("immutable failed")], ) as immutable_publish, patch.object( - deploy, "configure_artifact_cors" - ) as cors_configure, patch.object(deploy, "s3_sync") as mutable_sync, \ + deploy, "reconcile_artifact_cors" + ) as cors_reconcile, patch.object(deploy, "s3_sync") as mutable_sync, \ self.assertRaisesRegex( RuntimeError, "immutable failed" ): deploy.deploy_static("baseline", root, root, "token", cfg) self.assertEqual(2, immutable_publish.call_count) - cors_configure.assert_not_called() + cors_reconcile.assert_not_called() mutable_sync.assert_not_called() def test_all_cors_policies_complete_before_mutable_publication(self): @@ -755,7 +802,7 @@ class PublishingTests(unittest.TestCase): ), patch.object( deploy, "publish_route_immutables", side_effect=lambda *_args: events.append("immutable") ), patch.object( - deploy, "configure_artifact_cors", side_effect=lambda *_args: events.append("cors") + deploy, "reconcile_artifact_cors", side_effect=lambda *_args: events.append("cors") ), patch.object( deploy, "s3_sync", side_effect=lambda *_args: events.append("mutable") ), patch.object(deploy, "clone_apps", return_value=apps), patch.object( @@ -764,7 +811,7 @@ class PublishingTests(unittest.TestCase): deploy.deploy_static("baseline", root, root, "token", cfg) self.assertEqual( - ["immutable", "immutable", "cors", "cors", "mutable", "mutable"], events + ["immutable", "immutable", "cors", "mutable", "mutable"], events )