fix: make CORS reconciliation recoverable

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 `<bucket>.web.sjc001.fritzlab.net`
 ExternalName Service annotated to disable pass-host-header and a separate Ingress. Route

scripts/deploy.py

@@ -174,12 +174,8 @@ 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:
-        run([
-            "aws", "--endpoint-url", endpoint, "s3api", "delete-bucket-cors",
-            "--bucket", bucket,
-        ], env=aws_env)
-        return
+    config = None
+    if origins:
         config = {
             "CORSRules": [{
                 "AllowedOrigins": origins,
@@ -189,6 +185,17 @@ def configure_cors(bucket, origins, endpoint, aws_env):
                 "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
     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):
+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"],
-        publication_aws_env(artifact, credential_env_names),
+                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,

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)):

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
         )