From a61dc6703ad2d1ea5513f290bbd2c6e7f93ced28 Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Thu, 20 Aug 2026 16:03:42 +0200 Subject: [PATCH] Say what a refused request means, instead of its status code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every per-site endpoint now requires a credential, which makes a refusal the likeliest failure a misconfigured project meets. Until now none of the three write paths handled one: they fell through to "Patchstack returned 401", which names neither the cause nor the fix. That output is routinely read by an AI coding agent with nowhere else to look it up. There are three causes and three different remedies. Holding no credential is a setup step that never ran. Holding one that is refused means expired, revoked, or a site that no longer exists. A 403 means the credential is valid and simply belongs to another site — usually a config file carrying the wrong UUID — which was previously indistinguishable from having no credential at all. Also fixes a regression the boundary change introduces. The package-removed signal read "gone" off a 404, but a deleted site cannot answer 404 there any more: that route resolves the site from the credential, so once the site record is gone the credential resolves to nothing and the answer is 401. Unhandled, this would advise re-running login for a site that no longer exists. The question goes instead to the endpoint that can still answer it — public widget settings, which needs no credential and 404s for a removed site. One shared helper so the three paths cannot drift apart, and it returns null for anything that is not an authentication failure: relabelling a 500 as an auth problem would be worse than the bare status code it replaced. Both of those are covered by controls, along with the difference between the two 401 messages. Co-Authored-By: Claude Opus 5 (1M context) --- src/client.ts | 52 ++++++++++++ src/types.ts | 1 + tests/auth-failure-message.test.ts | 130 +++++++++++++++++++++++++++++ 3 files changed, 183 insertions(+) create mode 100644 tests/auth-failure-message.test.ts diff --git a/src/client.ts b/src/client.ts index 2f235a1..7f70ad6 100644 --- a/src/client.ts +++ b/src/client.ts @@ -12,6 +12,37 @@ export function buildEndpointUrl(base: string, siteUuid?: string | null): string : trimmed; } +/** + * What a refusal means, in terms someone can act on. + * + * Every Pulse route that addresses an existing site requires a credential, so a rejection is now the + * most likely failure a misconfigured project hits — and the three causes have three different fixes. + * Reporting them as `Patchstack returned 401` names none of them, and this output is frequently read by + * an AI agent that has no other way to find out what to do next. + * + * The distinction 401 cannot make on its own is whether we HELD a credential. Having none is a setup + * step that was never run; having one rejected is a credential that has expired, been revoked, or whose + * site no longer exists. A 403 is different again: the credential is valid and simply is not for this + * site, which usually means a `.patchstackrc.json` carrying someone else's UUID. + * + * @returns the message, or null when the status is not an authentication failure + */ +export function authFailureMessage(status: number, config: Config): string | null { + const hasCredential = typeof config.pulseAuth === 'string' && config.pulseAuth.length > 0; + + if (status === 401 && !hasCredential) { + return 'Patchstack requires an API credential for this site and none is configured. Run `npx patchstack-connect login`, or set PATCHSTACK_API_KEY.'; + } + if (status === 401) { + return 'Patchstack rejected this API credential. It may have expired, been revoked, or the site may no longer exist. Run `npx patchstack-connect login` to issue a new one.'; + } + if (status === 403) { + return 'This API credential is not permitted to act on this site. Check that siteUuid in .patchstackrc.json matches the credential (a credential is issued for one site).'; + } + + return null; +} + /** Build the live Pulse rules URL corresponding to a manifest endpoint override. */ export function buildRulesUrl(manifestEndpoint: string, siteUuid: string): string { const url = new URL(manifestEndpoint); @@ -114,6 +145,10 @@ export async function postInputMap( // one side is out of date, and guessing at compatibility is how a consumer misreads a document. return { result: 'failed', message: `Patchstack does not accept this map schema (version ${map.version}). Update @patchstack/connect.` }; } + const refused = authFailureMessage(response.status, config); + if (refused !== null) { + return { result: 'failed', message: refused }; + } if (!response.ok) { return { result: 'failed', message: `Patchstack returned ${response.status}.` }; } @@ -167,6 +202,18 @@ export async function postPackageRemoved(config: Config): Promise= 300) { throw new PatchstackError( `Patchstack returned ${response.status}: ${text.slice(0, 200)}`, diff --git a/src/types.ts b/src/types.ts index 4a76649..21d6ca3 100644 --- a/src/types.ts +++ b/src/types.ts @@ -85,6 +85,7 @@ export class PatchstackError extends Error { | 'NETWORK_ERROR' | 'NETWORK_TIMEOUT' | 'SITE_NOT_FOUND' + | 'UNAUTHORIZED' | 'VALIDATION_ERROR' | 'SERVER_ERROR', public readonly cause?: unknown, diff --git a/tests/auth-failure-message.test.ts b/tests/auth-failure-message.test.ts new file mode 100644 index 0000000..acf41cd --- /dev/null +++ b/tests/auth-failure-message.test.ts @@ -0,0 +1,130 @@ +import { describe, expect, it, vi } from 'vitest'; +import { authFailureMessage, postInputMap, postPackageRemoved, postManifest } from '../src/client.js'; +import { PatchstackError, type Config } from '../src/types.js'; + +/** + * A refusal has to say which of three things went wrong, because each has a different fix. + * + * Every Pulse route addressing an existing site requires a credential, so a rejection is the likeliest + * failure a misconfigured project meets — and `Patchstack returned 401` is the one report that helps + * nobody. This output is often read by an AI agent with no other source of the answer, so "which setup + * step did I miss" has to be answerable from the text alone. + */ +const config = (over: Partial = {}): Config => + ({ + endpoint: 'https://api.test/monitor/pulse/manifest', + siteUuid: '11111111-1111-4111-8111-111111111111', + apiKey: null, + pulseAuth: null, + timeoutMs: 5_000, + environment: 'production', + ...over, + }) as Config; + +const WITH_CREDENTIAL = config({ pulseAuth: 'a-secret-40-chars-long-enough-for-this-1' }); + +const respond = (status: number, body = '{}') => + vi.fn(async () => new Response(body, { status, headers: { 'Content-Type': 'application/json' } })); + +describe('authFailureMessage', () => { + it('tells an unconfigured project to obtain a credential', () => { + const said = authFailureMessage(401, config()); + + expect(said).toContain('login'); + expect(said).toContain('PATCHSTACK_API_KEY'); + }); + + it('distinguishes a rejected credential from a missing one', () => { + // The distinction 401 cannot make by itself, and the one that decides the remedy: nothing to set up + // versus something to renew. Asserted as a difference so neither message can drift into the other. + const missing = authFailureMessage(401, config()); + const rejected = authFailureMessage(401, WITH_CREDENTIAL); + + expect(rejected).not.toBe(missing); + expect(rejected).toMatch(/expired|revoked/); + // A removed site also answers 401 on the authenticated routes, so a message that named only expiry + // would send someone to re-run `login` against a site that no longer exists. + expect(rejected).toMatch(/no longer exist/); + }); + + it('reads 403 as the wrong site rather than a missing credential', () => { + const said = authFailureMessage(403, WITH_CREDENTIAL); + + expect(said).toContain('.patchstackrc.json'); + expect(said).not.toMatch(/expired|revoked/); + }); + + it('says nothing about statuses that are not authentication failures', () => { + // The control. A helper that answered for every status would relabel a 500 or a 422 as an auth + // problem, which is worse than the bare status code it replaced. + for (const status of [200, 404, 422, 429, 500, 503]) { + expect(authFailureMessage(status, WITH_CREDENTIAL), `status ${status}`).toBeNull(); + } + }); +}); + +describe('the write paths report a refusal in those terms', () => { + it('map upload fails with the remedy, not the number', async () => { + vi.stubGlobal('fetch', respond(401)); + + const outcome = await postInputMap(config(), { version: 3, endpoints: [] }); + + expect(outcome.result).toBe('failed'); + expect(outcome).toHaveProperty('message'); + const message = (outcome as { message: string }).message; + expect(message).not.toMatch(/^Patchstack returned/); + expect(message).toContain('login'); + }); + + it('manifest push throws UNAUTHORIZED rather than SERVER_ERROR', async () => { + vi.stubGlobal('fetch', respond(403)); + + await expect(postManifest(WITH_CREDENTIAL, { packages: [] } as never)).rejects.toMatchObject({ + code: 'UNAUTHORIZED', + }); + }); + + it('still reports a genuine server error as one', async () => { + // The other half of the control: the auth handling must not have swallowed every failure branch. + vi.stubGlobal('fetch', respond(500, 'upstream exploded')); + + await expect(postManifest(WITH_CREDENTIAL, { packages: [] } as never)).rejects.toBeInstanceOf( + PatchstackError, + ); + await expect(postManifest(WITH_CREDENTIAL, { packages: [] } as never)).rejects.toMatchObject({ + code: 'SERVER_ERROR', + }); + }); +}); + +describe('a removed site is still reported as removed', () => { + it('reads 401 on package-removed as "gone" when the site really is gone', async () => { + // The regression this guards. That route resolves the site FROM the credential, so a deleted site + // answers 401 rather than 404 — verified against the server. Left unhandled, uninstalling a site + // that was already deleted in the dashboard would advise re-running `login`. + const fetchMock = vi.fn(async (url: string) => { + if (url.includes('/widget/settings/')) return new Response('{}', { status: 404 }); + return new Response('{}', { status: 401 }); + }); + vi.stubGlobal('fetch', fetchMock); + + const outcome = await postPackageRemoved(WITH_CREDENTIAL); + + expect(outcome.result).toBe('gone'); + }); + + it('reports an auth failure when the site is still there', async () => { + // The control that keeps the branch above honest: 401 must not become a blanket "gone", or a genuine + // credential problem would read as a site that no longer exists and be silently ignored. + const fetchMock = vi.fn(async (url: string) => { + if (url.includes('/widget/settings/')) return new Response('{}', { status: 200 }); + return new Response('{}', { status: 401 }); + }); + vi.stubGlobal('fetch', fetchMock); + + const outcome = await postPackageRemoved(WITH_CREDENTIAL); + + expect(outcome.result).toBe('failed'); + expect((outcome as { message: string }).message).toMatch(/credential/i); + }); +});