From 45a2c82d2fdc75670b273c1b4e030493d8feae5b Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Tue, 7 Jul 2026 19:38:52 -0700 Subject: [PATCH 1/2] Address PR #8903 review feedback for sink-visibility runtime check - Skip runtime check when sink-visibility is omitted (empty string), preserving backward-compatible behavior for legacy configs - Remove duplicate logging from VerifySinkVisibility helper; let the caller (verifySinkVisibilityAtRuntime) handle all logging with serverID context - Return configured value when no override is needed, instead of returning the actual API value which could drift from compiler config (e.g. configured='private' but actual='internal' no longer returns 'internal') Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- internal/githubhttp/visibility.go | 29 +++++++++++--------------- internal/githubhttp/visibility_test.go | 2 +- internal/server/guard_init.go | 8 +++++++ 3 files changed, 21 insertions(+), 18 deletions(-) diff --git a/internal/githubhttp/visibility.go b/internal/githubhttp/visibility.go index ecf9b2fb2..f7ba593a8 100644 --- a/internal/githubhttp/visibility.go +++ b/internal/githubhttp/visibility.go @@ -87,12 +87,12 @@ func FetchRepoVisibility(ctx context.Context, apiBaseURL, nwo, authHeader string } // VerifySinkVisibility compares the configured sink-visibility against the -// actual repository visibility from the GitHub API. If the actual visibility -// is more public than configured, it returns the actual (more restrictive) -// visibility to prevent exfiltration. +// actual repository visibility from the GitHub API. Only overrides the +// configured value in the security-critical case: repo is actually public +// but config doesn't say "public". // // Returns: -// - The effective visibility to use (may override configured value) +// - The effective visibility to use (configured value unless overridden to "public") // - Whether an override occurred // - Any error encountered (non-fatal — callers should log and use configured value) func VerifySinkVisibility(ctx context.Context, apiBaseURL, nwo, authHeader, configuredVisibility string) (string, bool, error) { @@ -105,23 +105,18 @@ func VerifySinkVisibility(ctx context.Context, apiBaseURL, nwo, authHeader, conf return configuredVisibility, false, err } - actualStr := string(actual) configured := strings.ToLower(strings.TrimSpace(configuredVisibility)) - // If actual is "public" but configured is not "public" (or unset), + // If actual is "public" but configured is not "public", // override to "public" — this is the security-critical case. if actual == RepoVisibilityPublic && configured != "public" { - logger.LogWarn("difc", "Sink visibility override: configured=%q but repo %s is actually PUBLIC — overriding to \"public\" to prevent exfiltration", configured, nwo) - return actualStr, true, nil + return "public", true, nil } - // If configured says public but actual says private/internal, - // keep "public" (the more restrictive setting) — defense in depth. - if configured == "public" && actual != RepoVisibilityPublic { - logVisibility.Printf("Sink visibility: configured=public but repo %s is %s — keeping public (more restrictive)", nwo, actual) - return "public", false, nil - } - - logVisibility.Printf("Sink visibility verified: configured=%q, actual=%s — no override needed", configured, actual) - return actualStr, false, nil + // All other cases: return the configured value unchanged. + // This includes: + // - configured="public" but actual=private → keep "public" (more restrictive) + // - configured="private" and actual=private → no change needed + // - configured="internal" and actual=internal → no change needed + return configuredVisibility, false, nil } diff --git a/internal/githubhttp/visibility_test.go b/internal/githubhttp/visibility_test.go index 70eed79f0..6e8e01cbd 100644 --- a/internal/githubhttp/visibility_test.go +++ b/internal/githubhttp/visibility_test.go @@ -170,7 +170,7 @@ func TestVerifySinkVisibility(t *testing.T) { configured: "private", actualVis: "internal", actualPrivate: true, - wantEffective: "internal", + wantEffective: "private", wantOverridden: false, }, } diff --git a/internal/server/guard_init.go b/internal/server/guard_init.go index 0babf4d3e..a3d4cf427 100644 --- a/internal/server/guard_init.go +++ b/internal/server/guard_init.go @@ -391,6 +391,14 @@ func (us *UnifiedServer) getTrustedBots() []string { // Emits a warning when overriding the configured value. // Falls back to the configured value on any API error (non-fatal). func (us *UnifiedServer) verifySinkVisibilityAtRuntime(serverID, configuredVisibility string) string { + // Skip runtime check when sink-visibility is not explicitly configured. + // This preserves backward-compatible behavior where omitted sink-visibility + // uses accept patterns without any override. + if configuredVisibility == "" { + logGuardInit.Printf("sink-visibility runtime check skipped: sink-visibility not configured (serverID=%s)", serverID) + return configuredVisibility + } + nwo := os.Getenv("GITHUB_REPOSITORY") if nwo == "" { logGuardInit.Printf("sink-visibility runtime check skipped: GITHUB_REPOSITORY not set (serverID=%s)", serverID) From 08735e50e00c4997463156c703936af6e2cb5978 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Tue, 7 Jul 2026 19:42:16 -0700 Subject: [PATCH 2/2] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- internal/githubhttp/visibility.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/githubhttp/visibility.go b/internal/githubhttp/visibility.go index f7ba593a8..3b1ba15aa 100644 --- a/internal/githubhttp/visibility.go +++ b/internal/githubhttp/visibility.go @@ -118,5 +118,5 @@ func VerifySinkVisibility(ctx context.Context, apiBaseURL, nwo, authHeader, conf // - configured="public" but actual=private → keep "public" (more restrictive) // - configured="private" and actual=private → no change needed // - configured="internal" and actual=internal → no change needed - return configuredVisibility, false, nil + return configured, false, nil }