From dd008d42e44bc9a080ff71faf045d2c318292ff8 Mon Sep 17 00:00:00 2001 From: Schnitz <12687466+CptSchnitz@users.noreply.github.com> Date: Wed, 16 Sep 2026 10:00:17 +0300 Subject: [PATCH 1/6] feat(oci-registry): verify private registries with local Docker credentials MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A developer logged in with `docker login` now gets real verdicts for that registry's images instead of a blanket unverifiable. One that is not logged in gets `needs-login` carrying the host, which is the piece a caller needs to offer the fix rather than just reporting a failure. The whole credential chain is here because the shallow reading of "use local Docker credentials" silently fails on the registry most likely to matter. An Azure Container Registry `auths` entry carries both an `auth` field and an `identitytoken`, and that `auth` decodes to a null-GUID username with an EMPTY password. It is a placeholder, not a credential. A client that reads `auth` first therefore sends useless basic credentials and collects a 401 from what is, for this organisation, the primary registry — so the one registry the feature fails on would be the one everybody uses. Reading `identitytoken` first, and spending it as an OAuth2 refresh-token grant rather than as a password, is what makes that case work, and it costs nothing anywhere else. That ordering is asserted by a test that pairs an identity token with a usable password, so only the order can decide it; the ACR entry alone cannot prove the rule, because its empty password reaches the identity token whatever the order. Resolution follows Docker's own precedence — per-registry helper, global store, then the plaintext entry — falling through on each miss, because a miss is the normal state of a keychain that has only ever been asked about one registry. Config keys are normalized to a bare host before matching, since `docker login` writes Docker Hub under `https://index.docker.io/v1/` while a repository names it `docker.io`; a helper is still handed the original key, which is what it stores under. `missing-credential` splits into `needs-login` and `authentication-failure` because only the first is actionable. The registry host rides on the `needs-login` arm specifically, so a "log in to X" prompt is unbuildable without a host and no other reason can pretend to have one. The invariant is untouched: both are unverifiable, and unverifiable still produces no diagnostic. A second 401 after presenting a credential stays an authentication failure rather than becoming a not-found, because a registry may answer 401 for a repository the caller is not allowed to know about. Registries outside a known-public table are never contacted anonymously. The anonymous attempt would disclose a private repository name to whoever answers and buy nothing, since its 401 says no more than the config already did. The helper name comes out of a config file and is interpolated into a command name, so it is validated at the spawn boundary rather than trusted. `packages/oci-registry` is the only workspace touched; `apps/vscode` does not compile until its own unit lands. Refs #22 Co-Authored-By: Claude Opus 5 (1M context) --- packages/oci-registry/src/authorize.ts | 233 +++++++ .../src/check-image-existence.test.ts | 577 +++++++++++++++++- .../oci-registry/src/check-image-existence.ts | 98 ++- packages/oci-registry/src/credentials.ts | 341 +++++++++++ packages/oci-registry/src/fetch-like.ts | 5 +- packages/oci-registry/src/index.ts | 4 +- .../src/local-docker-credentials.ts | 116 ++++ packages/oci-registry/src/verdict.ts | 41 +- 8 files changed, 1360 insertions(+), 55 deletions(-) create mode 100644 packages/oci-registry/src/authorize.ts create mode 100644 packages/oci-registry/src/credentials.ts create mode 100644 packages/oci-registry/src/local-docker-credentials.ts diff --git a/packages/oci-registry/src/authorize.ts b/packages/oci-registry/src/authorize.ts new file mode 100644 index 0000000..2ec1b39 --- /dev/null +++ b/packages/oci-registry/src/authorize.ts @@ -0,0 +1,233 @@ +import type { RegistryCredential } from './credentials'; +import type { FetchLike } from './fetch-like'; + +// Splits a `WWW-Authenticate` value into its scheme and the parameter list +// behind it. The parameters are optional because a `Basic` challenge is +// frequently the bare word, with no realm at all. +const CHALLENGE_PATTERN = /^\s*([A-Za-z]+)(?:\s+([\s\S]*))?$/; + +// Only the quoted form is matched. RFC 7235 permits a bare token as a +// parameter value, but no registry emits one, and accepting unquoted values +// would mean guessing where a value ends in a header whose parameters are +// comma-separated and whose realms contain commas. +const PARAMETER_PATTERN = /([A-Za-z0-9_-]+)="([^"]*)"/g; + +// Identifies this client to the token endpoint during a refresh-token +// grant. Registries log it, and Azure Container Registry requires the field +// to be present, but no registry validates it against a registration. +const CLIENT_ID = 'infra-tools'; + +const FORM_CONTENT_TYPE = 'application/x-www-form-urlencoded'; + +type BasicCredential = Extract; + +/** + * A parsed `WWW-Authenticate` challenge. + * + * Every field behind the scheme is optional because the registry decides + * what it sends: Docker Hub names a realm, a service and a scope, a plain + * `Basic` challenge names nothing at all, and a private registry may omit + * the scope and expect the client to state the one it wants. Modelling them + * as optional keeps that negotiation visible instead of inventing values + * the registry never offered. + */ +interface AuthChallenge { + readonly scheme: 'bearer' | 'basic'; + readonly realm?: string; + readonly service?: string; + readonly scope?: string; +} + +interface AcquireBearerTokenParams { + readonly challenge: AuthChallenge; + readonly credential: RegistryCredential | undefined; + readonly repositoryName: string; + readonly fetch: FetchLike; +} + +interface TokenRequestParams { + readonly realm: string; + readonly challenge: AuthChallenge; + readonly scope: string; + readonly fetch: FetchLike; +} + +interface RefreshTokenRequestParams extends TokenRequestParams { + readonly refreshToken: string; +} + +interface BasicTokenRequestParams extends TokenRequestParams { + readonly credential: BasicCredential | undefined; +} + +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null; +} + +function readStringProperty(source: unknown, property: string): string | undefined { + if (!isRecord(source)) { + return undefined; + } + + const value = source[property]; + return typeof value === 'string' ? value : undefined; +} + +/** + * Parses a `WWW-Authenticate` header into the challenge it describes. + * + * The scheme is compared case-insensitively because the header is defined + * that way and registries disagree in practice — `Bearer`, `bearer` and + * `BEARER` all occur. An unrecognised scheme returns `undefined` rather than + * a partially-understood challenge, since guessing at a scheme this package + * cannot satisfy would mean sending a credential in a form the registry + * never asked for. + */ +function parseAuthenticateChallenge(header: string): AuthChallenge | undefined { + const match = CHALLENGE_PATTERN.exec(header); + + if (match === null) { + return undefined; + } + + const [, rawScheme = '', rawParameters = ''] = match; + const scheme = rawScheme.toLowerCase(); + + if (scheme !== 'bearer' && scheme !== 'basic') { + return undefined; + } + + const parameters = new Map(); + + for (const [, key, value] of rawParameters.matchAll(PARAMETER_PATTERN)) { + if (key !== undefined && value !== undefined) { + parameters.set(key.toLowerCase(), value); + } + } + + return { + scheme, + realm: parameters.get('realm'), + service: parameters.get('service'), + scope: parameters.get('scope'), + }; +} + +function basicAuthorizationHeader(credential: BasicCredential): string { + return `Basic ${Buffer.from(`${credential.username}:${credential.password}`, 'utf8').toString('base64')}`; +} + +function buildTokenUrl(params: TokenRequestParams): string | undefined { + const { realm, challenge, scope } = params; + + try { + const url = new URL(realm); + + if (challenge.service !== undefined) { + url.searchParams.set('service', challenge.service); + } + + url.searchParams.set('scope', scope); + return url.href; + } catch { + return undefined; + } +} + +/** + * Trades an identity token for an access token via the OAuth2 + * refresh-token grant. + * + * An identity token is deliberately not sent as a password: it is a refresh + * token, the endpoint that accepts it is the `POST` form-encoded grant, and + * presenting it over basic auth gets a 401 from every registry that issues + * one. The request carries no `authorization` header at all — the refresh + * token in the body is the credential, and adding a header alongside it is + * what makes Azure Container Registry reject an otherwise valid exchange. + */ +async function requestTokenWithRefreshToken(params: RefreshTokenRequestParams): Promise { + const { realm, challenge, scope, refreshToken, fetch } = params; + const body = new URLSearchParams(); + + body.set('grant_type', 'refresh_token'); + + if (challenge.service !== undefined) { + body.set('service', challenge.service); + } + + body.set('scope', scope); + body.set('client_id', CLIENT_ID); + body.set('refresh_token', refreshToken); + + const response = await fetch(realm, { + method: 'POST', + headers: { 'content-type': FORM_CONTENT_TYPE }, + body: body.toString(), + }); + + if (!response.ok) { + return undefined; + } + + return readStringProperty(await response.json(), 'access_token'); +} + +/** + * Asks the token endpoint for an access token, presenting a basic + * credential when there is one and nothing when there isn't. + * + * The credential-free call is not a degenerate case: public registries issue + * anonymous pull tokens from the same endpoint, so the only difference + * between a logged-in and an anonymous pull is whether the `authorization` + * header is present. The two response fields are both read because the + * distribution spec names `token` and OAuth2 names `access_token`, and real + * registries send one or the other. + */ +async function requestTokenWithBasic(params: BasicTokenRequestParams): Promise { + const { credential, fetch } = params; + const url = buildTokenUrl(params); + + if (url === undefined) { + return undefined; + } + + const headers: Record = credential === undefined ? {} : { authorization: basicAuthorizationHeader(credential) }; + const response = await fetch(url, { method: 'GET', headers }); + + if (!response.ok) { + return undefined; + } + + const body = await response.json(); + return readStringProperty(body, 'token') ?? readStringProperty(body, 'access_token'); +} + +/** + * Acquires the bearer token a `Bearer` challenge is asking for. + * + * The scope falls back to `repository::pull` when the challenge names + * none, because a token issued without a scope grants nothing and the + * retried manifest request would collect a second 401 — a registry that + * omits the scope expects the client to name the access it wants. + * + * Returning `undefined` covers every way this can come up empty, and all of + * them mean the same thing to the caller: the request cannot be + * authenticated, which is never evidence about whether the image exists. + */ +async function acquireBearerToken(params: AcquireBearerTokenParams): Promise { + const { challenge, credential, repositoryName, fetch } = params; + const { realm } = challenge; + + if (realm === undefined) { + return undefined; + } + + const scope = challenge.scope ?? `repository:${repositoryName}:pull`; + + return credential?.kind === 'identity-token' + ? requestTokenWithRefreshToken({ realm, challenge, scope, refreshToken: credential.refreshToken, fetch }) + : requestTokenWithBasic({ realm, challenge, scope, credential, fetch }); +} + +export { acquireBearerToken, basicAuthorizationHeader, parseAuthenticateChallenge }; +export type { AuthChallenge, BasicCredential }; diff --git a/packages/oci-registry/src/check-image-existence.test.ts b/packages/oci-registry/src/check-image-existence.test.ts index fa62ed0..a75a0a8 100644 --- a/packages/oci-registry/src/check-image-existence.test.ts +++ b/packages/oci-registry/src/check-image-existence.test.ts @@ -1,53 +1,132 @@ import { describe, expect, it, vi } from 'vitest'; import { checkImageExistence } from './check-image-existence'; -import type { FetchLike, FetchResponseLike } from './fetch-like'; +import type { CredentialEnvironment } from './credentials'; +import type { FetchLike, FetchRequestInit, FetchResponseLike } from './fetch-like'; + +const MANIFEST_ACCEPT_HEADER = [ + 'application/vnd.oci.image.manifest.v1+json', + 'application/vnd.oci.image.index.v1+json', + 'application/vnd.docker.distribution.manifest.v2+json', + 'application/vnd.docker.distribution.manifest.list.v2+json', +].join(', '); + +interface FakeResponseParams { + readonly status: number; + readonly body?: unknown; + readonly wwwAuthenticate?: string; +} + +function fakeFetchResponse(params: FakeResponseParams): FetchResponseLike { + const { status, body, wwwAuthenticate } = params; -function fakeFetchResponse(status: number, body: unknown): FetchResponseLike { return { status, ok: status >= 200 && status < 300, + headers: { get: (name) => (name.toLowerCase() === 'www-authenticate' ? (wwwAuthenticate ?? null) : null) }, // eslint-disable-next-line @typescript-eslint/promise-function-async -- trivial canned response, nothing to await json: () => Promise.resolve(body), }; } +/** The shape of `~/.docker/config.json` a test cares about, spelled out so each test reads as a developer's real config file. */ +interface DockerConfigFile { + readonly auths?: Readonly>>>; + readonly credsStore?: string; + readonly credHelpers?: Readonly>; +} + +interface DockerCredentialsParams { + readonly config?: DockerConfigFile; + /** Config text written out verbatim, for the cases where it is deliberately not valid JSON. */ + readonly configText?: string; + readonly runCredentialHelper?: CredentialEnvironment['runCredentialHelper']; +} + +function dockerCredentials(params: DockerCredentialsParams = {}): CredentialEnvironment { + const { config, configText, runCredentialHelper } = params; + const contents = configText ?? (config === undefined ? undefined : JSON.stringify(config)); + + return { + readDockerConfig: vi.fn().mockResolvedValue(contents), + runCredentialHelper: + runCredentialHelper ?? vi.fn().mockRejectedValue(new Error('no credential helper is installed')), + }; +} + +interface HelperOutputParams { + readonly serverUrl: string; + readonly username: string; + readonly secret: string; +} + +/** Stdout from a `docker-credential-*` helper. The capitalised keys are that protocol's wire format, not a name this repo chose. */ +function helperOutput(params: HelperOutputParams): string { + const { serverUrl, username, secret } = params; + // eslint-disable-next-line @typescript-eslint/naming-convention -- credential-helper wire field names + return JSON.stringify({ ServerURL: serverUrl, Username: username, Secret: secret }); +} + +function encodeAuth(username: string, password: string): string { + return Buffer.from(`${username}:${password}`).toString('base64'); +} + +/** Reads the nth `fetch` call, throwing rather than handing `undefined` to an assertion that would then pass vacuously. */ +function requestAt(calls: readonly (readonly [string, FetchRequestInit])[], index: number): { url: string; init: FetchRequestInit } { + const call = calls[index]; + + if (call === undefined) { + throw new Error(`expected a fetch call at index ${index}, got ${calls.length}`); + } + + const [url, init] = call; + return { url, init }; +} + function distributionError(code: string): { errors: { code: string }[] } { return { errors: [{ code }] }; } describe('checkImageExistence', () => { it('should issue a manifest GET with the OCI/Docker accept header and report exists on a 200', async () => { - const fetch = vi.fn().mockResolvedValue(fakeFetchResponse(200, { schemaVersion: 2 })); + const fetch = vi.fn().mockResolvedValue(fakeFetchResponse({ status: 200, body: { schemaVersion: 2 } })); - const verdict = await checkImageExistence({ repository: 'docker.io/library/nginx', tag: '1.19', fetch }); + const verdict = await checkImageExistence({ + repository: 'docker.io/library/nginx', + tag: '1.19', + fetch, + credentials: dockerCredentials(), + }); expect(fetch).toHaveBeenCalledTimes(1); expect(fetch).toHaveBeenCalledWith('https://docker.io/v2/library/nginx/manifests/1.19', { method: 'GET', - headers: { - accept: [ - 'application/vnd.oci.image.manifest.v1+json', - 'application/vnd.oci.image.index.v1+json', - 'application/vnd.docker.distribution.manifest.v2+json', - 'application/vnd.docker.distribution.manifest.list.v2+json', - ].join(', '), - }, + headers: { accept: MANIFEST_ACCEPT_HEADER }, }); expect(verdict).toEqual({ kind: 'exists', registry: 'docker.io' }); }); it('should report repository-not-found on a 404 whose body carries NAME_UNKNOWN', async () => { - const fetch = vi.fn().mockResolvedValue(fakeFetchResponse(404, distributionError('NAME_UNKNOWN'))); + const fetch = vi.fn().mockResolvedValue(fakeFetchResponse({ status: 404, body: distributionError('NAME_UNKNOWN') })); - const verdict = await checkImageExistence({ repository: 'ghcr.io/example/does-not-exist', tag: '1.0.0', fetch }); + const verdict = await checkImageExistence({ + repository: 'ghcr.io/example/does-not-exist', + tag: '1.0.0', + fetch, + credentials: dockerCredentials(), + }); expect(verdict).toEqual({ kind: 'repository-not-found', repository: 'ghcr.io/example/does-not-exist' }); }); it('should report tag-not-found on a 404 whose body carries MANIFEST_UNKNOWN', async () => { - const fetch = vi.fn().mockResolvedValue(fakeFetchResponse(404, distributionError('MANIFEST_UNKNOWN'))); + const fetch = vi.fn().mockResolvedValue(fakeFetchResponse({ status: 404, body: distributionError('MANIFEST_UNKNOWN') })); - const verdict = await checkImageExistence({ repository: 'docker.io/library/nginx', tag: 'does-not-exist', fetch }); + const verdict = await checkImageExistence({ + repository: 'docker.io/library/nginx', + tag: 'does-not-exist', + fetch, + credentials: dockerCredentials(), + }); expect(verdict).toEqual({ kind: 'tag-not-found', @@ -57,33 +136,54 @@ describe('checkImageExistence', () => { }); it('should report unverifiable, never a false negative, when a 404 body carries no recognised code', async () => { - const fetch = vi.fn().mockResolvedValue(fakeFetchResponse(404, {})); + const fetch = vi.fn().mockResolvedValue(fakeFetchResponse({ status: 404, body: {} })); - const verdict = await checkImageExistence({ repository: 'docker.io/library/nginx', tag: '1.19', fetch }); + const verdict = await checkImageExistence({ + repository: 'docker.io/library/nginx', + tag: '1.19', + fetch, + credentials: dockerCredentials(), + }); expect(verdict).toEqual({ kind: 'unverifiable', reason: 'unexpected-response' }); }); - it('should report unverifiable when the registry demands authentication this package cannot provide', async () => { - const fetch = vi.fn().mockResolvedValue(fakeFetchResponse(401, {})); + it('should report needs-login, without contacting the registry, when a non-public registry has no local credential', async () => { + const fetch = vi.fn(); - const verdict = await checkImageExistence({ repository: 'private.example.com/app', tag: '1.0.0', fetch }); + const verdict = await checkImageExistence({ + repository: 'private.example.com/app', + tag: '1.0.0', + fetch, + credentials: dockerCredentials(), + }); - expect(verdict).toEqual({ kind: 'unverifiable', reason: 'missing-credential' }); + expect(fetch).not.toHaveBeenCalled(); + expect(verdict).toEqual({ kind: 'unverifiable', reason: 'needs-login', registry: 'private.example.com' }); }); it('should report unverifiable on a network failure rather than surfacing the error', async () => { const fetch = vi.fn().mockRejectedValue(new Error('getaddrinfo ENOTFOUND')); - const verdict = await checkImageExistence({ repository: 'unreachable.example.com/app', tag: '1.0.0', fetch }); + const verdict = await checkImageExistence({ + repository: 'ghcr.io/example/app', + tag: '1.0.0', + fetch, + credentials: dockerCredentials(), + }); expect(verdict).toEqual({ kind: 'unverifiable', reason: 'network-error' }); }); it('should report unverifiable on an unexpected status code', async () => { - const fetch = vi.fn().mockResolvedValue(fakeFetchResponse(500, {})); + const fetch = vi.fn().mockResolvedValue(fakeFetchResponse({ status: 500, body: {} })); - const verdict = await checkImageExistence({ repository: 'docker.io/library/nginx', tag: '1.19', fetch }); + const verdict = await checkImageExistence({ + repository: 'docker.io/library/nginx', + tag: '1.19', + fetch, + credentials: dockerCredentials(), + }); expect(verdict).toEqual({ kind: 'unverifiable', reason: 'unexpected-response' }); }); @@ -91,7 +191,7 @@ describe('checkImageExistence', () => { it('should report unverifiable without issuing a request when the repository names no explicit host', async () => { const fetch = vi.fn(); - const verdict = await checkImageExistence({ repository: 'nginx', tag: 'latest', fetch }); + const verdict = await checkImageExistence({ repository: 'nginx', tag: 'latest', fetch, credentials: dockerCredentials() }); expect(fetch).not.toHaveBeenCalled(); expect(verdict).toEqual({ kind: 'unverifiable', reason: 'no-registry' }); @@ -100,7 +200,12 @@ describe('checkImageExistence', () => { it('should report unverifiable without issuing a request when the host segment smuggles userinfo', async () => { const fetch = vi.fn(); - const verdict = await checkImageExistence({ repository: 'docker.io@evil.example/library/nginx', tag: '1.19', fetch }); + const verdict = await checkImageExistence({ + repository: 'docker.io@evil.example/library/nginx', + tag: '1.19', + fetch, + credentials: dockerCredentials(), + }); expect(fetch).not.toHaveBeenCalled(); expect(verdict).toEqual({ kind: 'unverifiable', reason: 'no-registry' }); @@ -109,7 +214,12 @@ describe('checkImageExistence', () => { it('should report unverifiable without issuing a request when a name component is a dot-segment', async () => { const fetch = vi.fn(); - const verdict = await checkImageExistence({ repository: 'docker.io/../secrets', tag: '1.19', fetch }); + const verdict = await checkImageExistence({ + repository: 'docker.io/../secrets', + tag: '1.19', + fetch, + credentials: dockerCredentials(), + }); expect(fetch).not.toHaveBeenCalled(); expect(verdict).toEqual({ kind: 'unverifiable', reason: 'no-registry' }); @@ -118,9 +228,418 @@ describe('checkImageExistence', () => { it('should report unverifiable without issuing a request when the tag does not fit the OCI tag grammar', async () => { const fetch = vi.fn(); - const verdict = await checkImageExistence({ repository: 'docker.io/library/nginx', tag: '../../other', fetch }); + const verdict = await checkImageExistence({ + repository: 'docker.io/library/nginx', + tag: '../../other', + fetch, + credentials: dockerCredentials(), + }); expect(fetch).not.toHaveBeenCalled(); expect(verdict).toEqual({ kind: 'unverifiable', reason: 'malformed-reference' }); }); + + it('should send a plaintext auths credential as basic on the token request and the issued token on the manifest retry', async () => { + const fetch = vi + .fn() + .mockResolvedValueOnce( + fakeFetchResponse({ + status: 401, + body: {}, + wwwAuthenticate: 'Bearer realm="https://private.example.com/oauth2/token",service="private.example.com",scope="repository:app:pull"', + }) + ) + .mockResolvedValueOnce(fakeFetchResponse({ status: 200, body: { token: 'issued-token' } })) + .mockResolvedValueOnce(fakeFetchResponse({ status: 200, body: { schemaVersion: 2 } })); + + const verdict = await checkImageExistence({ + repository: 'private.example.com/app', + tag: '1.0.0', + fetch, + credentials: dockerCredentials({ config: { auths: { 'private.example.com': { auth: encodeAuth('dev', 's3cret') } } } }), + }); + + const token = requestAt(fetch.mock.calls, 1); + const tokenUrl = new URL(token.url); + + expect(token.init.method).toBe('GET'); + expect(`${tokenUrl.origin}${tokenUrl.pathname}`).toBe('https://private.example.com/oauth2/token'); + expect(tokenUrl.searchParams.get('service')).toBe('private.example.com'); + expect(tokenUrl.searchParams.get('scope')).toBe('repository:app:pull'); + expect(token.init.headers).toEqual({ authorization: `Basic ${encodeAuth('dev', 's3cret')}` }); + + expect(fetch).toHaveBeenNthCalledWith(3, 'https://private.example.com/v2/app/manifests/1.0.0', { + method: 'GET', + headers: { accept: MANIFEST_ACCEPT_HEADER, authorization: 'Bearer issued-token' }, + }); + expect(verdict).toEqual({ kind: 'exists', registry: 'private.example.com' }); + }); + + it('should resolve a credential through the global credsStore, handing the helper the config key it matched', async () => { + const runCredentialHelper = vi + .fn() + .mockResolvedValue(helperOutput({ serverUrl: 'https://private.example.com', username: 'store-user', secret: 'store-secret' })); + const fetch = vi + .fn() + .mockResolvedValueOnce( + fakeFetchResponse({ + status: 401, + body: {}, + wwwAuthenticate: 'Bearer realm="https://private.example.com/oauth2/token",service="private.example.com"', + }) + ) + .mockResolvedValueOnce(fakeFetchResponse({ status: 200, body: { token: 'issued-token' } })) + .mockResolvedValueOnce(fakeFetchResponse({ status: 200, body: { schemaVersion: 2 } })); + + const verdict = await checkImageExistence({ + repository: 'private.example.com/app', + tag: '1.0.0', + fetch, + credentials: dockerCredentials({ + config: { credsStore: 'desktop', auths: { 'https://private.example.com': {} } }, + runCredentialHelper, + }), + }); + + expect(runCredentialHelper).toHaveBeenCalledWith('desktop', 'https://private.example.com'); + + const token = requestAt(fetch.mock.calls, 1); + + expect(new URL(token.url).searchParams.get('scope')).toBe('repository:app:pull'); + expect(token.init.headers).toEqual({ authorization: `Basic ${encodeAuth('store-user', 'store-secret')}` }); + expect(verdict).toEqual({ kind: 'exists', registry: 'private.example.com' }); + }); + + it('should prefer a per-registry credHelpers entry over the global credsStore', async () => { + const runCredentialHelper = vi + .fn() + .mockResolvedValue(helperOutput({ serverUrl: 'private.example.com', username: 'helper-user', secret: 'helper-secret' })); + const fetch = vi + .fn() + .mockResolvedValueOnce( + fakeFetchResponse({ + status: 401, + body: {}, + wwwAuthenticate: 'Bearer realm="https://private.example.com/oauth2/token",service="private.example.com"', + }) + ) + .mockResolvedValueOnce(fakeFetchResponse({ status: 200, body: { token: 'issued-token' } })) + .mockResolvedValueOnce(fakeFetchResponse({ status: 200, body: { schemaVersion: 2 } })); + + const verdict = await checkImageExistence({ + repository: 'private.example.com/app', + tag: '1.0.0', + fetch, + credentials: dockerCredentials({ + config: { credsStore: 'desktop', credHelpers: { 'private.example.com': 'acr-env' } }, + runCredentialHelper, + }), + }); + + expect(runCredentialHelper).toHaveBeenCalledTimes(1); + expect(runCredentialHelper).toHaveBeenCalledWith('acr-env', 'private.example.com'); + expect(requestAt(fetch.mock.calls, 1).init.headers).toEqual({ + authorization: `Basic ${encodeAuth('helper-user', 'helper-secret')}`, + }); + expect(verdict).toEqual({ kind: 'exists', registry: 'private.example.com' }); + }); + + it('should exchange the identity token rather than the empty-password basic credential an ACR auth entry decodes to', async () => { + const fetch = vi + .fn() + .mockResolvedValueOnce( + fakeFetchResponse({ + status: 401, + body: {}, + wwwAuthenticate: 'Bearer realm="https://myorg.azurecr.io/oauth2/token",service="myorg.azurecr.io"', + }) + ) + .mockResolvedValueOnce( + fakeFetchResponse({ + status: 200, + // eslint-disable-next-line @typescript-eslint/naming-convention -- the OAuth2 wire field name the registry actually returns + body: { access_token: 'acr-access-token' }, + }) + ) + .mockResolvedValueOnce(fakeFetchResponse({ status: 200, body: { schemaVersion: 2 } })); + + const verdict = await checkImageExistence({ + repository: 'myorg.azurecr.io/app', + tag: '1.0.0', + fetch, + credentials: dockerCredentials({ + config: { + auths: { + 'myorg.azurecr.io': { + auth: encodeAuth('00000000-0000-0000-0000-000000000000', ''), + identitytoken: 'refresh-token-value', + }, + }, + }, + }), + }); + + const token = requestAt(fetch.mock.calls, 1); + const body = new URLSearchParams(token.init.body ?? ''); + + expect(token.url).toBe('https://myorg.azurecr.io/oauth2/token'); + expect(token.init.method).toBe('POST'); + expect(token.init.headers).toEqual({ 'content-type': 'application/x-www-form-urlencoded' }); + expect(body.get('grant_type')).toBe('refresh_token'); + expect(body.get('refresh_token')).toBe('refresh-token-value'); + expect(body.get('service')).toBe('myorg.azurecr.io'); + expect(body.get('scope')).toBe('repository:app:pull'); + expect(body.get('client_id')).toBe('infra-tools'); + + expect(fetch).toHaveBeenNthCalledWith(3, 'https://myorg.azurecr.io/v2/app/manifests/1.0.0', { + method: 'GET', + headers: { accept: MANIFEST_ACCEPT_HEADER, authorization: 'Bearer acr-access-token' }, + }); + expect(verdict).toEqual({ kind: 'exists', registry: 'myorg.azurecr.io' }); + }); + + it('should acquire a token anonymously when a public registry challenges and no credential matches', async () => { + const fetch = vi + .fn() + .mockResolvedValueOnce( + fakeFetchResponse({ + status: 401, + body: {}, + wwwAuthenticate: 'Bearer realm="https://ghcr.io/token",service="ghcr.io",scope="repository:example/app:pull"', + }) + ) + .mockResolvedValueOnce(fakeFetchResponse({ status: 200, body: { token: 'anonymous-token' } })) + .mockResolvedValueOnce(fakeFetchResponse({ status: 200, body: { schemaVersion: 2 } })); + + const verdict = await checkImageExistence({ + repository: 'ghcr.io/example/app', + tag: '1.0.0', + fetch, + credentials: dockerCredentials(), + }); + + expect(requestAt(fetch.mock.calls, 1).init.headers).toEqual({}); + expect(fetch).toHaveBeenNthCalledWith(3, 'https://ghcr.io/v2/example/app/manifests/1.0.0', { + method: 'GET', + headers: { accept: MANIFEST_ACCEPT_HEADER, authorization: 'Bearer anonymous-token' }, + }); + expect(verdict).toEqual({ kind: 'exists', registry: 'ghcr.io' }); + }); + + it('should report authentication-failure, never a not-found verdict, when the registry rejects the credential it was given', async () => { + const fetch = vi + .fn() + .mockResolvedValueOnce( + fakeFetchResponse({ + status: 401, + body: {}, + wwwAuthenticate: 'Bearer realm="https://private.example.com/oauth2/token",service="private.example.com"', + }) + ) + .mockResolvedValueOnce(fakeFetchResponse({ status: 200, body: { token: 'stale-token' } })) + .mockResolvedValueOnce(fakeFetchResponse({ status: 401, body: {} })); + + const verdict = await checkImageExistence({ + repository: 'private.example.com/app', + tag: '1.0.0', + fetch, + credentials: dockerCredentials({ config: { auths: { 'private.example.com': { auth: encodeAuth('dev', 'expired') } } } }), + }); + + expect(verdict).toEqual({ kind: 'unverifiable', reason: 'authentication-failure' }); + }); + + it('should report authentication-failure when a 401 carries no challenge to act on', async () => { + const fetch = vi.fn().mockResolvedValue(fakeFetchResponse({ status: 401, body: {} })); + + const verdict = await checkImageExistence({ + repository: 'ghcr.io/example/app', + tag: '1.0.0', + fetch, + credentials: dockerCredentials(), + }); + + expect(verdict).toEqual({ kind: 'unverifiable', reason: 'authentication-failure' }); + }); + + it('should fall through to the plaintext auths entry when the credential helper rejects', async () => { + const runCredentialHelper = vi + .fn() + .mockRejectedValue(new Error('credentials not found in native keychain')); + const fetch = vi + .fn() + .mockResolvedValueOnce( + fakeFetchResponse({ + status: 401, + body: {}, + wwwAuthenticate: 'Bearer realm="https://private.example.com/oauth2/token",service="private.example.com"', + }) + ) + .mockResolvedValueOnce(fakeFetchResponse({ status: 200, body: { token: 'issued-token' } })) + .mockResolvedValueOnce(fakeFetchResponse({ status: 200, body: { schemaVersion: 2 } })); + + const verdict = await checkImageExistence({ + repository: 'private.example.com/app', + tag: '1.0.0', + fetch, + credentials: dockerCredentials({ + config: { credsStore: 'desktop', auths: { 'private.example.com': { auth: encodeAuth('dev', 's3cret') } } }, + runCredentialHelper, + }), + }); + + expect(runCredentialHelper).toHaveBeenCalledWith('desktop', 'private.example.com'); + expect(requestAt(fetch.mock.calls, 1).init.headers).toEqual({ authorization: `Basic ${encodeAuth('dev', 's3cret')}` }); + expect(verdict).toEqual({ kind: 'exists', registry: 'private.example.com' }); + }); + + it('should match a Docker Hub config key written as https://index.docker.io/v1/ against a docker.io reference', async () => { + const fetch = vi + .fn() + .mockResolvedValueOnce( + fakeFetchResponse({ + status: 401, + body: {}, + wwwAuthenticate: 'Bearer realm="https://auth.docker.io/token",service="registry.docker.io"', + }) + ) + .mockResolvedValueOnce(fakeFetchResponse({ status: 200, body: { token: 'hub-token' } })) + .mockResolvedValueOnce(fakeFetchResponse({ status: 200, body: { schemaVersion: 2 } })); + + const verdict = await checkImageExistence({ + repository: 'docker.io/library/nginx', + tag: '1.19', + fetch, + credentials: dockerCredentials({ + config: { auths: { 'https://index.docker.io/v1/': { auth: encodeAuth('hub-user', 'hub-secret') } } }, + }), + }); + + const token = requestAt(fetch.mock.calls, 1); + + expect(new URL(token.url).searchParams.get('scope')).toBe('repository:library/nginx:pull'); + expect(token.init.headers).toEqual({ authorization: `Basic ${encodeAuth('hub-user', 'hub-secret')}` }); + expect(verdict).toEqual({ kind: 'exists', registry: 'docker.io' }); + }); + + // The ACR case above cannot prove the ordering on its own: its `auth` carries an empty + // password, so the empty-password rule would reach the identity token whatever the order. + // This entry pairs an identity token with a usable password, where only the order decides. + it('should prefer an identity token over an auth field that would otherwise yield a usable basic credential', async () => { + const fetch = vi + .fn() + .mockResolvedValueOnce( + fakeFetchResponse({ + status: 401, + body: {}, + wwwAuthenticate: 'Bearer realm="https://myorg.azurecr.io/oauth2/token",service="myorg.azurecr.io"', + }) + ) + .mockResolvedValueOnce( + fakeFetchResponse({ + status: 200, + // eslint-disable-next-line @typescript-eslint/naming-convention -- the OAuth2 wire field name the registry actually returns + body: { access_token: 'acr-access-token' }, + }) + ) + .mockResolvedValueOnce(fakeFetchResponse({ status: 200, body: { schemaVersion: 2 } })); + + const verdict = await checkImageExistence({ + repository: 'myorg.azurecr.io/app', + tag: '1.0.0', + fetch, + credentials: dockerCredentials({ + config: { + auths: { + 'myorg.azurecr.io': { auth: encodeAuth('stale-user', 'stale-password'), identitytoken: 'refresh-token-value' }, + }, + }, + }), + }); + + const token = requestAt(fetch.mock.calls, 1); + + expect(token.init.method).toBe('POST'); + expect(token.init.headers).toEqual({ 'content-type': 'application/x-www-form-urlencoded' }); + expect(new URLSearchParams(token.init.body ?? '').get('refresh_token')).toBe('refresh-token-value'); + expect(verdict).toEqual({ kind: 'exists', registry: 'myorg.azurecr.io' }); + }); + + it('should answer a Basic challenge by retrying the manifest request with the credential itself', async () => { + const fetch = vi + .fn() + .mockResolvedValueOnce(fakeFetchResponse({ status: 401, body: {}, wwwAuthenticate: 'Basic realm="private.example.com"' })) + .mockResolvedValueOnce(fakeFetchResponse({ status: 200, body: { schemaVersion: 2 } })); + + const verdict = await checkImageExistence({ + repository: 'private.example.com/app', + tag: '1.0.0', + fetch, + credentials: dockerCredentials({ config: { auths: { 'private.example.com': { auth: encodeAuth('dev', 's3cret') } } } }), + }); + + expect(fetch).toHaveBeenCalledTimes(2); + expect(fetch).toHaveBeenNthCalledWith(2, 'https://private.example.com/v2/app/manifests/1.0.0', { + method: 'GET', + headers: { accept: MANIFEST_ACCEPT_HEADER, authorization: `Basic ${encodeAuth('dev', 's3cret')}` }, + }); + expect(verdict).toEqual({ kind: 'exists', registry: 'private.example.com' }); + }); + + it('should exchange a helper secret reported under the username as a refresh token, not a password', async () => { + const runCredentialHelper = vi + .fn() + .mockResolvedValue(helperOutput({ serverUrl: 'private.example.com', username: '', secret: 'helper-refresh-token' })); + const fetch = vi + .fn() + .mockResolvedValueOnce( + fakeFetchResponse({ + status: 401, + body: {}, + wwwAuthenticate: 'Bearer realm="https://private.example.com/oauth2/token",service="private.example.com"', + }) + ) + .mockResolvedValueOnce( + fakeFetchResponse({ + status: 200, + // eslint-disable-next-line @typescript-eslint/naming-convention -- the OAuth2 wire field name the registry actually returns + body: { access_token: 'exchanged-token' }, + }) + ) + .mockResolvedValueOnce(fakeFetchResponse({ status: 200, body: { schemaVersion: 2 } })); + + const verdict = await checkImageExistence({ + repository: 'private.example.com/app', + tag: '1.0.0', + fetch, + credentials: dockerCredentials({ config: { credHelpers: { 'private.example.com': 'acr-env' } }, runCredentialHelper }), + }); + + const token = requestAt(fetch.mock.calls, 1); + const body = new URLSearchParams(token.init.body ?? ''); + + expect(token.init.method).toBe('POST'); + expect(token.init.headers).toEqual({ 'content-type': 'application/x-www-form-urlencoded' }); + expect(body.get('grant_type')).toBe('refresh_token'); + expect(body.get('refresh_token')).toBe('helper-refresh-token'); + expect(verdict).toEqual({ kind: 'exists', registry: 'private.example.com' }); + }); + + // The credential spelled out in the unparseable text is what makes this non-vacuous: were the + // config read at all, this registry would resolve a credential and reach the network instead. + it('should treat a docker config whose JSON does not parse as an empty config', async () => { + const fetch = vi.fn(); + + const verdict = await checkImageExistence({ + repository: 'private.example.com/app', + tag: '1.0.0', + fetch, + credentials: dockerCredentials({ + configText: `{ "auths": { "private.example.com": { "auth": "${encodeAuth('dev', 's3cret')}" }, } }`, + }), + }); + + expect(fetch).not.toHaveBeenCalled(); + expect(verdict).toEqual({ kind: 'unverifiable', reason: 'needs-login', registry: 'private.example.com' }); + }); }); diff --git a/packages/oci-registry/src/check-image-existence.ts b/packages/oci-registry/src/check-image-existence.ts index 46134ca..86ab0a8 100644 --- a/packages/oci-registry/src/check-image-existence.ts +++ b/packages/oci-registry/src/check-image-existence.ts @@ -1,3 +1,6 @@ +import { acquireBearerToken, basicAuthorizationHeader, parseAuthenticateChallenge } from './authorize'; +import type { CredentialEnvironment, RegistryCredential } from './credentials'; +import { isPublicRegistry, resolveRegistryCredential } from './credentials'; import type { FetchLike, FetchResponseLike } from './fetch-like'; import { resolveExplicitHost } from './resolve-explicit-host'; import type { ImageVerdict } from './verdict'; @@ -28,25 +31,27 @@ interface CheckImageExistenceParams { readonly repository: string; readonly tag: string; readonly fetch: FetchLike; + readonly credentials: CredentialEnvironment; } /** * Checks whether an image reference exists on a container registry. * * This is the package's single entry point: everything else — host - * detection, the request shape, the distinction between a missing - * repository and a missing tag — is reached only through here, on purpose, - * so a test asserts the requests issued and the verdict returned rather - * than an internal function. + * detection, credential resolution, the request shape, the distinction + * between a missing repository and a missing tag — is reached only through + * here, on purpose, so a test asserts the requests issued and the verdict + * returned rather than an internal function. * * Only a fully-qualified repository (one naming an explicit registry host) - * and only an anonymous request are supported so far — no document/registry - * fallback, no Docker Hub fallback, no credential chain. Anything this - * package cannot yet resolve, reach, or interpret comes back as - * `'unverifiable'`, never as a false negative. + * is supported so far — no document/registry fallback, no Docker Hub + * fallback. Credentials come from the local Docker config and nowhere else: + * this package never prompts, and never invents one. Anything it cannot + * resolve, reach, or interpret comes back as `'unverifiable'`, never as a + * false negative. */ async function checkImageExistence(params: CheckImageExistenceParams): Promise { - const { repository, tag, fetch } = params; + const { repository, tag, fetch, credentials } = params; const location = resolveExplicitHost(repository); if (location === undefined) { @@ -58,11 +63,41 @@ async function checkImageExistence(params: CheckImageExistenceParams): Promise { + const { response, credential, repositoryName, fetch } = params; + const header = response.headers.get('www-authenticate'); + + if (header === null) { + return undefined; + } + + const challenge = parseAuthenticateChallenge(header); + + if (challenge === undefined) { + return undefined; + } + + if (challenge.scheme === 'bearer') { + const token = await acquireBearerToken({ challenge, credential, repositoryName, fetch }); + return token === undefined ? undefined : `Bearer ${token}`; + } + + return credential?.kind === 'basic' ? basicAuthorizationHeader(credential) : undefined; +} + interface DistributionErrorBody { readonly errors?: readonly { readonly code?: string }[]; } diff --git a/packages/oci-registry/src/credentials.ts b/packages/oci-registry/src/credentials.ts new file mode 100644 index 0000000..d60415c --- /dev/null +++ b/packages/oci-registry/src/credentials.ts @@ -0,0 +1,341 @@ +/** + * Hosts an anonymous manifest request is allowed against. + * + * A table rather than a heuristic, because the consequence of guessing wrong + * runs one way: contacting an unlisted host without a credential leaks the + * repository name of a private image to whoever answers, and buys nothing — + * the 401 that comes back is indistinguishable from the one a real + * credential failure produces. Everything off this list with no credential + * resolves to `'needs-login'` instead, which is the only outcome a caller + * can turn into an actionable "log in to X" prompt. + */ +const PUBLIC_REGISTRIES = new Set([ + 'docker.io', + 'index.docker.io', + 'registry-1.docker.io', + 'ghcr.io', + 'quay.io', + 'mcr.microsoft.com', + 'public.ecr.aws', + 'registry.k8s.io', + 'gcr.io', + 'k8s.gcr.io', +]); + +const DOCKER_HUB_HOST = 'docker.io'; + +// The three spellings Docker Hub answers to. `docker login` writes the +// config entry under `https://index.docker.io/v1/`, a repository names the +// registry as `docker.io`, and the pull endpoint is `registry-1.docker.io`, +// so a credential written by one of them has to be found by the others. +const DOCKER_HUB_ALIASES = new Set([DOCKER_HUB_HOST, 'index.docker.io', 'registry-1.docker.io']); + +// The key `docker login` writes Docker Hub under, and therefore the server +// URL a credential helper expects to be asked about for Docker Hub. The +// bare host would be a cache miss in every helper a real developer has. +const DOCKER_HUB_SERVER_URL = 'https://index.docker.io/v1/'; + +const SCHEME_PREFIX_PATTERN = /^https?:\/\//; + +// Splits a decoded `auth` field on its FIRST colon: a registry password may +// contain colons, a username may not, so anything after the first one is +// password material and splitting on the last (or on every) colon silently +// truncates a legitimate secret. +const BASIC_AUTH_PATTERN = /^([^:]*):([\s\S]*)$/; + +// Docker's convention for "the Secret field is an identity token, not a +// password". A helper reports it in the username slot because the protocol +// has nowhere else to put the distinction. +const IDENTITY_TOKEN_USERNAME = ''; + +/** + * A credential resolved for one registry. + * + * Two arms rather than one username/password pair, because the two are spent + * in completely different requests: a basic credential goes out as an + * `authorization` header, while an identity token is exchanged in a `POST` + * body for an access token and must never travel as a password. A single + * shape carrying a magic username is exactly how a client ends up sending a + * refresh token where a password belongs. + */ +type RegistryCredential = + | { readonly kind: 'basic'; readonly username: string; readonly password: string } + | { readonly kind: 'identity-token'; readonly refreshToken: string }; + +/** The local Docker credential environment, injected so this package never reads disk or spawns a process itself. */ +interface CredentialEnvironment { + /** Raw contents of `~/.docker/config.json`, or `undefined` when there is none. */ + readonly readDockerConfig: () => Promise; + /** Runs `docker-credential- get` with `serverUrl` on stdin; resolves to stdout. */ + readonly runCredentialHelper: (helper: string, serverUrl: string) => Promise; +} + +/** + * One entry of a Docker config table, kept alongside the key exactly as it + * was written. + * + * The raw key survives normalization because it is what a credential helper + * has to be handed back: helpers key their store by the literal string + * `docker login` gave them, so asking one about a normalized `docker.io` + * when the config says `https://index.docker.io/v1/` reliably misses. + */ +interface DockerConfigEntry { + readonly key: string; + readonly value: TValue; +} + +interface DockerAuthEntry { + readonly identitytoken: string | undefined; + readonly auth: string | undefined; + readonly username: string | undefined; + readonly password: string | undefined; +} + +interface DockerConfig { + readonly credHelpers: readonly DockerConfigEntry[]; + readonly credsStore: string | undefined; + readonly auths: readonly DockerConfigEntry[]; +} + +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null; +} + +function readStringField(source: unknown, field: string): string | undefined { + if (!isRecord(source)) { + return undefined; + } + + const value = source[field]; + return typeof value === 'string' ? value : undefined; +} + +/** + * Reduces a config key or a registry host to the bare `host[:port]` the two + * can be compared on. + * + * Docker config keys are written inconsistently — `private.example.com`, + * `https://private.example.com`, `https://index.docker.io/v1/` all name a + * registry — because different Docker versions and different login flows + * wrote them. Comparing the raw strings would make a credential the user + * demonstrably has look absent, so both sides are reduced to the same shape + * before they meet. + */ +function normalizeConfigKey(key: string): string { + const [hostPort = ''] = key.replace(SCHEME_PREFIX_PATTERN, '').split('/'); + return DOCKER_HUB_ALIASES.has(hostPort) ? DOCKER_HUB_HOST : hostPort; +} + +function parseAuthEntry(value: unknown): DockerAuthEntry | undefined { + if (!isRecord(value)) { + return undefined; + } + + return { + identitytoken: readStringField(value, 'identitytoken'), + auth: readStringField(value, 'auth'), + username: readStringField(value, 'username'), + password: readStringField(value, 'password'), + }; +} + +function parseStringValue(value: unknown): string | undefined { + return typeof value === 'string' ? value : undefined; +} + +function parseTable(source: unknown, field: string, parseValue: (value: unknown) => TValue | undefined): DockerConfigEntry[] { + const table = isRecord(source) ? source[field] : undefined; + + if (!isRecord(table)) { + return []; + } + + const entries: DockerConfigEntry[] = []; + + for (const [key, rawValue] of Object.entries(table)) { + const value = parseValue(rawValue); + + if (value !== undefined) { + entries.push({ key, value }); + } + } + + return entries; +} + +/** + * Parses `config.json` into the three tables that matter, treating anything + * unparseable or unexpectedly shaped as absent. + * + * The config is a file on the developer's machine, edited by hand and by + * several tools, not a value this package controls. A stray comma in it is + * not a defect in the chart being checked, so throwing here would turn an + * unrelated typo into an extension-host error and take the whole check down + * with it. Every field is read through a runtime type check for the same + * reason: nothing in the file is guaranteed to be the type it should be. + */ +function parseDockerConfig(contents: string | undefined): DockerConfig { + let parsed: unknown; + + try { + parsed = contents === undefined ? undefined : JSON.parse(contents); + } catch { + parsed = undefined; + } + + return { + credHelpers: parseTable(parsed, 'credHelpers', parseStringValue), + credsStore: readStringField(parsed, 'credsStore'), + auths: parseTable(parsed, 'auths', parseAuthEntry), + }; +} + +function findEntry(entries: readonly DockerConfigEntry[], normalizedHost: string): DockerConfigEntry | undefined { + return entries.find((entry) => normalizeConfigKey(entry.key) === normalizedHost); +} + +/** + * Turns a credential helper's stdout into a credential. + * + * The protocol is a single JSON object, so a helper that has simply never + * seen this registry answers with a non-zero exit or a line of prose. That + * is the routine case on a machine whose keychain holds two logins, not a + * failure worth surfacing, which is why every malformed shape here comes + * back as "no credential" and lets the caller fall through to the next + * source instead of aborting the check. + */ +function credentialFromHelperOutput(stdout: string): RegistryCredential | undefined { + let payload: unknown; + + try { + payload = JSON.parse(stdout); + } catch { + return undefined; + } + + const secret = readStringField(payload, 'Secret'); + const username = readStringField(payload, 'Username') ?? ''; + + if (secret === undefined || secret === '') { + return undefined; + } + + if (username === IDENTITY_TOKEN_USERNAME) { + return { kind: 'identity-token', refreshToken: secret }; + } + + return { kind: 'basic', username, password: secret }; +} + +function credentialFromBasicAuth(auth: string): RegistryCredential | undefined { + const match = BASIC_AUTH_PATTERN.exec(Buffer.from(auth, 'base64').toString('utf8')); + + if (match === null) { + return undefined; + } + + const [, username = '', password = ''] = match; + return password === '' ? undefined : { kind: 'basic', username, password }; +} + +/** + * Reads a credential out of an `auths` entry, identity token first. + * + * The ordering is the whole point. An Azure Container Registry entry carries + * both an `auth` field and an `identitytoken`, and that `auth` decodes to a + * null-GUID username with an EMPTY password — it is a placeholder, not a + * credential. A client that reads `auth` first therefore sends useless basic + * credentials and collects a 401 from what is, for this organisation, the + * primary registry. Checking `identitytoken` first costs nothing anywhere + * else and makes that case work. + */ +function credentialFromAuthEntry(entry: DockerAuthEntry): RegistryCredential | undefined { + const { identitytoken, auth, username, password } = entry; + + if (identitytoken !== undefined && identitytoken !== '') { + return { kind: 'identity-token', refreshToken: identitytoken }; + } + + const decoded = auth === undefined ? undefined : credentialFromBasicAuth(auth); + + if (decoded !== undefined) { + return decoded; + } + + if (username !== undefined && password !== undefined && password !== '') { + return { kind: 'basic', username, password }; + } + + return undefined; +} + +/** The server URL to ask a helper about when no config key named this registry. */ +function defaultServerUrl(normalizedHost: string): string { + return normalizedHost === DOCKER_HUB_HOST ? DOCKER_HUB_SERVER_URL : normalizedHost; +} + +interface RunHelperParams { + readonly helper: string; + readonly serverUrl: string; + readonly environment: CredentialEnvironment; +} + +async function runHelper(params: RunHelperParams): Promise { + const { helper, serverUrl, environment } = params; + + try { + return credentialFromHelperOutput(await environment.runCredentialHelper(helper, serverUrl)); + } catch { + return undefined; + } +} + +/** + * Resolves the credential the local Docker setup holds for a registry, or + * `undefined` when it holds none. + * + * The order — per-registry helper, then the global store, then the + * plaintext `auths` entry — mirrors Docker's own precedence, so a developer + * who can `docker pull` an image sees this package agree with their shell. + * Each step falls through on a miss rather than failing, because a miss is + * the normal state of a keychain that has only ever been asked about one + * registry. + * + * This function does not throw. Every failure mode it has — no config file, + * unparseable config, a helper that crashes — means the same thing to the + * caller as an empty config, and the caller's job is to decide between + * "public, try anonymously" and "ask the user to log in". + */ +async function resolveRegistryCredential(host: string, environment: CredentialEnvironment): Promise { + let contents: string | undefined; + + try { + contents = await environment.readDockerConfig(); + } catch { + contents = undefined; + } + + const config = parseDockerConfig(contents); + const normalizedHost = normalizeConfigKey(host); + const helperEntry = findEntry(config.credHelpers, normalizedHost); + const authEntry = findEntry(config.auths, normalizedHost); + const serverUrl = helperEntry?.key ?? authEntry?.key ?? defaultServerUrl(normalizedHost); + const helpers = new Set([helperEntry?.value, config.credsStore].filter((helper) => helper !== undefined)); + + for (const helper of helpers) { + const credential = await runHelper({ helper, serverUrl, environment }); + + if (credential !== undefined) { + return credential; + } + } + + return authEntry === undefined ? undefined : credentialFromAuthEntry(authEntry.value); +} + +function isPublicRegistry(host: string): boolean { + return PUBLIC_REGISTRIES.has(host); +} + +export { isPublicRegistry, resolveRegistryCredential }; +export type { CredentialEnvironment, RegistryCredential }; diff --git a/packages/oci-registry/src/fetch-like.ts b/packages/oci-registry/src/fetch-like.ts index 6f83d3b..26caa1d 100644 --- a/packages/oci-registry/src/fetch-like.ts +++ b/packages/oci-registry/src/fetch-like.ts @@ -3,17 +3,20 @@ * Deliberately narrow, and deliberately not the real `Response` type — that * would tie every caller and every test double to a global type this * workspace's `lib` doesn't even carry, for a package that only ever reads - * three members off it. + * four members off it. */ export interface FetchResponseLike { readonly status: number; readonly ok: boolean; + /** Structurally satisfied by the platform's `Headers`. */ + readonly headers: { readonly get: (name: string) => string | null }; readonly json: () => Promise; } export interface FetchRequestInit { readonly method?: string; readonly headers?: Record; + readonly body?: string; } /** diff --git a/packages/oci-registry/src/index.ts b/packages/oci-registry/src/index.ts index e7f44f0..79ec07e 100644 --- a/packages/oci-registry/src/index.ts +++ b/packages/oci-registry/src/index.ts @@ -1,6 +1,8 @@ export { checkImageExistence } from './check-image-existence'; export type { CheckImageExistenceParams } from './check-image-existence'; +export type { CredentialEnvironment, RegistryCredential } from './credentials'; export type { FetchLike, FetchRequestInit, FetchResponseLike } from './fetch-like'; +export { localDockerCredentials } from './local-docker-credentials'; export { resolveExplicitHost } from './resolve-explicit-host'; export type { RepositoryLocation } from './resolve-explicit-host'; -export type { ImageVerdict, UnverifiableReason } from './verdict'; +export type { ImageVerdict, UnverifiableReason, UnverifiableVerdict } from './verdict'; diff --git a/packages/oci-registry/src/local-docker-credentials.ts b/packages/oci-registry/src/local-docker-credentials.ts new file mode 100644 index 0000000..e4adcd8 --- /dev/null +++ b/packages/oci-registry/src/local-docker-credentials.ts @@ -0,0 +1,116 @@ +import { spawn } from 'node:child_process'; +import { readFile } from 'node:fs/promises'; +import { homedir } from 'node:os'; +import { join } from 'node:path'; +import type { CredentialEnvironment } from './credentials'; + +// The helper name arrives out of `config.json`, which is a file any process +// on the machine can write. It is interpolated into a command name, so this +// is the boundary that has to refuse anything outside the character set +// real helper names use, rather than trusting the file or relying on +// `spawn` without a shell to make an arbitrary string harmless. +const HELPER_NAME_PATTERN = /^[a-zA-Z0-9_-]+$/; + +const SUCCESS_EXIT_CODE = 0; + +/** + * Locates `config.json`. + * + * `DOCKER_CONFIG` takes precedence because that is how the Docker CLI itself + * resolves the file, and a developer who sets it — CI images and + * multi-account setups do — has moved the credentials this package is + * looking for. An empty value counts as unset, since exporting an empty + * variable is how a shell says "no override", not "look in `/config.json`". + */ +function dockerConfigPath(): string { + const override = process.env['DOCKER_CONFIG']; + + if (override === undefined || override === '') { + return join(homedir(), '.docker', 'config.json'); + } + + return join(override, 'config.json'); +} + +/** + * Reads `config.json`, reporting every failure as "there is no config". + * + * A machine that has never run `docker login` has no such file, which is an + * ordinary state rather than an error, and the same is true of one whose + * home directory is unreadable to this process. Both mean the same thing to + * the caller — no credential is available here — so neither is worth + * distinguishing into an exception the check would have to survive. + */ +async function readDockerConfig(): Promise { + try { + return await readFile(dockerConfigPath(), 'utf8'); + } catch { + return undefined; + } +} + +/** + * Runs a credential helper and returns its stdout. + * + * The helper protocol is a subprocess: the server URL goes in on stdin, the + * JSON credential comes back on stdout, and "I don't have this one" is a + * non-zero exit. That last case is why this rejects rather than resolving + * empty — the caller treats a rejection as a miss and falls through to the + * next credential source, and collapsing a miss into an empty string would + * make it indistinguishable from a helper that answered with nothing. + */ +async function runCredentialHelper(helper: string, serverUrl: string): Promise { + if (!HELPER_NAME_PATTERN.test(helper)) { + throw new Error(`refusing to run a credential helper whose name is not a plain identifier: ${helper}`); + } + + const command = `docker-credential-${helper}`; + + return new Promise((resolve, reject) => { + // stderr is discarded: helpers write their "credentials not found" + // prose there, and that is the routine miss, not a diagnostic anyone + // needs. + const child = spawn(command, ['get'], { stdio: ['pipe', 'pipe', 'ignore'] }); + const { stdin, stdout: output } = child; + + let collected = ''; + + output.setEncoding('utf8'); + output.on('data', (chunk: string) => { + collected += chunk; + }); + + child.on('error', reject); + + // A helper that rejects the request by exiting before it ever reads + // stdin leaves the write below failing with EPIPE, and an unhandled + // `error` on a stream is thrown rather than returned — inside the + // extension host that takes down the process over what is, to this + // package, an ordinary miss. Routed to `reject` so it stays one. + stdin.on('error', reject); + + child.on('close', (code) => { + if (code === SUCCESS_EXIT_CODE) { + resolve(collected); + return; + } + + reject(new Error(`${command} exited with code ${String(code)}`)); + }); + + stdin.end(`${serverUrl}\n`); + }); +} + +/** + * The production credential environment: the real `config.json` and the + * real helper subprocesses. + * + * It exists as a value rather than as the default behaviour of the + * resolution code so that the disk and the process table stay on one side + * of a seam. A test names the config contents and the helper output it + * wants; nothing else in this package can reach the filesystem at all. + */ +const localDockerCredentials: CredentialEnvironment = { readDockerConfig, runCredentialHelper }; + +export { localDockerCredentials }; diff --git a/packages/oci-registry/src/verdict.ts b/packages/oci-registry/src/verdict.ts index 15a1798..ef4626f 100644 --- a/packages/oci-registry/src/verdict.ts +++ b/packages/oci-registry/src/verdict.ts @@ -1,20 +1,30 @@ /** * The reason an image's existence could not be determined. * - * This list is expected to grow — credential handling, the document/registry - * fallback chain, and the Docker Hub guess-downgrade rule from spec #17 each - * add reasons of their own — but every reason, present or future, resolves - * to the same {@link ImageVerdict} `'unverifiable'` kind. That is the whole - * point of the shape: no caller can special-case a *reason* into rendering a - * diagnostic, because only the verdict `kind` controls that. + * This list is expected to grow — the document/registry fallback chain and + * the Docker Hub guess-downgrade rule from spec #17 each add reasons of + * their own — but every reason, present or future, resolves to the same + * {@link ImageVerdict} `'unverifiable'` kind. That is the whole point of the + * shape: no caller can special-case a *reason* into rendering a diagnostic, + * because only the verdict `kind` controls that. */ export type UnverifiableReason = /** The repository names no explicit registry host, and this package does * not yet resolve one any other way (a document-declared registry, a * workspace override set, or the Docker Hub fallback). */ | 'no-registry' - /** The registry demanded authentication this package cannot yet provide. */ - | 'missing-credential' + /** The registry is not one of the public ones, and no credential for it + * could be resolved from the local Docker config. Distinct from + * `'authentication-failure'` on purpose: this is the one reason a caller + * can act on, by prompting for a `docker login`, and it is reached without + * contacting the registry at all. */ + | 'needs-login' + /** A credential existed and the registry rejected it. Nothing the user can + * fix by logging in again in the general case — the token may be scoped + * away from this repository — and, critically, not evidence the image is + * missing: a registry is free to answer 401 rather than 404 for a + * repository the caller may not know about. */ + | 'authentication-failure' /** The request itself failed — DNS, connection refused, timeout, and so on. */ | 'network-error' /** The registry responded, but not in a way this checker understands. */ @@ -24,6 +34,19 @@ export type UnverifiableReason = * crafted values file smuggles a path traversal into the request. */ | 'malformed-reference'; +/** + * An unverifiable verdict, split so that only `'needs-login'` carries the + * registry it applies to. + * + * The host rides on that arm specifically rather than on the whole kind: a + * "log in to X" prompt is unbuildable without a host, so the type refuses to + * let a caller reach that reason without one, and no other reason can + * pretend to have a host it never established. + */ +export type UnverifiableVerdict = + | { readonly kind: 'unverifiable'; readonly reason: 'needs-login'; readonly registry: string } + | { readonly kind: 'unverifiable'; readonly reason: Exclude }; + /** * The outcome of checking whether an image reference exists. * @@ -37,4 +60,4 @@ export type ImageVerdict = | { readonly kind: 'exists'; readonly registry: string } | { readonly kind: 'repository-not-found'; readonly repository: string } | { readonly kind: 'tag-not-found'; readonly repository: string; readonly tag: string } - | { readonly kind: 'unverifiable'; readonly reason: UnverifiableReason }; + | UnverifiableVerdict; From 72849eb05f2f7fb0ba134ca43b56db8039192232 Mon Sep 17 00:00:00 2001 From: Schnitz <12687466+CptSchnitz@users.noreply.github.com> Date: Wed, 16 Sep 2026 10:04:51 +0300 Subject: [PATCH 2/6] fix(oci-registry): match Docker's store precedence and refuse plaintext realms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two corrections to the credential chain, both found in review. Docker picks exactly one credential store per registry: a `credHelpers` entry naming the registry replaces the global `credsStore` rather than being tried ahead of it, and whichever one applies falls through on a miss to the plaintext `auths` entry, never to the other store. Trying both, as the chain first did, lets this package verify an image with a credential `docker pull` would not have sent — a quieter kind of wrong answer than a failure, because it looks like agreement with the developer's shell while being something else. The `realm` a bearer challenge names is chosen by whoever answered the manifest request, and the request built from it is the one carrying the credential. An `http://` realm therefore put a password or a refresh token on the wire in the clear at the say-so of a response header. Only `https` is accepted now, with loopback exempted so a local registry — which has no certificate and tells nothing to anyone off the machine — keeps working. Refusing costs an `authentication-failure`, which renders nothing, so being strict here fails silent rather than wrong. Both are asserted by tests that fail against the previous code: the first by counting helper invocations, the second by counting requests, since neither rule is visible in the verdict alone. Refs #22 Co-Authored-By: Claude Opus 5 (1M context) --- packages/oci-registry/src/authorize.ts | 58 ++++++++----- .../src/check-image-existence.test.ts | 82 +++++++++++++++++++ packages/oci-registry/src/credentials.ts | 26 +++--- 3 files changed, 133 insertions(+), 33 deletions(-) diff --git a/packages/oci-registry/src/authorize.ts b/packages/oci-registry/src/authorize.ts index 2ec1b39..ab4ce88 100644 --- a/packages/oci-registry/src/authorize.ts +++ b/packages/oci-registry/src/authorize.ts @@ -19,6 +19,11 @@ const CLIENT_ID = 'infra-tools'; const FORM_CONTENT_TYPE = 'application/x-www-form-urlencoded'; +// Hosts a plaintext token endpoint is tolerated on. A local registry has no +// certificate and nothing it is told leaves the machine, and `localhost` is +// already a first-class registry host in `resolve-explicit-host`. +const LOOPBACK_HOSTNAMES = new Set(['localhost', '127.0.0.1', '[::1]']); + type BasicCredential = Extract; /** @@ -46,7 +51,7 @@ interface AcquireBearerTokenParams { } interface TokenRequestParams { - readonly realm: string; + readonly realm: URL; readonly challenge: AuthChallenge; readonly scope: string; readonly fetch: FetchLike; @@ -117,21 +122,40 @@ function basicAuthorizationHeader(credential: BasicCredential): string { return `Basic ${Buffer.from(`${credential.username}:${credential.password}`, 'utf8').toString('base64')}`; } -function buildTokenUrl(params: TokenRequestParams): string | undefined { - const { realm, challenge, scope } = params; +/** + * Parses the realm a challenge named, refusing any plaintext endpoint off + * the loopback interface. + * + * The realm is chosen by whoever answered the manifest request, and the + * request built from it is the one carrying the credential — an `http://` + * realm would put a password or a refresh token on the wire in the clear, + * at the say-so of a header. Refusing it costs a verdict of + * `'authentication-failure'`, which renders nothing, so the failure mode of + * being strict here is silence rather than a wrong answer. + */ +function parseRealm(realm: string): URL | undefined { + let url: URL; try { - const url = new URL(realm); - - if (challenge.service !== undefined) { - url.searchParams.set('service', challenge.service); - } - - url.searchParams.set('scope', scope); - return url.href; + url = new URL(realm); } catch { return undefined; } + + return url.protocol === 'https:' || LOOPBACK_HOSTNAMES.has(url.hostname) ? url : undefined; +} + +function buildTokenUrl(params: TokenRequestParams): string { + const { realm, challenge, scope } = params; + const url = new URL(realm.href); + + if (challenge.service !== undefined) { + url.searchParams.set('service', challenge.service); + } + + url.searchParams.set('scope', scope); + + return url.href; } /** @@ -159,7 +183,7 @@ async function requestTokenWithRefreshToken(params: RefreshTokenRequestParams): body.set('client_id', CLIENT_ID); body.set('refresh_token', refreshToken); - const response = await fetch(realm, { + const response = await fetch(realm.href, { method: 'POST', headers: { 'content-type': FORM_CONTENT_TYPE }, body: body.toString(), @@ -185,14 +209,8 @@ async function requestTokenWithRefreshToken(params: RefreshTokenRequestParams): */ async function requestTokenWithBasic(params: BasicTokenRequestParams): Promise { const { credential, fetch } = params; - const url = buildTokenUrl(params); - - if (url === undefined) { - return undefined; - } - const headers: Record = credential === undefined ? {} : { authorization: basicAuthorizationHeader(credential) }; - const response = await fetch(url, { method: 'GET', headers }); + const response = await fetch(buildTokenUrl(params), { method: 'GET', headers }); if (!response.ok) { return undefined; @@ -216,7 +234,7 @@ async function requestTokenWithBasic(params: BasicTokenRequestParams): Promise { const { challenge, credential, repositoryName, fetch } = params; - const { realm } = challenge; + const realm = challenge.realm === undefined ? undefined : parseRealm(challenge.realm); if (realm === undefined) { return undefined; diff --git a/packages/oci-registry/src/check-image-existence.test.ts b/packages/oci-registry/src/check-image-existence.test.ts index a75a0a8..815e555 100644 --- a/packages/oci-registry/src/check-image-existence.test.ts +++ b/packages/oci-registry/src/check-image-existence.test.ts @@ -344,6 +344,46 @@ describe('checkImageExistence', () => { expect(verdict).toEqual({ kind: 'exists', registry: 'private.example.com' }); }); + it('should fall through to the plaintext entry, not to the global credsStore, when a per-registry helper misses', async () => { + const runCredentialHelper = vi + .fn() + .mockRejectedValue(new Error('credentials not found in native keychain')); + const fetch = vi + .fn() + .mockResolvedValueOnce( + fakeFetchResponse({ + status: 401, + body: {}, + wwwAuthenticate: 'Bearer realm="https://private.example.com/oauth2/token",service="private.example.com"', + }) + ) + .mockResolvedValueOnce(fakeFetchResponse({ status: 200, body: { token: 'issued-token' } })) + .mockResolvedValueOnce(fakeFetchResponse({ status: 200, body: { schemaVersion: 2 } })); + + const verdict = await checkImageExistence({ + repository: 'private.example.com/app', + tag: '1.0.0', + fetch, + credentials: dockerCredentials({ + config: { + credsStore: 'desktop', + credHelpers: { 'private.example.com': 'acr-env' }, + auths: { 'private.example.com': { auth: encodeAuth('dev', 's3cret') } }, + }, + runCredentialHelper, + }), + }); + + // Docker picks exactly one store per registry, so a `credHelpers` entry + // replaces the global one instead of being tried ahead of it. Asking + // `desktop` here would verify with a credential `docker pull` would + // never send. + expect(runCredentialHelper).toHaveBeenCalledTimes(1); + expect(runCredentialHelper).toHaveBeenCalledWith('acr-env', 'private.example.com'); + expect(requestAt(fetch.mock.calls, 1).init.headers).toEqual({ authorization: `Basic ${encodeAuth('dev', 's3cret')}` }); + expect(verdict).toEqual({ kind: 'exists', registry: 'private.example.com' }); + }); + it('should exchange the identity token rather than the empty-password basic credential an ACR auth entry decodes to', async () => { const fetch = vi .fn() @@ -462,6 +502,48 @@ describe('checkImageExistence', () => { expect(verdict).toEqual({ kind: 'unverifiable', reason: 'authentication-failure' }); }); + it('should refuse a plaintext token endpoint rather than put the credential on the wire in the clear', async () => { + const fetch = vi.fn().mockResolvedValue( + fakeFetchResponse({ + status: 401, + body: {}, + wwwAuthenticate: 'Bearer realm="http://private.example.com/oauth2/token",service="private.example.com"', + }) + ); + + const verdict = await checkImageExistence({ + repository: 'private.example.com/app', + tag: '1.0.0', + fetch, + credentials: dockerCredentials({ config: { auths: { 'private.example.com': { auth: encodeAuth('dev', 's3cret') } } } }), + }); + + // Whoever answered the manifest request chose that realm, and the + // request built from it is the one carrying the password. + expect(fetch).toHaveBeenCalledTimes(1); + expect(verdict).toEqual({ kind: 'unverifiable', reason: 'authentication-failure' }); + }); + + it('should allow a plaintext token endpoint on loopback, where a local registry has no certificate', async () => { + const fetch = vi + .fn() + .mockResolvedValueOnce( + fakeFetchResponse({ status: 401, body: {}, wwwAuthenticate: 'Bearer realm="http://localhost:5000/token",service="localhost:5000"' }) + ) + .mockResolvedValueOnce(fakeFetchResponse({ status: 200, body: { token: 'issued-token' } })) + .mockResolvedValueOnce(fakeFetchResponse({ status: 200, body: { schemaVersion: 2 } })); + + const verdict = await checkImageExistence({ + repository: 'localhost:5000/app', + tag: '1.0.0', + fetch, + credentials: dockerCredentials({ config: { auths: { 'localhost:5000': { auth: encodeAuth('dev', 's3cret') } } } }), + }); + + expect(requestAt(fetch.mock.calls, 1).url).toBe('http://localhost:5000/token?service=localhost%3A5000&scope=repository%3Aapp%3Apull'); + expect(verdict).toEqual({ kind: 'exists', registry: 'localhost:5000' }); + }); + it('should fall through to the plaintext auths entry when the credential helper rejects', async () => { const runCredentialHelper = vi .fn() diff --git a/packages/oci-registry/src/credentials.ts b/packages/oci-registry/src/credentials.ts index d60415c..7590e40 100644 --- a/packages/oci-registry/src/credentials.ts +++ b/packages/oci-registry/src/credentials.ts @@ -294,12 +294,15 @@ async function runHelper(params: RunHelperParams): Promise helper !== undefined)); + const helper = helperEntry?.value ?? config.credsStore; + const credential = helper === undefined ? undefined : await runHelper({ helper, serverUrl, environment }); - for (const helper of helpers) { - const credential = await runHelper({ helper, serverUrl, environment }); - - if (credential !== undefined) { - return credential; - } + if (credential !== undefined) { + return credential; } return authEntry === undefined ? undefined : credentialFromAuthEntry(authEntry.value); From 60121f64e84a712807d7998b0eef9fe17a2902d0 Mon Sep 17 00:00:00 2001 From: Schnitz <12687466+CptSchnitz@users.noreply.github.com> Date: Wed, 16 Sep 2026 14:15:46 +0300 Subject: [PATCH 3/6] refactor(oci-registry): stop exporting the types only authorize.ts uses `AuthChallenge` and `BasicCredential` describe the inside of the bearer challenge flow and are named by nothing outside the file that defines them. Exporting them made `knip` fail the repo's unused-code check, which it has been doing since the credential chain landed. Refs #22 Co-Authored-By: Claude Opus 5 (1M context) --- packages/oci-registry/src/authorize.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/oci-registry/src/authorize.ts b/packages/oci-registry/src/authorize.ts index ab4ce88..fa406f7 100644 --- a/packages/oci-registry/src/authorize.ts +++ b/packages/oci-registry/src/authorize.ts @@ -248,4 +248,3 @@ async function acquireBearerToken(params: AcquireBearerTokenParams): Promise Date: Wed, 16 Sep 2026 14:16:02 +0300 Subject: [PATCH 4/6] feat(vscode): prompt for a docker login when a registry has no credential MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A registry the developer has never logged in to now says so, once, with the fix attached: a notification naming the host, an action that opens a terminal on `docker login `, and an action that silences that registry for good. A status bar count keeps the total visible after the notification is gone, so "why is half this file unverified" has an answer that does not require remembering a toast. None of this is a diagnostic, deliberately. Not having logged in to a registry is a fact about the developer's machine, not a defect in the values file, and a Problems panel that mixes the two teaches people to stop reading it. The registry package already routes the case to an unverifiable verdict, which renders no error; this commit is only about giving it somewhere to go. Dismissals persist in extension state rather than in settings. A dismissal is not configuration — it records that one developer stopped caring about one registry — and putting it in settings would accumulate that in a file the whole team has checked in, where it would then need explaining. The state is one map from registry to `'prompted' | 'dismissed'`, not a pair of sets, because every rule this surface has is a statement about it. At most once per session is that nothing leaves `'prompted'` except into `'dismissed'`. Never again after a window reload is that `'dismissed'` is what gets persisted and what seeds the map next time. The status bar count is how many entries are `'prompted'`. Both deduplication points are tested, because a values file naming ten images on one private registry is the ordinary case, and ten notifications for it would be the feature's worst behaviour. `report` swallows its own failures for the same reason the mark code does: the open listener fires it without awaiting, so an escaping rejection is an unhandled one, and that takes the extension host down. A check still in flight when the window closes finds the status bar item disposed, which is exactly that case. `MarkKind` loses its export in passing. Nothing outside `marks.ts` names it, and it was already failing the repo's unused-code check before this branch. Refs #22 Co-Authored-By: Claude Opus 5 (1M context) --- apps/vscode/src/diagnostics.test.ts | 19 +-- apps/vscode/src/extension.test.ts | 62 ++++++++-- apps/vscode/src/extension.ts | 22 +++- apps/vscode/src/hover.test.ts | 10 +- apps/vscode/src/hover.ts | 3 +- apps/vscode/src/login-prompts.test.ts | 116 +++++++++++++++++++ apps/vscode/src/login-prompts.ts | 148 ++++++++++++++++++++++++ apps/vscode/src/marks.ts | 1 - apps/vscode/src/reference-check.test.ts | 7 +- apps/vscode/src/reference-check.ts | 44 ++++++- apps/vscode/test/fake-context.ts | 30 +++++ apps/vscode/test/fake-credentials.ts | 18 +++ apps/vscode/test/fake-fetch.ts | 8 +- apps/vscode/test/vscode-stub.ts | 103 ++++++++++++++++- 14 files changed, 555 insertions(+), 36 deletions(-) create mode 100644 apps/vscode/src/login-prompts.test.ts create mode 100644 apps/vscode/src/login-prompts.ts create mode 100644 apps/vscode/test/fake-context.ts create mode 100644 apps/vscode/test/fake-credentials.ts diff --git a/apps/vscode/src/diagnostics.test.ts b/apps/vscode/src/diagnostics.test.ts index 1946656..5a1c6d4 100644 --- a/apps/vscode/src/diagnostics.test.ts +++ b/apps/vscode/src/diagnostics.test.ts @@ -11,13 +11,14 @@ const VALUES_YAML = ['image:', ` repository: ${REPOSITORY}`, ` tag: "${TAG}"`, // A record rather than a list, so a new reason fails the build here instead // of going untested. -const UNCHECKED_REASONS: Record = { - 'no-tag': true, - 'no-registry': true, - 'missing-credential': true, - 'network-error': true, - 'unexpected-response': true, - 'malformed-reference': true, +const UNCHECKED_VERDICTS: Record = { + 'no-tag': { kind: 'unverifiable', reason: 'no-tag' }, + 'no-registry': { kind: 'unverifiable', reason: 'no-registry' }, + 'needs-login': { kind: 'unverifiable', reason: 'needs-login', registry: REPOSITORY }, + 'authentication-failure': { kind: 'unverifiable', reason: 'authentication-failure' }, + 'network-error': { kind: 'unverifiable', reason: 'network-error' }, + 'unexpected-response': { kind: 'unverifiable', reason: 'unexpected-response' }, + 'malformed-reference': { kind: 'unverifiable', reason: 'malformed-reference' }, }; /** The source range of `text`'s first occurrence in {@link VALUES_YAML}. */ @@ -75,8 +76,8 @@ describe('diagnostics', () => { it('should report nothing for any unverifiable reason, since an unreachable registry is not a missing image', () => { const document = createFakeDocument('/repo/chart/values.yaml', VALUES_YAML); - for (const reason of Object.keys(UNCHECKED_REASONS) as UncheckedReason[]) { - expect(diagnosticsFor(document, [createCheck({ kind: 'unverifiable', reason })])).toEqual([]); + for (const verdict of Object.values(UNCHECKED_VERDICTS)) { + expect(diagnosticsFor(document, [createCheck(verdict)])).toEqual([]); } }); }); diff --git a/apps/vscode/src/extension.test.ts b/apps/vscode/src/extension.test.ts index 1f1e3c4..78cdd56 100644 --- a/apps/vscode/src/extension.test.ts +++ b/apps/vscode/src/extension.test.ts @@ -1,5 +1,7 @@ import * as vscode from 'vscode'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { createFakeContext } from '../test/fake-context'; +import { noDockerCredentials } from '../test/fake-credentials'; import { bumpVersion, createFakeDocument } from '../test/fake-document'; import { fakeFetchResponse } from '../test/fake-fetch'; import { @@ -9,6 +11,8 @@ import { getLastDiagnosticCollection, getRegisteredHoverProvider, setVisibleTextEditors, + setWarningMessageAnswer, + window, type TextEditorStub, } from '../test/vscode-stub'; import { activate, deactivate } from './extension'; @@ -66,7 +70,7 @@ describe('extension', () => { let context: vscode.ExtensionContext; beforeEach(() => { - context = { subscriptions: [] } as unknown as vscode.ExtensionContext; + context = createFakeContext(); }); afterEach(() => { @@ -75,10 +79,12 @@ describe('extension', () => { } setVisibleTextEditors([]); + setWarningMessageAnswer(undefined); + window.showWarningMessage.mockClear(); }); it('should create an output channel and register it for disposal on activate', () => { - activate(context, { fetch: vi.fn() }); + activate(context, { fetch: vi.fn(), credentials: noDockerCredentials }); expect(vscode.window.createOutputChannel).toHaveBeenCalledWith('Infra Tools'); expect(context.subscriptions.length).toBeGreaterThanOrEqual(1); @@ -89,20 +95,20 @@ describe('extension', () => { }); it('should register a yaml hover provider on activate', () => { - activate(context, { fetch: vi.fn() }); + activate(context, { fetch: vi.fn(), credentials: noDockerCredentials }); expect(vscode.languages.registerHoverProvider).toHaveBeenCalledWith({ language: 'yaml' }, expect.anything()); }); it('should create one mark decoration type on activate', () => { - activate(context, { fetch: vi.fn() }); + activate(context, { fetch: vi.fn(), credentials: noDockerCredentials }); expect(vscode.window.createTextEditorDecorationType).toHaveBeenCalledWith({ after: { margin: '0 0 0 0.5em' } }); }); it('should set both diagnostics and marks when a values file opens', async () => { const fetch = vi.fn().mockResolvedValue(fakeFetchResponse(404, { errors: [{ code: 'MANIFEST_UNKNOWN' }] })); - activate(context, { fetch }); + activate(context, { fetch, credentials: noDockerCredentials }); const document = createFakeDocument('/repo/chart/values.yaml', VALUES_YAML); const editor = await openInVisibleEditor(document); @@ -115,7 +121,7 @@ describe('extension', () => { it('should hover a checked reference, and stop once the document has been edited past the check', async () => { const fetch = vi.fn().mockResolvedValue(fakeFetchResponse(200)); - activate(context, { fetch }); + activate(context, { fetch, credentials: noDockerCredentials }); const document = createFakeDocument('/repo/chart/values.yaml', VALUES_YAML); await openInVisibleEditor(document); @@ -129,7 +135,7 @@ describe('extension', () => { it('should re-apply marks to an editor that becomes visible after the document was checked', async () => { const fetch = vi.fn().mockResolvedValue(fakeFetchResponse(200)); - activate(context, { fetch }); + activate(context, { fetch, credentials: noDockerCredentials }); const document = createFakeDocument('/repo/chart/values.yaml', VALUES_YAML); @@ -144,7 +150,7 @@ describe('extension', () => { it('should survive an editor disposed mid-check, since an unhandled rejection kills the extension host', async () => { const fetch = vi.fn().mockResolvedValue(fakeFetchResponse(200)); - activate(context, { fetch }); + activate(context, { fetch, credentials: noDockerCredentials }); const document = createFakeDocument('/repo/chart/values.yaml', VALUES_YAML); const disposedEditor: TextEditorStub = { @@ -160,9 +166,47 @@ describe('extension', () => { expect(getLastDiagnosticCollection()?.set).toHaveBeenCalledWith(document.uri, []); }); + it('should create a status bar item on activate', () => { + activate(context, { fetch: vi.fn(), credentials: noDockerCredentials }); + + expect(vscode.window.createStatusBarItem).toHaveBeenCalled(); + }); + + it('should notify, and raise no diagnostic, for a registry with no local credential', async () => { + const fetch = vi.fn(); + activate(context, { fetch, credentials: noDockerCredentials }); + + const document = createFakeDocument( + '/repo/chart/values.yaml', + ['image:', ' repository: private.example.com/svc', ' tag: 1.0.0', ''].join('\n') + ); + await openInVisibleEditor(document); + + // `void`-ed in the listener, so the notification is raised during the + // check but its promise is not what the listener awaits. + expect(window.showWarningMessage).toHaveBeenCalledWith(expect.stringContaining('private.example.com'), expect.any(String), expect.any(String)); + + // A missing credential is a fact about this machine, not a defect in the + // file. Putting it in the Problems panel beside real errors is how a + // panel earns being ignored. + expect(getLastDiagnosticCollection()?.set).toHaveBeenCalledWith(document.uri, []); + expect(fetch).not.toHaveBeenCalled(); + }); + + it('should notify once per registry across files, not once per file', async () => { + activate(context, { fetch: vi.fn(), credentials: noDockerCredentials }); + + const values = ['image:', ' repository: private.example.com/svc', ' tag: 1.0.0', ''].join('\n'); + + await emitDidOpenTextDocument(createFakeDocument('/repo/chart-a/values.yaml', values)); + await emitDidOpenTextDocument(createFakeDocument('/repo/chart-b/values.yaml', values)); + + expect(window.showWarningMessage).toHaveBeenCalledTimes(1); + }); + it('should issue no request for a document that is not a values file', async () => { const fetch = vi.fn(); - activate(context, { fetch }); + activate(context, { fetch, credentials: noDockerCredentials }); await emitDidOpenTextDocument(createFakeDocument('/repo/chart/deployment.yaml', VALUES_YAML)); diff --git a/apps/vscode/src/extension.ts b/apps/vscode/src/extension.ts index b2ab114..caeceee 100644 --- a/apps/vscode/src/extension.ts +++ b/apps/vscode/src/extension.ts @@ -1,15 +1,18 @@ import * as vscode from 'vscode'; -import type { FetchLike } from 'oci-registry'; +import { localDockerCredentials, type CredentialEnvironment, type FetchLike } from 'oci-registry'; import { diagnosticsFor } from './diagnostics'; import { hoverFor } from './hover'; +import { createLoginPrompts } from './login-prompts'; import { applyMarks, createMarkDecorationType } from './marks'; -import { checkImageReferencesInDocument, checksAsOf, type DocumentChecks } from './reference-check'; +import { checkImageReferencesInDocument, checksAsOf, registriesNeedingLogin, type DocumentChecks } from './reference-check'; const DIAGNOSTIC_COLLECTION_NAME = 'infra-tools-images'; interface ActivateDependencies { /** Only tests override this; production activation uses the platform's `fetch`. */ readonly fetch?: FetchLike; + /** Only tests override this; production activation reads the developer's real Docker config. */ + readonly credentials?: CredentialEnvironment; } /** @@ -21,7 +24,10 @@ function activate(context: vscode.ExtensionContext, dependencies: ActivateDepend channel.appendLine('Infra Tools extension activated.'); context.subscriptions.push(channel); - const fetchImpl = dependencies.fetch ?? (globalThis as unknown as { fetch: FetchLike }).fetch; + const checkDependencies = { + fetch: dependencies.fetch ?? (globalThis as unknown as { fetch: FetchLike }).fetch, + credentials: dependencies.credentials ?? localDockerCredentials, + }; const diagnostics = vscode.languages.createDiagnosticCollection(DIAGNOSTIC_COLLECTION_NAME); context.subscriptions.push(diagnostics); @@ -29,12 +35,15 @@ function activate(context: vscode.ExtensionContext, dependencies: ActivateDepend const markDecorationType = createMarkDecorationType(); context.subscriptions.push(markDecorationType); + const loginPrompts = createLoginPrompts(context.globalState); + context.subscriptions.push(loginPrompts); + context.subscriptions.push( vscode.workspace.onDidOpenTextDocument(async (document) => { // VS Code never awaits a listener, so anything escaping this callback // is an unhandled rejection, and that takes the extension host down. try { - const checked = await checkImageReferencesInDocument(document, fetchImpl); + const checked = await checkImageReferencesInDocument(document, checkDependencies); if (checked === undefined) { return; @@ -43,6 +52,11 @@ function activate(context: vscode.ExtensionContext, dependencies: ActivateDepend checksByDocument.set(document.uri.toString(), checked); diagnostics.set(document.uri, diagnosticsFor(document, checksAsOf(checked, document))); applyMarks(vscode.window.visibleTextEditors, checksByDocument, markDecorationType); + + // Not awaited: a notification stays up until the developer answers + // it, and holding an open-document listener for that long would tie + // this file's check to a dialog about a registry. + void loginPrompts.report(registriesNeedingLogin(checked.checks)); } catch (error) { channel.appendLine(`Checking image references in ${document.uri.toString()} failed: ${String(error)}`); } diff --git a/apps/vscode/src/hover.test.ts b/apps/vscode/src/hover.test.ts index 3d97057..81cda4e 100644 --- a/apps/vscode/src/hover.test.ts +++ b/apps/vscode/src/hover.test.ts @@ -16,12 +16,18 @@ const VERIFIED_VERDICT: ReferenceVerdict = { kind: 'exists', registry: 'mirror.e const UNCHECKED_REASON_SENTENCES: Record = { 'no-tag': 'the reference names no tag to check.', 'no-registry': 'the repository names no registry host.', - 'missing-credential': 'the registry requires credentials this extension cannot supply yet.', + 'needs-login': 'no local Docker credential for that registry. Run `docker login` against it.', + 'authentication-failure': 'the registry refused the local Docker credential for it.', 'network-error': 'the registry could not be reached.', 'unexpected-response': 'the registry answered in a form this extension does not understand.', 'malformed-reference': 'the tag is not a valid OCI tag.', }; +/** The unverifiable verdict a reason produces. Only `needs-login` carries a registry, so the shape cannot be built generically. */ +function unverifiableVerdict(reason: UncheckedReason): ReferenceVerdict { + return reason === 'needs-login' ? { kind: 'unverifiable', reason, registry: 'registry.example.com' } : { kind: 'unverifiable', reason }; +} + /** The source range of `text`'s first occurrence in {@link VALUES_YAML}. */ function rangeOfText(text: string): SourceRange { const start = VALUES_YAML.indexOf(text); @@ -80,7 +86,7 @@ describe('hover', () => { const document = createFakeDocument('/repo/chart/values.yaml', VALUES_YAML); for (const [reason, sentence] of Object.entries(UNCHECKED_REASON_SENTENCES) as [UncheckedReason, string][]) { - const hoverText = getHoverText(hoverAtText(document, [createCheck({ kind: 'unverifiable', reason })], REPOSITORY)); + const hoverText = getHoverText(hoverAtText(document, [createCheck(unverifiableVerdict(reason))], REPOSITORY)); expect(hoverText).toBe(`Not verified: ${sentence}`); } diff --git a/apps/vscode/src/hover.ts b/apps/vscode/src/hover.ts index 1a354a7..642d02d 100644 --- a/apps/vscode/src/hover.ts +++ b/apps/vscode/src/hover.ts @@ -7,7 +7,8 @@ import { containsOffset } from './source-range'; const UNCHECKED_REASON_TEXT: Record = { 'no-tag': 'the reference names no tag to check.', 'no-registry': 'the repository names no registry host.', - 'missing-credential': 'the registry requires credentials this extension cannot supply yet.', + 'needs-login': 'no local Docker credential for that registry. Run `docker login` against it.', + 'authentication-failure': 'the registry refused the local Docker credential for it.', 'network-error': 'the registry could not be reached.', 'unexpected-response': 'the registry answered in a form this extension does not understand.', 'malformed-reference': 'the tag is not a valid OCI tag.', diff --git a/apps/vscode/src/login-prompts.test.ts b/apps/vscode/src/login-prompts.test.ts new file mode 100644 index 0000000..e60e8b1 --- /dev/null +++ b/apps/vscode/src/login-prompts.test.ts @@ -0,0 +1,116 @@ +import * as vscode from 'vscode'; +import { afterEach, describe, expect, it } from 'vitest'; +import { createFakeContext } from '../test/fake-context'; +import { getLastStatusBarItem, getLastTerminal, setWarningMessageAnswer, window } from '../test/vscode-stub'; +import { createLoginPrompts, DISMISSED_REGISTRIES_KEY } from './login-prompts'; + +const REGISTRY = 'private.example.com'; +const OTHER_REGISTRY = 'other.example.com'; +const LOG_IN_ACTION = 'Log in'; +const DISMISS_ACTION = 'Never for this registry'; + +/** Every argument `showWarningMessage` was called with, flattened to the registry each notification named. */ +function notifiedMessages(): string[] { + return window.showWarningMessage.mock.calls.map(([message]) => String(message)); +} + +describe('login-prompts', () => { + afterEach(() => { + // The stub's staged answer is module-level state. Leaving one test's + // choice in place would answer the next test's notification for it. + setWarningMessageAnswer(undefined); + window.showWarningMessage.mockClear(); + }); + + it('should notify once per registry, however many references and files report it', async () => { + const prompts = createLoginPrompts(createFakeContext().globalState); + + await prompts.report([REGISTRY, REGISTRY]); + await prompts.report([REGISTRY]); + await prompts.report([REGISTRY, OTHER_REGISTRY]); + + expect(notifiedMessages()).toHaveLength(2); + expect(notifiedMessages()[0]).toContain(REGISTRY); + expect(notifiedMessages()[1]).toContain(OTHER_REGISTRY); + }); + + it('should offer the two actions alongside the message', async () => { + const prompts = createLoginPrompts(createFakeContext().globalState); + + await prompts.report([REGISTRY]); + + expect(window.showWarningMessage).toHaveBeenCalledWith(expect.stringContaining(REGISTRY), LOG_IN_ACTION, DISMISS_ACTION); + }); + + it('should run the login command with the hostname already filled in', async () => { + setWarningMessageAnswer(LOG_IN_ACTION); + + const prompts = createLoginPrompts(createFakeContext().globalState); + + await prompts.report([REGISTRY]); + + expect(getLastTerminal()?.sendText).toHaveBeenCalledWith(`docker login ${REGISTRY}`); + expect(getLastTerminal()?.show).toHaveBeenCalled(); + }); + + it('should persist a dismissal to extension state rather than to settings, and never notify that registry again', async () => { + setWarningMessageAnswer(DISMISS_ACTION); + + const context = createFakeContext(); + const prompts = createLoginPrompts(context.globalState); + + await prompts.report([REGISTRY]); + + expect(context.globalState.get(DISMISSED_REGISTRIES_KEY)).toEqual([REGISTRY]); + + window.showWarningMessage.mockClear(); + await prompts.report([REGISTRY]); + + expect(window.showWarningMessage).not.toHaveBeenCalled(); + }); + + it('should never notify a registry dismissed in an earlier session, which is what surviving a window reload means', async () => { + const context = createFakeContext({ [DISMISSED_REGISTRIES_KEY]: [REGISTRY] }); + const prompts = createLoginPrompts(context.globalState); + + await prompts.report([REGISTRY, OTHER_REGISTRY]); + + expect(notifiedMessages()).toHaveLength(1); + expect(notifiedMessages()[0]).toContain(OTHER_REGISTRY); + }); + + it('should count the registries needing attention in the status bar, and stay hidden until there are any', async () => { + const prompts = createLoginPrompts(createFakeContext().globalState); + const statusBarItem = getLastStatusBarItem(); + + expect(window.createStatusBarItem).toHaveBeenCalledWith(vscode.StatusBarAlignment.Right, expect.any(Number)); + expect(statusBarItem?.visible).toBe(false); + + await prompts.report([REGISTRY, OTHER_REGISTRY]); + + expect(statusBarItem?.visible).toBe(true); + expect(statusBarItem?.text).toContain('2'); + expect(statusBarItem?.tooltip).toContain(REGISTRY); + expect(statusBarItem?.tooltip).toContain(OTHER_REGISTRY); + }); + + it('should drop a dismissed registry from the count, and hide once none are left', async () => { + setWarningMessageAnswer(DISMISS_ACTION); + + const prompts = createLoginPrompts(createFakeContext().globalState); + const statusBarItem = getLastStatusBarItem(); + + await prompts.report([REGISTRY]); + + expect(statusBarItem?.visible).toBe(false); + }); + + it('should dispose its status bar item', () => { + const prompts = createLoginPrompts(createFakeContext().globalState); + const statusBarItem = getLastStatusBarItem(); + + prompts.dispose(); + + expect(statusBarItem?.dispose).toHaveBeenCalled(); + }); +}); diff --git a/apps/vscode/src/login-prompts.ts b/apps/vscode/src/login-prompts.ts new file mode 100644 index 0000000..92fb617 --- /dev/null +++ b/apps/vscode/src/login-prompts.ts @@ -0,0 +1,148 @@ +import * as vscode from 'vscode'; + +// Where the dismissed registries live. In extension state rather than in +// settings, because a dismissal is not configuration: it records that one +// developer stopped caring about one registry, and putting it in settings +// would accumulate that in a file the whole team has checked in. +const DISMISSED_REGISTRIES_KEY = 'infraTools.dismissedLoginRegistries'; + +const LOG_IN_ACTION = 'Log in'; +const DISMISS_ACTION = 'Never for this registry'; + +// Right of the line and column indicator, at the default priority. Nothing +// here competes for a specific slot. +const STATUS_BAR_PRIORITY = 0; + +/** + * What has already been said to the developer about one registry. + * + * Two states rather than a pair of sets, because every rule this surface has + * is a statement about one of them. "At most once per registry per session" + * is that nothing leaves `'prompted'` except into `'dismissed'`. "Never + * again, across window reloads" is that `'dismissed'` is what gets persisted + * and what seeds the map next time. The status bar count is simply how many + * entries are `'prompted'`. + */ +type PromptState = 'prompted' | 'dismissed'; + +interface LoginPrompts { + /** + * Reports the registries a document's checks could not reach for want of a + * login. Never throws and never rejects: it runs inside a document-open + * listener, where an escaping rejection takes the extension host down. + */ + readonly report: (registries: Iterable) => Promise; + readonly dispose: () => void; +} + +function readDismissedRegistries(state: vscode.Memento): string[] { + const stored = state.get(DISMISSED_REGISTRIES_KEY); + + return Array.isArray(stored) ? stored.filter((entry): entry is string => typeof entry === 'string') : []; +} + +/** + * The registries a developer has been told about, and the status bar item + * counting them. + * + * This is the whole surface for a missing credential, and deliberately none + * of it is a diagnostic. Not having logged in to a registry is a fact about + * the developer's machine, not a defect in the values file, and putting it + * in the Problems panel beside real errors is how a panel earns being + * ignored. + */ +function createLoginPrompts(state: vscode.Memento): LoginPrompts { + const promptStates = new Map(readDismissedRegistries(state).map((registry) => [registry, 'dismissed'])); + const statusBarItem = vscode.window.createStatusBarItem(vscode.StatusBarAlignment.Right, STATUS_BAR_PRIORITY); + + function refreshStatusBar(): void { + const waiting = [...promptStates].filter(([, promptState]) => promptState === 'prompted').map(([registry]) => registry); + + if (waiting.length === 0) { + statusBarItem.hide(); + return; + } + + statusBarItem.text = `$(key) ${String(waiting.length)}`; + statusBarItem.tooltip = `Registries needing a Docker login: ${waiting.join(', ')}`; + statusBarItem.show(); + } + + function dismiss(registry: string): Thenable { + promptStates.set(registry, 'dismissed'); + refreshStatusBar(); + + return state.update( + DISMISSED_REGISTRIES_KEY, + [...promptStates].filter(([, promptState]) => promptState === 'dismissed').map(([key]) => key) + ); + } + + function runLogin(registry: string): void { + // A terminal rather than a task or a child process: `docker login` + // prompts for a password, so the developer has to be able to type into + // whatever runs it. + const terminal = vscode.window.createTerminal(`docker login ${registry}`); + + terminal.show(); + terminal.sendText(`docker login ${registry}`); + } + + async function prompt(registry: string): Promise { + const action = await vscode.window.showWarningMessage( + `No Docker credential for \`${registry}\`, so its images cannot be verified.`, + LOG_IN_ACTION, + DISMISS_ACTION + ); + + if (action === LOG_IN_ACTION) { + runLogin(registry); + return; + } + + if (action === DISMISS_ACTION) { + await dismiss(registry); + } + } + + return { + report: async (registries: Iterable): Promise => { + try { + // Deduplicated here as well as by the caller, because `filter` reads + // the whole batch before `map` marks any of it: a registry named + // twice in one batch would otherwise pass the "not prompted yet" + // test twice and raise two notifications for it. + // + // Every notification is raised before the first `await`, so a caller + // that fires this and walks away still gets them all shown. + const pending = [...new Set(registries)] + .filter((registry) => !promptStates.has(registry)) + .map((registry) => { + promptStates.set(registry, 'prompted'); + + return registry; + }); + + if (pending.length === 0) { + return; + } + + refreshStatusBar(); + + // Settled, not raced: a developer who dismisses the second of three + // prompts must have that dismissal persisted before this resolves. + await Promise.allSettled(pending.map(prompt)); + } catch { + // The caller fires this without awaiting it, so anything escaping + // here is an unhandled rejection, and that takes the extension host + // down. A check already in flight when the window closes finds this + // object's status bar item disposed, which is exactly that case. + } + }, + dispose: (): void => { + statusBarItem.dispose(); + }, + }; +} + +export { createLoginPrompts, DISMISSED_REGISTRIES_KEY }; diff --git a/apps/vscode/src/marks.ts b/apps/vscode/src/marks.ts index 9499fcc..547f659 100644 --- a/apps/vscode/src/marks.ts +++ b/apps/vscode/src/marks.ts @@ -93,4 +93,3 @@ function applyMarks( } export { applyMarks, createMarkDecorationType, marksFor }; -export type { MarkKind }; diff --git a/apps/vscode/src/reference-check.test.ts b/apps/vscode/src/reference-check.test.ts index 93ef51a..03887a6 100644 --- a/apps/vscode/src/reference-check.test.ts +++ b/apps/vscode/src/reference-check.test.ts @@ -2,6 +2,7 @@ import type * as vscode from 'vscode'; import { describe, expect, it, vi } from 'vitest'; import type { FetchLike } from 'oci-registry'; import { bumpVersion, createFakeDocument } from '../test/fake-document'; +import { noDockerCredentials } from '../test/fake-credentials'; import { fakeFetchResponse } from '../test/fake-fetch'; import { checkImageReferencesInDocument, checksAsOf, isHelmValuesFile, type DocumentChecks } from './reference-check'; @@ -12,7 +13,7 @@ const TAGLESS_VALUES_YAML = ['image:', ' repository: registry.example.com/svc', /** Checks a document this feature is expected to have something to say about. */ async function checkValuesFile(document: vscode.TextDocument, fetch: FetchLike): Promise { - const checked = await checkImageReferencesInDocument(document, fetch); + const checked = await checkImageReferencesInDocument(document, { fetch, credentials: noDockerCredentials }); if (checked === undefined) { throw new Error('expected the document to be checked'); @@ -27,7 +28,7 @@ describe('reference-check', () => { const document = createFakeDocument('/repo/chart/deployment.yaml', VALUES_YAML); expect(isHelmValuesFile(document)).toBe(false); - await expect(checkImageReferencesInDocument(document, fetch)).resolves.toBeUndefined(); + await expect(checkImageReferencesInDocument(document, { fetch, credentials: noDockerCredentials })).resolves.toBeUndefined(); expect(fetch).not.toHaveBeenCalled(); }); @@ -36,7 +37,7 @@ describe('reference-check', () => { const document = createFakeDocument('/repo/chart/values.yaml', VALUES_YAML, 'plaintext'); expect(isHelmValuesFile(document)).toBe(false); - await expect(checkImageReferencesInDocument(document, fetch)).resolves.toBeUndefined(); + await expect(checkImageReferencesInDocument(document, { fetch, credentials: noDockerCredentials })).resolves.toBeUndefined(); expect(fetch).not.toHaveBeenCalled(); }); diff --git a/apps/vscode/src/reference-check.ts b/apps/vscode/src/reference-check.ts index 17cb046..026ca15 100644 --- a/apps/vscode/src/reference-check.ts +++ b/apps/vscode/src/reference-check.ts @@ -1,6 +1,6 @@ import type * as vscode from 'vscode'; import { extractImageReferences, type ImageReference } from 'helm'; -import { checkImageExistence, type FetchLike, type ImageVerdict, type UnverifiableReason } from 'oci-registry'; +import { checkImageExistence, type CredentialEnvironment, type FetchLike, type ImageVerdict, type UnverifiableReason } from 'oci-registry'; // Matching any YAML file beneath a chart directory, and excluding that // chart's templates directory, is Helm chart-context knowledge this ticket @@ -10,7 +10,15 @@ const VALUES_FILE_NAME_PATTERN = /^values\.ya?ml$/i; /** Every reason the registry package reports, plus the ones settled before asking it. */ type UncheckedReason = UnverifiableReason | 'no-tag'; -type ReferenceVerdict = Exclude | { readonly kind: 'unverifiable'; readonly reason: UncheckedReason }; +/** + * A registry verdict, or the one outcome the extension settles itself. + * + * `ImageVerdict` is taken whole rather than picked apart, so the registry + * package's own split — only `'needs-login'` carries a host — survives the + * trip to the UI instead of being flattened into an optional field the + * notification code would then have to re-check. + */ +type ReferenceVerdict = ImageVerdict | { readonly kind: 'unverifiable'; readonly reason: 'no-tag' }; /** * One checked reference, projected onto all three surfaces, so diagnostics, @@ -43,11 +51,17 @@ function isHelmValuesFile(document: vscode.TextDocument): boolean { * checked document that produced no findings: the caller replaces a * document's diagnostics and marks only when it gets checks back. */ -async function checkImageReferencesInDocument(document: vscode.TextDocument, fetch: FetchLike): Promise { +interface CheckDependencies { + readonly fetch: FetchLike; + readonly credentials: CredentialEnvironment; +} + +async function checkImageReferencesInDocument(document: vscode.TextDocument, dependencies: CheckDependencies): Promise { if (!isHelmValuesFile(document)) { return undefined; } + const { fetch, credentials } = dependencies; const version = document.version; // No guard around this: the extractor collects YAML syntax errors rather @@ -64,7 +78,7 @@ async function checkImageReferencesInDocument(document: vscode.TextDocument, fet verdict: reference.tag === undefined ? ({ kind: 'unverifiable', reason: 'no-tag' } as const) - : await checkImageExistence({ repository: reference.repository.text, tag: reference.tag.text, fetch }), + : await checkImageExistence({ repository: reference.repository.text, tag: reference.tag.text, fetch, credentials }), })) ); @@ -80,5 +94,25 @@ function checksAsOf(checked: DocumentChecks, document: vscode.TextDocument): rea return checked.version === document.version ? checked.checks : []; } -export { checkImageReferencesInDocument, checksAsOf, isHelmValuesFile }; +/** + * The distinct registries a document's checks could not reach for want of a + * login. + * + * Deduplicated here rather than by the caller, because a values file naming + * ten images on one private registry is the ordinary case, and ten identical + * prompts for it would be the feature's worst behaviour. + */ +function registriesNeedingLogin(checks: readonly ReferenceCheck[]): string[] { + const registries = new Set(); + + for (const { verdict } of checks) { + if (verdict.kind === 'unverifiable' && verdict.reason === 'needs-login') { + registries.add(verdict.registry); + } + } + + return [...registries]; +} + +export { checkImageReferencesInDocument, checksAsOf, isHelmValuesFile, registriesNeedingLogin }; export type { DocumentChecks, ReferenceCheck, ReferenceVerdict, UncheckedReason }; diff --git a/apps/vscode/test/fake-context.ts b/apps/vscode/test/fake-context.ts new file mode 100644 index 0000000..56a0984 --- /dev/null +++ b/apps/vscode/test/fake-context.ts @@ -0,0 +1,30 @@ +import type * as vscode from 'vscode'; + +/** + * Builds a fake `vscode.ExtensionContext` with a working in-memory + * `globalState`. + * + * The state is real rather than a spy, because what the tests need to assert + * is that a value written in one activation is read back by the next — which + * is what "a dismissed registry never notifies again across window reloads" + * means. Seeding `stored` is how a test stages a previous session. + */ +function createFakeContext(stored: Readonly> = {}): vscode.ExtensionContext { + const values = new Map(Object.entries(stored)); + + return { + subscriptions: [], + globalState: { + get: (key: string, fallback?: unknown) => values.get(key) ?? fallback, + // eslint-disable-next-line @typescript-eslint/promise-function-async -- an in-memory write, nothing to await + update: (key: string, value: unknown) => { + values.set(key, value); + + return Promise.resolve(); + }, + keys: () => [...values.keys()], + }, + } as unknown as vscode.ExtensionContext; +} + +export { createFakeContext }; diff --git a/apps/vscode/test/fake-credentials.ts b/apps/vscode/test/fake-credentials.ts new file mode 100644 index 0000000..6079e80 --- /dev/null +++ b/apps/vscode/test/fake-credentials.ts @@ -0,0 +1,18 @@ +import type { CredentialEnvironment } from 'oci-registry'; + +/** + * A machine that has never run `docker login`. + * + * The extension's tests assert wiring, not credential resolution — that + * lives behind the registry package's own entry point and is tested there — + * so they all want the same empty environment, and reaching a registry at + * all means naming one on that package's public list. + */ +const noDockerCredentials: CredentialEnvironment = { + /* eslint-disable @typescript-eslint/promise-function-async -- canned answers, nothing to await */ + readDockerConfig: () => Promise.resolve(undefined), + runCredentialHelper: () => Promise.reject(new Error('no credential helper is installed')), + /* eslint-enable @typescript-eslint/promise-function-async */ +}; + +export { noDockerCredentials }; diff --git a/apps/vscode/test/fake-fetch.ts b/apps/vscode/test/fake-fetch.ts index bfad001..5632a2e 100644 --- a/apps/vscode/test/fake-fetch.ts +++ b/apps/vscode/test/fake-fetch.ts @@ -2,10 +2,16 @@ const SUCCESS_STATUS_START = 200; const SUCCESS_STATUS_END = 300; /** A canned fetch `Response`-shaped object for the injected fetch fake. */ -function fakeFetchResponse(status: number, body: unknown = {}): { status: number; ok: boolean; json: () => Promise } { +function fakeFetchResponse( + status: number, + body: unknown = {} +): { status: number; ok: boolean; headers: { get: () => string | null }; json: () => Promise } { return { status, ok: status >= SUCCESS_STATUS_START && status < SUCCESS_STATUS_END, + // These tests never drive an auth challenge; the registry package's own + // suite is where the `WWW-Authenticate` flow is exercised. + headers: { get: () => null }, // eslint-disable-next-line @typescript-eslint/promise-function-async -- trivial canned response, nothing to await json: () => Promise.resolve(body), }; diff --git a/apps/vscode/test/vscode-stub.ts b/apps/vscode/test/vscode-stub.ts index f8ec3c5..6195326 100644 --- a/apps/vscode/test/vscode-stub.ts +++ b/apps/vscode/test/vscode-stub.ts @@ -176,16 +176,113 @@ function createEventEmitterStub(): { const onDidOpenTextDocumentEmitter = createEventEmitterStub(); const onDidChangeVisibleTextEditorsEmitter = createEventEmitterStub(); +// Mirrors the real `vscode.StatusBarAlignment` member names and values +// exactly, for the same reason `DiagnosticSeverity` above does: extension +// code reads them off the real `@types/vscode` declaration. +/* eslint-disable @typescript-eslint/naming-convention, @typescript-eslint/no-magic-numbers -- + mirrors vscode.StatusBarAlignment's real member names and fixed values */ +enum StatusBarAlignment { + Left = 1, + Right = 2, +} +/* eslint-enable @typescript-eslint/naming-convention, @typescript-eslint/no-magic-numbers */ + +interface StatusBarItemStub { + text: string; + tooltip: string | undefined; + readonly alignment: StatusBarAlignment; + readonly priority: number | undefined; + readonly show: ReturnType; + readonly hide: ReturnType; + readonly dispose: ReturnType; + /** Test-only: whether the last call was `show` rather than `hide`. Not part of the real API. */ + visible: boolean; +} + +const statusBarItems: StatusBarItemStub[] = []; + +function createStatusBarItemStub(alignment: StatusBarAlignment, priority: number | undefined): StatusBarItemStub { + const item: StatusBarItemStub = { + text: '', + tooltip: undefined, + alignment, + priority, + visible: false, + show: vi.fn(() => { + item.visible = true; + }), + hide: vi.fn(() => { + item.visible = false; + }), + dispose: vi.fn(), + }; + + statusBarItems.push(item); + + return item; +} + +interface TerminalStub { + readonly name: string; + readonly show: ReturnType; + readonly sendText: ReturnType; + readonly dispose: ReturnType; +} + +const terminals: TerminalStub[] = []; + +function createTerminalStub(name: string): TerminalStub { + const terminal: TerminalStub = { name, show: vi.fn(), sendText: vi.fn(), dispose: vi.fn() }; + + terminals.push(terminal); + + return terminal; +} + +// Which notification action a test has staged the developer as choosing. +// `undefined` is the real default: a warning the developer ignores resolves +// to `undefined`, not to a rejection. +let warningMessageAnswer: string | undefined; + const window = { createOutputChannel: vi.fn(() => ({ appendLine: vi.fn(), dispose: vi.fn(), })), createTextEditorDecorationType: vi.fn(() => ({ key: 'decoration-type', dispose: vi.fn() })), + createStatusBarItem: vi.fn((alignment: StatusBarAlignment, priority?: number) => createStatusBarItemStub(alignment, priority)), + createTerminal: vi.fn((name: string) => createTerminalStub(name)), + // Real VS Code can only hand back an action that was offered, so the + // staged answer is filtered through the offered list rather than returned + // blindly — a test that stages an action nobody offers gets the + // `undefined` a dismissed notification really produces. + // eslint-disable-next-line @typescript-eslint/promise-function-async -- mirrors the real Thenable-returning signature + showWarningMessage: vi.fn((message: string, ...actions: string[]) => + Promise.resolve(warningMessageAnswer !== undefined && actions.includes(warningMessageAnswer) ? warningMessageAnswer : undefined) + ), visibleTextEditors: [] as readonly TextEditorStub[], onDidChangeVisibleTextEditors: onDidChangeVisibleTextEditorsEmitter.event, }; +/** + * Test-only helper staging which notification action the developer picks. + * Not part of the real `vscode` API. Reset it between tests, or one test's + * choice answers the next test's notification. + */ +function setWarningMessageAnswer(answer: string | undefined): void { + warningMessageAnswer = answer; +} + +/** Test-only helper: the most recently created status bar item. */ +function getLastStatusBarItem(): StatusBarItemStub | undefined { + return statusBarItems[statusBarItems.length - 1]; +} + +/** Test-only helper: the most recently created terminal. */ +function getLastTerminal(): TerminalStub | undefined { + return terminals[terminals.length - 1]; +} + const workspace = { onDidOpenTextDocument: onDidOpenTextDocumentEmitter.event, }; @@ -217,7 +314,7 @@ async function emitDidChangeVisibleTextEditors(editors: readonly TextEditorStub[ await onDidChangeVisibleTextEditorsEmitter.fire(editors); } -export type { TextEditorStub }; +export type { StatusBarItemStub, TerminalStub, TextEditorStub }; export { createTextEditorStub, Diagnostic, @@ -225,6 +322,8 @@ export { emitDidChangeVisibleTextEditors, emitDidOpenTextDocument, getLastDiagnosticCollection, + getLastStatusBarItem, + getLastTerminal, getRegisteredHoverProvider, Hover, languages, @@ -232,6 +331,8 @@ export { Position, Range, setVisibleTextEditors, + setWarningMessageAnswer, + StatusBarAlignment, ThemeColor, window, workspace, From a1f6e15b5b35c739ff32b1009a3e6ec87e3625d1 Mon Sep 17 00:00:00 2001 From: Schnitz <12687466+CptSchnitz@users.noreply.github.com> Date: Wed, 16 Sep 2026 14:23:44 +0300 Subject: [PATCH 5/6] fix(oci-registry): ask Docker Hub's real API, not the host that redirects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A manifest `GET` against `docker.io` is redirected to the marketing site, which answers `200`. `fetch` follows the redirect, so every Docker Hub image came back as existing — including tags that do not. Verified against the live hosts: https://docker.io/v2/library/nginx/manifests/no-such-tag-xyz -> 302 -> https://www.docker.com/ -> 200 https://registry-1.docker.io/v2/library/nginx/manifests/no-such-tag-xyz -> 401 A false checkmark is the one wrong answer this feature cannot survive. The whole point of marking an image verified is that the mark means something, and one that appears for every Hub reference means nothing at all — worse than staying quiet, because it is silent and it looks like success. Requests now go to `registry-1.docker.io`, which serves the distribution API and challenges properly, while the verdict keeps naming the registry the file named. Those are deliberately two different values: the mark appends a registry only when it differs from the file's, so reporting the endpoint would append `registry-1.docker.io` to every Hub image on screen. The test pins both halves separately, because conflating them is how this comes back. Hub's three names now live in one module. They were already duplicated between credential lookup and this fix, and no two of them are interchangeable: a repository names `docker.io`, `docker login` writes the credential under `https://index.docker.io/v1/`, and only `registry-1.docker.io` answers `/v2`. This predates the branch — it arrived with the first existence check in 4c44c70 — but the branch is what put Docker Hub on the anonymous-request list and taught the credential chain to find its login, so it is the branch that made the claim this breaks. Refs #22 Co-Authored-By: Claude Opus 5 (1M context) --- .../src/check-image-existence.test.ts | 79 ++++++++++++------- .../oci-registry/src/check-image-existence.ts | 7 +- packages/oci-registry/src/credentials.ts | 31 +------- packages/oci-registry/src/docker-hub.ts | 43 ++++++++++ 4 files changed, 103 insertions(+), 57 deletions(-) create mode 100644 packages/oci-registry/src/docker-hub.ts diff --git a/packages/oci-registry/src/check-image-existence.test.ts b/packages/oci-registry/src/check-image-existence.test.ts index 815e555..0ddf8b0 100644 --- a/packages/oci-registry/src/check-image-existence.test.ts +++ b/packages/oci-registry/src/check-image-existence.test.ts @@ -42,7 +42,7 @@ interface DockerCredentialsParams { readonly runCredentialHelper?: CredentialEnvironment['runCredentialHelper']; } -function dockerCredentials(params: DockerCredentialsParams = {}): CredentialEnvironment { +function fakeDockerCredentials(params: DockerCredentialsParams = {}): CredentialEnvironment { const { config, configText, runCredentialHelper } = params; const contents = configText ?? (config === undefined ? undefined : JSON.stringify(config)); @@ -94,17 +94,38 @@ describe('checkImageExistence', () => { repository: 'docker.io/library/nginx', tag: '1.19', fetch, - credentials: dockerCredentials(), + credentials: fakeDockerCredentials(), }); expect(fetch).toHaveBeenCalledTimes(1); - expect(fetch).toHaveBeenCalledWith('https://docker.io/v2/library/nginx/manifests/1.19', { + expect(fetch).toHaveBeenCalledWith('https://registry-1.docker.io/v2/library/nginx/manifests/1.19', { method: 'GET', headers: { accept: MANIFEST_ACCEPT_HEADER }, }); expect(verdict).toEqual({ kind: 'exists', registry: 'docker.io' }); }); + it('should send a docker.io reference to the host that serves the API, while still reporting docker.io', async () => { + const fetch = vi.fn().mockResolvedValue(fakeFetchResponse({ status: 200, body: { schemaVersion: 2 } })); + + const verdict = await checkImageExistence({ + repository: 'docker.io/library/nginx', + tag: '1.19', + fetch, + credentials: fakeDockerCredentials(), + }); + + // `docker.io` redirects a manifest GET to the marketing site, which + // answers 200. Requesting it would report every Hub image as existing, + // missing tags included — the one wrong answer a checkmark cannot + // survive. Only `registry-1.docker.io` serves the distribution API. + expect(requestAt(fetch.mock.calls, 0).url).toBe('https://registry-1.docker.io/v2/library/nginx/manifests/1.19'); + + // The verdict names the registry the file did, so the mark stays quiet + // instead of appending an endpoint nobody wrote down. + expect(verdict).toEqual({ kind: 'exists', registry: 'docker.io' }); + }); + it('should report repository-not-found on a 404 whose body carries NAME_UNKNOWN', async () => { const fetch = vi.fn().mockResolvedValue(fakeFetchResponse({ status: 404, body: distributionError('NAME_UNKNOWN') })); @@ -112,7 +133,7 @@ describe('checkImageExistence', () => { repository: 'ghcr.io/example/does-not-exist', tag: '1.0.0', fetch, - credentials: dockerCredentials(), + credentials: fakeDockerCredentials(), }); expect(verdict).toEqual({ kind: 'repository-not-found', repository: 'ghcr.io/example/does-not-exist' }); @@ -125,7 +146,7 @@ describe('checkImageExistence', () => { repository: 'docker.io/library/nginx', tag: 'does-not-exist', fetch, - credentials: dockerCredentials(), + credentials: fakeDockerCredentials(), }); expect(verdict).toEqual({ @@ -142,7 +163,7 @@ describe('checkImageExistence', () => { repository: 'docker.io/library/nginx', tag: '1.19', fetch, - credentials: dockerCredentials(), + credentials: fakeDockerCredentials(), }); expect(verdict).toEqual({ kind: 'unverifiable', reason: 'unexpected-response' }); @@ -155,7 +176,7 @@ describe('checkImageExistence', () => { repository: 'private.example.com/app', tag: '1.0.0', fetch, - credentials: dockerCredentials(), + credentials: fakeDockerCredentials(), }); expect(fetch).not.toHaveBeenCalled(); @@ -169,7 +190,7 @@ describe('checkImageExistence', () => { repository: 'ghcr.io/example/app', tag: '1.0.0', fetch, - credentials: dockerCredentials(), + credentials: fakeDockerCredentials(), }); expect(verdict).toEqual({ kind: 'unverifiable', reason: 'network-error' }); @@ -182,7 +203,7 @@ describe('checkImageExistence', () => { repository: 'docker.io/library/nginx', tag: '1.19', fetch, - credentials: dockerCredentials(), + credentials: fakeDockerCredentials(), }); expect(verdict).toEqual({ kind: 'unverifiable', reason: 'unexpected-response' }); @@ -191,7 +212,7 @@ describe('checkImageExistence', () => { it('should report unverifiable without issuing a request when the repository names no explicit host', async () => { const fetch = vi.fn(); - const verdict = await checkImageExistence({ repository: 'nginx', tag: 'latest', fetch, credentials: dockerCredentials() }); + const verdict = await checkImageExistence({ repository: 'nginx', tag: 'latest', fetch, credentials: fakeDockerCredentials() }); expect(fetch).not.toHaveBeenCalled(); expect(verdict).toEqual({ kind: 'unverifiable', reason: 'no-registry' }); @@ -204,7 +225,7 @@ describe('checkImageExistence', () => { repository: 'docker.io@evil.example/library/nginx', tag: '1.19', fetch, - credentials: dockerCredentials(), + credentials: fakeDockerCredentials(), }); expect(fetch).not.toHaveBeenCalled(); @@ -218,7 +239,7 @@ describe('checkImageExistence', () => { repository: 'docker.io/../secrets', tag: '1.19', fetch, - credentials: dockerCredentials(), + credentials: fakeDockerCredentials(), }); expect(fetch).not.toHaveBeenCalled(); @@ -232,7 +253,7 @@ describe('checkImageExistence', () => { repository: 'docker.io/library/nginx', tag: '../../other', fetch, - credentials: dockerCredentials(), + credentials: fakeDockerCredentials(), }); expect(fetch).not.toHaveBeenCalled(); @@ -256,7 +277,7 @@ describe('checkImageExistence', () => { repository: 'private.example.com/app', tag: '1.0.0', fetch, - credentials: dockerCredentials({ config: { auths: { 'private.example.com': { auth: encodeAuth('dev', 's3cret') } } } }), + credentials: fakeDockerCredentials({ config: { auths: { 'private.example.com': { auth: encodeAuth('dev', 's3cret') } } } }), }); const token = requestAt(fetch.mock.calls, 1); @@ -295,7 +316,7 @@ describe('checkImageExistence', () => { repository: 'private.example.com/app', tag: '1.0.0', fetch, - credentials: dockerCredentials({ + credentials: fakeDockerCredentials({ config: { credsStore: 'desktop', auths: { 'https://private.example.com': {} } }, runCredentialHelper, }), @@ -330,7 +351,7 @@ describe('checkImageExistence', () => { repository: 'private.example.com/app', tag: '1.0.0', fetch, - credentials: dockerCredentials({ + credentials: fakeDockerCredentials({ config: { credsStore: 'desktop', credHelpers: { 'private.example.com': 'acr-env' } }, runCredentialHelper, }), @@ -364,7 +385,7 @@ describe('checkImageExistence', () => { repository: 'private.example.com/app', tag: '1.0.0', fetch, - credentials: dockerCredentials({ + credentials: fakeDockerCredentials({ config: { credsStore: 'desktop', credHelpers: { 'private.example.com': 'acr-env' }, @@ -407,7 +428,7 @@ describe('checkImageExistence', () => { repository: 'myorg.azurecr.io/app', tag: '1.0.0', fetch, - credentials: dockerCredentials({ + credentials: fakeDockerCredentials({ config: { auths: { 'myorg.azurecr.io': { @@ -455,7 +476,7 @@ describe('checkImageExistence', () => { repository: 'ghcr.io/example/app', tag: '1.0.0', fetch, - credentials: dockerCredentials(), + credentials: fakeDockerCredentials(), }); expect(requestAt(fetch.mock.calls, 1).init.headers).toEqual({}); @@ -483,7 +504,7 @@ describe('checkImageExistence', () => { repository: 'private.example.com/app', tag: '1.0.0', fetch, - credentials: dockerCredentials({ config: { auths: { 'private.example.com': { auth: encodeAuth('dev', 'expired') } } } }), + credentials: fakeDockerCredentials({ config: { auths: { 'private.example.com': { auth: encodeAuth('dev', 'expired') } } } }), }); expect(verdict).toEqual({ kind: 'unverifiable', reason: 'authentication-failure' }); @@ -496,7 +517,7 @@ describe('checkImageExistence', () => { repository: 'ghcr.io/example/app', tag: '1.0.0', fetch, - credentials: dockerCredentials(), + credentials: fakeDockerCredentials(), }); expect(verdict).toEqual({ kind: 'unverifiable', reason: 'authentication-failure' }); @@ -515,7 +536,7 @@ describe('checkImageExistence', () => { repository: 'private.example.com/app', tag: '1.0.0', fetch, - credentials: dockerCredentials({ config: { auths: { 'private.example.com': { auth: encodeAuth('dev', 's3cret') } } } }), + credentials: fakeDockerCredentials({ config: { auths: { 'private.example.com': { auth: encodeAuth('dev', 's3cret') } } } }), }); // Whoever answered the manifest request chose that realm, and the @@ -537,7 +558,7 @@ describe('checkImageExistence', () => { repository: 'localhost:5000/app', tag: '1.0.0', fetch, - credentials: dockerCredentials({ config: { auths: { 'localhost:5000': { auth: encodeAuth('dev', 's3cret') } } } }), + credentials: fakeDockerCredentials({ config: { auths: { 'localhost:5000': { auth: encodeAuth('dev', 's3cret') } } } }), }); expect(requestAt(fetch.mock.calls, 1).url).toBe('http://localhost:5000/token?service=localhost%3A5000&scope=repository%3Aapp%3Apull'); @@ -564,7 +585,7 @@ describe('checkImageExistence', () => { repository: 'private.example.com/app', tag: '1.0.0', fetch, - credentials: dockerCredentials({ + credentials: fakeDockerCredentials({ config: { credsStore: 'desktop', auths: { 'private.example.com': { auth: encodeAuth('dev', 's3cret') } } }, runCredentialHelper, }), @@ -592,7 +613,7 @@ describe('checkImageExistence', () => { repository: 'docker.io/library/nginx', tag: '1.19', fetch, - credentials: dockerCredentials({ + credentials: fakeDockerCredentials({ config: { auths: { 'https://index.docker.io/v1/': { auth: encodeAuth('hub-user', 'hub-secret') } } }, }), }); @@ -630,7 +651,7 @@ describe('checkImageExistence', () => { repository: 'myorg.azurecr.io/app', tag: '1.0.0', fetch, - credentials: dockerCredentials({ + credentials: fakeDockerCredentials({ config: { auths: { 'myorg.azurecr.io': { auth: encodeAuth('stale-user', 'stale-password'), identitytoken: 'refresh-token-value' }, @@ -657,7 +678,7 @@ describe('checkImageExistence', () => { repository: 'private.example.com/app', tag: '1.0.0', fetch, - credentials: dockerCredentials({ config: { auths: { 'private.example.com': { auth: encodeAuth('dev', 's3cret') } } } }), + credentials: fakeDockerCredentials({ config: { auths: { 'private.example.com': { auth: encodeAuth('dev', 's3cret') } } } }), }); expect(fetch).toHaveBeenCalledTimes(2); @@ -694,7 +715,7 @@ describe('checkImageExistence', () => { repository: 'private.example.com/app', tag: '1.0.0', fetch, - credentials: dockerCredentials({ config: { credHelpers: { 'private.example.com': 'acr-env' } }, runCredentialHelper }), + credentials: fakeDockerCredentials({ config: { credHelpers: { 'private.example.com': 'acr-env' } }, runCredentialHelper }), }); const token = requestAt(fetch.mock.calls, 1); @@ -716,7 +737,7 @@ describe('checkImageExistence', () => { repository: 'private.example.com/app', tag: '1.0.0', fetch, - credentials: dockerCredentials({ + credentials: fakeDockerCredentials({ configText: `{ "auths": { "private.example.com": { "auth": "${encodeAuth('dev', 's3cret')}" }, } }`, }), }); diff --git a/packages/oci-registry/src/check-image-existence.ts b/packages/oci-registry/src/check-image-existence.ts index 86ab0a8..dfb9946 100644 --- a/packages/oci-registry/src/check-image-existence.ts +++ b/packages/oci-registry/src/check-image-existence.ts @@ -1,5 +1,6 @@ import { acquireBearerToken, basicAuthorizationHeader, parseAuthenticateChallenge } from './authorize'; import type { CredentialEnvironment, RegistryCredential } from './credentials'; +import { registryEndpoint } from './docker-hub'; import { isPublicRegistry, resolveRegistryCredential } from './credentials'; import type { FetchLike, FetchResponseLike } from './fetch-like'; import { resolveExplicitHost } from './resolve-explicit-host'; @@ -73,7 +74,11 @@ async function checkImageExistence(params: CheckImageExistenceParams): Promise[]; } -function isRecord(value: unknown): value is Record { - return typeof value === 'object' && value !== null; -} - -function readStringField(source: unknown, field: string): string | undefined { - if (!isRecord(source)) { - return undefined; - } - - const value = source[field]; - return typeof value === 'string' ? value : undefined; -} - /** * Reduces a config key or a registry host to the bare `host[:port]` the two * can be compared on. @@ -123,7 +100,7 @@ function readStringField(source: unknown, field: string): string | undefined { */ function normalizeConfigKey(key: string): string { const [hostPort = ''] = key.replace(SCHEME_PREFIX_PATTERN, '').split('/'); - return DOCKER_HUB_ALIASES.has(hostPort) ? DOCKER_HUB_HOST : hostPort; + return isDockerHub(hostPort) ? DOCKER_HUB_HOST : hostPort; } function parseAuthEntry(value: unknown): DockerAuthEntry | undefined { diff --git a/packages/oci-registry/src/docker-hub.ts b/packages/oci-registry/src/docker-hub.ts new file mode 100644 index 0000000..8beb8f7 --- /dev/null +++ b/packages/oci-registry/src/docker-hub.ts @@ -0,0 +1,43 @@ +/** + * The names Docker Hub answers to, in one place because no two of them are + * interchangeable and every one of them is load-bearing somewhere. + * + * `docker.io` is what a repository string names. `docker login` writes the + * credential under `https://index.docker.io/v1/`. And only + * `registry-1.docker.io` actually serves the distribution API: a manifest + * `GET` against `docker.io` is redirected to the marketing site, which + * answers `200`, so a checker that trusts `response.ok` reports every Hub + * image as existing — including tags that do not. That failure is silent + * and it is a false positive, which is the one kind of wrong answer a + * checkmark cannot survive. + */ + +const DOCKER_HUB_HOST = 'docker.io'; + +/** Where Hub's distribution API actually lives. */ +const DOCKER_HUB_ENDPOINT = 'registry-1.docker.io'; + +// The key `docker login` writes Docker Hub under, and therefore the server +// URL a credential helper expects to be asked about for Docker Hub. The +// bare host would be a cache miss in every helper a real developer has. +const DOCKER_HUB_SERVER_URL = 'https://index.docker.io/v1/'; + +const DOCKER_HUB_ALIASES = new Set([DOCKER_HUB_HOST, 'index.docker.io', DOCKER_HUB_ENDPOINT]); + +function isDockerHub(host: string): boolean { + return DOCKER_HUB_ALIASES.has(host); +} + +/** + * The host that serves the distribution API for a registry named `host`. + * + * Only Docker Hub needs the translation. Every other registry serves its + * own API from the host a repository names it by, and inventing an endpoint + * for one would mean sending a credential somewhere the file never asked + * for. + */ +function registryEndpoint(host: string): string { + return isDockerHub(host) ? DOCKER_HUB_ENDPOINT : host; +} + +export { DOCKER_HUB_HOST, DOCKER_HUB_SERVER_URL, isDockerHub, registryEndpoint }; From fa077e480e65e04e99150ce0119cdbc06b75311b Mon Sep 17 00:00:00 2001 From: Schnitz <12687466+CptSchnitz@users.noreply.github.com> Date: Wed, 16 Sep 2026 14:24:01 +0300 Subject: [PATCH 6/6] refactor(oci-registry): fold the review findings from the two-axis pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Non-behavioural cleanups from reviewing the branch, grouped so the feature commits stay readable. `isRecord` was byte-identical in `authorize.ts` and `credentials.ts`, and the same reader lived under two names — `readStringProperty` and `readStringField`. Both now come from one module, which is also the place to say once why these readers never throw: the values they parse are a config file on the developer's machine and a token endpoint's response, neither of which this package controls, and neither of which is worth an exception when it comes back the wrong shape. `RegistryCredential`, `UnverifiableVerdict` and `CheckDependencies` stopped being exported. Nothing outside their own files names them, and knip does not flag exports from a package entry point, so nothing would have caught it later. `verdict.ts` moves to a footer export block, matching the rest of the package. `local-docker-credentials.ts` gains the tests it never had. It is the only file here that really touches the disk and the process table, and the helper-name guard in it is a security boundary — the name comes out of a config file and is interpolated into a command name — so it had no business being the one untested thing. The tests cover `DOCKER_CONFIG` resolution, an absent config reading as "no config" rather than an error, the guard refusing `a; rm -rf /`, and a missing helper rejecting so the chain reads it as a miss. In `login-prompts.ts`, one `registriesIn(wanted)` replaces the two copies of the same filter-and-map. Its parameter is `wanted`, not `state`, because `state` already means the `Memento` in that closure. In `reference-check.ts`, the doc comment for `checkImageReferencesInDocument` had ended up above the `CheckDependencies` interface introduced beneath it, documenting the wrong symbol. Refs #22 Co-Authored-By: Claude Opus 5 (1M context) --- apps/vscode/src/login-prompts.ts | 11 ++-- apps/vscode/src/reference-check.ts | 10 ++-- packages/oci-registry/src/authorize.ts | 18 ++----- packages/oci-registry/src/index.ts | 4 +- .../src/local-docker-credentials.test.ts | 54 +++++++++++++++++++ packages/oci-registry/src/read-json.ts | 27 ++++++++++ packages/oci-registry/src/verdict.ts | 8 +-- 7 files changed, 102 insertions(+), 30 deletions(-) create mode 100644 packages/oci-registry/src/local-docker-credentials.test.ts create mode 100644 packages/oci-registry/src/read-json.ts diff --git a/apps/vscode/src/login-prompts.ts b/apps/vscode/src/login-prompts.ts index 92fb617..ec5eec5 100644 --- a/apps/vscode/src/login-prompts.ts +++ b/apps/vscode/src/login-prompts.ts @@ -55,8 +55,12 @@ function createLoginPrompts(state: vscode.Memento): LoginPrompts { const promptStates = new Map(readDismissedRegistries(state).map((registry) => [registry, 'dismissed'])); const statusBarItem = vscode.window.createStatusBarItem(vscode.StatusBarAlignment.Right, STATUS_BAR_PRIORITY); + function registriesIn(wanted: PromptState): string[] { + return [...promptStates].filter(([, promptState]) => promptState === wanted).map(([registry]) => registry); + } + function refreshStatusBar(): void { - const waiting = [...promptStates].filter(([, promptState]) => promptState === 'prompted').map(([registry]) => registry); + const waiting = registriesIn('prompted'); if (waiting.length === 0) { statusBarItem.hide(); @@ -72,10 +76,7 @@ function createLoginPrompts(state: vscode.Memento): LoginPrompts { promptStates.set(registry, 'dismissed'); refreshStatusBar(); - return state.update( - DISMISSED_REGISTRIES_KEY, - [...promptStates].filter(([, promptState]) => promptState === 'dismissed').map(([key]) => key) - ); + return state.update(DISMISSED_REGISTRIES_KEY, registriesIn('dismissed')); } function runLogin(registry: string): void { diff --git a/apps/vscode/src/reference-check.ts b/apps/vscode/src/reference-check.ts index 026ca15..ddab886 100644 --- a/apps/vscode/src/reference-check.ts +++ b/apps/vscode/src/reference-check.ts @@ -45,17 +45,17 @@ function isHelmValuesFile(document: vscode.TextDocument): boolean { return VALUES_FILE_NAME_PATTERN.test(fileName); } +interface CheckDependencies { + readonly fetch: FetchLike; + readonly credentials: CredentialEnvironment; +} + /** * Checks a document's image references. `undefined` means this feature has * nothing to say about the document at all, which is not the same as a * checked document that produced no findings: the caller replaces a * document's diagnostics and marks only when it gets checks back. */ -interface CheckDependencies { - readonly fetch: FetchLike; - readonly credentials: CredentialEnvironment; -} - async function checkImageReferencesInDocument(document: vscode.TextDocument, dependencies: CheckDependencies): Promise { if (!isHelmValuesFile(document)) { return undefined; diff --git a/packages/oci-registry/src/authorize.ts b/packages/oci-registry/src/authorize.ts index fa406f7..62bbc18 100644 --- a/packages/oci-registry/src/authorize.ts +++ b/packages/oci-registry/src/authorize.ts @@ -1,4 +1,5 @@ import type { RegistryCredential } from './credentials'; +import { readStringField } from './read-json'; import type { FetchLike } from './fetch-like'; // Splits a `WWW-Authenticate` value into its scheme and the parameter list @@ -65,19 +66,6 @@ interface BasicTokenRequestParams extends TokenRequestParams { readonly credential: BasicCredential | undefined; } -function isRecord(value: unknown): value is Record { - return typeof value === 'object' && value !== null; -} - -function readStringProperty(source: unknown, property: string): string | undefined { - if (!isRecord(source)) { - return undefined; - } - - const value = source[property]; - return typeof value === 'string' ? value : undefined; -} - /** * Parses a `WWW-Authenticate` header into the challenge it describes. * @@ -193,7 +181,7 @@ async function requestTokenWithRefreshToken(params: RefreshTokenRequestParams): return undefined; } - return readStringProperty(await response.json(), 'access_token'); + return readStringField(await response.json(), 'access_token'); } /** @@ -217,7 +205,7 @@ async function requestTokenWithBasic(params: BasicTokenRequestParams): Promise { + const originalDockerConfig = process.env['DOCKER_CONFIG']; + + afterEach(() => { + if (originalDockerConfig === undefined) { + delete process.env['DOCKER_CONFIG']; + } else { + process.env['DOCKER_CONFIG'] = originalDockerConfig; + } + }); + + it('should read config.json out of DOCKER_CONFIG when that names a directory', async () => { + const directory = await mkdtemp(join(tmpdir(), 'infra-tools-docker-')); + const contents = JSON.stringify({ auths: { 'private.example.com': {} } }); + + await writeFile(join(directory, 'config.json'), contents, 'utf8'); + process.env['DOCKER_CONFIG'] = directory; + + await expect(localDockerCredentials.readDockerConfig()).resolves.toBe(contents); + }); + + it('should report no config rather than throwing when the file is absent', async () => { + // A machine that has never run `docker login` is an ordinary state, not + // an error the check has to survive. + process.env['DOCKER_CONFIG'] = await mkdtemp(join(tmpdir(), 'infra-tools-docker-')); + + await expect(localDockerCredentials.readDockerConfig()).resolves.toBeUndefined(); + }); + + it('should refuse to spawn a credential helper whose name is not a plain identifier', async () => { + // The name comes out of config.json, which any process on the machine + // can write, and it is interpolated into a command name. This is the + // boundary that has to refuse it. + await expect(localDockerCredentials.runCredentialHelper('a; rm -rf /', 'private.example.com')).rejects.toThrow(/refusing/iu); + }); + + it('should reject, rather than resolve empty, when the helper does not exist', async () => { + // A rejection is what the chain reads as a miss, so it falls through to + // the plaintext entry instead of aborting the check. + await expect(localDockerCredentials.runCredentialHelper('infra-tools-no-such-helper', 'private.example.com')).rejects.toThrow(); + }); +}); diff --git a/packages/oci-registry/src/read-json.ts b/packages/oci-registry/src/read-json.ts new file mode 100644 index 0000000..19d1c62 --- /dev/null +++ b/packages/oci-registry/src/read-json.ts @@ -0,0 +1,27 @@ +/** + * Readers for values this package parses but does not control: a + * `config.json` on the developer's machine, and a token endpoint's response + * body. + * + * Neither is guaranteed to be the shape it should be, and neither is worth + * an exception when it isn't — a stray comma in a Docker config is not a + * defect in the chart being checked. So every field comes back `undefined` + * rather than throwing, and the caller decides what an absent field means. + */ + +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null; +} + +/** The named field of `source`, when `source` is an object and the field holds a string. */ +function readStringField(source: unknown, field: string): string | undefined { + if (!isRecord(source)) { + return undefined; + } + + const value = source[field]; + + return typeof value === 'string' ? value : undefined; +} + +export { isRecord, readStringField }; diff --git a/packages/oci-registry/src/verdict.ts b/packages/oci-registry/src/verdict.ts index ef4626f..b78039c 100644 --- a/packages/oci-registry/src/verdict.ts +++ b/packages/oci-registry/src/verdict.ts @@ -8,7 +8,7 @@ * shape: no caller can special-case a *reason* into rendering a diagnostic, * because only the verdict `kind` controls that. */ -export type UnverifiableReason = +type UnverifiableReason = /** The repository names no explicit registry host, and this package does * not yet resolve one any other way (a document-declared registry, a * workspace override set, or the Docker Hub fallback). */ @@ -43,7 +43,7 @@ export type UnverifiableReason = * let a caller reach that reason without one, and no other reason can * pretend to have a host it never established. */ -export type UnverifiableVerdict = +type UnverifiableVerdict = | { readonly kind: 'unverifiable'; readonly reason: 'needs-login'; readonly registry: string } | { readonly kind: 'unverifiable'; readonly reason: Exclude }; @@ -56,8 +56,10 @@ export type UnverifiableVerdict = * an unverifiable verdict produces no diagnostic — is the reason this type * exists in this shape rather than a simpler one. */ -export type ImageVerdict = +type ImageVerdict = | { readonly kind: 'exists'; readonly registry: string } | { readonly kind: 'repository-not-found'; readonly repository: string } | { readonly kind: 'tag-not-found'; readonly repository: string; readonly tag: string } | UnverifiableVerdict; + +export type { ImageVerdict, UnverifiableReason };