Skip to content

Commit 1a75624

Browse files
authored
refactor(guard): extract validateIntegrityField to eliminate triplicated switch blocks (#4944)
The integrity-level validation switch (`none|unapproved|approved|merged`) was inlined identically three times in `buildStrictLabelAgentPayload`, meaning any new integrity level required three synchronized edits. ## Changes - **`internal/guard/wasm_validate.go`** *(new)*: single `validateIntegrityField(fieldName string, raw interface{}) error` helper — handles both the `string` type assertion and the validity switch in one place - **`internal/guard/wasm_payload.go`**: replaces the three duplicate blocks for `integrity`, `disapproval-integrity`, and `endorser-min-integrity` with calls to the helper ```go // before — repeated verbatim three times disInt, ok := disIntRaw.(string) if !ok { return nil, fmt.Errorf("invalid disapproval-integrity value: ...") } switch strings.ToLower(strings.TrimSpace(disInt)) { case "none", "unapproved", "approved", "merged": default: return nil, fmt.Errorf("invalid disapproval-integrity value: ...") } // after if err := validateIntegrityField("disapproval-integrity", disIntRaw); err != nil { return nil, err } ``` > [!WARNING] > > <details> > <summary>Firewall rules blocked me from connecting to one or more addresses (expand for details)</summary> > > #### I tried to connect to the following addresses, but was blocked by firewall rules: > > - `example.com` > - Triggering command: `/tmp/go-build3106413706/b513/launcher.test /tmp/go-build3106413706/b513/launcher.test -test.testlogfile=/tmp/go-build3106413706/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s -o .cfg 3753103/b314/ x_amd64/vet -p github.com/githu--version -lang=go1.25 x_amd64/vet .cfg�� 3753103/b364/_pkg_.a ache/go/1.25.9/x64/src/net/http/internal/httpcommon/httpcommon.ggoogle.golang.org/protobuf/types-qE x_amd64/vet --gdwarf-5 --64 -o 42W3fCn/gP4cHa5aRI3EQQAZUMxk` (dns block) > - `invalid-host-that-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build3106413706/b495/config.test /tmp/go-build3106413706/b495/config.test -test.testlogfile=/tmp/go-build3106413706/b495/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build3106413706/b393/vet.cfg 1.80.0/resolver/dns/dns_resolver.go 64/src/crypto/x509/cert_pool.go x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet -o g_.a -trimpath x_amd64/vet -p gzip -lang=go1.25 x_amd64/vet` (dns block) > - `nonexistent.local` > - Triggering command: `/tmp/go-build3106413706/b513/launcher.test /tmp/go-build3106413706/b513/launcher.test -test.testlogfile=/tmp/go-build3106413706/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s -o .cfg 3753103/b314/ x_amd64/vet -p github.com/githu--version -lang=go1.25 x_amd64/vet .cfg�� 3753103/b364/_pkg_.a ache/go/1.25.9/x64/src/net/http/internal/httpcommon/httpcommon.ggoogle.golang.org/protobuf/types-qE x_amd64/vet --gdwarf-5 --64 -o 42W3fCn/gP4cHa5aRI3EQQAZUMxk` (dns block) > - `slow.example.com` > - Triggering command: `/tmp/go-build3106413706/b513/launcher.test /tmp/go-build3106413706/b513/launcher.test -test.testlogfile=/tmp/go-build3106413706/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s -o .cfg 3753103/b314/ x_amd64/vet -p github.com/githu--version -lang=go1.25 x_amd64/vet .cfg�� 3753103/b364/_pkg_.a ache/go/1.25.9/x64/src/net/http/internal/httpcommon/httpcommon.ggoogle.golang.org/protobuf/types-qE x_amd64/vet --gdwarf-5 --64 -o 42W3fCn/gP4cHa5aRI3EQQAZUMxk` (dns block) > - `this-host-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build3106413706/b522/mcp.test /tmp/go-build3106413706/b522/mcp.test -test.testlogfile=/tmp/go-build3106413706/b522/testlog.txt -test.paniconexit0 -test.timeout=10m0s -I .cfg -I x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet .cfg�� 3753103/b400/_pkg_.a -dynimport x_amd64/vet -dynout g/grpc/internal//usr/bin/runc p/bin/git x_amd64/vet` (dns block) > > If you need me to access, download, or install something from one of these locations, you can either: > > - Configure [Actions setup steps](https://gh.io/copilot/actions-setup-steps) to set up my environment, which run before the firewall is enabled > - Add the appropriate URLs or hosts to the custom allowlist in this repository's [Copilot coding agent settings](https://github.com/github/gh-aw-mcpg/settings/copilot/coding_agent) (admins only) > > </details>
2 parents 5c065ca + d1461dd commit 1a75624

3 files changed

Lines changed: 47 additions & 26 deletions

File tree

internal/guard/wasm_payload.go

Lines changed: 8 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -92,8 +92,10 @@ func buildStrictLabelAgentPayload(policy interface{}) (map[string]interface{}, e
9292

9393
reposRaw, hasRepos := allowOnly["repos"]
9494
integrityRaw, hasIntegrity := allowOnly["min-integrity"]
95+
integrityFieldName := "min-integrity"
9596
if !hasIntegrity {
9697
integrityRaw, hasIntegrity = allowOnly["integrity"]
98+
integrityFieldName = "integrity"
9799
}
98100
if !hasRepos || !hasIntegrity {
99101
return nil, fmt.Errorf("invalid guard policy transport shape: missing required fields repos and/or min-integrity in allow-only object")
@@ -114,15 +116,8 @@ func buildStrictLabelAgentPayload(policy interface{}) (map[string]interface{}, e
114116
return nil, fmt.Errorf("invalid repos value: expected all, public, or non-empty array of scoped strings")
115117
}
116118

117-
integrity, ok := integrityRaw.(string)
118-
if !ok {
119-
return nil, fmt.Errorf("invalid integrity value: expected one of none|unapproved|approved|merged")
120-
}
121-
122-
switch strings.ToLower(strings.TrimSpace(integrity)) {
123-
case "none", "unapproved", "approved", "merged":
124-
default:
125-
return nil, fmt.Errorf("invalid integrity value: expected one of none|unapproved|approved|merged")
119+
if err := validateIntegrityField(integrityFieldName, integrityRaw); err != nil {
120+
return nil, err
126121
}
127122

128123
// Validate blocked-users if present: must be a non-empty array of non-empty strings.
@@ -199,27 +194,15 @@ func buildStrictLabelAgentPayload(policy interface{}) (map[string]interface{}, e
199194

200195
// Validate disapproval-integrity if present.
201196
if disIntRaw, ok := allowOnly["disapproval-integrity"]; ok {
202-
disInt, ok := disIntRaw.(string)
203-
if !ok {
204-
return nil, fmt.Errorf("invalid disapproval-integrity value: expected one of none|unapproved|approved|merged")
205-
}
206-
switch strings.ToLower(strings.TrimSpace(disInt)) {
207-
case "none", "unapproved", "approved", "merged":
208-
default:
209-
return nil, fmt.Errorf("invalid disapproval-integrity value: expected one of none|unapproved|approved|merged")
197+
if err := validateIntegrityField("disapproval-integrity", disIntRaw); err != nil {
198+
return nil, err
210199
}
211200
}
212201

213202
// Validate endorser-min-integrity if present.
214203
if endMinRaw, ok := allowOnly["endorser-min-integrity"]; ok {
215-
endMin, ok := endMinRaw.(string)
216-
if !ok {
217-
return nil, fmt.Errorf("invalid endorser-min-integrity value: expected one of none|unapproved|approved|merged")
218-
}
219-
switch strings.ToLower(strings.TrimSpace(endMin)) {
220-
case "none", "unapproved", "approved", "merged":
221-
default:
222-
return nil, fmt.Errorf("invalid endorser-min-integrity value: expected one of none|unapproved|approved|merged")
204+
if err := validateIntegrityField("endorser-min-integrity", endMinRaw); err != nil {
205+
return nil, err
223206
}
224207
}
225208

internal/guard/wasm_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -358,7 +358,7 @@ func TestBuildStrictLabelAgentPayloadExtended(t *testing.T) {
358358

359359
_, err := buildStrictLabelAgentPayload(policy)
360360
require.Error(t, err)
361-
assert.Contains(t, err.Error(), "invalid integrity value")
361+
assert.Contains(t, err.Error(), "invalid min-integrity value")
362362
})
363363

364364
t.Run("valid allow-only policy succeeds", func(t *testing.T) {

internal/guard/wasm_validate.go

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
package guard
2+
3+
import (
4+
"fmt"
5+
"strings"
6+
)
7+
8+
// allowedIntegrityLevels is the single source of truth for valid integrity-level values.
9+
var allowedIntegrityLevels = []string{"none", "unapproved", "approved", "merged"}
10+
11+
var allowedIntegrityLevelSet = map[string]struct{}{
12+
"none": {},
13+
"unapproved": {},
14+
"approved": {},
15+
"merged": {},
16+
}
17+
18+
func invalidIntegrityFieldError(fieldName string) error {
19+
return fmt.Errorf(
20+
"invalid %s value: expected one of %s",
21+
fieldName,
22+
strings.Join(allowedIntegrityLevels, "|"),
23+
)
24+
}
25+
26+
// validateIntegrityField returns an error if raw is not a valid integrity-level
27+
// string. fieldName is used in the error message (e.g. "disapproval-integrity").
28+
func validateIntegrityField(fieldName string, raw interface{}) error {
29+
s, ok := raw.(string)
30+
if !ok {
31+
return invalidIntegrityFieldError(fieldName)
32+
}
33+
normalized := strings.ToLower(strings.TrimSpace(s))
34+
if _, ok := allowedIntegrityLevelSet[normalized]; ok {
35+
return nil
36+
}
37+
return invalidIntegrityFieldError(fieldName)
38+
}

0 commit comments

Comments
 (0)