Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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}.` };
}
Expand Down Expand Up @@ -167,6 +202,18 @@ export async function postPackageRemoved(config: Config): Promise<PackageRemoved
if (response.status === 404) {
return { result: 'gone', message: null };
}
// A site that has been deleted answers 401 here, not 404: this route resolves the site FROM the
// credential, so once the site record is gone the credential resolves to nothing. Reporting that as
// an auth failure would tell someone to re-run `login` over a site that no longer exists, so the
// question is put to the endpoint that can still answer it — widget settings, which needs no
// credential and 404s for a removed site.
if (response.status === 401 && (await fetchSiteStatus(config)) === 'removed') {
return { result: 'gone', message: null };
}
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}.` };
}
Expand Down Expand Up @@ -277,6 +324,11 @@ export async function postManifest(
);
}

const refused = authFailureMessage(response.status, config);
if (refused !== null) {
throw new PatchstackError(refused, 'UNAUTHORIZED');
}

if (response.status < 200 || response.status >= 300) {
throw new PatchstackError(
`Patchstack returned ${response.status}: ${text.slice(0, 200)}`,
Expand Down
1 change: 1 addition & 0 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
130 changes: 130 additions & 0 deletions tests/auth-failure-message.test.ts
Original file line number Diff line number Diff line change
@@ -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> = {}): 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);
});
});
Loading