From 756183525eeab149594af0919db0eab2a4a5e1cf Mon Sep 17 00:00:00 2001 From: Russell Bryant Date: Thu, 16 Jul 2026 10:55:41 -0400 Subject: [PATCH 1/4] fix(providers): allow git clone/fetch via default GitHub provider The github.com:443 git-transport endpoint used the read-only access preset, which expands to GET/HEAD/OPTIONS only. Git smart HTTP requires a POST to */git-upload-pack for clone and fetch, so the L7 proxy denied those operations and `gh repo clone` / `git clone https://...` failed. Replace the preset with explicit rules that permit the read-only methods plus POST */git-upload-pack, so clone/fetch work while push (git-receive-pack) stays blocked. Enabling push still requires an explicit policy proposal. Why allowing this POST is still read-only: in git's smart HTTP protocol POST is an RPC transport, not a write. A clone/fetch does GET */info/refs (ref discovery) followed by POST */git-upload-pack, whose body is only the client's want/have negotiation; the server responds with a packfile and nothing on the server is modified (data flows server -> client). The service names are from the server's perspective: git-upload-pack = the server uploads a pack to the client (a read/ download), while git-receive-pack = the server receives a pack from the client (the actual write/push). The new rule is scoped to */git-upload-pack only, so push (git-receive-pack) and arbitrary POSTs to github.com remain denied. Add a provider-profile regression test and a rego enforcement test covering ref discovery, upload-pack (allowed), and receive-pack (denied). Closes #1769 Signed-off-by: Russell Bryant --- crates/openshell-providers/src/profiles.rs | 48 ++++++++++++++-- crates/openshell-server/src/grpc/policy.rs | 27 +++++++-- .../openshell-supervisor-network/src/opa.rs | 55 +++++++++++++++++++ providers/github.yaml | 12 +++- 4 files changed, 129 insertions(+), 13 deletions(-) diff --git a/crates/openshell-providers/src/profiles.rs b/crates/openshell-providers/src/profiles.rs index 62809138cd..4265f25ecf 100644 --- a/crates/openshell-providers/src/profiles.rs +++ b/crates/openshell-providers/src/profiles.rs @@ -1763,15 +1763,53 @@ mod tests { "github profile should include read-only GraphQL endpoint" ); assert!( - proto - .endpoints - .iter() - .all(|endpoint| endpoint.access == "read-only"), - "github profile endpoints should all be read-only" + proto.endpoints.iter().all(|endpoint| { + // The REST/GraphQL API endpoints stay read-only. The git + // transport endpoint (github.com) carries explicit rules + // instead so it can allow clone/fetch while blocking push. + if endpoint.host == "github.com" { + endpoint.access.is_empty() + } else { + endpoint.access == "read-only" + } + }), + "github API endpoints should be read-only; git transport uses explicit rules" ); assert_eq!(proto.binaries.len(), 4); } + #[test] + fn github_git_transport_allows_clone_but_not_push() { + let profile = builtin_profile("github"); + let proto = profile.to_proto(); + + let git_transport = proto + .endpoints + .iter() + .find(|endpoint| endpoint.host == "github.com" && endpoint.port == 443) + .expect("github.com git transport endpoint"); + + // Clone/fetch over git smart HTTP issues POST to */git-upload-pack. + assert!( + git_transport.rules.iter().any(|rule| { + rule.allow.as_ref().is_some_and(|allow| { + allow.method == "POST" && allow.path.contains("git-upload-pack") + }) + }), + "git transport must allow POST to */git-upload-pack for clone/fetch" + ); + + // Push uses git-receive-pack and must stay blocked by default. + assert!( + !git_transport.rules.iter().any(|rule| { + rule.allow + .as_ref() + .is_some_and(|allow| allow.path.contains("git-receive-pack")) + }), + "git transport must not allow git-receive-pack (push)" + ); + } + #[test] fn credential_env_vars_are_deduplicated_in_profile_order() { let profile = builtin_profile("claude-code"); diff --git a/crates/openshell-server/src/grpc/policy.rs b/crates/openshell-server/src/grpc/policy.rs index 6fccf1eade..3b1d1f0d87 100644 --- a/crates/openshell-server/src/grpc/policy.rs +++ b/crates/openshell-server/src/grpc/policy.rs @@ -5116,12 +5116,27 @@ mod tests { "github provider policy should include read-only GraphQL endpoint" ); assert!( - layers[0] - .rule - .endpoints - .iter() - .all(|endpoint| endpoint.access == "read-only"), - "github provider policy should be read-only by default" + layers[0].rule.endpoints.iter().all(|endpoint| { + // API endpoints stay read-only; the github.com git transport + // carries explicit rules so clone/fetch works (see #1769). + if endpoint.host == "github.com" { + endpoint.access.is_empty() + } else { + endpoint.access == "read-only" + } + }), + "github API endpoints should be read-only; git transport uses explicit rules" + ); + assert!( + layers[0].rule.endpoints.iter().any(|endpoint| { + endpoint.host == "github.com" + && endpoint.rules.iter().any(|rule| { + rule.allow.as_ref().is_some_and(|allow| { + allow.method == "POST" && allow.path.contains("git-upload-pack") + }) + }) + }), + "github git transport should allow POST git-upload-pack for clone/fetch" ); } diff --git a/crates/openshell-supervisor-network/src/opa.rs b/crates/openshell-supervisor-network/src/opa.rs index 829c63caa4..3b9854d91e 100644 --- a/crates/openshell-supervisor-network/src/opa.rs +++ b/crates/openshell-supervisor-network/src/opa.rs @@ -3443,6 +3443,61 @@ network_policies: assert!(!eval_l7(&engine, &get_with_method)); } + // Mirrors the default GitHub provider's github.com git-transport endpoint + // (providers/github.yaml). Git smart HTTP clone/fetch performs a GET on + // */info/refs followed by a POST to */git-upload-pack; push uses + // */git-receive-pack, which must stay blocked. Regression test for #1769. + #[test] + fn l7_github_git_transport_allows_clone_blocks_push() { + let data = r#" +network_policies: + github: + name: github + endpoints: + - host: github.com + port: 443 + protocol: rest + enforcement: enforce + rules: + - allow: { method: GET, path: "**" } + - allow: { method: HEAD, path: "**" } + - allow: { method: OPTIONS, path: "**" } + - allow: { method: POST, path: "/**/git-upload-pack" } + binaries: + # l7_input() issues requests as /usr/bin/curl. + - { path: /usr/bin/curl } +"#; + let engine = OpaEngine::from_strings(TEST_POLICY, data).expect("engine from yaml"); + + // Reference discovery (GET) is allowed. + let refs = l7_input("github.com", 443, "GET", "/NVIDIA/OpenShell.git/info/refs"); + assert!(eval_l7(&engine, &refs), "GET info/refs should be allowed"); + + // Clone/fetch (POST git-upload-pack) is allowed. + let upload_pack = l7_input( + "github.com", + 443, + "POST", + "/NVIDIA/OpenShell.git/git-upload-pack", + ); + assert!( + eval_l7(&engine, &upload_pack), + "POST git-upload-pack should be allowed for clone/fetch" + ); + + // Push (POST git-receive-pack) is denied. + let receive_pack = l7_input( + "github.com", + 443, + "POST", + "/NVIDIA/OpenShell.git/git-receive-pack", + ); + assert!( + !eval_l7(&engine, &receive_pack), + "POST git-receive-pack (push) must be denied" + ); + } + #[test] fn l7_jsonrpc_request_params_do_not_affect_method_policy() { let data = r#" diff --git a/providers/github.yaml b/providers/github.yaml index 4ce5af2d31..4f23c2e3d8 100644 --- a/providers/github.yaml +++ b/providers/github.yaml @@ -29,10 +29,18 @@ endpoints: protocol: graphql access: read-only enforcement: enforce - # github.com is the git transport (clone / fetch by default). + # github.com is the git transport (clone / fetch by default). Git smart + # HTTP needs POST to */git-upload-pack for clone/fetch, which the + # read-only preset (GET/HEAD/OPTIONS) blocks. Spell the rules out so + # clone/fetch works while push (git-receive-pack) stays denied — enabling + # push requires an explicit policy proposal. - host: github.com port: 443 protocol: rest - access: read-only enforcement: enforce + rules: + - allow: { method: GET, path: "**" } + - allow: { method: HEAD, path: "**" } + - allow: { method: OPTIONS, path: "**" } + - allow: { method: POST, path: "/**/git-upload-pack" } binaries: [/usr/bin/gh, /usr/local/bin/gh, /usr/bin/git, /usr/local/bin/git] From afb0205f6978abca804e2951b18fba065ee9d21a Mon Sep 17 00:00:00 2001 From: Russell Bryant Date: Fri, 17 Jul 2026 13:54:41 -0400 Subject: [PATCH 2/4] test(providers): strengthen git-transport regression and add clone e2e Pin the exact allowed rule set for the built-in github git-transport endpoint in both the provider-profile and composed-policy tests, so a broader or additional POST rule (e.g. POST **) that could enable push via git-receive-pack fails the test instead of passing a substring check. Add an e2e test that attaches the built-in github provider and clones a public repo over HTTPS, exercising provider attachment, effective-policy composition, TLS interception, and real git behavior. Update the Providers V2 docs so the github.com git-transport endpoint shows explicit clone/fetch rules instead of the stale read-only preset. Refs #1769 Signed-off-by: Russell Bryant --- crates/openshell-providers/src/profiles.rs | 56 +++++++++---- crates/openshell-server/src/grpc/policy.rs | 46 ++++++++--- docs/sandboxes/providers-v2.mdx | 14 +++- e2e/python/test_sandbox_providers.py | 91 ++++++++++++++++++++++ 4 files changed, 182 insertions(+), 25 deletions(-) diff --git a/crates/openshell-providers/src/profiles.rs b/crates/openshell-providers/src/profiles.rs index 4265f25ecf..c770a1e9eb 100644 --- a/crates/openshell-providers/src/profiles.rs +++ b/crates/openshell-providers/src/profiles.rs @@ -1789,24 +1789,52 @@ mod tests { .find(|endpoint| endpoint.host == "github.com" && endpoint.port == 443) .expect("github.com git transport endpoint"); - // Clone/fetch over git smart HTTP issues POST to */git-upload-pack. + // The git transport carries explicit rules rather than an access preset + // (an empty preset would otherwise expand to GET/HEAD/OPTIONS). assert!( - git_transport.rules.iter().any(|rule| { - rule.allow.as_ref().is_some_and(|allow| { - allow.method == "POST" && allow.path.contains("git-upload-pack") - }) - }), - "git transport must allow POST to */git-upload-pack for clone/fetch" + git_transport.access.is_empty(), + "git transport must use explicit rules, not an access preset" ); - // Push uses git-receive-pack and must stay blocked by default. - assert!( - !git_transport.rules.iter().any(|rule| { - rule.allow + // Assert the EXACT allowed rule set. Clone/fetch over git smart HTTP + // performs GET */info/refs (ref discovery) followed by POST + // */git-upload-pack. A substring check alone is not enough: a broader or + // additional POST rule (e.g. POST **) would also permit push via + // git-receive-pack while still passing a "some rule allows upload-pack" + // check. Pinning the whole set fails on any such regression. See #1769. + let mut allowed: Vec<(&str, &str)> = git_transport + .rules + .iter() + .map(|rule| { + let allow = rule + .allow .as_ref() - .is_some_and(|allow| allow.path.contains("git-receive-pack")) - }), - "git transport must not allow git-receive-pack (push)" + .expect("git transport rules must be allow rules"); + (allow.method.as_str(), allow.path.as_str()) + }) + .collect(); + allowed.sort_unstable(); + + let mut expected = vec![ + ("GET", "**"), + ("HEAD", "**"), + ("OPTIONS", "**"), + ("POST", "/**/git-upload-pack"), + ]; + expected.sort_unstable(); + + assert_eq!( + allowed, expected, + "git transport allow rules must be exactly the read-only methods \ + plus POST */git-upload-pack (clone/fetch); a broader or extra POST \ + rule would enable push (git-receive-pack)" + ); + + // Blocking push must not depend on a deny rule, which could mask an + // over-broad allow and hide a regression. + assert!( + git_transport.deny_rules.is_empty(), + "git transport should block push via its narrow allow set, not deny rules" ); } diff --git a/crates/openshell-server/src/grpc/policy.rs b/crates/openshell-server/src/grpc/policy.rs index 3b1d1f0d87..cb71ad5703 100644 --- a/crates/openshell-server/src/grpc/policy.rs +++ b/crates/openshell-server/src/grpc/policy.rs @@ -5127,16 +5127,44 @@ mod tests { }), "github API endpoints should be read-only; git transport uses explicit rules" ); + // Pin the exact composed rule set for the git transport. Clone/fetch + // needs GET */info/refs then POST */git-upload-pack; a broader or extra + // POST rule (e.g. POST **) would also permit push (git-receive-pack), so + // matching the whole set fails on any such regression (see #1769). + let git_transport = layers[0] + .rule + .endpoints + .iter() + .find(|endpoint| endpoint.host == "github.com") + .expect("composed policy should include the github.com git transport"); + let mut allowed: Vec<(&str, &str)> = git_transport + .rules + .iter() + .map(|rule| { + let allow = rule + .allow + .as_ref() + .expect("git transport rules must be allow rules"); + (allow.method.as_str(), allow.path.as_str()) + }) + .collect(); + allowed.sort_unstable(); + let mut expected = vec![ + ("GET", "**"), + ("HEAD", "**"), + ("OPTIONS", "**"), + ("POST", "/**/git-upload-pack"), + ]; + expected.sort_unstable(); + assert_eq!( + allowed, expected, + "composed git transport allow rules must be exactly the read-only \ + methods plus POST */git-upload-pack; a broader POST rule would \ + enable push (git-receive-pack)" + ); assert!( - layers[0].rule.endpoints.iter().any(|endpoint| { - endpoint.host == "github.com" - && endpoint.rules.iter().any(|rule| { - rule.allow.as_ref().is_some_and(|allow| { - allow.method == "POST" && allow.path.contains("git-upload-pack") - }) - }) - }), - "github git transport should allow POST git-upload-pack for clone/fetch" + git_transport.deny_rules.is_empty(), + "composed git transport should block push via its narrow allow set, not deny rules" ); } diff --git a/docs/sandboxes/providers-v2.mdx b/docs/sandboxes/providers-v2.mdx index c3c84b8383..df35b93534 100644 --- a/docs/sandboxes/providers-v2.mdx +++ b/docs/sandboxes/providers-v2.mdx @@ -605,11 +605,17 @@ endpoints: - host: github.com port: 443 protocol: rest - access: read-only enforcement: enforce + rules: + - allow: { method: GET, path: "**" } + - allow: { method: HEAD, path: "**" } + - allow: { method: OPTIONS, path: "**" } + - allow: { method: POST, path: "/**/git-upload-pack" } binaries: [/usr/bin/gh, /usr/local/bin/gh, /usr/bin/git, /usr/local/bin/git] ``` +The `github.com` git-transport endpoint uses explicit rules instead of the `read-only` preset so HTTPS clone and fetch work out of the box. Git smart HTTP performs a `GET` on `*/info/refs` followed by a `POST` to `*/git-upload-pack`; the read-only preset (`GET`/`HEAD`/`OPTIONS`) blocks that `POST`. The rules permit the read-only methods plus `POST */git-upload-pack` only, so clone and fetch succeed while push (`git-receive-pack`) and other API mutations stay denied. Enabling push requires an explicit policy proposal. + If a sandbox attaches a provider named `work-github`, the effective policy includes a generated provider rule: ```yaml wordWrap showLineNumbers={false} @@ -642,8 +648,12 @@ network_policies: - host: github.com port: 443 protocol: rest - access: read-only enforcement: enforce + rules: + - allow: { method: GET, path: "**" } + - allow: { method: HEAD, path: "**" } + - allow: { method: OPTIONS, path: "**" } + - allow: { method: POST, path: "/**/git-upload-pack" } binaries: - path: /usr/bin/gh - path: /usr/local/bin/gh diff --git a/e2e/python/test_sandbox_providers.py b/e2e/python/test_sandbox_providers.py index 083a424921..5c80cf6011 100644 --- a/e2e/python/test_sandbox_providers.py +++ b/e2e/python/test_sandbox_providers.py @@ -99,6 +99,34 @@ def _delete_provider(stub: object, name: str) -> None: raise +@contextmanager +def _providers_v2_enabled(stub: object) -> Iterator[None]: + """Enable the gateway-global ``providers_v2_enabled`` opt-in for the block. + + Composing a provider's network policy onto a sandbox is gated behind this + setting, which defaults off. The built-in github profile's git-transport + rules only reach the sandbox with it enabled. ``global`` is a Python keyword, + so it is passed positionally through a dict expansion. + """ + stub.UpdateConfig( + openshell_pb2.UpdateConfigRequest( + setting_key="providers_v2_enabled", + setting_value=sandbox_pb2.SettingValue(bool_value=True), + **{"global": True}, + ) + ) + try: + yield + finally: + stub.UpdateConfig( + openshell_pb2.UpdateConfigRequest( + setting_key="providers_v2_enabled", + delete_setting=True, + **{"global": True}, + ) + ) + + # =========================================================================== # Tests: placeholder visibility # =========================================================================== @@ -484,3 +512,66 @@ def test_update_provider_rejects_type_change( assert "type cannot be changed" in exc_info.value.details() finally: _delete_provider(stub, name) + + +# =========================================================================== +# Tests: git transport network policy +# =========================================================================== + + +def test_github_provider_allows_https_git_clone( + sandbox: Callable[..., Sandbox], + sandbox_client: SandboxClient, +) -> None: + """Built-in github provider permits anonymous HTTPS clone/fetch (#1769). + + Git smart HTTP clone/fetch issues a POST to ``*/git-upload-pack``. The + read-only preset (GET/HEAD/OPTIONS) denied that POST, so ``git clone`` over + HTTPS failed. Attaching the github provider composes its network policy onto + the sandbox, exercising provider attachment, effective-policy composition, + TLS interception, and real git behavior end to end. git delegates HTTPS to a + ``git-remote-https`` helper whose ancestor is ``/usr/bin/git``, so the + profile's git binary covers it via ancestor matching. + """ + with _providers_v2_enabled(sandbox_client._stub), provider( + sandbox_client._stub, + name="e2e-test-github-clone", + provider_type="github", + # A required credential value is needed to create the provider, but an + # anonymous clone of a public repo never uses it: git only sends the + # token when a credential helper is configured. + credentials={"GITHUB_TOKEN": "e2e-placeholder-unused"}, + ) as provider_name: + # git opens /dev/null O_RDWR, so it must be read-write; the shared + # _default_policy only grants /dev/urandom. Everything else (binaries, + # CA bundle, clone target) is covered by the standard allowlist. + policy = _default_policy() + policy.filesystem.read_write.append("/dev/null") + spec = datamodel_pb2.SandboxSpec( + policy=policy, + providers=[provider_name], + ) + + with sandbox(spec=spec, delete_on_exit=True) as sb: + clone = sb.exec( + [ + "git", + "clone", + "--depth", + "1", + "https://github.com/octocat/Hello-World.git", + "/tmp/hello-world", + ], + timeout_seconds=120, + ) + assert clone.exit_code == 0, ( + "git clone over HTTPS should succeed with the github provider " + f"attached; stdout={clone.stdout!r} stderr={clone.stderr!r}" + ) + + # A completed clone materializes .git/HEAD, proving ref discovery + # (GET) and upload-pack (POST) both succeeded, not just a handshake. + head = sb.exec(["cat", "/tmp/hello-world/.git/HEAD"]) + assert head.exit_code == 0, ( + f"cloned repo is missing .git/HEAD; stderr={head.stderr!r}" + ) From dc397b892cd00b890aec3a4f9aa7cb3dc257a7bf Mon Sep 17 00:00:00 2001 From: Russell Bryant Date: Fri, 17 Jul 2026 14:39:15 -0400 Subject: [PATCH 3/4] test(providers): isolate providers_v2 mutation in clone e2e The clone e2e enables the gateway-global providers_v2_enabled setting. Restore its exact prior value (or absence) captured via GetGatewayConfig instead of unconditionally deleting it, and serialize the mutation across xdist workers with an exclusive file lock on the run's shared base temp dir, so a shared or pre-configured gateway is left untouched and parallel workers cannot race the read-modify-restore. Refs #1769 Signed-off-by: Russell Bryant --- e2e/python/test_sandbox_providers.py | 82 +++++++++++++++++++++------- 1 file changed, 63 insertions(+), 19 deletions(-) diff --git a/e2e/python/test_sandbox_providers.py b/e2e/python/test_sandbox_providers.py index 5c80cf6011..e2be2e7d6e 100644 --- a/e2e/python/test_sandbox_providers.py +++ b/e2e/python/test_sandbox_providers.py @@ -11,6 +11,7 @@ from __future__ import annotations +import fcntl import time from contextlib import contextmanager from typing import TYPE_CHECKING @@ -99,32 +100,72 @@ def _delete_provider(stub: object, name: str) -> None: raise -@contextmanager -def _providers_v2_enabled(stub: object) -> Iterator[None]: - """Enable the gateway-global ``providers_v2_enabled`` opt-in for the block. +@pytest.fixture +def providers_v2_enabled( + sandbox_client: SandboxClient, + tmp_path_factory: pytest.TempPathFactory, +) -> Iterator[None]: + """Enable the gateway-global ``providers_v2_enabled`` opt-in for one test. Composing a provider's network policy onto a sandbox is gated behind this - setting, which defaults off. The built-in github profile's git-transport - rules only reach the sandbox with it enabled. ``global`` is a Python keyword, - so it is passed positionally through a dict expansion. + setting, which defaults off; the built-in github profile's git-transport + rules only reach the sandbox with it enabled. + + The setting is gateway-global, so this mutates shared state. To keep it safe + on an existing or shared gateway and under parallel workers: + + - The mutation is serialized across xdist workers with an exclusive file lock + on the run's shared base temp dir (``getbasetemp().parent``), held for the + whole test so the read-modify-restore cannot interleave. + - The exact prior value (or absence) is captured via ``GetGatewayConfig`` and + restored in ``finally``, leaving the gateway as it was found. + + ``global`` is a Python keyword, so it is passed through a dict expansion. """ - stub.UpdateConfig( - openshell_pb2.UpdateConfigRequest( - setting_key="providers_v2_enabled", - setting_value=sandbox_pb2.SettingValue(bool_value=True), - **{"global": True}, + stub = sandbox_client._stub + key = "providers_v2_enabled" + lock_path = tmp_path_factory.getbasetemp().parent / "providers-v2-setting.lock" + with lock_path.open("w") as lock_file: + fcntl.flock(lock_file, fcntl.LOCK_EX) + + # GetGatewayConfig returns known keys even when unset, with an empty + # SettingValue (no populated oneof). Only treat the setting as explicitly + # present when its value oneof is actually set; otherwise restore = delete. + config = stub.GetGatewayConfig(sandbox_pb2.GetGatewayConfigRequest()) + prior_value = sandbox_pb2.SettingValue() + had_prior = ( + key in config.settings + and config.settings[key].WhichOneof("value") is not None ) - ) - try: - yield - finally: + if had_prior: + prior_value.CopyFrom(config.settings[key]) + stub.UpdateConfig( openshell_pb2.UpdateConfigRequest( - setting_key="providers_v2_enabled", - delete_setting=True, + setting_key=key, + setting_value=sandbox_pb2.SettingValue(bool_value=True), **{"global": True}, ) ) + try: + yield + finally: + if had_prior: + stub.UpdateConfig( + openshell_pb2.UpdateConfigRequest( + setting_key=key, + setting_value=prior_value, + **{"global": True}, + ) + ) + else: + stub.UpdateConfig( + openshell_pb2.UpdateConfigRequest( + setting_key=key, + delete_setting=True, + **{"global": True}, + ) + ) # =========================================================================== @@ -519,6 +560,7 @@ def test_update_provider_rejects_type_change( # =========================================================================== +@pytest.mark.usefixtures("providers_v2_enabled") def test_github_provider_allows_https_git_clone( sandbox: Callable[..., Sandbox], sandbox_client: SandboxClient, @@ -531,9 +573,11 @@ def test_github_provider_allows_https_git_clone( the sandbox, exercising provider attachment, effective-policy composition, TLS interception, and real git behavior end to end. git delegates HTTPS to a ``git-remote-https`` helper whose ancestor is ``/usr/bin/git``, so the - profile's git binary covers it via ancestor matching. + profile's git binary covers it via ancestor matching. The + ``providers_v2_enabled`` fixture turns on the gateway-global gate that + composes the provider's network policy. """ - with _providers_v2_enabled(sandbox_client._stub), provider( + with provider( sandbox_client._stub, name="e2e-test-github-clone", provider_type="github", From b1d100be1ccfcdbc951f3bf265b1fc22fda41105 Mon Sep 17 00:00:00 2001 From: Russell Bryant Date: Fri, 17 Jul 2026 15:14:06 -0400 Subject: [PATCH 4/4] test(providers): serialize providers_v2 mutation with a suite-wide guard The clone e2e's per-fixture lock only coordinated fixtures that acquired it; other xdist workers hit the same gateway without it and could observe the transiently-enabled providers_v2_enabled global during their own sandbox creation (CWE-362). Add an autouse readers-writer guard in conftest: every test holds a shared lock on the gateway config, and a test marked exclusive_gateway_config holds an exclusive lock. Mark the clone test exclusive so no other worker is mid-test while it enables and restores the gateway-global setting. Exact prior-value restoration is retained. Refs #1769 Signed-off-by: Russell Bryant --- e2e/python/conftest.py | 33 ++++++++++ e2e/python/test_sandbox_providers.py | 94 +++++++++++++--------------- 2 files changed, 77 insertions(+), 50 deletions(-) diff --git a/e2e/python/conftest.py b/e2e/python/conftest.py index 07b7feb34b..45bd1ba4d1 100644 --- a/e2e/python/conftest.py +++ b/e2e/python/conftest.py @@ -3,6 +3,7 @@ from __future__ import annotations +import fcntl import os import time from typing import TYPE_CHECKING @@ -16,6 +17,38 @@ from collections.abc import Callable, Iterator +def pytest_configure(config: pytest.Config) -> None: + config.addinivalue_line( + "markers", + "exclusive_gateway_config: hold an exclusive lock on gateway-global " + "config so no other xdist worker observes a transient global setting", + ) + + +@pytest.fixture(autouse=True) +def _gateway_config_guard( + request: pytest.FixtureRequest, + tmp_path_factory: pytest.TempPathFactory, +) -> Iterator[None]: + """Readers-writer guard over gateway-global config mutations. + + The e2e gateway is shared across xdist workers, so a test that flips a + gateway-global setting can leak that transient value into another worker's + sandbox creation. Every test holds a shared lock by default; a test marked + ``exclusive_gateway_config`` holds an exclusive lock, so while it mutates and + restores the setting no other worker is mid-test and none can observe the + transient value. The lock file lives in the run's shared base temp dir + (``getbasetemp().parent``), which is common to all xdist workers. + """ + lock_path = tmp_path_factory.getbasetemp().parent / "gateway-config.lock" + exclusive = ( + request.node.get_closest_marker("exclusive_gateway_config") is not None + ) + with lock_path.open("w") as lock_file: + fcntl.flock(lock_file, fcntl.LOCK_EX if exclusive else fcntl.LOCK_SH) + yield + + @pytest.fixture(scope="session") def cluster_name() -> str | None: return os.environ.get("OPENSHELL_GATEWAY") diff --git a/e2e/python/test_sandbox_providers.py b/e2e/python/test_sandbox_providers.py index e2be2e7d6e..21820f2e48 100644 --- a/e2e/python/test_sandbox_providers.py +++ b/e2e/python/test_sandbox_providers.py @@ -11,7 +11,6 @@ from __future__ import annotations -import fcntl import time from contextlib import contextmanager from typing import TYPE_CHECKING @@ -103,7 +102,7 @@ def _delete_provider(stub: object, name: str) -> None: @pytest.fixture def providers_v2_enabled( sandbox_client: SandboxClient, - tmp_path_factory: pytest.TempPathFactory, + _gateway_config_guard: None, ) -> Iterator[None]: """Enable the gateway-global ``providers_v2_enabled`` opt-in for one test. @@ -111,61 +110,55 @@ def providers_v2_enabled( setting, which defaults off; the built-in github profile's git-transport rules only reach the sandbox with it enabled. - The setting is gateway-global, so this mutates shared state. To keep it safe - on an existing or shared gateway and under parallel workers: - - - The mutation is serialized across xdist workers with an exclusive file lock - on the run's shared base temp dir (``getbasetemp().parent``), held for the - whole test so the read-modify-restore cannot interleave. - - The exact prior value (or absence) is captured via ``GetGatewayConfig`` and - restored in ``finally``, leaving the gateway as it was found. - - ``global`` is a Python keyword, so it is passed through a dict expansion. + The setting is gateway-global. Exclusivity against other xdist workers is + provided by the ``exclusive_gateway_config`` marker plus the autouse + ``_gateway_config_guard`` guard (see conftest): no concurrent worker is + mid-test while this fixture mutates and restores the setting, so none can + observe the transient value. Depending on the guard here also orders the + exclusive lock acquisition before the mutation. + + ``GetGatewayConfig`` returns known keys even when unset, with an empty + ``SettingValue`` (no populated oneof), so the setting is treated as present + only when its value oneof is set; otherwise restore is a delete. ``global`` + is a Python keyword, so it is passed through a dict expansion. """ stub = sandbox_client._stub key = "providers_v2_enabled" - lock_path = tmp_path_factory.getbasetemp().parent / "providers-v2-setting.lock" - with lock_path.open("w") as lock_file: - fcntl.flock(lock_file, fcntl.LOCK_EX) - - # GetGatewayConfig returns known keys even when unset, with an empty - # SettingValue (no populated oneof). Only treat the setting as explicitly - # present when its value oneof is actually set; otherwise restore = delete. - config = stub.GetGatewayConfig(sandbox_pb2.GetGatewayConfigRequest()) - prior_value = sandbox_pb2.SettingValue() - had_prior = ( - key in config.settings - and config.settings[key].WhichOneof("value") is not None + config = stub.GetGatewayConfig(sandbox_pb2.GetGatewayConfigRequest()) + prior_value = sandbox_pb2.SettingValue() + had_prior = ( + key in config.settings + and config.settings[key].WhichOneof("value") is not None + ) + if had_prior: + prior_value.CopyFrom(config.settings[key]) + + stub.UpdateConfig( + openshell_pb2.UpdateConfigRequest( + setting_key=key, + setting_value=sandbox_pb2.SettingValue(bool_value=True), + **{"global": True}, ) + ) + try: + yield + finally: if had_prior: - prior_value.CopyFrom(config.settings[key]) - - stub.UpdateConfig( - openshell_pb2.UpdateConfigRequest( - setting_key=key, - setting_value=sandbox_pb2.SettingValue(bool_value=True), - **{"global": True}, - ) - ) - try: - yield - finally: - if had_prior: - stub.UpdateConfig( - openshell_pb2.UpdateConfigRequest( - setting_key=key, - setting_value=prior_value, - **{"global": True}, - ) + stub.UpdateConfig( + openshell_pb2.UpdateConfigRequest( + setting_key=key, + setting_value=prior_value, + **{"global": True}, ) - else: - stub.UpdateConfig( - openshell_pb2.UpdateConfigRequest( - setting_key=key, - delete_setting=True, - **{"global": True}, - ) + ) + else: + stub.UpdateConfig( + openshell_pb2.UpdateConfigRequest( + setting_key=key, + delete_setting=True, + **{"global": True}, ) + ) # =========================================================================== @@ -560,6 +553,7 @@ def test_update_provider_rejects_type_change( # =========================================================================== +@pytest.mark.exclusive_gateway_config @pytest.mark.usefixtures("providers_v2_enabled") def test_github_provider_allows_https_git_clone( sandbox: Callable[..., Sandbox],