diff --git a/AUDIT.md b/AUDIT.md new file mode 100644 index 0000000..e559798 --- /dev/null +++ b/AUDIT.md @@ -0,0 +1,369 @@ +# Chargily Pay JavaScript SDK - Security & Code Audit Report + +**Author:** [@ramdaniAli](https://github.com/ramdaniAli) +**Date:** 2026-03-08 +**SDK Version:** 2.1.0 +**Scope:** Full codebase review (src/*, package.json, tsconfig.json) + +--- + +## Executive Summary + +This audit covers the entire `@chargily/chargily-pay` SDK codebase. The SDK is a functional HTTP wrapper around the Chargily Pay V2 API, but it contains **5 critical bugs**, **8 high-severity issues**, and lacks essential infrastructure for a production payment library. + +The SDK has **no tests, no CI/CD, no linting, no retry/timeout logic, no caching, and no typed error handling** — all of which are expected in a payment gateway SDK. + +--- + +## Critical Bugs (5) + +### CRITICAL-1: `updateProduct`, `updatePrice`, `updatePaymentLink` use POST instead of PATCH + +**Files:** `src/classes/client.ts` lines 192, 263, 386 + +`updateCustomer` correctly uses `PATCH`, but the three other update methods use `POST`. This will either create duplicate resources or return 405 errors from the API. + +```ts +// client.ts:192 — BUG +return this.request(`products/${product_id}`, 'POST', update_data); +// Should be: 'PATCH' + +// client.ts:263 — BUG +return this.request(`prices/${price_id}`, 'POST', update_data); +// Should be: 'PATCH' + +// client.ts:386-390 — BUG +return this.request(`payment-links/${payment_link_id}`, 'POST', update_data); +// Should be: 'PATCH' +``` + +**Fix:** Replace `'POST'` with `'PATCH'` in all three methods. + +--- + +### CRITICAL-2: `verifySignature` throws instead of returning `false` + +**File:** `src/utils/index.ts` lines 14-37 + +The function signature says `returns boolean`, and the early guard (line 16) correctly returns `false`. But when the signature is actually invalid (the main failure case), it **throws an Error** instead of returning `false`. + +```ts +// Line 16 — correct behavior +if (!signature) return false; + +// Lines 29-34 — inconsistent behavior +if (signatureBuffer.length !== digest.length || !crypto.timingSafeEqual(digest, signatureBuffer)) { + throw new Error('The signature is invalid.'); // BUG: should return false +} +``` + +This forces developers into a confusing try/catch + boolean pattern for a single function. + +**Fix:** Replace the `throw` with `return false`. + +--- + +### CRITICAL-3: `console.log` in production library code + +**File:** `src/utils/index.ts` line 36 + +```ts +console.log('The signature is valid'); // Pollutes every consumer's stdout +return true; +``` + +Library code must never log to the console. This will print on every valid webhook in every app using this SDK. + +**Fix:** Remove the `console.log` statement. + +--- + +### CRITICAL-4: `success_url` validation logic is broken + +**File:** `src/classes/client.ts` lines 294-299 + +```ts +if ( + !checkout_data.success_url.startsWith('http') && + !checkout_data.success_url.startsWith('https') +) +``` + +Since `'https://...'` always starts with `'http'`, the second condition is **dead code**. This also means `'httpanything'` or `'http-malicious'` pass validation. Additionally, `failure_url` and `webhook_endpoint` are never validated. + +**Fix:** Use `new URL()` constructor for validation, and validate all URL fields. + +--- + +### CRITICAL-5: API error response body is never parsed + +**File:** `src/classes/client.ts` lines 88-100 + +When the API returns 4xx/5xx, the error body (which contains the actual error message, validation details, etc.) is discarded: + +```ts +if (!response.ok) { + // The JSON body with {message, errors} is never read + throw new Error(`API request failed with status ${response.status}: ${response.statusText}`); +} +``` + +Developers cannot programmatically know *why* an API call failed — they only get "status 422: Unprocessable Entity" with no details. + +**Fix:** Parse the error body and throw a typed `ChargilyApiError` with `status`, `statusText`, and `body`. + +--- + +## High Severity Issues (8) + +### HIGH-1: `sigPrefix` is an empty placeholder + +**File:** `src/utils/index.ts` line 19 + +```ts +const sigPrefix = ''; // Define if there's a specific prefix used +``` + +This is a TODO comment left as production code. If Chargily's API uses a prefix like `sha256=` (as GitHub webhooks do), all signature verifications will silently fail. + +--- + +### HIGH-2: No pagination beyond page 1 + +**File:** `src/classes/client.ts` — all `list*()` methods + +All list methods accept `per_page` but have **no `page` parameter**. Developers cannot access data beyond the first page. The `ListResponse` type correctly models `current_page`, `last_page`, `next_page_url` — but the SDK provides no way to use them. + +```ts +// Every list method looks like this: +public async listCustomers(per_page: number = 10): Promise> { + const endpoint = `customers?per_page=${per_page}`; + // No page parameter — stuck on page 1 forever +} +``` + +--- + +### HIGH-3: `getCheckoutItems` and `getPaymentLinkItems` return wrong types + +**File:** `src/classes/client.ts` lines 344-354, 424-434 + +These methods return `ListResponse` and `ListResponse` (input types), but the API returns `CheckoutItem` and `PaymentLinkItem` (response types with `id`, `amount`, `currency`, etc.). TypeScript will lie to developers about the shape of the data. + +--- + +### HIGH-4: `CreateCustomerParams` allows empty objects + +**File:** `src/types/param.ts` lines 3-18 + +All fields are optional (`?`). The Chargily API requires at least `name`, but `createCustomer({})` compiles fine and fails at runtime. + +--- + +### HIGH-5: No `per_page` validation + +**File:** `src/classes/client.ts` — all `list*()` methods + +`per_page: 0`, `per_page: -1`, or `per_page: 999999` are sent to the API without any bounds checking. + +--- + +### HIGH-6: `UpdatePriceParams.metadata` is required instead of optional + +**File:** `src/types/param.ts` lines 79-82 + +```ts +export interface UpdatePriceParams { + metadata: Record; // Missing '?' — forces metadata even if you don't want to update it +} +``` + +--- + +### HIGH-7: Type mismatch on `shipping_address` + +**File:** `src/types/param.ts` line 127 vs `src/types/data.ts` line 222 + +Input type says `shipping_address?: string` but the response type says `shipping_address: Address` (an object with `country`, `state`, `address`). These are incompatible. + +--- + +### HIGH-8: Non-nullable fields that should be nullable + +**File:** `src/types/data.ts` + +Several `Checkout` fields are typed as non-nullable but are optional at creation: +- `description: string` → should be `string | null` +- `failure_url: string` → should be `string | null` +- `webhook_endpoint: string` → should be `string | null` +- `shipping_address: Address` → should be `Address | null` + +Code like `checkout.description.toLowerCase()` will crash at runtime when the API returns `null`. + +--- + +## Missing Features — Proposals + +### 1. Typed Error Hierarchy + +```ts +class ChargilyError extends Error { } +class ChargilyApiError extends ChargilyError { + constructor( + public status: number, + public statusText: string, + public body: { message: string; errors?: Record } | null + ) { } +} +class ChargilyValidationError extends ChargilyError { } +class ChargilyNetworkError extends ChargilyError { } +``` + +This lets developers distinguish network failures from API validation errors from authentication issues. + +### 2. Retry with Exponential Backoff + Timeout + +```ts +interface ChargilyClientOptions { + api_key: string; + mode: 'test' | 'live'; + timeout?: number; // default: 30000ms + maxRetries?: number; // default: 3 + retryDelay?: number; // default: 1000ms (base for exponential backoff) +} +``` + +The `request()` method should: +- Use `AbortController` for request timeout +- Retry on 429 (rate limit) with `Retry-After` header support +- Retry on 503 (service unavailable) with exponential backoff +- Never retry on 4xx (client errors are permanent) + +### 3. Idempotency Key Support + +```ts +public async createCheckout( + checkout_data: CreateCheckoutParams, + options?: { idempotencyKey?: string } +): Promise +``` + +Essential for payment operations to prevent duplicate charges on network retries. + +### 4. Webhook Event Types + +```ts +type WebhookEventType = 'checkout.paid' | 'checkout.failed' | 'checkout.expired' | 'checkout.canceled'; + +interface WebhookEvent { + id: string; + type: WebhookEventType; + data: T; + created_at: number; +} + +interface CheckoutPaidEvent extends WebhookEvent { + type: 'checkout.paid'; +} + +// Discriminated union for type-safe event handling +type ChargilyWebhookEvent = CheckoutPaidEvent | CheckoutFailedEvent | ...; +``` + +### 5. Full Pagination Support + +```ts +// Option A: Add page parameter +public async listCustomers(options?: { per_page?: number; page?: number }): Promise> + +// Option B: Auto-paginating async generator +public async *listAllCustomers(per_page: number = 10): AsyncGenerator +``` + +### 6. API Status / Health Check + +```ts +public async getApiStatus(): Promise<{ status: 'operational' | 'degraded' | 'down'; mode: 'test' | 'live'; latency_ms: number }> +``` + +With mode mismatch detection: +```ts +// Warn developers when API key mode doesn't match configured mode +const balance = await client.getBalance(); +if (balance.livemode !== (this.mode === 'live')) { + console.warn('[chargily] Mode mismatch: configured as ${this.mode} but API key is ${balance.livemode ? "live" : "test"}'); +} +``` + +### 7. Payment Link Customization + +If the API supports it, expose options like: +```ts +interface CreatePaymentLinkParams { + // ... existing fields + theme?: { primary_color?: string; logo_url?: string; }; + custom_fields?: Array<{ label: string; type: 'text' | 'email' | 'phone'; required?: boolean }>; +} +``` + +### 8. Caching Layer (Optional Redis Support) + +```ts +interface ChargilyClientOptions { + // ... existing fields + cache?: { + adapter: 'memory' | 'redis'; + ttl?: number; // seconds + redisUrl?: string; + }; +} +``` + +Cache `getProduct`, `getPrice`, `getCustomer` responses to reduce API calls under high load. + +--- + +## Missing Infrastructure + +| Category | Status | Recommendation | +|----------|--------|---------------| +| **Testing** | None | Add Vitest with unit tests for all 25 methods + webhook utils | +| **CI/CD** | None | GitHub Actions: build, test, lint on every PR | +| **Linting** | None | ESLint + Prettier with pre-commit hooks (Husky + lint-staged) | +| **Type checking** | Partial | Update `@types/node` from `^14` to `^18`, add `engines: { node: ">=18" }` | +| **Changelog** | None | Add CHANGELOG.md, consider conventional commits | +| **Contributing guide** | None | Add CONTRIBUTING.md with setup instructions | +| **Documentation** | README only | Add TypeDoc for auto-generated API docs | +| **tsconfig** | Outdated | Target `es2017`+ instead of `es5` (SDK uses `fetch` which is Node 18+) | +| **Security** | No policy | Add SECURITY.md with vulnerability reporting process | + +--- + +## How to Reproduce + +```bash +git clone https://github.com/chargily/chargily-pay-javascript.git +# Open src/classes/client.ts lines 192, 263, 386 — POST instead of PATCH +# Open src/utils/index.ts lines 29-36 — throw + console.log +# Open src/classes/client.ts lines 294-299 — broken URL validation +# Open src/classes/client.ts lines 88-100 — error body discarded +``` + +--- + +## Next Steps + +I (@ramdaniAli) am willing to contribute fixes for all the issues listed above. I propose to work in the following order: + +1. **PR #1 — Bug fixes:** Fix all 5 critical bugs + 8 high-severity issues +2. **PR #2 — Error handling:** Add typed error hierarchy + parse API error bodies +3. **PR #3 — Resilience:** Add retry, timeout, idempotency key support +4. **PR #4 — Pagination:** Add `page` parameter + `listAll*()` async generators +5. **PR #5 — Webhook DX:** Add typed webhook events + remove console.log +6. **PR #6 — Infrastructure:** Add Vitest, ESLint, Prettier, GitHub Actions CI + +I'd appreciate maintainer feedback on priorities and any API constraints before starting implementation. + +--- + +*This audit was conducted as a community contribution to improve the SDK's reliability and developer experience.* diff --git a/src/classes/client.ts b/src/classes/client.ts index 8bab95b..0fd6ee4 100644 --- a/src/classes/client.ts +++ b/src/classes/client.ts @@ -52,6 +52,9 @@ export class ChargilyClient { * @param {ChargilyClientOptions} options - Configuration options including API key and mode. */ constructor(options: ChargilyClientOptions) { + // AUDIT [MEDIUM-3]: No API key validation — empty string accepted silently. + // AUDIT [MEDIUM-4]: Invalid mode (e.g. 'staging') defaults to LIVE silently. + // This could cause accidental real charges during development. this.api_key = options.api_key; this.base_url = options.mode === 'test' ? CHARGILY_TEST_URL : CHARGILY_LIVE_URL; @@ -85,6 +88,11 @@ export class ChargilyClient { fetchOptions.body = JSON.stringify(body); } + // AUDIT [CRITICAL-5]: API error response body is never parsed. + // When the API returns 4xx/5xx, the JSON body containing {message, errors} + // is discarded. Developers cannot know WHY a call failed. + // Proposal: Parse error body and throw a typed ChargilyApiError with + // status, statusText, and body instead of a generic Error. try { const response = await fetch(url, fetchOptions); @@ -157,6 +165,10 @@ export class ChargilyClient { * @param {number} [per_page=10] - The number of customers to return per page. * @returns {Promise>} - A promise that resolves to a paginated list of customers. */ + // AUDIT [HIGH-2]: No 'page' parameter — all list methods are stuck on page 1. + // ListResponse has current_page, last_page, next_page_url but they can't be used. + // AUDIT [HIGH-5]: No per_page validation — 0, -1, 99999 are sent as-is. + // Proposal: Add page param + listAll*() async generators for auto-pagination. public async listCustomers( per_page: number = 10 ): Promise> { @@ -189,6 +201,9 @@ export class ChargilyClient { product_id: string, update_data: UpdateProductParams ): Promise { + // AUDIT [CRITICAL-1]: Uses 'POST' instead of 'PATCH'. updateCustomer (line 141) + // correctly uses 'PATCH'. This will create duplicates or return 405. + // Fix: Change 'POST' to 'PATCH'. return this.request(`products/${product_id}`, 'POST', update_data); } @@ -260,6 +275,8 @@ export class ChargilyClient { price_id: string, update_data: UpdatePriceParams ): Promise { + // AUDIT [CRITICAL-1]: Same bug — uses 'POST' instead of 'PATCH'. + // Fix: Change 'POST' to 'PATCH'. return this.request(`prices/${price_id}`, 'POST', update_data); } @@ -291,6 +308,10 @@ export class ChargilyClient { public async createCheckout( checkout_data: CreateCheckoutParams ): Promise { + // AUDIT [CRITICAL-4]: The https check is dead code because 'https' always + // starts with 'http'. Also accepts 'httpanything'. failure_url and + // webhook_endpoint are never validated. + // Fix: Use new URL() constructor for proper validation on all URL fields. if ( !checkout_data.success_url.startsWith('http') && !checkout_data.success_url.startsWith('https') @@ -341,6 +362,9 @@ export class ChargilyClient { * @param {number} [per_page=10] - The number of items to return per page. * @returns {Promise>} A paginated list of items in the checkout session. */ + // AUDIT [HIGH-3]: Returns ListResponse (input type) + // instead of ListResponse (response type with id, amount, etc.). + // Fix: Change return type to ListResponse. public async getCheckoutItems( checkout_id: string, per_page: number = 10 @@ -383,6 +407,8 @@ export class ChargilyClient { payment_link_id: string, update_data: UpdatePaymentLinkParams ): Promise { + // AUDIT [CRITICAL-1]: Same bug — uses 'POST' instead of 'PATCH'. + // Fix: Change 'POST' to 'PATCH'. return this.request( `payment-links/${payment_link_id}`, 'POST', @@ -421,6 +447,8 @@ export class ChargilyClient { * @param {number} [per_page=10] - The number of items to return per page. * @returns {Promise>} A paginated list of items associated with the payment link. */ + // AUDIT [HIGH-3]: Same issue — returns ListResponse + // instead of ListResponse. public async getPaymentLinkItems( payment_link_id: string, per_page: number = 10 diff --git a/src/types/data.ts b/src/types/data.ts index bff492f..12fd753 100644 --- a/src/types/data.ts +++ b/src/types/data.ts @@ -185,6 +185,9 @@ export interface Checkout { /** The language of the checkout page. */ locale: 'ar' | 'en' | 'fr'; + // AUDIT [HIGH-8]: description, failure_url, webhook_endpoint, and + // shipping_address are optional at creation but typed as non-nullable here. + // The API returns null when not provided. Fix: Add '| null' to each. /** A description of the transaction. */ description: string; diff --git a/src/types/param.ts b/src/types/param.ts index 5ca49c6..28077e6 100644 --- a/src/types/param.ts +++ b/src/types/param.ts @@ -1,5 +1,8 @@ import { Address } from './data'; +// AUDIT [HIGH-4]: All fields are optional — createCustomer({}) compiles but +// fails at runtime. The API requires at least 'name'. +// Fix: Make 'name' required, or use a union type to enforce at least one field. export interface CreateCustomerParams { /** The name of the customer. */ name?: string; @@ -76,6 +79,8 @@ export interface CreatePriceParams { metadata?: Record; } +// AUDIT [HIGH-6]: 'metadata' is required instead of optional. +// Fix: Add '?' to make it optional. export interface UpdatePriceParams { /** A set of key-value pairs for additional information about the price. */ metadata: Record; @@ -123,6 +128,8 @@ export interface CreateCheckoutParams { /** Optional: The ID of an existing customer. */ customer_id?: string; + // AUDIT [HIGH-7]: Type is 'string' here but 'Address' in the Checkout response + // type (data.ts). These are incompatible — needs clarification against API spec. /** Optional: The shipping address for the checkout. */ shipping_address?: string; diff --git a/src/utils/index.ts b/src/utils/index.ts index 42e8d44..ef8cfe5 100644 --- a/src/utils/index.ts +++ b/src/utils/index.ts @@ -16,6 +16,9 @@ export function verifySignature( return false; } + // AUDIT [HIGH-1]: sigPrefix is an empty placeholder. If Chargily's API + // uses a prefix (e.g. 'sha256=' like GitHub), all verifications will + // silently fail. Needs verification against actual API spec. const sigPrefix = ''; // Define if there's a specific prefix used const sigHashAlg = 'sha256'; // Define the hashing algorithm const computedSignature = crypto @@ -26,6 +29,9 @@ export function verifySignature( const digest = Buffer.from(sigPrefix + computedSignature, 'utf8'); const signatureBuffer = Buffer.from(signature, 'utf8'); + // AUDIT [CRITICAL-2]: Throws instead of returning false. This contradicts + // the return type 'boolean' and the early guard above (line 16) which + // correctly returns false. Fix: Replace throw with 'return false'. if ( signatureBuffer.length !== digest.length || !crypto.timingSafeEqual(digest, signatureBuffer) @@ -33,6 +39,9 @@ export function verifySignature( throw new Error('The signature is invalid.'); } + // AUDIT [CRITICAL-3]: Library code must never log to the console. + // This pollutes stdout of every app using this SDK in production. + // Fix: Remove this line entirely. console.log('The signature is valid'); return true; }