Add sink-visibility runtime verification and integration tests#8903
Merged
Conversation
- Add runtime repo visibility check via GitHub API at gateway startup - Override configured sink-visibility to 'public' with warning if repo is actually public (defense-in-depth against stale compile-time config) - Gracefully skip check when GITHUB_REPOSITORY or token unavailable - Fall back to configured value on API errors (non-fatal) - Add FetchRepoVisibility and VerifySinkVisibility in githubhttp package - Add comprehensive integration tests covering: - Valid config acceptance (public/private/internal/omitted/case-insensitive) - Invalid values fall back to noop guard - Runtime override emits warning when repo more public than configured - Runtime check skipped without GITHUB_REPOSITORY - Runtime check skipped without GitHub token - API failure graceful fallback - Effective sink-visibility logged correctly Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a defense-in-depth runtime check for the write-sink guard’s sink-visibility by querying the GitHub repos API at startup, and introduces unit + integration test coverage to validate override/skip/fallback behavior.
Changes:
- Add GitHub API helper functions to fetch repository visibility and compute an effective sink visibility.
- Wire runtime sink-visibility verification into write-sink guard creation during server startup.
- Add unit tests for the GitHub visibility client and a comprehensive integration test suite for sink-visibility scenarios.
Show a summary per file
| File | Description |
|---|---|
| test/integration/sink_visibility_test.go | New integration tests covering accepted/invalid values and runtime verification outcomes (override/skip/fallback). |
| internal/server/guard_init.go | Adds runtime verification hook when constructing the write-sink guard. |
| internal/githubhttp/visibility.go | New GitHub API client helpers: fetch repo visibility and compute effective sink-visibility. |
| internal/githubhttp/visibility_test.go | Unit tests validating visibility parsing and verification logic. |
| docs/CONFIGURATION.md | Documents the new runtime verification behavior for sink-visibility. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Low
Comment on lines
+393
to
+398
| func (us *UnifiedServer) verifySinkVisibilityAtRuntime(serverID, configuredVisibility string) string { | ||
| nwo := os.Getenv("GITHUB_REPOSITORY") | ||
| if nwo == "" { | ||
| logGuardInit.Printf("sink-visibility runtime check skipped: GITHUB_REPOSITORY not set (serverID=%s)", serverID) | ||
| return configuredVisibility | ||
| } |
Comment on lines
+111
to
+116
| // If actual is "public" but configured is not "public" (or unset), | ||
| // 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 | ||
| } |
Comment on lines
+118
to
+126
| // 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 |
This was referenced Jul 8, 2026
lpcox
added a commit
that referenced
this pull request
Jul 8, 2026
## Summary Addresses the three review feedback items from #8903 for the sink-visibility runtime verification. ## Changes ### 1. Skip runtime check when sink-visibility is omitted When `sink-visibility` is not explicitly configured (empty string), the runtime API check is now skipped entirely. This preserves the documented backward-compatible behavior where omitted sink-visibility uses accept patterns without any surprise upgrade to "public". **Before**: Omitted config + public repo → runtime check overrides to "public" (breaking backward compat) **After**: Omitted config → runtime check skipped, accept patterns used as-is ### 2. Remove duplicate logging from `VerifySinkVisibility` The helper function no longer logs override warnings via `logger.LogWarn`. All logging is now handled by the caller (`verifySinkVisibilityAtRuntime`) which has the `serverID` context for proper correlation. ### 3. Return configured value when no override needed `VerifySinkVisibility` now returns the configured value unchanged in all non-override cases, instead of returning the actual API visibility. This prevents policy drift where `configured="private"` but `actual="internal"` would have returned "internal". **Before**: configured="private", actual="internal" → returns "internal" **After**: configured="private", actual="internal" → returns "private" (unchanged) The only case that changes the return value is the security-critical one: repo is actually public but config doesn't say "public". ## Testing All integration tests and unit tests pass, including the existing sink-visibility test suite. ## Related - #8903 — Original PR with review comments
This was referenced Jul 8, 2026
huangyingting
pushed a commit
to repomesh/gh-aw-mcpg
that referenced
this pull request
Jul 8, 2026
- 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>
4 tasks
This was referenced Jul 8, 2026
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
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
Adds runtime repo visibility verification as defense-in-depth for the sink-visibility feature (#8895), plus comprehensive integration tests.
Runtime Verification
At gateway startup, when a write-sink guard has
sink-visibilityconfigured, the gateway now verifies the actual repo visibility viaGET /repos/{owner}/{repo}:GITHUB_REPOSITORYor GitHub token unavailableIntegration Tests
7 test groups with 20+ subtests covering:
ConfigAcceptedInvalidValueRuntimeOverrideRuntimeCheckSkippedWithoutRepoRuntimeCheckSkippedWithoutTokenRuntimeCheckAPIFailureWriteSinkGuardLogsSinkVisibilityFiles
internal/githubhttp/visibility.go—FetchRepoVisibilityandVerifySinkVisibilityfunctionsinternal/githubhttp/visibility_test.go— Unit tests for the visibility API clientinternal/server/guard_init.go—verifySinkVisibilityAtRuntimemethod wired into guard creationtest/integration/sink_visibility_test.go— Integration tests using mock HTTP backends + mock GitHub APIdocs/CONFIGURATION.md— Added "Runtime Verification" documentation sectionRelated
sink-visibilityin write-sink guard policy based on target repo visibility gh-aw#44153 — Compiler issue to emit sink-visibility at compile time