Add API key management and webhook infrastructure with Express + PostgreSQL#15
Add API key management and webhook infrastructure with Express + PostgreSQL#15
Conversation
- Install express, pg, argon2, helmet, express-rate-limit dependencies - Extend Zod config schema for database and webhook settings - Add database migration SQL for api_keys, webhooks, webhook_deliveries tables - Create database client with connection pool - Implement API key service with argon2 hashing - Add authentication middleware for Bearer tokens - Create API key management endpoints (POST, GET, DELETE) - Implement webhook service with HMAC SHA-256 signing - Add webhook endpoints (POST, GET, DELETE, test, replay) - Bootstrap Express server with health check and security middleware - Update index.ts to start HTTP server - Add comprehensive README documentation - Update .env.example with new configuration variables Co-authored-by: lippytm <65956507+lippytm@users.noreply.github.com>
- Add webhook signature verification tests - Verify server startup and health endpoint - All tests passing (6/6) - Build successful Co-authored-by: lippytm <65956507+lippytm@users.noreply.github.com>
- Remove sensitive query text from database logs to prevent credential exposure - Add production warning for unauthenticated API key creation endpoint - Clarify idempotency token behavior in documentation - Add note about webhook secret plaintext storage - Add warning about in-memory idempotency store limitations - Document timing attack vulnerability in API key verification - Add note about webhook delivery concurrency limits Co-authored-by: lippytm <65956507+lippytm@users.noreply.github.com>
lippytm
left a comment
There was a problem hiding this comment.
I hope its okay I don’t know what to do! I need someone to fix these issues
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive API key management and webhook delivery system integrated with the existing TypeScript infrastructure. The implementation adds Express HTTP server capabilities, PostgreSQL database integration, and security features including Argon2id password hashing and HMAC webhook signatures.
Changes:
- Added database schema with three tables (api_keys, webhooks, webhook_deliveries) including indexes and triggers
- Implemented API key service with secure generation, verification, and scope-based authorization
- Implemented webhook service with HMAC signature verification and retry logic with exponential backoff
- Added Express server with security middleware (Helmet, rate limiting) and authentication middleware
- Extended Zod configuration schema for database, API key, and webhook settings
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 30 comments.
Show a summary per file
| File | Description |
|---|---|
| migrations/001_initial_schema.sql | PostgreSQL schema defining api_keys, webhooks, and webhook_deliveries tables with indexes and triggers |
| src/database/index.ts | Database connection pooling and query execution with logging |
| src/config/index.ts | Extended configuration schema for database, API key, and webhook settings |
| src/services/api-key.service.ts | API key generation, verification with Argon2id hashing, and scope management |
| src/services/webhook.service.ts | Webhook HMAC signing, delivery with retry logic, and event triggering |
| src/middleware/auth.middleware.ts | Bearer token authentication with scope enforcement |
| src/routes/api-keys.routes.ts | REST endpoints for API key creation, listing, and revocation with idempotency support |
| src/routes/webhooks.routes.ts | REST endpoints for webhook management, testing, and replay functionality |
| src/server/index.ts | Express application setup with security middleware and rate limiting |
| src/index.ts | Main entry point with database initialization and graceful shutdown |
| test/webhook.test.js | Unit tests for webhook signature creation and verification |
| README.md | Comprehensive documentation for API endpoints and webhook usage |
| package.json | Added dependencies for Express, PostgreSQL, Argon2, and security middleware |
| .env.example | Environment variable examples for new configuration options |
| .eslintrc.js | Updated ESLint config to allow namespace declarations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| router.post('/', authenticate(), async (req: Request, res: Response): Promise<void> => { | ||
| try { | ||
| const { url, events, secret, active } = req.body; | ||
| const owner = req.apiKey!.owner; | ||
|
|
||
| // Validate required fields | ||
| if (!url || !events) { | ||
| res.status(400).json({ error: 'Missing required fields: url, events' }); | ||
| return; | ||
| } | ||
|
|
||
| if (!Array.isArray(events)) { | ||
| res.status(400).json({ error: 'events must be an array' }); | ||
| return; | ||
| } | ||
|
|
||
| const webhook = await createWebhook({ | ||
| owner, | ||
| url, | ||
| events, | ||
| secret, | ||
| active, | ||
| }); |
There was a problem hiding this comment.
The webhook URL is not validated. Malicious or misconfigured URLs could be used to perform Server-Side Request Forgery (SSRF) attacks against internal services. Add URL validation to ensure the webhook URL points to an allowed destination (e.g., block private IP ranges, localhost, metadata endpoints).
| // Validate required fields | ||
| if (!name || !owner || !scopes) { | ||
| res.status(400).json({ error: 'Missing required fields: name, owner, scopes' }); | ||
| return; | ||
| } | ||
|
|
||
| if (!Array.isArray(scopes)) { | ||
| res.status(400).json({ error: 'scopes must be an array' }); | ||
| return; |
There was a problem hiding this comment.
Missing validation for the 'owner' field format and length. Without constraints, users could create invalid or malicious owner identifiers. Consider validating that the owner is a valid email or username format, and enforce a maximum length constraint.
| export async function updateDeliveryStatus( | ||
| deliveryId: string, | ||
| status: string, | ||
| responseCode?: number, | ||
| responseMs?: number | ||
| ): Promise<void> { | ||
| await query( | ||
| `UPDATE webhook_deliveries | ||
| SET status = $1, | ||
| attempts = attempts + 1, | ||
| response_code = $2, | ||
| response_ms = $3, | ||
| last_attempt_at = CURRENT_TIMESTAMP | ||
| WHERE id = $4`, | ||
| [status, responseCode, responseMs, deliveryId] | ||
| ); |
There was a problem hiding this comment.
Each call to updateDeliveryStatus increments the attempts counter independently. If multiple updates happen concurrently (which shouldn't happen in the current code but could in future modifications), the attempts count could be incorrect. While not currently a problem, consider using a more atomic approach or ensuring the attempts counter matches the actual retry logic (e.g., passing the attempt number rather than incrementing).
| router.post('/', async (req: Request, res: Response): Promise<void> => { | ||
| try { | ||
| const { name, owner, scopes, prefix, pepper } = req.body; | ||
|
|
||
| // Validate required fields | ||
| if (!name || !owner || !scopes) { | ||
| res.status(400).json({ error: 'Missing required fields: name, owner, scopes' }); | ||
| return; | ||
| } | ||
|
|
||
| if (!Array.isArray(scopes)) { | ||
| res.status(400).json({ error: 'scopes must be an array' }); | ||
| return; | ||
| } | ||
|
|
||
| // Handle idempotency | ||
| const idempotencyKey = req.headers['idempotency-key'] as string; | ||
| if (idempotencyKey) { | ||
| const cachedResponse = idempotencyStore.get(idempotencyKey); | ||
| if (cachedResponse) { | ||
| logger.debug('Returning cached response for idempotency key', { idempotencyKey }); | ||
| res.status(200).json(cachedResponse); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| const apiKey = await createApiKey({ name, owner, scopes, prefix, pepper }); | ||
|
|
||
| // Audit log | ||
| logger.info('API key created via endpoint', { | ||
| id: apiKey.id, | ||
| name: apiKey.name, | ||
| owner: apiKey.owner, | ||
| scopes: apiKey.scopes, | ||
| }); | ||
|
|
||
| const response = { | ||
| id: apiKey.id, | ||
| name: apiKey.name, | ||
| owner: apiKey.owner, | ||
| scopes: apiKey.scopes, | ||
| prefix: apiKey.prefix, | ||
| token: apiKey.token, // Only returned once | ||
| created_at: apiKey.created_at, | ||
| }; | ||
|
|
||
| // Store for idempotency | ||
| if (idempotencyKey) { | ||
| idempotencyStore.set(idempotencyKey, response); | ||
| // Clean up after 24 hours | ||
| setTimeout(() => idempotencyStore.delete(idempotencyKey), 24 * 60 * 60 * 1000); | ||
| } | ||
|
|
||
| res.status(201).json(response); | ||
| } catch (error) { | ||
| logger.error('Error creating API key', error instanceof Error ? error : undefined); | ||
| res.status(500).json({ error: 'Internal server error' }); | ||
| } |
There was a problem hiding this comment.
When creating an API key with a duplicate name/owner combination, the database will throw a unique constraint violation error. This error is not caught and handled gracefully in the route handler, resulting in a generic 500 error instead of a more informative 409 Conflict response. Consider catching constraint violation errors and returning appropriate HTTP status codes.
| if (idempotencyKey) { | ||
| idempotencyStore.set(idempotencyKey, response); | ||
| // Clean up after 24 hours | ||
| setTimeout(() => idempotencyStore.delete(idempotencyKey), 24 * 60 * 60 * 1000); |
There was a problem hiding this comment.
Same issue with setTimeout for idempotency cleanup as in the POST endpoint. Timer references are not stored and cannot be cleared on shutdown, potentially causing memory leaks and lost cleanup tasks across restarts.
| import { test } from 'node:test'; | ||
| import assert from 'node:assert'; | ||
| import { | ||
| createWebhookSignature, | ||
| verifyWebhookSignature, | ||
| } from '../dist/services/webhook.service.js'; | ||
|
|
||
| test('webhook signature - creates valid HMAC SHA-256 signature', () => { | ||
| const payload = JSON.stringify({ event: 'test', data: { foo: 'bar' } }); | ||
| const secret = 'test-secret'; | ||
|
|
||
| const signature = createWebhookSignature(payload, secret); | ||
|
|
||
| assert.ok(signature.startsWith('sha256=')); | ||
| assert.strictEqual(signature.length, 71); // 'sha256=' + 64 hex chars | ||
| }); | ||
|
|
||
| test('webhook signature - verifies valid signature', () => { | ||
| const payload = JSON.stringify({ event: 'test', data: { foo: 'bar' } }); | ||
| const secret = 'test-secret'; | ||
|
|
||
| const signature = createWebhookSignature(payload, secret); | ||
| const isValid = verifyWebhookSignature(payload, signature, secret); | ||
|
|
||
| assert.strictEqual(isValid, true); | ||
| }); | ||
|
|
||
| test('webhook signature - rejects invalid signature', () => { | ||
| const payload = JSON.stringify({ event: 'test', data: { foo: 'bar' } }); | ||
| const secret = 'test-secret'; | ||
| const wrongSecret = 'wrong-secret'; | ||
|
|
||
| const signature = createWebhookSignature(payload, wrongSecret); | ||
| const isValid = verifyWebhookSignature(payload, signature, secret); | ||
|
|
||
| assert.strictEqual(isValid, false); | ||
| }); | ||
|
|
||
| test('webhook signature - rejects tampered payload', () => { | ||
| const payload = JSON.stringify({ event: 'test', data: { foo: 'bar' } }); | ||
| const tamperedPayload = JSON.stringify({ event: 'test', data: { foo: 'baz' } }); | ||
| const secret = 'test-secret'; | ||
|
|
||
| const signature = createWebhookSignature(payload, secret); | ||
| const isValid = verifyWebhookSignature(tamperedPayload, signature, secret); | ||
|
|
||
| assert.strictEqual(isValid, false); | ||
| }); |
There was a problem hiding this comment.
The tests don't cover the edge case where signature lengths differ (which will cause timingSafeEqual to throw an error with the current implementation). Add a test case that verifies behavior when a malformed signature is provided (e.g., wrong length, missing prefix).
|
|
||
| -- Index for faster lookups by active status | ||
| CREATE INDEX IF NOT EXISTS idx_webhooks_active ON webhooks(active); | ||
|
|
There was a problem hiding this comment.
The query filters by both active = true AND event type, but there's no composite index on (active, events). While individual indexes exist, a composite index or GIN index on the events array would improve performance when triggering webhooks. Consider adding an index like CREATE INDEX idx_webhooks_active_events ON webhooks USING GIN(events) WHERE active = true;
| -- Index to efficiently query active webhooks by event type | |
| CREATE INDEX IF NOT EXISTS idx_webhooks_active_events | |
| ON webhooks USING GIN (events) | |
| WHERE active = true; |
| router.post('/', authenticate(), async (req: Request, res: Response): Promise<void> => { | ||
| try { | ||
| const { url, events, secret, active } = req.body; | ||
| const owner = req.apiKey!.owner; | ||
|
|
||
| // Validate required fields | ||
| if (!url || !events) { | ||
| res.status(400).json({ error: 'Missing required fields: url, events' }); | ||
| return; | ||
| } | ||
|
|
||
| if (!Array.isArray(events)) { | ||
| res.status(400).json({ error: 'events must be an array' }); | ||
| return; | ||
| } | ||
|
|
||
| const webhook = await createWebhook({ | ||
| owner, | ||
| url, | ||
| events, | ||
| secret, | ||
| active, | ||
| }); | ||
|
|
||
| // Audit log | ||
| logger.info('Webhook created via endpoint', { | ||
| id: webhook.id, | ||
| owner: webhook.owner, | ||
| url: webhook.url, | ||
| events: webhook.events, | ||
| }); | ||
|
|
||
| res.status(201).json({ | ||
| id: webhook.id, | ||
| owner: webhook.owner, | ||
| url: webhook.url, | ||
| events: webhook.events, | ||
| secret: webhook.secret, | ||
| active: webhook.active, | ||
| created_at: webhook.created_at, | ||
| updated_at: webhook.updated_at, | ||
| }); | ||
| } catch (error) { | ||
| logger.error('Error creating webhook', error instanceof Error ? error : undefined); | ||
| res.status(500).json({ error: 'Internal server error' }); | ||
| } | ||
| }); | ||
|
|
||
| /** | ||
| * GET /api/webhooks - List webhooks | ||
| */ | ||
| router.get('/', authenticate(), async (req: Request, res: Response): Promise<void> => { | ||
| try { | ||
| const owner = req.apiKey!.owner; | ||
| const webhooks = await listWebhooks(owner); | ||
|
|
||
| res.status(200).json( | ||
| webhooks.map((webhook) => ({ | ||
| id: webhook.id, | ||
| owner: webhook.owner, | ||
| url: webhook.url, | ||
| events: webhook.events, | ||
| active: webhook.active, | ||
| created_at: webhook.created_at, | ||
| updated_at: webhook.updated_at, | ||
| })) | ||
| ); | ||
| } catch (error) { | ||
| logger.error('Error listing webhooks', error instanceof Error ? error : undefined); | ||
| res.status(500).json({ error: 'Internal server error' }); | ||
| } | ||
| }); | ||
|
|
||
| /** | ||
| * DELETE /api/webhooks/:id - Delete a webhook | ||
| */ | ||
| router.delete('/:id', authenticate(), async (req: Request, res: Response): Promise<void> => { | ||
| try { | ||
| const id = req.params.id as string; | ||
| const owner = req.apiKey!.owner; | ||
|
|
||
| const success = await deleteWebhook(id, owner); | ||
|
|
||
| if (!success) { | ||
| res.status(404).json({ error: 'Webhook not found' }); | ||
| return; | ||
| } | ||
|
|
||
| // Audit log | ||
| logger.info('Webhook deleted via endpoint', { id, owner }); | ||
|
|
||
| res.status(200).json({ message: 'Webhook deleted successfully' }); | ||
| } catch (error) { | ||
| logger.error('Error deleting webhook', error instanceof Error ? error : undefined); | ||
| res.status(500).json({ error: 'Internal server error' }); | ||
| } | ||
| }); | ||
|
|
||
| /** | ||
| * POST /api/webhooks/test - Send a test webhook delivery | ||
| */ | ||
| router.post('/test', authenticate(), async (req: Request, res: Response): Promise<void> => { |
There was a problem hiding this comment.
The authenticate middleware is called without passing the configured pepper value. This means API keys created with a pepper (from config or user-provided) cannot be verified. The middleware should load the global pepper from config and use it for verification. This is a critical issue that breaks authentication when a pepper is configured.
| secret: string | ||
| ): boolean { | ||
| const expectedSignature = createWebhookSignature(payload, secret); | ||
| return crypto.timingSafeEqual(Buffer.from(signature), Buffer.from(expectedSignature)); |
There was a problem hiding this comment.
The timingSafeEqual function requires both buffers to be the same length. If the signature strings differ in length, this will throw an error instead of returning false. Add length check before timingSafeEqual or wrap in try-catch to handle length mismatches gracefully.
| return crypto.timingSafeEqual(Buffer.from(signature), Buffer.from(expectedSignature)); | |
| const receivedBuffer = Buffer.from(signature); | |
| const expectedBuffer = Buffer.from(expectedSignature); | |
| if (receivedBuffer.length !== expectedBuffer.length) { | |
| return false; | |
| } | |
| return crypto.timingSafeEqual(receivedBuffer, expectedBuffer); |
| hmac.update(payload); | ||
| const expectedSignature = `sha256=${hmac.digest('hex')}`; | ||
|
|
||
| return crypto.timingSafeEqual(Buffer.from(signature), Buffer.from(expectedSignature)); |
There was a problem hiding this comment.
The example webhook verification code has the same timingSafeEqual bug as the service code - it will throw an error if the signature lengths don't match. The documentation example should include a length check before calling timingSafeEqual to match best practices.
| return crypto.timingSafeEqual(Buffer.from(signature), Buffer.from(expectedSignature)); | |
| // Safely compare signatures without throwing on length mismatch | |
| if (typeof signature !== 'string') { | |
| return false; | |
| } | |
| const receivedBuffer = Buffer.from(signature, 'utf8'); | |
| const expectedBuffer = Buffer.from(expectedSignature, 'utf8'); | |
| if (receivedBuffer.length !== expectedBuffer.length) { | |
| return false; | |
| } | |
| return crypto.timingSafeEqual(receivedBuffer, expectedBuffer); |
Implements API key authentication and outbound webhook delivery system integrated with existing Zod config and telemetry infrastructure.
Database Schema
Three tables with proper indexes and triggers:
api_keys: Argon2id-hashed secrets, scopes array, prefix-based tokenswebhooks: Event subscriptions with HMAC secretswebhook_deliveries: Delivery tracking with retry metadataMigration:
migrations/001_initial_schema.sqlAPI Key Service
prefix_<64-char-hex>format, Argon2id hashing (64MB, t=3, p=4)Webhook Service
X-Signature: sha256=<hash>headerConfiguration
Extended Zod schema:
DATABASE_URL: PostgreSQL connection (optional, warns if missing)API_KEY_PREFIX/PEPPER: Token customizationWEBHOOK_MAX_ATTEMPTS/BACKOFF_BASE_MS/TIMEOUT_MS: Retry configurationProduction Notes
Security
Original prompt
This pull request was created from Copilot chat.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.