Skip to content

Commit 0d1135a

Browse files
luhenryclaude
andcommitted
fix: address mirror review feedback
- scope mirror-token to the mirror host and send it verbatim - route non-repo mirrors straight to the URL fetch instead of throwing - authenticate the manifest fetch - warn on slash branches, and on mirror with PyPy/GraalPy - memoize mirror validation - exercise the direct-URL path in the E2E job Addresses actions#1302 (comment) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent f30f2fe commit 0d1135a

7 files changed

Lines changed: 380 additions & 87 deletions

File tree

.github/workflows/test-python.yml

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,11 +72,14 @@ jobs:
7272
- name: Checkout
7373
uses: actions/checkout@v6
7474

75+
# The refs/heads/ form serves the same manifest as the default mirror but
76+
# deliberately does not match {owner}/{repo}/{branch}, so this exercises
77+
# the direct-URL manifest fetch that the default coordinates skip.
7578
- name: setup-python with explicit mirror
7679
uses: ./
7780
with:
7881
python-version: 3.12
79-
mirror: https://raw.githubusercontent.com/actions/python-versions/main
82+
mirror: https://raw.githubusercontent.com/actions/python-versions/refs/heads/main
8083

8184
- name: Run simple code
8285
run: python -c 'import sys; print(sys.version)'

__tests__/install-python-mirror.test.ts

Lines changed: 149 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -82,8 +82,10 @@ const httpm = await import('@actions/http-client');
8282
const tc = await import('@actions/tool-cache');
8383
const {
8484
getManifestUrl,
85+
getManifest,
8586
getManifestFromRepo,
8687
getManifestFromURL,
88+
resolveRepoCoords,
8789
installCpythonFromRelease
8890
} = await import('../src/install-python.js');
8991

@@ -142,6 +144,34 @@ describe('getManifestUrl', () => {
142144
setInputs({mirror: 'not a url'});
143145
expect(() => getManifestUrl()).toThrow(/Invalid 'mirror' URL/);
144146
});
147+
148+
it('keeps throwing the same error when called repeatedly', () => {
149+
setInputs({mirror: 'not a url'});
150+
expect(() => getManifestUrl()).toThrow(/Invalid 'mirror' URL/);
151+
// Memoized, so the second call must not silently succeed or change shape —
152+
// find-python.ts calls this while building the "version not found" message.
153+
expect(() => getManifestUrl()).toThrow(/Invalid 'mirror' URL/);
154+
});
155+
});
156+
157+
describe('resolveRepoCoords', () => {
158+
it('warns and returns null for a raw.githubusercontent.com mirror with a slash in the branch', () => {
159+
setInputs({
160+
mirror: 'https://raw.githubusercontent.com/foo/bar/feature/riscv'
161+
});
162+
163+
expect(resolveRepoCoords()).toBeNull();
164+
expect(core.warning).toHaveBeenCalledWith(
165+
expect.stringMatching(/Branch names containing '\/' are not supported/)
166+
);
167+
});
168+
169+
it('does not warn for a non-GitHub mirror', () => {
170+
setInputs({mirror: 'https://mirror.example/py'});
171+
172+
expect(resolveRepoCoords()).toBeNull();
173+
expect(core.warning).not.toHaveBeenCalled();
174+
});
145175
});
146176

147177
describe('getManifestFromRepo mirror resolution', () => {
@@ -193,9 +223,9 @@ describe('getManifestFromRepo mirror resolution', () => {
193223
);
194224
});
195225

196-
it('throws for a non-GitHub mirror so the caller falls back to the raw URL', () => {
226+
it('returns null for a non-GitHub mirror so the caller uses the raw URL', () => {
197227
setInputs({mirror: 'https://mirror.example/py'});
198-
expect(() => getManifestFromRepo()).toThrow(/not a GitHub repo URL/);
228+
expect(resolveRepoCoords()).toBeNull();
199229
expect(tc.getManifestFromRepo).not.toHaveBeenCalled();
200230
});
201231

@@ -209,6 +239,8 @@ describe('getManifestFromRepo mirror resolution', () => {
209239

210240
await getManifestFromRepo();
211241

242+
// The API requires the `token ` prefix, and naming a repo mirror is explicit intent to
243+
// read that repo, so mirror-token is prefixed here even though downloads send it verbatim.
212244
expect(tc.getManifestFromRepo).toHaveBeenCalledWith(
213245
'foo',
214246
'bar',
@@ -232,19 +264,81 @@ describe('getManifestFromRepo mirror resolution', () => {
232264
});
233265

234266
describe('getManifestFromURL mirror resolution', () => {
235-
it('fetches {mirror}/versions-manifest.json without attaching auth', async () => {
267+
it('fetches {mirror}/versions-manifest.json without auth when no mirror-token is set', async () => {
236268
setInputs({token: 'TKN', mirror: 'https://mirror.example/py'});
237269
const getJson = jest.fn(async () => ({result: mockManifest}));
238270
(httpm.HttpClient as jest.Mock<any>).mockImplementation(() => ({getJson}));
239271

240272
await getManifestFromURL();
241273

274+
// `token` must not reach a non-GitHub mirror.
242275
expect(getJson).toHaveBeenCalledWith(
243-
'https://mirror.example/py/versions-manifest.json'
276+
'https://mirror.example/py/versions-manifest.json',
277+
undefined
278+
);
279+
});
280+
281+
it('sends mirror-token verbatim on the manifest fetch', async () => {
282+
setInputs({
283+
token: 'TKN',
284+
'mirror-token': 'Bearer MTOK',
285+
mirror: 'https://mirror.example/py'
286+
});
287+
const getJson = jest.fn(async () => ({result: mockManifest}));
288+
(httpm.HttpClient as jest.Mock<any>).mockImplementation(() => ({getJson}));
289+
290+
await getManifestFromURL();
291+
292+
expect(getJson).toHaveBeenCalledWith(
293+
'https://mirror.example/py/versions-manifest.json',
294+
{authorization: 'Bearer MTOK'}
295+
);
296+
});
297+
298+
it('sends token as a prefixed header for a GitHub-hosted raw manifest', async () => {
299+
setInputs({
300+
token: 'TKN',
301+
mirror: 'https://raw.githubusercontent.com/foo/bar/refs/heads/main'
302+
});
303+
const getJson = jest.fn(async () => ({result: mockManifest}));
304+
(httpm.HttpClient as jest.Mock<any>).mockImplementation(() => ({getJson}));
305+
306+
await getManifestFromURL();
307+
308+
expect(getJson).toHaveBeenCalledWith(
309+
'https://raw.githubusercontent.com/foo/bar/refs/heads/main/versions-manifest.json',
310+
{authorization: 'token TKN'}
244311
);
245312
});
246313
});
247314

315+
describe('getManifest source routing', () => {
316+
it('skips the GitHub API entirely for a non-GitHub mirror', async () => {
317+
setInputs({mirror: 'https://mirror.example/py'});
318+
const getJson = jest.fn(async () => ({result: mockManifest}));
319+
(httpm.HttpClient as jest.Mock<any>).mockImplementation(() => ({getJson}));
320+
321+
await expect(getManifest()).resolves.toEqual(mockManifest);
322+
323+
// Routing straight to the URL fetch avoids 3 retries with backoff on a
324+
// call that could never succeed.
325+
expect(tc.getManifestFromRepo).not.toHaveBeenCalled();
326+
expect(getJson).toHaveBeenCalledTimes(1);
327+
});
328+
329+
it('uses the GitHub API for a repo mirror without touching the raw URL', async () => {
330+
setInputs({token: 'TKN'});
331+
(tc.getManifestFromRepo as jest.Mock<any>).mockResolvedValue(mockManifest);
332+
const getJson = jest.fn(async () => ({result: mockManifest}));
333+
(httpm.HttpClient as jest.Mock<any>).mockImplementation(() => ({getJson}));
334+
335+
await expect(getManifest()).resolves.toEqual(mockManifest);
336+
337+
expect(tc.getManifestFromRepo).toHaveBeenCalledTimes(1);
338+
expect(getJson).not.toHaveBeenCalled();
339+
});
340+
});
341+
248342
describe('installCpythonFromRelease auth gating', () => {
249343
const makeRelease = (downloadUrl: string) =>
250344
({
@@ -311,21 +405,66 @@ describe('installCpythonFromRelease auth gating', () => {
311405
).resolves.toBeUndefined();
312406
});
313407

314-
it('forwards mirror-token to a non-GitHub download URL', async () => {
408+
it('forwards mirror-token verbatim to the mirror host', async () => {
315409
setInputs({
316410
token: 'TKN',
317-
'mirror-token': 'MTOK',
411+
'mirror-token': 'Bearer MTOK',
318412
mirror: 'https://cdn.example'
319413
});
320414
await expect(
321415
downloadAuthFor('https://cdn.example/py.tar.gz')
322-
).resolves.toBe('token MTOK');
416+
).resolves.toBe('Bearer MTOK');
323417
});
324418

325-
it('prefers mirror-token over token for GitHub download URLs', async () => {
326-
setInputs({token: 'TKN', 'mirror-token': 'MTOK'});
419+
it('does not prefix or rewrite a mirror-token', async () => {
420+
setInputs({
421+
'mirror-token': 'Basic dXNlcjpwYXNz',
422+
mirror: 'https://cdn.example'
423+
});
327424
await expect(
328-
downloadAuthFor('https://github.com/o/r/releases/download/v/py.tar.gz')
425+
downloadAuthFor('https://cdn.example/py.tar.gz')
426+
).resolves.toBe('Basic dXNlcjpwYXNz');
427+
});
428+
429+
it('withholds mirror-token from an incidental GitHub host and uses token there', async () => {
430+
setInputs({
431+
token: 'TKN',
432+
'mirror-token': 'MTOK',
433+
mirror: 'https://cdn.example'
434+
});
435+
// A manifest hosted on the private mirror may still point release assets at
436+
// GitHub; the private credential must not follow them there.
437+
await expect(
438+
downloadAuthFor('https://objects.githubusercontent.com/x/python.tar.gz')
439+
).resolves.toBe('token TKN');
440+
});
441+
442+
it('withholds mirror-token from a GitHub host when no token is set', async () => {
443+
setInputs({'mirror-token': 'MTOK', mirror: 'https://cdn.example'});
444+
await expect(
445+
downloadAuthFor('https://objects.githubusercontent.com/x/python.tar.gz')
446+
).resolves.toBeUndefined();
447+
});
448+
449+
it('withholds mirror-token from a third host that is neither the mirror nor GitHub', async () => {
450+
setInputs({
451+
token: 'TKN',
452+
'mirror-token': 'MTOK',
453+
mirror: 'https://cdn.example'
454+
});
455+
await expect(
456+
downloadAuthFor('https://other.example/py.tar.gz')
457+
).resolves.toBeUndefined();
458+
});
459+
460+
it('uses mirror-token for a GitHub mirror host when it is the nominated host', async () => {
461+
setInputs({
462+
token: 'TKN',
463+
'mirror-token': 'token MTOK',
464+
mirror: 'https://raw.githubusercontent.com/foo/bar/main'
465+
});
466+
await expect(
467+
downloadAuthFor('https://raw.githubusercontent.com/foo/bar/py.tar.gz')
329468
).resolves.toBe('token MTOK');
330469
});
331470

action.yml

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,13 @@ inputs:
1616
description: "Set this option if you want the action to check for the latest available version that satisfies the version spec."
1717
default: false
1818
token:
19-
description: "The token used to authenticate when fetching Python distributions from https://github.com/actions/python-versions. When running this action on github.com, the default value is sufficient. When running on GHES, you can pass a personal access token for github.com if you are experiencing rate limiting. When 'mirror-token' is set, it takes precedence over this input."
19+
description: "The token used to authenticate when fetching Python distributions from https://github.com/actions/python-versions. When running this action on github.com, the default value is sufficient. When running on GHES, you can pass a personal access token for github.com if you are experiencing rate limiting. This token is only sent to GitHub-owned hosts, never to a custom 'mirror'."
2020
default: ${{ github.server_url == 'https://github.com' && github.token || '' }}
2121
mirror:
22-
description: "Base URL for downloading Python distributions. Defaults to https://raw.githubusercontent.com/actions/python-versions/main. See docs/advanced-usage.md for details."
22+
description: "Base URL for downloading Python distributions (only applies to CPython; PyPy and GraalPy are unaffected). Defaults to https://raw.githubusercontent.com/actions/python-versions/main. See docs/advanced-usage.md for details."
2323
default: "https://raw.githubusercontent.com/actions/python-versions/main"
2424
mirror-token:
25-
description: "Token used to authenticate requests to 'mirror'. Takes precedence over 'token'."
25+
description: "Token used to authenticate requests to the host named in 'mirror'. Sent verbatim as the Authorization header, so include a scheme if your mirror needs one (e.g. 'Bearer <token>')."
2626
required: false
2727
cache-dependency-path:
2828
description: "Used to specify the path to dependency files. Supports wildcards or a list of file names for caching multiple dependencies."

0 commit comments

Comments
 (0)