Skip to content

Allow add URL importer auth for api.githubcopilot.com#33402

Merged
pelikhan merged 5 commits into
mainfrom
copilot/add-approved-domain-support
May 20, 2026
Merged

Allow add URL importer auth for api.githubcopilot.com#33402
pelikhan merged 5 commits into
mainfrom
copilot/add-approved-domain-support

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 19, 2026

The add / add-wizard URL importer allowed authenticated fetches for github.com and GH_HOST, but not for https://api.githubcopilot.com/.... This updates the importer so Copilot-hosted workflow URLs are treated as approved authenticated import sources.

  • Importer allowlist

    • Extend the approved HTTPS host set used by FetchImportURL / attachImportAuthHeader to include api.githubcopilot.com.
    • Preserve the existing exact-host matching behavior and HTTPS-only token attachment.
  • Code contract

    • Update the importer’s inline contract comments so the allowed-host behavior is explicit and matches implementation.
  • Regression coverage

    • Add a focused CLI test that verifies Authorization: Bearer <token> is attached for https://api.githubcopilot.com/....
req, _ := http.NewRequest(http.MethodGet, "https://api.githubcopilot.com/workflow.md", nil)
attachImportAuthHeader(req, "https://api.githubcopilot.com/workflow.md")
// Authorization header is now attached when GH_TOKEN/GITHUB_TOKEN is set.

Copilot AI and others added 2 commits May 19, 2026 21:40
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title Allow add URL importer to auth against api.githubcopilot.com Allow add URL importer auth for api.githubcopilot.com May 19, 2026
Copilot AI requested a review from pelikhan May 19, 2026 21:46
@pelikhan
Copy link
Copy Markdown
Collaborator

@copilot allow domains from the GitHub domain set by default.

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
@pelikhan pelikhan marked this pull request as ready for review May 19, 2026 22:21
Copilot AI review requested due to automatic review settings May 19, 2026 22:21
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the CLI URL importer so authenticated fetches can include Copilot-hosted workflow URLs on api.githubcopilot.com, and aligns allowlist behavior/tests accordingly.

Changes:

  • Expands the importer auth-attachment logic from exact-host matching to a pattern-based host match derived from the github ecosystem domains, plus api.githubcopilot.com and GH_HOST.
  • Updates inline contract comments describing when Authorization: Bearer <token> is attached.
  • Adds regression tests for api.githubcopilot.com and additional GitHub content hosts; fixes a Datadog application key env var reference in a workflow lock file.
Show a summary per file
File Description
pkg/cli/import_url_fetcher.go Switches auth-attachment allowlist to domain patterns + adds helper matchers and Copilot MCP domain inclusion.
pkg/cli/import_url_fetcher_test.go Adds tests asserting auth header attachment for Copilot and additional GitHub content domains, plus a suffix-confusion case.
.github/workflows/smoke-otel-backends.lock.yml Fixes Datadog header env var placeholder to use DD_APPLICATION_KEY.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 3/3 changed files
  • Comments generated: 3

Comment thread pkg/cli/import_url_fetcher.go Outdated
Comment on lines 193 to 197
// Authoritative GitHub hosts to which the token may be sent.
allowedHosts := []string{"github.com"}
allowedHosts := getDefaultImportAuthHostPatterns()
if ghHost := os.Getenv("GH_HOST"); ghHost != "" {
// GH_HOST may carry a scheme prefix; extract just the hostname.
if u, parseErr := url.Parse(ghHost); parseErr == nil && u.Host != "" {
Comment thread pkg/cli/import_url_fetcher.go Outdated
Comment on lines 193 to 195
// Authoritative GitHub hosts to which the token may be sent.
allowedHosts := []string{"github.com"}
allowedHosts := getDefaultImportAuthHostPatterns()
if ghHost := os.Getenv("GH_HOST"); ghHost != "" {
Comment thread pkg/cli/import_url_fetcher.go Outdated
Comment on lines +170 to +175
// attachImportAuthHeader adds "Authorization: Bearer <token>" to req if and only if
// ALL of the following are true:
// - the request scheme is "https" (tokens are never sent over plaintext HTTP)
// - the request host is an exact match for one of the allowed GitHub hosts:
// "github.com" or the hostname extracted from the GH_HOST environment variable
// - the request host matches one of the default GitHub ecosystem domain
// patterns (plus api.githubcopilot.com), or is an exact match for the
// hostname extracted from the GH_HOST environment variable
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 19, 2026

@copilot allow domains from the GitHub domain set by default.

Updated in ceb874f to derive the default approved import hosts from the GitHub domain set, while still allowing api.githubcopilot.com and GH_HOST. I also added focused coverage for additional GitHub-hosted import URLs and wildcard/suffix-safety cases.

@github-actions github-actions Bot mentioned this pull request May 19, 2026
@github-actions
Copy link
Copy Markdown
Contributor

🧪 Test Quality Sentinel Report

Test Quality Score: 78/100

⚠️ Acceptable — Good behavioral coverage with room for improvement

Metric Value
New/modified tests analyzed 4
✅ Design tests (behavioral contracts) 4 (100%)
⚠️ Implementation tests (low value) 0 (0%)
Tests with error/edge cases 1 (25%)
Duplicate test clusters 0
Test inflation detected No (ratio 1.10:1)
🚨 Coding-guideline violations 0

Test Classification Details

Test File Classification Issues Detected
TestAttachImportAuthHeader_GitHubCopilot pkg/cli/import_url_fetcher_test.go:149 ✅ Design None
TestAttachImportAuthHeader_RawGitHubContent pkg/cli/import_url_fetcher_test.go:157 ✅ Design None
TestAttachImportAuthHeader_GitHubUserContentWildcard pkg/cli/import_url_fetcher_test.go:165 ✅ Design None
TestAttachImportAuthHeader_GitHubUserContentSuffixConfusion_NoToken pkg/cli/import_url_fetcher_test.go:229 ✅ Design (Security) None — Excellent security boundary test

Analysis Summary

This PR adds support for authenticating URL imports from api.githubcopilot.com and other GitHub ecosystem domains by extending the domain pattern matching in attachImportAuthHeader.

Strengths:

100% behavioral coverage — All tests verify observable behavior (whether the Authorization header is correctly attached or omitted)

Security-focusedTestAttachImportAuthHeader_GitHubUserContentSuffixConfusion_NoToken is an excellent security test that prevents domain suffix confusion attacks (e.g., githubusercontent.com.evil.com should NOT receive tokens)

No guideline violations — Proper //go:build !integration tag, no forbidden mock libraries, clear assertion messages via descriptive test names

Proportional growth — Test file grew 32 lines vs 29 net lines in production code (ratio 1.10:1, well under the 2:1 threshold)

Improvement Opportunities:

⚠️ Limited edge case coverage (25%) — Only 1 of 4 tests covers a negative/error path. Consider adding tests for:

  • Mixed-case domain names (e.g., API.GitHubCopilot.COM)
  • Domain patterns with ports (e.g., api.githubcopilot.com:443)
  • HTTP (non-HTTPS) variants of approved domains (should NOT receive tokens)
  • Wildcard pattern edge cases (e.g., subdomain.subdomain.githubusercontent.com)

Language Support

Tests analyzed:

  • 🐹 Go (*_test.go): 4 tests (unit, //go:build !integration)
  • 🟨 JavaScript (*.test.cjs, *.test.js): 0 tests

Verdict

Check passed. 0% of new tests are implementation tests (threshold: 30%). Test quality is acceptable with 100% behavioral coverage.

Recommendation: Consider adding 2-3 edge case tests (mixed-case domains, HTTP vs HTTPS, subdomain depth) to increase robustness from 78/100 to 85+/100.


📖 Understanding Test Classifications

Design Tests (High Value) verify what the system does:

  • Assert on observable outputs, return values, or state changes
  • Cover error paths and boundary conditions
  • Would catch a behavioral regression if deleted
  • Remain valid even after internal refactoring

Implementation Tests (Low Value) verify how the system does it:

  • Assert on internal function calls (mocking internals)
  • Only test the happy path with typical inputs
  • Break during legitimate refactoring even when behavior is correct
  • Give false assurance: they pass even when the system is wrong

Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.

🧪 Test quality analysis by Test Quality Sentinel · ● 4.2M ·

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Test Quality Sentinel: 78/100. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%). All 4 new tests verify behavioral contracts with 100% design test coverage.

@pelikhan
Copy link
Copy Markdown
Collaborator

@copilot review all comments

Copilot AI and others added 2 commits May 19, 2026 23:27
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 19, 2026

@copilot review all comments

Reviewed the actionable comments and pushed 363f342. The importer auth policy is now narrowed to an explicit cached host allowlist plus GH_HOST, the contract wording matches exact-host behavior again, and there are regressions covering both allowed GitHub import hosts and denied broader GitHub domains.

@pelikhan pelikhan merged commit 7c92266 into main May 20, 2026
@pelikhan pelikhan deleted the copilot/add-approved-domain-support branch May 20, 2026 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants