Fix guard fallback to preserve non-noop guards when server guard policies exist#1741
Fix guard fallback to preserve non-noop guards when server guard policies exist#1741
Conversation
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
…cies exist The issue was in requireGuardPolicyIfGuardEnabled where guards were being downgraded to noop during registration even when guard policies were configured at the server level. This happened because: 1. Guards are registered BEFORE auto-detection runs 2. During registration, if resolveGuardPolicy returns nil, it fell back to noop 3. After registration, auto-detection enables DIFC based on hasServerGuardPolicies 4. But by then, the guard was already downgraded to noop The fix checks if server guard policies exist before falling back to noop guard. If guard policies are configured, the non-noop guard is kept and will be properly initialized when DIFC is auto-enabled. This fixes the smoke test failure where AllowOnly guard enforcement was not working after the v0.1.12 release which introduced automatic DIFC detection. Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes a DIFC guard registration timing issue where a discovered non-noop guard could be downgraded to noop during startup (before DIFC auto-enablement runs), which breaks enforcement when per-server guard policies are configured.
Changes:
- Update
requireGuardPolicyIfGuardEnabledto preserve a non-noop guard when per-serverGuardPoliciesexist, even if no policy is resolved at registration time. - Add unit tests around guard policy parsing/resolution and DIFC auto-detection of server guard policies.
- Add unit tests for guard fallback behavior with/without configured policies.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/server/unified.go | Adjusts guard fallback behavior to avoid downgrading discovered guards to noop when server guard policies are configured. |
| internal/server/require_guard_policy_test.go | Adds tests for requireGuardPolicyIfGuardEnabled fallback behavior. |
| internal/server/guard_policy_parsing_test.go | Adds tests for parsing server guard policies and resolving policies from server config. |
| internal/server/has_server_guard_policies_test.go | Adds tests for detecting whether any server has guard policies configured. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "allow-only": map[string]interface{}{ | ||
| "min-integrity": "approved", | ||
| "repos": []interface{}{"github/gh-aw*"}, |
There was a problem hiding this comment.
This test doesn’t appear to exercise the new policy == nil && len(serverCfg.GuardPolicies) > 0 branch. With an allow-only guard-policies map present, resolveGuardPolicy() should parse it and return a non-nil policy, so requireGuardPolicyIfGuardEnabled would keep the guard even without the PR change. Consider adjusting the setup to make resolveGuardPolicy() return nil while GuardPolicies is still non-empty (or otherwise asserting the specific branch), so the test protects against regressions of the timing bug being fixed here.
| "allow-only": map[string]interface{}{ | |
| "min-integrity": "approved", | |
| "repos": []interface{}{"github/gh-aw*"}, | |
| "unknown-policy": map[string]interface{}{ | |
| "unexpected-field": "value", |
| // Repos can be either a string or a slice after parsing | ||
| // Verify the repos field is present and has the expected value | ||
| reposSlice, ok := policy.AllowOnly.Repos.([]interface{}) | ||
| if ok { | ||
| require.Len(t, reposSlice, 1, "repos should have 1 element") | ||
| assert.Equal(t, "github/gh-aw*", reposSlice[0], "repos[0] should be 'github/gh-aw*'") | ||
| } else { | ||
| t.Fatalf("repos is not a []interface{}, got %T: %v", policy.AllowOnly.Repos, policy.AllowOnly.Repos) |
There was a problem hiding this comment.
This assertion makes the test depend on the concrete Go type used to represent AllowOnly.Repos (currently []interface{} from JSON unmarshalling). That’s an implementation detail and may validly become []string (NormalizeGuardPolicy supports both), causing a false failure. Prefer asserting on normalized policy output (e.g., via config.NormalizeGuardPolicy) or accepting both slice types in the test.
| // Repos can be either a string or a slice after parsing | |
| // Verify the repos field is present and has the expected value | |
| reposSlice, ok := policy.AllowOnly.Repos.([]interface{}) | |
| if ok { | |
| require.Len(t, reposSlice, 1, "repos should have 1 element") | |
| assert.Equal(t, "github/gh-aw*", reposSlice[0], "repos[0] should be 'github/gh-aw*'") | |
| } else { | |
| t.Fatalf("repos is not a []interface{}, got %T: %v", policy.AllowOnly.Repos, policy.AllowOnly.Repos) | |
| // Repos can be represented as different slice types after parsing. | |
| // Verify the repos field is present and has the expected value without depending on concrete type. | |
| switch repos := policy.AllowOnly.Repos.(type) { | |
| case []interface{}: | |
| require.Len(t, repos, 1, "repos should have 1 element") | |
| str, ok := repos[0].(string) | |
| require.True(t, ok, "repos[0] should be a string, got %T", repos[0]) | |
| assert.Equal(t, "github/gh-aw*", str, "repos[0] should be 'github/gh-aw*'") | |
| case []string: | |
| require.Len(t, repos, 1, "repos should have 1 element") | |
| assert.Equal(t, "github/gh-aw*", repos[0], "repos[0] should be 'github/gh-aw*'") | |
| default: | |
| t.Fatalf("repos has unexpected type %T: %v", policy.AllowOnly.Repos, policy.AllowOnly.Repos) |
| // If not, fall back to noop guard. | ||
| if us.cfg != nil && us.cfg.Servers != nil { | ||
| if serverCfg, ok := us.cfg.Servers[serverID]; ok && serverCfg != nil && len(serverCfg.GuardPolicies) > 0 { | ||
| log.Printf("[DIFC] Guard '%s' loaded for server '%s' with guard-policies config (policy will be resolved during guard initialization)", g.Name(), serverID) |
There was a problem hiding this comment.
The new log line says the policy "will be resolved during guard initialization", but policy resolution already happens via resolveGuardPolicy() and ensureGuardInitialized() will skip initialization entirely when policy remains nil. Consider rewording this message to reflect the actual behavior (e.g., policy currently unresolved but guard-policies are configured, so the guard is preserved) to avoid implying that resolution is guaranteed later.
| log.Printf("[DIFC] Guard '%s' loaded for server '%s' with guard-policies config (policy will be resolved during guard initialization)", g.Name(), serverID) | |
| log.Printf("[DIFC] Guard '%s' loaded for server '%s' with guard-policies config (policy currently unresolved; preserving guard because guard-policies are configured)", g.Name(), serverID) |
The v0.1.12 auto-detection introduced a timing bug where guards were downgraded to noop during registration, before DIFC auto-enablement ran. This broke AllowOnly enforcement in smoke tests.
Problem
Guard registration (line 180) happens before auto-detection (line 188). When
requireGuardPolicyIfGuardEnabledfinds no resolved policy during registration, it falls back to noop. By the timehasServerGuardPolicies(cfg)triggers auto-enablement, the guard is already noop.Solution
Modified
requireGuardPolicyIfGuardEnabledto checkserverCfg.GuardPoliciesbefore falling back:This preserves the WASM guard discovered from
/guards/{serverID}/when guard policies exist, allowing proper initialization duringensureGuardInitialized.Changes
parseServerGuardPolicyandresolveGuardPolicyWarning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
example.com/tmp/go-build2269947796/b332/launcher.test /tmp/go-build2269947796/b332/launcher.test -test.testlogfile=/tmp/go-build2269947796/b332/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true --abbrev-ref HEAD /usr/bin/base64 --abbrev-ref 877973+lpcox@userev-parse ache/Python/3.12--abbrev-ref base64 -d 64/pkg/tool/linu--abbrev-ref base64 /usr/bin/dirname x_amd64/compile 64/pkg/tool/linu-d ndor/bin/git t } } { printf "%s%s", sep, HEAD(dns block)invalid-host-that-does-not-exist-12345.com/tmp/go-build2269947796/b314/config.test /tmp/go-build2269947796/b314/config.test -test.testlogfile=/tmp/go-build2269947796/b314/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true --abbrev-ref HEAD /usr/bin/base64 m/CLAUDE.md m/CODE_OF_CONDUC-o ache/go/1.25.7/x/tmp/go-build1778197618/b062/_pkg_.a base64 -d m/LICENSE.md m/Makefile /home/REDACTED/go/-lang=go1.25 m/README.md m/ROADMAP.md Policy_ServerGuaccs13v1iKv05Y3GP5cax/ccs13v1iKv05Y3GP5cax git(dns block)nonexistent.local/tmp/go-build2269947796/b332/launcher.test /tmp/go-build2269947796/b332/launcher.test -test.testlogfile=/tmp/go-build2269947796/b332/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true --abbrev-ref HEAD /usr/bin/base64 --abbrev-ref 877973+lpcox@userev-parse ache/Python/3.12--abbrev-ref base64 -d 64/pkg/tool/linu--abbrev-ref base64 /usr/bin/dirname x_amd64/compile 64/pkg/tool/linu-d ndor/bin/git t } } { printf "%s%s", sep, HEAD(dns block)slow.example.com/tmp/go-build2269947796/b332/launcher.test /tmp/go-build2269947796/b332/launcher.test -test.testlogfile=/tmp/go-build2269947796/b332/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true --abbrev-ref HEAD /usr/bin/base64 --abbrev-ref 877973+lpcox@userev-parse ache/Python/3.12--abbrev-ref base64 -d 64/pkg/tool/linu--abbrev-ref base64 /usr/bin/dirname x_amd64/compile 64/pkg/tool/linu-d ndor/bin/git t } } { printf "%s%s", sep, HEAD(dns block)this-host-does-not-exist-12345.com/tmp/go-build2269947796/b341/mcp.test /tmp/go-build2269947796/b341/mcp.test -test.testlogfile=/tmp/go-build2269947796/b341/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true --abbrev-ref HEAD ptables --abbrev-ref 877973+lpcox@use-c tnet/tools/git base64(dns block)If you need me to access, download, or install something from one of these locations, you can either: