From 6bbbff2f59b52125a14f5b42b6d06814f2c85e69 Mon Sep 17 00:00:00 2001 From: Aaron Sachs <898627+asachs01@users.noreply.github.com> Date: Fri, 4 Sep 2026 14:33:17 -0400 Subject: [PATCH] fix(http): stop labeling resource-endpoint 400s as credential errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HttpClient.handleResponse() threw HaloPsaAuthenticationError with the message "Bad request - invalid credentials or parameters" for any 400 response that wasn't in the recognized validation-error shape, regardless of the actual endpoint. That's misleading: skipAuth is never true outside this file (the OAuth token endpoint is fetched directly by AuthManager, not through HttpClient), so every 400 that reaches this branch is from an authenticated resource call, where a bad/expired Bearer token already fails as 401, not 400. A 400 here is always the request body itself being rejected. We hit this via halopsa-mcp: a POST /Actions call missing a server-required field (outcome isn't marked required in ActionCreateData) came back as a plain 400, surfaced to the caller as an "invalid credentials" error, and was read as an API-application permissions problem — sending the customer down the wrong path entirely. Add HaloPsaBadRequestError for this case and use it instead. The message now names the endpoint and says nothing about credentials. HaloPsaAuthenticationError keeps its 401 role plus the token endpoint's own legitimate 400 (thrown separately by AuthManager, unaffected by this change). Fixes #78 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01JgXg1NhHo6DvvAqoPowppi --- src/errors.ts | 21 ++++++++++++++++++++- src/http.ts | 15 +++++++++++---- src/index.ts | 1 + tests/unit/http.test.ts | 34 ++++++++++++++++++++++++++++++++++ 4 files changed, 66 insertions(+), 5 deletions(-) diff --git a/src/errors.ts b/src/errors.ts index 0e2f32d..b67cc54 100644 --- a/src/errors.ts +++ b/src/errors.ts @@ -21,7 +21,10 @@ export class HaloPsaError extends Error { } /** - * Authentication error (400 bad credentials, 401 unauthorized) + * Authentication error (401 unauthorized; also 400 from the OAuth token + * endpoint itself, thrown directly by AuthManager — never by HttpClient, + * since resource requests never carry skipAuth and so never reach + * HttpClient's own 400 branch, see HaloPsaBadRequestError) */ export class HaloPsaAuthenticationError extends HaloPsaError { constructor(message: string, statusCode: number = 401, response?: unknown) { @@ -68,6 +71,22 @@ export class HaloPsaValidationError extends HaloPsaError { } } +/** + * Bad request error (400 from a resource endpoint that isn't a recognized + * validation-error shape — a malformed or incomplete request payload, not + * a credentials problem. Resource requests are always authenticated via a + * Bearer token by the time they reach here, so a plain 400 here can't be + * "bad credentials" the way it legitimately can be on the OAuth token + * endpoint itself; see HaloPsaAuthenticationError) + */ +export class HaloPsaBadRequestError extends HaloPsaError { + constructor(message: string, response?: unknown) { + super(message, 400, response); + this.name = 'HaloPsaBadRequestError'; + Object.setPrototypeOf(this, HaloPsaBadRequestError.prototype); + } +} + /** * Rate limit exceeded error (429) */ diff --git a/src/http.ts b/src/http.ts index b078481..13a0b97 100644 --- a/src/http.ts +++ b/src/http.ts @@ -8,6 +8,7 @@ import type { RateLimiter } from './rate-limiter.js'; import { HaloPsaError, HaloPsaAuthenticationError, + HaloPsaBadRequestError, HaloPsaForbiddenError, HaloPsaNotFoundError, HaloPsaValidationError, @@ -163,14 +164,20 @@ export class HttpClient { switch (response.status) { case 400: - // Could be bad credentials on token request or validation error + // Every request that reaches here is a resource call (skipAuth is + // never true outside this file — the OAuth token endpoint is + // fetched directly by AuthManager, not through HttpClient), so a + // 400 here is never a credentials problem: the Bearer token, if + // wrong, fails as a 401 below, not a 400. This is the request body + // itself being rejected — a missing/invalid field HaloPSA's + // server-side validation requires but this SDK doesn't mark + // `required` (e.g. Actions' `outcome`), or similar. if (this.isValidationError(responseBody)) { const errors = this.parseValidationErrors(responseBody); throw new HaloPsaValidationError('Validation error', errors, responseBody); } - throw new HaloPsaAuthenticationError( - 'Bad request - invalid credentials or parameters', - 400, + throw new HaloPsaBadRequestError( + `Bad request (400): ${method} ${url} rejected the request parameters`, responseBody ); diff --git a/src/index.ts b/src/index.ts index 30f1a6f..324aa7c 100644 --- a/src/index.ts +++ b/src/index.ts @@ -14,6 +14,7 @@ export { DEFAULT_RATE_LIMIT_CONFIG } from './config.js'; export { HaloPsaError, HaloPsaAuthenticationError, + HaloPsaBadRequestError, HaloPsaForbiddenError, HaloPsaNotFoundError, HaloPsaValidationError, diff --git a/tests/unit/http.test.ts b/tests/unit/http.test.ts index 856d5e1..fd9dfa7 100644 --- a/tests/unit/http.test.ts +++ b/tests/unit/http.test.ts @@ -14,8 +14,11 @@ import { AuthManager } from '../../src/auth.js'; import { RateLimiter } from '../../src/rate-limiter.js'; import { HaloPsaError, + HaloPsaAuthenticationError, + HaloPsaBadRequestError, HaloPsaNotFoundError, HaloPsaServerError, + HaloPsaValidationError, } from '../../src/errors.js'; import type { ResolvedConfig } from '../../src/config.js'; @@ -124,6 +127,37 @@ describe('HttpClient response handling', () => { expect((err as HaloPsaServerError).response).toEqual({ message: 'boom' }); }, 15000); + it('a non-validation-shaped 400 raises HaloPsaBadRequestError, not an auth error', async () => { + // Regression: node-halopsa#78 — a 400 from a resource endpoint (e.g. a + // required field like Actions' `outcome` missing) used to throw + // HaloPsaAuthenticationError with a "invalid credentials or parameters" + // message, which read as a permissions/credentials problem to callers + // even though the Bearer token was never in question — a bad token + // fails as 401, not 400. This body has neither `errors` nor + // `validation_errors`, so it isn't the recognized validation shape. + vi.mocked(fetch).mockResolvedValue( + realResponse('{"message":"outcome is required"}', { status: 400 }) + ); + const err = await makeClient() + .request('/Actions', { method: 'POST', body: [{ ticket_id: 1, note: 'hi' }] }) + .catch((e: unknown) => e); + expect(err).toBeInstanceOf(HaloPsaBadRequestError); + expect(err).not.toBeInstanceOf(HaloPsaAuthenticationError); + expect((err as HaloPsaBadRequestError).message).not.toMatch(/credentials/i); + expect((err as HaloPsaBadRequestError).response).toEqual({ message: 'outcome is required' }); + }); + + it('a validation-shaped 400 still raises HaloPsaValidationError', async () => { + vi.mocked(fetch).mockResolvedValue( + realResponse('{"errors":[{"field":"outcome","message":"is required"}]}', { status: 400 }) + ); + const err = await makeClient() + .request('/Actions', { method: 'POST', body: [{ ticket_id: 1, note: 'hi' }] }) + .catch((e: unknown) => e); + expect(err).toBeInstanceOf(HaloPsaValidationError); + expect((err as HaloPsaValidationError).errors).toEqual([{ field: 'outcome', message: 'is required' }]); + }); + it('generic non-2xx statuses raise HaloPsaError with the raw body', async () => { vi.mocked(fetch).mockResolvedValue( realResponse('teapot', { status: 418, headers: { 'content-type': 'text/plain' } })