From 2911ccfe2bbd8cf618d12b75c8bc9465a954700a Mon Sep 17 00:00:00 2001 From: Anso Date: Fri, 26 Jun 2026 21:06:40 -0400 Subject: [PATCH] fix: request registry tokens with the target repository scope (#1478) * fix: request registry tokens with the target repository scope The image-update detector authenticated to registries by reusing the scope echoed in the registry's GET /v2/ ping. That ping carries no repository context, and ghcr.io answers it with a placeholder scope (repository:user/image:pull), so the token was requested for the wrong repository and rejected. Every ghcr.io-backed image (including lscr.io, which delegates auth to ghcr.io) then failed its manifest lookup and was reported as "Registry unreachable", while Docker Hub and quay.io kept working. Always request a pull scope for the repository being checked rather than the echoed placeholder. Also report the actual failure cause: getRemoteDigestResult now distinguishes an authentication failure, a rate limit (with retry-after), a missing image, a registry error, and a genuinely unreachable registry, instead of collapsing every failure into "Registry unreachable". getRemoteDigest stays a digest-or-null wrapper so the update-preview path is unchanged, and listRegistryTags shares the same token path so it now resolves on ghcr.io/lscr.io too. * fix: neutralize control characters in the registry digest error log The error-path console.error in getRemoteDigestResult interpolated the image ref and the caught error message, both of which originate from compose-authored input. Route them through sanitizeForLog so a crafted image string or upstream error text cannot forge multi-line log entries (log injection). The returned reason and the digest logic are unchanged. --- .../__tests__/image-update-service.test.ts | 40 +++++++ backend/src/__tests__/registry-api.test.ts | 109 +++++++++++++++++- backend/src/services/ImageUpdateService.ts | 9 +- backend/src/services/registry-api.ts | 97 +++++++++++++--- docs/features/auto-update-policies.mdx | 4 +- 5 files changed, 231 insertions(+), 28 deletions(-) diff --git a/backend/src/__tests__/image-update-service.test.ts b/backend/src/__tests__/image-update-service.test.ts index 3fbc8fcf..d624cb99 100644 --- a/backend/src/__tests__/image-update-service.test.ts +++ b/backend/src/__tests__/image-update-service.test.ts @@ -94,6 +94,14 @@ vi.mock('../services/NodeRegistry', () => ({ }, })); +// getRemoteDigestResult is module-scoped inside checkImage; mock it to drive the remote +// outcome while keeping the real parseImageRef / repoDigestMatchesRef. +const { mockGetRemoteDigestResult } = vi.hoisted(() => ({ mockGetRemoteDigestResult: vi.fn() })); +vi.mock('../services/registry-api', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, getRemoteDigestResult: mockGetRemoteDigestResult }; +}); + // ── Re-export internal helpers via the module ───────────────────────── // We need the internal functions. Import the module after mocks are set up. @@ -186,6 +194,38 @@ describe('ImageUpdateService - image ref parsing (via checkImage)', () => { }); }); +// ── checkImage surfaces the remote-digest reason ─────────────────────── + +describe('ImageUpdateService - checkImage surfaces the remote-digest reason', () => { + let service: ImageUpdateService; + + beforeEach(() => { + vi.clearAllMocks(); + (ImageUpdateService as any).instance = undefined; + service = ImageUpdateService.getInstance(); + }); + + // One RepoDigest matching the ref so the local digest resolves and the flow reaches + // getRemoteDigestResult. + const dockerWithLocalDigest = (digest: string) => ({ + getDocker: () => ({ + getImage: () => ({ inspect: vi.fn().mockResolvedValue({ RepoDigests: [`ghcr.io/linuxserver/radarr@${digest}`] }) }), + }), + } as any); + + it('surfaces the specific failure reason (not a generic "unreachable") as the check error', async () => { + mockGetRemoteDigestResult.mockResolvedValue({ ok: false, reason: 'Authentication failed for ghcr.io/linuxserver/radarr:latest' }); + const result = await service.checkImage(dockerWithLocalDigest('sha256:local'), 'ghcr.io/linuxserver/radarr:latest'); + expect(result).toEqual({ hasUpdate: false, error: 'Authentication failed for ghcr.io/linuxserver/radarr:latest' }); + }); + + it('reports an update when the resolved remote digest differs from the local one', async () => { + mockGetRemoteDigestResult.mockResolvedValue({ ok: true, digest: 'sha256:remote' }); + const result = await service.checkImage(dockerWithLocalDigest('sha256:local'), 'ghcr.io/linuxserver/radarr:latest'); + expect(result).toEqual({ hasUpdate: true }); + }); +}); + // ── Rate limiting ───────────────────────────────────────────────────── describe('ImageUpdateService - manual refresh cooldown', () => { diff --git a/backend/src/__tests__/registry-api.test.ts b/backend/src/__tests__/registry-api.test.ts index 6f0f07bc..9afcf084 100644 --- a/backend/src/__tests__/registry-api.test.ts +++ b/backend/src/__tests__/registry-api.test.ts @@ -1,7 +1,8 @@ /** * Unit tests for the registry HTTP client: digest-name matching (the local - * RepoDigest vs image-ref comparison) and getRemoteDigest's HEAD-first lookup - * with GET fallback. + * RepoDigest vs image-ref comparison), getAuthToken's token-scope construction, + * getRemoteDigest's HEAD-first lookup with GET fallback, and getRemoteDigestResult's + * status-to-reason mapping. */ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { EventEmitter } from 'events'; @@ -38,7 +39,7 @@ function fakeRequest(url: string, options: { method?: string }, cb: (res: EventE vi.mock('https', () => ({ default: { request: (...args: unknown[]) => fakeRequest(...(args as Parameters)) } })); vi.mock('http', () => ({ default: { request: (...args: unknown[]) => fakeRequest(...(args as Parameters)) } })); -import { repoDigestMatchesRef, getRemoteDigest, parseImageRef } from '../services/registry-api'; +import { repoDigestMatchesRef, getRemoteDigest, getRemoteDigestResult, getAuthToken, parseImageRef } from '../services/registry-api'; const TOKEN_BODY = JSON.stringify({ token: 'test-token' }); const REMOTE = 'sha256:remote000000000000000000000000000000000000000000000000000000'; @@ -132,3 +133,105 @@ describe('getRemoteDigest HEAD-first lookup', () => { expect(calls.filter(c => c.url.includes('/manifests/')).map(c => c.method)).toEqual(['HEAD']); }); }); + +describe('getAuthToken builds the token request for the target repository', () => { + beforeEach(() => { + calls.length = 0; + }); + + // ghcr.io (and lscr.io, which delegates auth to it) echo a placeholder scope in the + // context-less /v2/ ping. The token must be requested for the repo we actually want; + // reusing the echoed scope made ghcr.io mint a token for the wrong repo and reject it. + const GHCR_CHALLENGE = 'Bearer realm="https://ghcr.io/token",service="ghcr.io",scope="repository:user/image:pull"'; + + it('ignores the placeholder scope echoed by the /v2/ ping and uses the target repo', async () => { + route = (url, method): FakeResp => { + if (url === 'https://ghcr.io/v2/') return { statusCode: 401, headers: { 'www-authenticate': GHCR_CHALLENGE } }; + if (url.startsWith('https://ghcr.io/token')) return { statusCode: 200, headers: {}, body: TOKEN_BODY }; + return method === 'HEAD' ? { statusCode: 200, headers: { 'docker-content-digest': REMOTE } } : { statusCode: 500, headers: {} }; + }; + const token = await getAuthToken('ghcr.io', 'linuxserver/radarr', null); + expect(token).toBe('test-token'); + const tokenCall = calls.find(c => c.url.startsWith('https://ghcr.io/token')); + expect(tokenCall).toBeTruthy(); + const decoded = decodeURIComponent(tokenCall?.url ?? ''); + expect(decoded).toContain('scope=repository:linuxserver/radarr:pull'); + expect(decoded).not.toContain('user/image'); + }); + + it('returns null when the token endpoint rejects the request (403)', async () => { + route = (url): FakeResp => { + if (url === 'https://ghcr.io/v2/') return { statusCode: 401, headers: { 'www-authenticate': GHCR_CHALLENGE } }; + if (url.startsWith('https://ghcr.io/token')) return { statusCode: 403, headers: {} }; + return { statusCode: 200, headers: { 'docker-content-digest': REMOTE } }; + }; + expect(await getAuthToken('ghcr.io', 'linuxserver/radarr', null)).toBeNull(); + }); +}); + +describe('getRemoteDigestResult failure reasons', () => { + beforeEach(() => { + calls.length = 0; + }); + + const REF = 'registry-1.docker.io/library/nginx:latest'; + const get = () => getRemoteDigestResult('registry-1.docker.io', 'library/nginx', 'latest'); + // Token always succeeds; the HEAD response under test drives the outcome. + const headResp = (resp: FakeResp) => (url: string, method: string): FakeResp => + tokenOk(url) ?? (method === 'HEAD' ? resp : { statusCode: 200, headers: { 'docker-content-digest': REMOTE } }); + + it('returns the digest on a HEAD 200', async () => { + route = headResp({ statusCode: 200, headers: { 'docker-content-digest': REMOTE } }); + expect(await get()).toEqual({ ok: true, digest: REMOTE }); + }); + + it('maps 401 to an authentication failure', async () => { + route = headResp({ statusCode: 401, headers: {} }); + expect(await get()).toEqual({ ok: false, reason: `Authentication failed for ${REF}` }); + }); + + it('maps 429 to a rate-limit reason including retry-after', async () => { + route = headResp({ statusCode: 429, headers: { 'retry-after': '3600' } }); + expect(await get()).toEqual({ ok: false, reason: `Rate limited by registry for ${REF} (retry after 3600)` }); + }); + + it('maps 429 without retry-after to a plain rate-limit reason', async () => { + route = headResp({ statusCode: 429, headers: {} }); + expect(await get()).toEqual({ ok: false, reason: `Rate limited by registry for ${REF}` }); + }); + + it('maps 403 to an authentication failure', async () => { + route = headResp({ statusCode: 403, headers: {} }); + expect(await get()).toEqual({ ok: false, reason: `Authentication failed for ${REF}` }); + }); + + it('maps an unexpected status to a generic reason with the status code', async () => { + route = headResp({ statusCode: 400, headers: {} }); + expect(await get()).toEqual({ ok: false, reason: `Registry returned status 400 for ${REF}` }); + }); + + it('maps 404 to image not found', async () => { + route = headResp({ statusCode: 404, headers: {} }); + expect(await get()).toEqual({ ok: false, reason: `Image not found: ${REF}` }); + }); + + it('maps 5xx to a registry error with the status', async () => { + route = headResp({ statusCode: 503, headers: {} }); + expect(await get()).toEqual({ ok: false, reason: `Registry error (503) for ${REF}` }); + }); + + it('derives the reason from the GET fallback when HEAD is 405', async () => { + route = (url, method) => tokenOk(url) ?? (method === 'HEAD' ? { statusCode: 405, headers: {} } : { statusCode: 401, headers: {} }); + expect(await get()).toEqual({ ok: false, reason: `Authentication failed for ${REF}` }); + }); + + it('fails when both HEAD and GET are 200 but omit the digest header', async () => { + route = (url, method) => tokenOk(url) ?? (method === 'HEAD' ? { statusCode: 200, headers: {} } : { statusCode: 200, headers: {} }); + expect(await get()).toEqual({ ok: false, reason: `Registry returned no digest for ${REF}` }); + }); + + it('reports unreachable when the request throws, including the error cause', async () => { + route = () => { throw new Error('ENOTFOUND'); }; + expect(await get()).toEqual({ ok: false, reason: `Registry unreachable for ${REF} (ENOTFOUND)` }); + }); +}); diff --git a/backend/src/services/ImageUpdateService.ts b/backend/src/services/ImageUpdateService.ts index 708a3396..ddb1a524 100644 --- a/backend/src/services/ImageUpdateService.ts +++ b/backend/src/services/ImageUpdateService.ts @@ -8,7 +8,7 @@ import { RegistryService } from './RegistryService'; import { NodeRegistry } from './NodeRegistry'; import { NotificationService } from './NotificationService'; import { sanitizeNotificationMessage } from '../utils/notificationMessage'; -import { parseImageRef, getRemoteDigest, repoDigestMatchesRef } from './registry-api'; +import { parseImageRef, getRemoteDigestResult, repoDigestMatchesRef } from './registry-api'; import { isDebugEnabled } from '../utils/debug'; import { getErrorMessage } from '../utils/errors'; import { sanitizeForLog } from '../utils/safeLog'; @@ -705,10 +705,11 @@ export class ImageUpdateService { return { hasUpdate: false, error: `Could not resolve a local registry digest for "${imageRef}"` }; } - const remoteDigest = await getRemoteDigest(parsed.registry, parsed.repo, parsed.tag, credentials); - if (!remoteDigest) { - return { hasUpdate: false, error: `Registry unreachable for ${parsed.registry}/${parsed.repo}:${parsed.tag}` }; + const remote = await getRemoteDigestResult(parsed.registry, parsed.repo, parsed.tag, credentials); + if (!remote.ok) { + return { hasUpdate: false, error: remote.reason }; } + const remoteDigest = remote.digest; const hasUpdate = localDigest !== remoteDigest; console.log( diff --git a/backend/src/services/registry-api.ts b/backend/src/services/registry-api.ts index d63712d7..5f8c1772 100644 --- a/backend/src/services/registry-api.ts +++ b/backend/src/services/registry-api.ts @@ -1,5 +1,6 @@ import https from 'https'; import http from 'http'; +import { sanitizeForLog } from '../utils/safeLog'; export interface ParsedRef { registry: string; @@ -113,12 +114,15 @@ export async function getAuthToken( const realmMatch = wwwAuth.match(/realm="([^"]+)"/); const serviceMatch = wwwAuth.match(/service="([^"]+)"/); - const scopeMatch = wwwAuth.match(/scope="([^"]+)"/); if (!realmMatch) return null; const params = new URLSearchParams(); if (serviceMatch) params.set('service', serviceMatch[1]); - params.set('scope', scopeMatch ? scopeMatch[1] : `repository:${repo}:pull`); + // The /v2/ ping carries no repository context, so any scope it echoes is a + // placeholder (ghcr.io returns repository:user/image:pull). Always request + // the scope for the repository we actually want; reusing the echoed scope + // makes ghcr.io mint a token for the wrong repo and then reject the pull. + params.set('scope', `repository:${repo}:pull`); tokenUrl = `${realmMatch[1]}?${params.toString()}`; } @@ -164,43 +168,98 @@ export function repoDigestMatchesRef(repoDigest: string, parsed: ParsedRef): boo && parsedName.repo === parsed.repo; } -export async function getRemoteDigest( +/** Outcome of a remote-digest lookup: the digest, or a human-readable reason it failed. */ +export type RemoteDigestResult = + | { ok: true; digest: string } + | { ok: false; reason: string }; + +/** + * Map a non-success manifest status to a specific reason so a caller can tell an auth + * failure from a rate limit, a missing image, or a server error, rather than collapsing + * them all into "unreachable". `ref` is the resolved "/:" for the + * image after Docker Hub normalization, not necessarily the literal string the user wrote. + */ +function manifestFailureReason(statusCode: number, ref: string, headers: HttpResult['headers']): string { + if (statusCode === 401 || statusCode === 403) return `Authentication failed for ${ref}`; + if (statusCode === 429) { + const retry = headers['retry-after']; + const retryStr = Array.isArray(retry) ? retry[0] : retry; + return retryStr + ? `Rate limited by registry for ${ref} (retry after ${retryStr})` + : `Rate limited by registry for ${ref}`; + } + if (statusCode === 404) return `Image not found: ${ref}`; + if (statusCode >= 500) return `Registry error (${statusCode}) for ${ref}`; + return `Registry returned status ${statusCode} for ${ref}`; +} + +/** + * Resolve the remote manifest digest for an image, returning either the digest or the + * reason the lookup failed. Same HEAD-first/GET-fallback transport as before (HEAD + * returns docker-content-digest without transferring the body, so it does not draw down + * Docker Hub's anonymous pull-rate budget the way a GET can); only the failure handling + * is richer. A 401/403/404/429/5xx HEAD reports its specific reason without a GET retry, + * since the bearer token is fetched up-front, so a 401 here is a real auth failure rather + * than a token-scope challenge to retry. + */ +export async function getRemoteDigestResult( registry: string, repo: string, tag: string, credentials?: RegistryCredentials | null, -): Promise { +): Promise { + const ref = `${registry}/${repo}:${tag}`; try { const token = await getAuthToken(registry, repo, credentials); const headers: Record = { Accept: MANIFEST_ACCEPT }; if (token) headers['Authorization'] = `Bearer ${token}`; const url = `https://${registry}/v2/${repo}/manifests/${tag}`; - // HEAD first: the registry returns docker-content-digest without - // transferring the manifest body, so it does not draw down Docker Hub's - // anonymous pull-rate budget the way a GET does (a GET can self-inflict a - // 429). Fall back to GET only when the registry rejects HEAD (405/501) or - // omits the digest header on a 200. A 401/403/404/429/5xx HEAD returns - // null without a GET retry: the bearer token is fetched up-front, so a - // 401 here is a real auth failure, not a token-scope challenge to retry. const head = await httpRequest(url, 'HEAD', headers); if (head.statusCode === 200) { const digest = head.headers['docker-content-digest']; - if (typeof digest === 'string') return digest; + if (typeof digest === 'string') return { ok: true, digest }; + // 200 without the digest header: fall through to GET to read it from there. } else if (head.statusCode !== 405 && head.statusCode !== 501) { - // 401/403/404/429/5xx: a GET would fail the same way. Report as unreachable. - return null; + return { ok: false, reason: manifestFailureReason(head.statusCode, ref, head.headers) }; } const res = await httpRequest(url, 'GET', headers); - if (res.statusCode !== 200) return null; - const digest = res.headers['docker-content-digest']; - return typeof digest === 'string' ? digest : null; - } catch { - return null; + if (res.statusCode === 200) { + const digest = res.headers['docker-content-digest']; + if (typeof digest === 'string') return { ok: true, digest }; + // 200 on both HEAD and GET but no digest header: a spec-violating registry. + return { ok: false, reason: `Registry returned no digest for ${ref}` }; + } + return { ok: false, reason: manifestFailureReason(res.statusCode, ref, res.headers) }; + } catch (e) { + // Bind and log the cause: a bare catch here would flatten DNS, TLS, connection- + // refused, and timeout failures into one opaque string with nothing in the logs, + // the silent-failure mode this function exists to remove. Prefer the errno code + // (ENOTFOUND/ECONNREFUSED/ETIMEDOUT/...) over a verbose message so the reason + // stays short in the sidebar tooltip; fall back to the message otherwise. + const cause = e instanceof Error ? ((e as NodeJS.ErrnoException).code ?? e.message) : String(e); + // ref and cause derive from the compose-authored image string and upstream error + // text, so neutralize control characters before they reach the log line. + console.error(`[registry-api] Remote digest lookup for ${sanitizeForLog(ref)} failed:`, sanitizeForLog(cause)); + return { ok: false, reason: `Registry unreachable for ${ref} (${cause})` }; } } +/** + * Digest-or-null view of {@link getRemoteDigestResult} for callers that only need the + * digest and treat any failure as "unknown" (e.g. the update-preview tag/digest diff). + */ +export async function getRemoteDigest( + registry: string, + repo: string, + tag: string, + credentials?: RegistryCredentials | null, +): Promise { + const result = await getRemoteDigestResult(registry, repo, tag, credentials); + return result.ok ? result.digest : null; +} + export async function listRegistryTags( registry: string, repo: string, diff --git a/docs/features/auto-update-policies.mdx b/docs/features/auto-update-policies.mdx index 4bddd2a9..cdcd23e2 100644 --- a/docs/features/auto-update-policies.mdx +++ b/docs/features/auto-update-policies.mdx @@ -47,11 +47,11 @@ The readiness hero shows this instance's cadence at a glance: when it last check ## When a check cannot complete -A registry check can fail for reasons that have nothing to do with whether an update exists: the registry is unreachable, an authenticated registry has no stored credentials, or Docker Hub's anonymous pull-rate limit has been hit. Sencho treats these as a distinct **check failed** state instead of reporting "up to date", so a failed check is never mistaken for a current image. +A registry check can fail for reasons that have nothing to do with whether an update exists: the registry is unreachable, an authenticated registry has no stored credentials, or Docker Hub's anonymous pull-rate limit has been hit. Sencho treats these as a distinct **check failed** state instead of reporting "up to date", so a failed check is never mistaken for a current image. The recorded reason names the specific cause, such as an authentication failure, a rate limit, a missing image, or an unreachable registry, so you can tell them apart at a glance. Where it shows: -- **Sidebar.** A stack whose latest check could not be determined shows a muted indicator on its row. Hover it to read the reason (for example, "Registry unreachable for ghcr.io/acme/api:v1"). +- **Sidebar.** A stack whose latest check could not be determined shows a muted indicator on its row. Hover it to read the reason (for example, "Authentication failed for ghcr.io/acme/api:v1" or "Registry unreachable for ghcr.io/acme/api:v1"). - **Update board.** Stacks whose check failed appear in a "could not be checked" advisory above the card grid, each with its reason, so a stack with no confirmed update is never silently absent. A confirmed update from an earlier successful check is kept through a later failed check, so a momentary registry blip does not make a pending update vanish.