fix(sandbox,server): fix chunk merge duplicates and OPA variable collision with overlapping policies#571
Merged
johntmyers merged 6 commits intomainfrom Mar 24, 2026
Conversation
johntmyers
changed the base branch from
main
to
fix/implicit-allowed-ips-for-ip-host
johntmyers
force-pushed
the
fix/chunk-merge-and-opa-duplicate-policies
branch
from
2df59b9 to
4318fe3
Compare
drew
previously approved these changes
Mar 24, 2026
…ision with overlapping policies
Two related bugs triggered when a draft rule approval creates a second
policy entry for the same host:port:
1. merge_chunk_into_policy looked up existing rules by chunk.rule_name
(auto-generated as allow_{host}_{port}), which never matched the
user's original rule name. Now scans all network_policies entries
for a host:port endpoint match before falling back to insertion,
and merges allowed_ips into the existing endpoint.
2. The Rego allow_request rule and _matching_endpoint_configs
comprehension used 'some ep; ep := policy.endpoints[_]' which
caused regorus to error with 'duplicated definition of local
variable ep' when multiple policies covered the same host:port.
Refactored to isolate endpoint iteration inside helper functions
(_policy_allows_l7, _policy_endpoint_configs) so variables are
scoped per-policy evaluation.
Refs: #567
… allowed_ips - Update FWD-2 (test_forward_proxy_denied_without_allowed_ips -> test_forward_proxy_allows_private_ip_host_without_allowed_ips): literal IP host no longer requires explicit allowed_ips, expects 200. - Add OVL-1: overlapping L4 policies for same host:port must not crash OPA and should allow forward proxy connections. - Add OVL-2: overlapping L7 policies for same host:port must not crash OPA and should allow CONNECT tunnel establishment. Refs: #567
SSRF-6: Private IP with literal IP host now gets implicit allowed_ips from PR #570, so CONNECT returns 200 instead of 403. SSRF-3: Loopback is still blocked but via the always-blocked path (implicit allowed_ips is synthesized, then resolve_and_check_allowed_ips catches it). Log message says 'always-blocked' instead of 'internal address'.
johntmyers
force-pushed
the
fix/chunk-merge-and-opa-duplicate-policies
branch
from
5996c22 to
c73845d
Compare
…arget port When the SSRF check passes but nothing listens on the target port, recv() returns empty bytes. Use 'assert 403 not in' (matching SSRF-4 pattern) instead of 'assert 200 in'.
PR #569 changed credential redaction from clearing the map to replacing values with 'REDACTED'. Update e2e assertions to expect credential keys with REDACTED values instead of an empty map.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix two related bugs where approving a draft network rule creates duplicate policy entries for the same host:port, causing the OPA engine to crash with
duplicated definition of local variable.Related Issue
Refs #567
Changes
Bug 1: Chunk merge creates duplicates (
grpc.rs)merge_chunk_into_policynow scans allnetwork_policiesentries for a host:port endpoint match before falling back to rule_name lookupallowed_ips, binaries, and endpoints into the existing entry, preserving the user's original rule nameBug 2: OPA Rego variable collision (
sandbox-policy.rego)allow_requestto isolate endpoint iteration inside a_policy_allows_l7helper function, scoping theepvariable per-policy evaluation_matching_endpoint_configsto use a_policy_endpoint_configshelper with distinct variable names (cfgvsep) to avoid regorus collisionsduplicated definition of local variable eperror when multiple policies cover the same host:portE2E tests
allowed_ips(depends on fix(sandbox): treat literal IP in policy host as implicit allowed_ips #570)Root Cause
Bug 1:
merge_chunk_into_policylooked up existing policy entries bychunk.rule_name(auto-generated asallow_{host}_{port}by the mechanistic mapper), which never matches user-authored names liketest_server. The merge fell through to insertion, creating a duplicate.Bug 2: The Rego
allow_requestrule usedsome ep; ep := policy.endpoints[_]directly in a complete rule body. When two policies matched the same host:port, regorus sawepbound to different values across iterations and errored. The existing_matching_endpoint_configscomprehension had the same latent issue.Testing
mise run pre-commitpassescargo test -p openshell-sandboxpasses (315 tests)cargo test -p openshell-serverpasses (197 tests)Checklist