Fix GitHub API endpoint selection based on host parsing by bc-lee · Pull Request #1850 · microsoft/vcpkg-tool

bc-lee · 2025-11-17T19:59:42Z

Previously, when GITHUB_SERVER_URL was set, vcpkg always treated it as a GitHub Enterprise Server(GHES) endpoint and appended /api/v3 for GITHUB_SERVER_URL=https://github.com, this produced an incorrect endpoint (https://github.com/api/v3/...).

This change special-cases the exact value https://github.com and uses https://api.github.com for dependency graph submission. All other server URLs keep the existing behavior (<server>/api/v3/...).

Added tests for endpoint selection behavior in src/vcpkg-test/downloads.cpp.

Fixes microsoft/vcpkg#48347

BillyONeal · 2025-11-24T21:59:39Z

src/vcpkg/base/downloads.cpp

+    }
+
+    // Extracts the host part from a URL string.
+    std::string extract_host(StringView url)


This thing needs unit tests.

URL parsing is an insanely complicated problem; a parsing function that just returns whether we get github.com or not may be easier. Otherwise we may want to look at using trurl.

Thanks for the review. I’ll revisit this once the curl library integration has been re-landed.

BillyONeal · 2025-11-24T22:01:37Z

src/vcpkg/base/downloads.cpp

+        const char* at_sign = Strings::find_first_of(url, "@");
+        size_t at_pos = at_sign[0] == '\0' ? std::string::npos : static_cast<size_t>(at_sign - url.data());


This is undefined behavior: you can't dereference the returned iterator if there was no match. And there is no requirement that there is a \0 after a StringView

Ditto several times below

BillyONeal · 2025-11-24T22:02:23Z

src/vcpkg/base/downloads.cpp

+        }
+
+        // Remove userinfo if present (e.g., user:pass@host)
+        const char* at_sign = Strings::find_first_of(url, "@");


Seems little reason to use find_first_of when there is only one char you're looking for?

BillyONeal · 2026-02-12T21:13:50Z

Ah crap it looks like the URL APIs were added in 7.62.0 which is more recent than we currently assume. So we might be stuck using a handwritten thing like this or possibly copy/pasta-ing the relevant bits from libcurl's urlapi.c (with attribution)

BillyONeal · 2026-02-12T21:14:21Z

(I guess that most systems GitHub cares about would have greater than 7.62 so that may be fine)

bc-lee · 2026-02-16T11:22:43Z

I reviewed your suggestion to use libcurl’s URL API for URL parsing.

curl’s urlapi.c is fairly complex and tightly coupled to other parts of curl. If the goal is to minimize drift from upstream, embedding that code in vcpkg-tool is still hard to read and maintain. If the goal is to minimize code size, we effectively end up rewriting URL parsing logic ourselves, which is risky given how subtle URL parsing can be.

While reading https://learn.microsoft.com/en-us/vcpkg/concepts/supported-hosts and https://github.com/microsoft/vcpkg-tool/blob/792f2dba1715f180412eef13b4de55f8085c82ca/cmake/FindLibCURL.cmake, I noticed that vcpkg-tool is currently too permissive about the minimum libcurl version it accepts. The libcurl URL API was introduced in 7.62.0 (released on 2018-10-31), and the lowest supported libcurl version on Linux is 7.74.0, which is already newer than 7.62.0. That means we should be able to use the libcurl URL API within vcpkg-tool’s supported host baseline.

If you’re OK with that direction, I’d like to split this work into two PRs:

Bump the minimum required libcurl version to 7.74.0 in FindLibCURL.cmake and update the documentation accordingly.
Use libcurl’s URL parsing functions in src/vcpkg/base/downloads.cpp instead of a custom implementation.

BillyONeal · 2026-02-18T21:37:15Z

@ras0219-msft suggests (and @MahmoudGSaleh agrees): rather than doing the whole URI parsing, can we just check "if it's https://github.com" (just a string match) do the special github thing, otherwise do the existing behavior? That way we don't need to make the curl situation more complicated.

(And I'm sorry for not noticing this before when I first reviewed this 😅)

Previously, any provided GitHub URL was treated as a GitHub Enterprise Server (GHES) instance when selecting the API endpoint. This assumption was incorrect because the URL can also be for GitHub.com (e.g., https://github.com), which led to using the wrong API endpoint. This commit adds a function to extract the host from a URL. This is used to differentiate between GitHub.com and GHES instances to select the correct API endpoint.

bc-lee · 2026-02-18T23:46:02Z

I simplified the logic significantly to align with your suggestion. Could you please take another look?

BillyONeal · 2026-03-18T22:32:28Z

src/vcpkg/base/downloads.cpp

+        {
+            if (*github_server_url == github_com_url)
+            {
+                uri = api_github_com_url.data();


Suggested change

uri = api_github_com_url.data();

uri.assign(api_github_com_url.data(), api_github_com_url.size());

BillyONeal · 2026-03-18T23:37:15Z

src/vcpkg-test/downloads.cpp

+    REQUIRE(github_dependency_graph_snapshots_uri(Optional<std::string>{"https://github.com/"}, "owner/repo") ==
+            "https://github.com//api/v3/repos/owner/repo/dependency-graph/snapshots");


Why doesn't this one become api.? (Should it use starts with rather than ==?)

BillyONeal · 2026-03-18T23:39:40Z

src/vcpkg/base/downloads.cpp

+                uri = *github_server_url;
+                uri.append("/api/v3");


Should this defend against github_server_url ending with /?

Yes, I applied.

BillyONeal · 2026-03-18T23:40:13Z

src/vcpkg/base/downloads.cpp

+        }
+        else
+        {
+            uri = api_github_com_url.data();


Suggested change

uri = api_github_com_url.data();

uri.assign(api_github_com_url.data(), api_github_com_url.size());

bc-lee · 2026-03-19T00:47:28Z

As you commented earlier, I intentionally kept the change narrow because URL parsing is hard, so I initially avoided URL normalization.
As requested, I updated it to normalize URL by trimming a trailing slash. Please take another look.

BillyONeal · 2026-03-19T07:41:33Z

As you commented earlier, I intentionally kept the change narrow because URL parsing is hard, so I initially avoided URL normalization. As requested, I updated it to normalize URL by trimming a trailing slash. Please take another look.

I guess I should have clarified: I wasn't asking you to normalize the URL. This is about a value that comes from GitHub. I was kind of expecting a 'well I tested and GitHub sets it to this'. (I'm mostly nervous about replacing one block of code that is apparently broken with a different block that may be broken...)

bc-lee · 2026-03-24T21:51:18Z

Ah, I may have misunderstood your intent.

You were asking for evidence of what GitHub actually sets, not for URL normalization.

GitHub documents the Actions environment variables here:
https://docs.github.com/en/actions/reference/workflows-and-actions/variables

I verified this recently in a GitHub.com Actions run:
https://github.com/bc-lee/dump-github-env-vars/actions/runs/23513789186/job/68441175977

GITHUB_SERVER_URL=https://github.com
GITHUB_API_URL=https://api.github.com

So for GitHub.com, GITHUB_SERVER_URL does not include a trailing slash.
As a follow-up, it may be better to use GITHUB_API_URL directly instead.

I no longer have access to a GHES instance, although I did when I started working on this PR in 2025-11, so I cannot confidently confirm the current GHES values.

BillyONeal requested changes Nov 24, 2025

View reviewed changes

bc-lee marked this pull request as draft November 25, 2025 02:37

bc-lee added 3 commits February 19, 2026 08:10

Refactor GH dependency-graph endpoint selection into helper; add tests

96e4029

Use exact GITHUB_SERVER_URL match for github.com endpoint selection

da9910c

bc-lee force-pushed the fix-github-api-url branch from 79462ea to da9910c Compare February 18, 2026 23:43

Remove unnecessary whitespace changes

6adec92

bc-lee marked this pull request as ready for review February 18, 2026 23:44

bc-lee requested a review from BillyONeal February 18, 2026 23:44

BillyONeal reviewed Mar 18, 2026

View reviewed changes

Normalize trailing slash in GITHUB_SERVER_URL endpoint handling

ff21a82

		const char* at_sign = Strings::find_first_of(url, "@");
		size_t at_pos = at_sign[0] == '\0' ? std::string::npos : static_cast<size_t>(at_sign - url.data());

	uri = api_github_com_url.data();
	uri.assign(api_github_com_url.data(), api_github_com_url.size());

		REQUIRE(github_dependency_graph_snapshots_uri(Optional<std::string>{"https://github.com/"}, "owner/repo") ==
		"https://github.com//api/v3/repos/owner/repo/dependency-graph/snapshots");

Conversation

bc-lee commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BillyONeal commented Feb 12, 2026

Uh oh!

BillyONeal commented Feb 12, 2026

Uh oh!

bc-lee commented Feb 16, 2026

Uh oh!

BillyONeal commented Feb 18, 2026

Uh oh!

bc-lee commented Feb 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bc-lee commented Mar 19, 2026

Uh oh!

BillyONeal commented Mar 19, 2026

Uh oh!

bc-lee commented Mar 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bc-lee commented Nov 17, 2025 •

edited

Loading