feat(site-publish): reconcile split-surface CORS #6

Merged
architect merged 2 commits from feat/split-surface-cors into main 2026-08-29 23:53:47 +00:00
5 changed files with 222 additions and 2 deletions
Showing only changes of commit 310ae6a29d - Show all commits
+6
View File
@@ -52,6 +52,7 @@ artifacts:
- name: distributions - name: distributions
type: static type: static
content_dir: dist content_dir: dist
cors_origins: ['*']
publish: publish:
bucket: baseline-dist bucket: baseline-dist
credentials: credentials:
@@ -151,6 +152,11 @@ protected input from entering a public artifact through dereference. Split
storage endpoints are pinned to Garage, and each website storage endpoints are pinned to Garage, and each website
authority is derived from its bucket; a site cannot expose an arbitrary backend. 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.
Each split route gets a bucket-specific `<bucket>.web.sjc001.fritzlab.net` 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 ExternalName Service annotated to disable pass-host-header and a separate Ingress. Route
Ingresses share the hostname's certificate Secret. The access middleware and Ingresses share the hostname's certificate Secret. The access middleware and
+41
View File
@@ -8,6 +8,7 @@ import os
import re import re
import shutil import shutil
import subprocess import subprocess
import tempfile
from pathlib import Path from pathlib import Path
from urllib.error import HTTPError, URLError from urllib.error import HTTPError, URLError
from urllib.request import Request, urlopen from urllib.request import Request, urlopen
@@ -169,6 +170,41 @@ def publication_aws_env(artifact, credential_env_names=None):
return aws_env return aws_env
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 = {
"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()
run([
"aws", "--endpoint-url", endpoint, "s3api", "put-bucket-cors",
"--bucket", bucket, "--cors-configuration", f"file://{handle.name}",
], 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 publish_route_immutables(artifact, route, site_dir, credential_env_names=None): def publish_route_immutables(artifact, route, site_dir, credential_env_names=None):
"""Publish one route's immutable partitions during the global preflight.""" """Publish one route's immutable partitions during the global preflight."""
html_dir = site_dir / artifact["build_dir"] html_dir = site_dir / artifact["build_dir"]
1
@@ -460,6 +496,11 @@ def deploy_static(site_name, site_dir, action_dir, token, cfg):
publish_route_immutables( publish_route_immutables(
artifact_by_name[route["artifact"]], route, site_dir, credential_env_names, artifact_by_name[route["artifact"]], route, site_dir, credential_env_names,
) )
# 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"]:
Outdated
Review

Observation: this loop adds exactly one serialized S3 control-plane request per split artifact—two calls for the checked-in fixture; legacy publication adds zero. Fine today; 20 artifacts would mean 20 round trips before mutable sync. Attribution: CI does not exercise Garage latency, so no deploy-time delta can be isolated here. Next measurement: time this loop in one real two-artifact publish.

Observation: this loop adds exactly one serialized S3 control-plane request per split artifact—two calls for the checked-in fixture; legacy publication adds zero. Fine today; 20 artifacts would mean 20 round trips before mutable sync. Attribution: CI does not exercise Garage latency, so no deploy-time delta can be isolated here. Next measurement: time this loop in one real two-artifact publish.
configure_artifact_cors(artifact, credential_env_names)
Outdated
Review

You can cut off existing browser consumers here: if CORS succeeds for artifact A and fails for B, A's live policy stays changed while publication aborts. Restore earlier policies on failure, or provide an atomic reconciliation.

You can cut off existing browser consumers here: if CORS succeeds for artifact A and fails for B, A's live policy stays changed while publication aborts. Restore earlier policies on failure, or provide an atomic reconciliation.
for route in cfg["routes"]: for route in cfg["routes"]:
s3_sync( s3_sync(
artifact_by_name[route["artifact"]], route, site_dir, credential_env_names, artifact_by_name[route["artifact"]], route, site_dir, credential_env_names,
+60 -1
View File
@@ -1,5 +1,6 @@
"""Shared utilities for the site-publish action.""" """Shared utilities for the site-publish action."""
import ipaddress
import os import os
import re import re
import shutil import shutil
@@ -104,6 +105,51 @@ def _strings(value, label):
return values return values
def _cors_origins(value, label):
origins = _list(value, label)
if any(not isinstance(item, str) or not item for item in origins):
raise ConfigError(f"{label} must be a list of non-empty strings")
canonical = []
for origin in origins:
if origin == "*":
canonical.append(origin)
continue
parsed = urlparse(origin)
try:
port = parsed.port
except ValueError:
raise ConfigError(f"{label} must contain '*' or canonical HTTPS origins") from None
if parsed.scheme != "https" or not parsed.hostname or parsed.path or parsed.params or (
parsed.query or parsed.fragment or parsed.username or parsed.password
):
raise ConfigError(f"{label} must contain '*' or canonical HTTPS origins")
try:
address = ipaddress.ip_address(parsed.hostname)
Review

ipaddress.ip_address accepts scoped IPv6 strings such as fe80::1%eth0 and fe80::1%25eth0; this function then accepts https://[fe80::1%eth0] and its encoded form unchanged. Node's WHATWG URL parser rejects both, so a deployment can succeed with a CORS origin no browser can emit. Reject IPv6 addresses with a non-null scope ID and cover both forms.

`ipaddress.ip_address` accepts scoped IPv6 strings such as `fe80::1%eth0` and `fe80::1%25eth0`; this function then accepts `https://[fe80::1%eth0]` and its encoded form unchanged. Node's WHATWG URL parser rejects both, so a deployment can succeed with a CORS origin no browser can emit. Reject IPv6 addresses with a non-null scope ID and cover both forms.
except ValueError:
try:
hostname = parsed.hostname.encode("idna").decode("ascii")
except UnicodeError:
raise ConfigError(
f"{label} must contain '*' or canonical HTTPS origins"
) from None
try:
_hostname(hostname, f"{label} hostname")
except ConfigError:
raise ConfigError(
f"{label} must contain '*' or canonical HTTPS origins"
) from None
else:
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)):
raise ConfigError(f"{label} must not contain duplicate canonical origins")
if "*" in canonical and len(canonical) != 1:
raise ConfigError(f"{label} wildcard must be the only origin")
if origins != canonical:
raise ConfigError(f"{label} must contain '*' or canonical HTTPS origins")
return origins
def _hostname(value, label): def _hostname(value, label):
if not isinstance(value, str) or len(value) > 253 or value.endswith("."): if not isinstance(value, str) or len(value) > 253 or value.endswith("."):
raise ConfigError(f"{label} must be a lowercase DNS hostname without a trailing dot") raise ConfigError(f"{label} must be a lowercase DNS hostname without a trailing dot")
@@ -215,6 +261,7 @@ def _legacy_config(raw, site_name):
"website_authority": "garage-s3.storage.svc.k8s.sjc001.fritzlab.net", "website_authority": "garage-s3.storage.svc.k8s.sjc001.fritzlab.net",
"credentials": {"access_key_env": "AWS_ACCESS_KEY_ID", "secret_key_env": "AWS_SECRET_ACCESS_KEY"}, "credentials": {"access_key_env": "AWS_ACCESS_KEY_ID", "secret_key_env": "AWS_SECRET_ACCESS_KEY"},
"cache_rules": [{"path": "", "cache_control": DEFAULT_CACHE_CONTROL}], "cache_rules": [{"path": "", "cache_control": DEFAULT_CACHE_CONTROL}],
"cors_origins": None,
} }
return { return {
"version": 1, "version": 1,
@@ -234,7 +281,11 @@ def _legacy_config(raw, site_name):
def _artifact(item, index): def _artifact(item, index):
label = f"artifacts[{index}]" label = f"artifacts[{index}]"
item = _mapping(item, label) item = _mapping(item, label)
_known_keys(item, {"name", "type", "content_dir", "tidy", "excludes", "publish", "cache"}, label) _known_keys(
item,
{"name", "type", "content_dir", "tidy", "excludes", "publish", "cache", "cors_origins"},
label,
)
name = item.get("name") name = item.get("name")
if not isinstance(name, str) or not NAME_RE.fullmatch(name): if not isinstance(name, str) or not NAME_RE.fullmatch(name):
raise ConfigError(f"{label}.name must be a DNS label") raise ConfigError(f"{label}.name must be a DNS label")
@@ -303,6 +354,10 @@ def _artifact(item, index):
"website_authority": authority, "website_authority": authority,
"credentials": normalized_credentials, "credentials": normalized_credentials,
"cache_rules": cache_rules, "cache_rules": cache_rules,
"cors_origins": (
_cors_origins(item["cors_origins"], f"{label}.cors_origins")
if "cors_origins" in item else []
),
} }
@@ -389,6 +444,10 @@ def _validate_multi(cfg):
raise ConfigError(f"protected route {route['name']} cannot use shared-cache max-age") raise ConfigError(f"protected route {route['name']} cannot use shared-cache max-age")
if route["access"] == "public" and "private" in directives: if route["access"] == "public" and "private" in directives:
raise ConfigError(f"public route {route['name']} cannot use private cache policy") raise ConfigError(f"public route {route['name']} cannot use private cache policy")
if route["access"] == "protected" and "*" in artifact["cors_origins"]:
raise ConfigError(
f"protected route {route['name']} cannot allow wildcard CORS"
)
root = next(route for route in routes if route["path"] == "/") root = next(route for route in routes if route["path"] == "/")
if any(route["access"] == "protected" for route in routes) and root["access"] == "public": if any(route["access"] == "protected" for route in routes) and root["access"] == "public":
raise ConfigError("a public '/' catch-all would expose unmatched protected content") raise ConfigError("a public '/' catch-all would expose unmatched protected content")
+1
View File
@@ -15,6 +15,7 @@ artifacts:
- name: distributions - name: distributions
type: static type: static
content_dir: dist content_dir: dist
cors_origins: ['*']
publish: publish:
bucket: baseline-dist bucket: baseline-dist
credentials: credentials:
+114 -1
View File
@@ -113,6 +113,67 @@ class ConfigContractTests(unittest.TestCase):
self.assertEqual(["/dist", "/"], [route["path"] for route in cfg["routes"]]) self.assertEqual(["/dist", "/"], [route["path"] for route in cfg["routes"]])
credentials = {artifact["name"]: artifact["credentials"] for artifact in cfg["artifacts"]} credentials = {artifact["name"]: artifact["credentials"] for artifact in cfg["artifacts"]}
self.assertNotEqual(credentials["distributions"], credentials["portal"]) self.assertNotEqual(credentials["distributions"], credentials["portal"])
distributions = next(
artifact for artifact in cfg["artifacts"] if artifact["name"] == "distributions"
)
self.assertEqual(["*"], distributions["cors_origins"])
def test_cors_origins_are_https_origins_or_wildcard(self):
self.assert_invalid(
lambda raw: raw["artifacts"][1].__setitem__(
"cors_origins", ["http://consumer.example"]
),
"must contain '\\*' or canonical HTTPS origins",
)
for origin in (
"https://consumer example",
"https://Consumer.example",
"https://consumer.example:443",
"https://consumer.example/",
"https://consumer.example:invalid",
):
with self.subTest(origin=origin):
self.assert_invalid(
lambda raw, origin=origin: raw["artifacts"][1].__setitem__(
"cors_origins", [origin]
),
"must contain '\\*' or canonical HTTPS origins",
)
self.assert_invalid(
lambda raw: raw["artifacts"][1].__setitem__(
"cors_origins", ["https://consumer.example", "https://consumer.example:443"]
),
"must not contain duplicate canonical origins",
)
self.assert_invalid(
lambda raw: raw["artifacts"][1].__setitem__(
"cors_origins", ["*", "https://consumer.example"]
),
"wildcard must be the only origin",
)
raw = copy.deepcopy(self.raw)
raw["artifacts"][1]["cors_origins"] = ["https://[2602:817:3000::1]:8443"]
cfg = normalize_site_config(raw, "baseline.fritzlab.net")
self.assertEqual(
["https://[2602:817:3000::1]:8443"],
next(a for a in cfg["artifacts"] if a["name"] == "distributions")["cors_origins"],
)
def test_declared_cors_origins_must_be_a_list(self):
for value in (None, False, 0, "", {}):
with self.subTest(value=value):
self.assert_invalid(
lambda raw, value=value: raw["artifacts"][1].__setitem__(
"cors_origins", value
),
"cors_origins must be a list",
)
def test_protected_route_rejects_wildcard_cors(self):
self.assert_invalid(
lambda raw: raw["artifacts"][0].__setitem__("cors_origins", ["*"]),
"protected route portal cannot allow wildcard CORS",
)
def test_equivalent_route_paths_are_ambiguous(self): def test_equivalent_route_paths_are_ambiguous(self):
self.assert_invalid(lambda raw: raw["routes"].append({ self.assert_invalid(lambda raw: raw["routes"].append({
@@ -131,6 +192,7 @@ class ConfigContractTests(unittest.TestCase):
raw["routes"][1]["access"] = { raw["routes"][1]["access"] = {
"mode": "protected", "middleware": "authentik-forwardauth" "mode": "protected", "middleware": "authentik-forwardauth"
} }
raw["artifacts"][1]["cors_origins"] = []
raw["artifacts"][0]["cache"]["rules"][0]["cache_control"] = ( raw["artifacts"][0]["cache"]["rules"][0]["cache_control"] = (
"public, max-age=0, must-revalidate" "public, max-age=0, must-revalidate"
) )
@@ -341,6 +403,30 @@ class BuildTests(unittest.TestCase):
class PublishingTests(unittest.TestCase): class PublishingTests(unittest.TestCase):
def test_cors_policy_is_reconciled_as_read_only_browser_access(self):
captured = {}
def capture(command, **_kwargs):
config_path = command[command.index("--cors-configuration") + 1]
captured["command"] = command
captured["config"] = json.loads(Path(config_path.removeprefix("file://")).read_text())
with patch.object(deploy, "run", side_effect=capture):
deploy.configure_cors(
"baseline-dist", ["*"], "http://garage-s3.storage.svc:3900", {}
)
self.assertIn("put-bucket-cors", captured["command"])
self.assertEqual(["GET", "HEAD"], captured["config"]["CORSRules"][0]["AllowedMethods"])
self.assertEqual(["*"], captured["config"]["CORSRules"][0]["AllowedOrigins"])
def test_empty_cors_policy_removes_stale_bucket_cors(self):
with patch.object(deploy, "run") as request:
deploy.configure_cors(
"baseline-catalogue", [], "http://garage-s3.storage.svc:3900", {}
)
self.assertIn("delete-bucket-cors", request.call_args.args[0])
def test_apps_clone_never_places_token_in_argv_or_log(self): def test_apps_clone_never_places_token_in_argv_or_log(self):
calls = [] calls = []
secret = "clone-secret-must-not-appear" secret = "clone-secret-must-not-appear"
@@ -645,15 +731,42 @@ class PublishingTests(unittest.TestCase):
patch.object(deploy, "clone_apps", return_value=root / "apps"), patch.object( patch.object(deploy, "clone_apps", return_value=root / "apps"), patch.object(
deploy, "publish_route_immutables", deploy, "publish_route_immutables",
side_effect=[None, RuntimeError("immutable failed")], side_effect=[None, RuntimeError("immutable failed")],
) as immutable_publish, patch.object(deploy, "s3_sync") as mutable_sync, \ ) as immutable_publish, patch.object(
deploy, "configure_artifact_cors"
) as cors_configure, patch.object(deploy, "s3_sync") as mutable_sync, \
self.assertRaisesRegex( self.assertRaisesRegex(
RuntimeError, "immutable failed" RuntimeError, "immutable failed"
): ):
deploy.deploy_static("baseline", root, root, "token", cfg) deploy.deploy_static("baseline", root, root, "token", cfg)
self.assertEqual(2, immutable_publish.call_count) self.assertEqual(2, immutable_publish.call_count)
cors_configure.assert_not_called()
mutable_sync.assert_not_called() mutable_sync.assert_not_called()
def test_all_cors_policies_complete_before_mutable_publication(self):
cfg = normalize_site_config(fixture("split-site.yaml"), "baseline.fritzlab.net")
events = []
with tempfile.TemporaryDirectory() as tmp:
root = Path(tmp)
apps = root / "apps"
apps.mkdir()
with patch.object(deploy, "validate_publication_environment"), patch.object(
deploy, "validate_artifact_output"
), 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")
), patch.object(
deploy, "s3_sync", side_effect=lambda *_args: events.append("mutable")
), patch.object(deploy, "clone_apps", return_value=apps), patch.object(
deploy, "render_site_manifests"
), patch.object(deploy, "commit_and_push"):
deploy.deploy_static("baseline", root, root, "token", cfg)
self.assertEqual(
["immutable", "immutable", "cors", "cors", "mutable", "mutable"], events
)
def test_absent_artifact_is_detected_before_publish(self): def test_absent_artifact_is_detected_before_publish(self):
cfg = normalize_site_config(fixture("split-site.yaml"), "baseline.fritzlab.net") cfg = normalize_site_config(fixture("split-site.yaml"), "baseline.fritzlab.net")