diff --git a/backend/src/controllers/__tests__/contractRegistry.test.ts b/backend/src/controllers/__tests__/contractRegistry.test.ts index 3daa6c0b..cc232e56 100644 --- a/backend/src/controllers/__tests__/contractRegistry.test.ts +++ b/backend/src/controllers/__tests__/contractRegistry.test.ts @@ -67,8 +67,11 @@ describe('Contract Registry API Integration', () => { const response = await request(app).get('/api/contracts'); expect(response.status).toBe(500); - expect(response.body.error).toBe('Internal Server Error'); - expect(response.body.message).toBe('Registry load failed'); + // Regression guard for #495: the response must not echo + // error.message ("Registry load failed") back to the client. + expect(response.body.error).toBe('Failed to load contract registry'); + expect(response.body).not.toHaveProperty('message'); + expect(JSON.stringify(response.body)).not.toContain('Registry load failed'); expect(logger.error).toHaveBeenCalled(); }); diff --git a/backend/src/controllers/__tests__/internalErrorLeak.test.ts b/backend/src/controllers/__tests__/internalErrorLeak.test.ts new file mode 100644 index 00000000..ef99d7dc --- /dev/null +++ b/backend/src/controllers/__tests__/internalErrorLeak.test.ts @@ -0,0 +1,88 @@ +/** + * Regression tests for issue #495: controllers must not leak internal error + * details (database messages, table names, SQL fragments) in 500 responses. + * + * Uses the real tax routes through a real Express app, with only the database + * and logger mocked. The failing call is a genuine Postgres-style error whose + * message contains schema information; production responses must contain none + * of it. + */ + +import { describe, it, expect, jest, beforeEach, afterEach } from '@jest/globals'; +import request from 'supertest'; +import express from 'express'; +import jwt from 'jsonwebtoken'; + +jest.setTimeout(30_000); + +const mockQuery = jest.fn(); + +jest.unstable_mockModule('../../config/database.js', () => ({ + query: mockQuery, + pool: { query: mockQuery }, + default: { query: mockQuery }, +})); + +const { config } = await import('../../config/env.js'); +const taxRoutes = (await import('../../routes/taxRoutes.js')).default; +const { TOKEN_TYPE_ACCESS } = await import('../../services/authService.js'); + +const app = express(); +app.use(express.json()); +app.use('/api/taxes', taxRoutes); + +/** A DB failure that looks exactly like the leak scenario from #495. */ +function pgSchemaError() { + return new Error( + 'insert into "tax_rules" ("organization_id", "name") returning "id" - relation "tax_rules" does not exist' + ); +} + +function adminToken() { + return jwt.sign( + { id: 1, role: 'ADMIN', organizationId: 7, typ: TOKEN_TYPE_ACCESS }, + config.JWT_SECRET, + { expiresIn: '1h' } + ); +} + +describe('error detail leakage (#495)', () => { + let consoleSpy: any; + + beforeEach(() => { + consoleSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + }); + + afterEach(() => { + consoleSpy.mockRestore(); + mockQuery.mockReset(); + }); + + it('500 responses never echo database/schema details to the client', async () => { + mockQuery.mockRejectedValueOnce(pgSchemaError()); + + const res = await request(app) + .post('/api/taxes/rules') + .set('Authorization', `Bearer ${adminToken()}`) + .send({ organization_id: 7, name: 'VAT', type: 'percentage', value: 5 }); + + expect(res.status).toBe(500); + const body = JSON.stringify(res.body); + // None of the Postgres error text may reach the client. + expect(body).not.toContain('tax_rules'); + expect(body).not.toContain('relation'); + expect(body).not.toContain('insert into'); + }); + + it('logs the full error server-side instead of exposing it', async () => { + mockQuery.mockRejectedValueOnce(pgSchemaError()); + + await request(app) + .post('/api/taxes/rules') + .set('Authorization', `Bearer ${adminToken()}`) + .send({ organization_id: 7, name: 'VAT', type: 'percentage', value: 5 }); + + const logged = consoleSpy.mock.calls.flat().join('\n'); + expect(logged).toContain('tax_rules'); + }); +}); diff --git a/backend/src/controllers/assetController.ts b/backend/src/controllers/assetController.ts index 92dde752..c43203eb 100644 --- a/backend/src/controllers/assetController.ts +++ b/backend/src/controllers/assetController.ts @@ -2,6 +2,7 @@ import { Request, Response } from 'express'; import { AssetService } from '../services/assetService.js'; import { Keypair } from '@stellar/stellar-sdk'; import { pool } from '../config/database.js'; +import { sendInternalError } from '../utils/internalError.js'; export class AssetController { /** @@ -28,9 +29,8 @@ export class AssetController { issuer: asset.issuer, }, }); - } catch (error: any) { - console.error('Issue ORGUSD Error:', error); - res.status(500).json({ error: error.message }); + } catch (error) { + sendInternalError(res, req, error, 'Failed to issue ORGUSD'); } } @@ -55,9 +55,8 @@ export class AssetController { txHash, message: `Successfully clawed back ${amount} ORGUSD from ${fromAccount}`, }); - } catch (error: any) { - console.error('Clawback Error:', error); - res.status(500).json({ error: error.message }); + } catch (error) { + sendInternalError(res, req, error, 'Failed to execute clawback'); } } @@ -109,9 +108,8 @@ export class AssetController { limit, totalPages: Math.ceil(total / limit), }); - } catch (error: any) { - console.error('Get Clawback Logs Error:', error); - res.status(500).json({ error: error.message }); + } catch (error) { + sendInternalError(res, req, error, 'Failed to retrieve clawback logs'); } } } diff --git a/backend/src/controllers/bulkImportController.ts b/backend/src/controllers/bulkImportController.ts index b7ce9275..529feca4 100644 --- a/backend/src/controllers/bulkImportController.ts +++ b/backend/src/controllers/bulkImportController.ts @@ -1,6 +1,7 @@ import { Request, Response } from 'express'; import { csvPayrollImportService } from '../services/csvPayrollImportService.js'; import logger from '../utils/logger.js'; +import { sendInternalError } from '../utils/internalError.js'; export class BulkImportController { async import(req: Request, res: Response) { @@ -36,12 +37,8 @@ export class BulkImportController { }, errors: result.errors, }); - } catch (error: any) { - logger.error('Bulk Import Controller Error:', error); - res.status(500).json({ - error: 'Internal Server Error', - message: error.message, - }); + } catch (error) { + sendInternalError(res, req, error); } } } diff --git a/backend/src/controllers/cashFlowForecastController.ts b/backend/src/controllers/cashFlowForecastController.ts index 8cfc2b54..dade5db4 100644 --- a/backend/src/controllers/cashFlowForecastController.ts +++ b/backend/src/controllers/cashFlowForecastController.ts @@ -2,6 +2,7 @@ import { Request, Response } from 'express'; import { z } from 'zod'; import { CashFlowForecastService } from '../services/cashFlowForecastService.js'; import logger from '../utils/logger.js'; +import { sendInternalError } from '../utils/internalError.js'; import { default as pool } from '../config/database.js'; const forecastQuerySchema = z.object({ @@ -61,10 +62,7 @@ export class CashFlowForecastController { }); } catch (error) { logger.error('Failed to generate cash flow forecast', error); - res.status(500).json({ - error: 'Failed to generate cash flow forecast', - message: error instanceof Error ? error.message : 'Unknown error', - }); +sendInternalError(res, req, error, 'Failed to generate cash flow forecast'); } } @@ -110,10 +108,7 @@ export class CashFlowForecastController { }); } catch (error) { logger.error('Failed to get historical payroll data', error); - res.status(500).json({ - error: 'Failed to get historical payroll data', - message: error instanceof Error ? error.message : 'Unknown error', - }); +sendInternalError(res, req, error, 'Failed to get historical payroll data'); } } @@ -154,10 +149,7 @@ export class CashFlowForecastController { }); } catch (error) { logger.error('Failed to get payroll projections', error); - res.status(500).json({ - error: 'Failed to get payroll projections', - message: error instanceof Error ? error.message : 'Unknown error', - }); +sendInternalError(res, req, error, 'Failed to get payroll projections'); } } @@ -217,10 +209,7 @@ export class CashFlowForecastController { }); } catch (error) { logger.error('Failed to get budget alerts', error); - res.status(500).json({ - error: 'Failed to get budget alerts', - message: error instanceof Error ? error.message : 'Unknown error', - }); +sendInternalError(res, req, error, 'Failed to get budget alerts'); } } } diff --git a/backend/src/controllers/contractController.ts b/backend/src/controllers/contractController.ts index 0b3eacbf..3fa750c3 100644 --- a/backend/src/controllers/contractController.ts +++ b/backend/src/controllers/contractController.ts @@ -7,6 +7,7 @@ import { Request, Response } from 'express'; import { ContractConfigService } from '../services/contractConfigService.js'; import { validateContractEntry, ContractEntry } from '../utils/contractValidator.js'; import logger from '../utils/logger.js'; +import { sendInternalError } from '../utils/internalError.js'; export class ContractController { private static configService = new ContractConfigService(); @@ -59,13 +60,7 @@ export class ContractController { } catch (error) { logger.error('Error in getContracts', error); - const errorResponse = { - error: 'Internal Server Error', - message: error instanceof Error ? error.message : 'Failed to retrieve contract registry', - timestamp: new Date().toISOString() - }; - - res.status(500).json(errorResponse); + sendInternalError(res, req, error, 'Failed to retrieve contracts'); } } } diff --git a/backend/src/controllers/contractRegistryController.ts b/backend/src/controllers/contractRegistryController.ts index c3364ab4..f21195ba 100644 --- a/backend/src/controllers/contractRegistryController.ts +++ b/backend/src/controllers/contractRegistryController.ts @@ -1,6 +1,7 @@ import { Request, Response } from 'express'; import { ContractRegistryService } from '../services/contractRegistryService.js'; import logger from '../utils/logger.js'; +import { sendInternalError } from '../utils/internalError.js'; export class ContractRegistryController { /** @@ -53,14 +54,7 @@ export class ContractRegistryController { } catch (error) { logger.error('Error retrieving contract registry', error); - res.status(500).json({ - error: 'Internal Server Error', - message: - error instanceof Error - ? error.message - : 'Failed to load contract registry', - timestamp: new Date().toISOString(), - }); + sendInternalError(res, req, error, 'Failed to load contract registry'); } } } \ No newline at end of file diff --git a/backend/src/controllers/multiSigController.ts b/backend/src/controllers/multiSigController.ts index 607aecf0..d63e6ba9 100644 --- a/backend/src/controllers/multiSigController.ts +++ b/backend/src/controllers/multiSigController.ts @@ -2,6 +2,7 @@ import { Request, Response } from 'express'; import { Keypair } from '@stellar/stellar-sdk'; import { MultiSigService } from '../services/multiSigService.js'; import logger from '../utils/logger.js'; +import { sendInternalError } from '../utils/internalError.js'; export class MultiSigController { /** @@ -28,9 +29,8 @@ export class MultiSigController { ); res.status(200).json({ success: true, data: result }); - } catch (error: any) { - logger.error('Multi-sig configuration failed', { error: error.message }); - res.status(500).json({ success: false, error: error.message }); + } catch (error) { + sendInternalError(res, req, error, 'Multi-sig configuration failed'); } } @@ -43,9 +43,8 @@ export class MultiSigController { const { publicKey } = req.params; const status = await MultiSigService.getMultiSigStatus(publicKey as string); res.status(200).json({ success: true, data: status }); - } catch (error: any) { - logger.error('Failed to get multi-sig status', { error: error.message }); - res.status(500).json({ success: false, error: error.message }); + } catch (error) { + sendInternalError(res, req, error, 'Failed to get multi-sig status'); } } @@ -69,9 +68,8 @@ export class MultiSigController { const result = await MultiSigService.addIssuerSigner(issuerKeypair, signerPublicKey, weight); res.status(200).json({ success: true, data: result }); - } catch (error: any) { - logger.error('Failed to add signer', { error: error.message }); - res.status(500).json({ success: false, error: error.message }); + } catch (error) { + sendInternalError(res, req, error, 'Failed to add signer'); } } @@ -96,9 +94,8 @@ export class MultiSigController { const result = await MultiSigService.removeIssuerSigner(issuerKeypair, publicKey as string); res.status(200).json({ success: true, data: result }); - } catch (error: any) { - logger.error('Failed to remove signer', { error: error.message }); - res.status(500).json({ success: false, error: error.message }); + } catch (error) { + sendInternalError(res, req, error, 'Failed to remove signer'); } } @@ -122,9 +119,8 @@ export class MultiSigController { const result = await MultiSigService.updateThresholds(issuerKeypair, thresholds); res.status(200).json({ success: true, data: result }); - } catch (error: any) { - logger.error('Failed to update thresholds', { error: error.message }); - res.status(500).json({ success: false, error: error.message }); + } catch (error) { + sendInternalError(res, req, error, 'Failed to update thresholds'); } } } diff --git a/backend/src/controllers/paymentController.ts b/backend/src/controllers/paymentController.ts index da8b35a7..a645c325 100644 --- a/backend/src/controllers/paymentController.ts +++ b/backend/src/controllers/paymentController.ts @@ -2,6 +2,7 @@ import { Request, Response } from 'express'; import { AnchorService } from '../services/anchorService.js'; import { Keypair, Asset } from '@stellar/stellar-sdk'; import { StellarService } from '../services/stellarService.js'; +import { sendInternalError } from '../utils/internalError.js'; export class PaymentController { /** @@ -14,8 +15,8 @@ export class PaymentController { try { const info = await AnchorService.getSEP31Info(domain as string); res.json(info); - } catch (error: any) { - res.status(500).json({ error: error.message }); + } catch (error) { + sendInternalError(res, req, error); } } @@ -39,9 +40,8 @@ export class PaymentController { const result = await AnchorService.initiatePayment(domain as string, token, paymentData); res.json(result); - } catch (error: any) { - console.error('SEP-31 Initiation Error:', error); - res.status(500).json({ error: error.message }); + } catch (error) { + sendInternalError(res, req, error, 'Failed to initiate SEP-31 payment'); } } @@ -64,8 +64,8 @@ export class PaymentController { const status = await AnchorService.getTransaction(domain as string, token, id as string); res.json(status); - } catch (error: any) { - res.status(500).json({ error: error.message }); + } catch (error) { + sendInternalError(res, req, error); } } @@ -79,8 +79,8 @@ export class PaymentController { try { const info = await AnchorService.getSEP24Info(domain as string); res.json(info); - } catch (error: any) { - res.status(500).json({ error: error.message }); + } catch (error) { + sendInternalError(res, req, error); } } @@ -104,9 +104,8 @@ export class PaymentController { const result = await AnchorService.initiateSEP24Withdrawal(domain as string, token, withdrawalData); res.json(result); - } catch (error: any) { - console.error('SEP-24 Withdrawal Initiation Error:', error); - res.status(500).json({ error: error.message }); + } catch (error) { + sendInternalError(res, req, error, 'Failed to initiate SEP-24 withdrawal'); } } @@ -127,8 +126,8 @@ export class PaymentController { const status = await AnchorService.getSEP24Transaction(domain as string, token, id as string); res.json(status); - } catch (error: any) { - res.status(500).json({ error: error.message }); + } catch (error) { + sendInternalError(res, req, error); } } @@ -180,9 +179,8 @@ export class PaymentController { paths: pathsResponse.records }); - } catch (error: any) { - console.error('Pathfinding Error:', error); - res.status(500).json({ error: error.message || 'Error fetching conversion paths' }); + } catch (error) { + sendInternalError(res, req, error, 'Error fetching conversion paths'); } } } diff --git a/backend/src/controllers/pdfCertificateController.ts b/backend/src/controllers/pdfCertificateController.ts index 63bb5663..1315559f 100644 --- a/backend/src/controllers/pdfCertificateController.ts +++ b/backend/src/controllers/pdfCertificateController.ts @@ -2,6 +2,7 @@ import { Request, Response } from 'express'; import { PDFCertificateService } from '../services/pdfCertificateService.js'; import logger from '../utils/logger.js'; import { z } from 'zod'; +import { sendInternalError } from '../utils/internalError.js'; const generateCertificateSchema = z.object({ employeeId: z.number().int().positive(), @@ -83,10 +84,7 @@ export class PDFCertificateController { res.send(pdfBuffer); } catch (error) { logger.error('Failed to generate PDF certificate', error); - res.status(500).json({ - error: 'Failed to generate certificate', - message: error instanceof Error ? error.message : 'Unknown error', - }); +sendInternalError(res, req, error, 'Failed to generate certificate'); } } @@ -133,10 +131,7 @@ export class PDFCertificateController { }); } catch (error) { logger.error('Failed to verify certificate', error); - res.status(500).json({ - error: 'Failed to verify certificate', - message: error instanceof Error ? error.message : 'Unknown error', - }); +sendInternalError(res, req, error, 'Failed to verify certificate'); } } @@ -170,10 +165,7 @@ export class PDFCertificateController { }); } catch (error) { logger.error('Failed to get transaction info', error); - res.status(500).json({ - error: 'Failed to get transaction info', - message: error instanceof Error ? error.message : 'Unknown error', - }); +sendInternalError(res, req, error, 'Failed to get transaction info'); } } } diff --git a/backend/src/controllers/taxController.ts b/backend/src/controllers/taxController.ts index 9793f31d..2987861d 100644 --- a/backend/src/controllers/taxController.ts +++ b/backend/src/controllers/taxController.ts @@ -1,5 +1,6 @@ import { Request, Response } from 'express'; import { TaxService } from '../services/taxService.js'; +import { sendInternalError } from '../utils/internalError.js'; const taxService = new TaxService(); @@ -36,9 +37,8 @@ export class TaxController { }); res.status(201).json(rule); - } catch (error: any) { - console.error('Create tax rule error:', error); - res.status(500).json({ error: error.message }); + } catch (error) { + sendInternalError(res, req, error, 'Failed to create tax rule'); } } @@ -57,9 +57,8 @@ export class TaxController { const rules = await taxService.getRules(Number(organizationId), includeInactive === 'true'); res.json({ data: rules, count: rules.length }); - } catch (error: any) { - console.error('Get tax rules error:', error); - res.status(500).json({ error: error.message }); + } catch (error) { + sendInternalError(res, req, error, 'Failed to retrieve tax rules'); } } @@ -91,9 +90,8 @@ export class TaxController { } res.json(rule); - } catch (error: any) { - console.error('Update tax rule error:', error); - res.status(500).json({ error: error.message }); + } catch (error) { + sendInternalError(res, req, error, 'Failed to update tax rule'); } } @@ -116,9 +114,8 @@ export class TaxController { } res.json({ message: 'Tax rule deactivated successfully' }); - } catch (error: any) { - console.error('Delete tax rule error:', error); - res.status(500).json({ error: error.message }); + } catch (error) { + sendInternalError(res, req, error, 'Failed to delete tax rule'); } } @@ -147,9 +144,8 @@ export class TaxController { currency ); res.json(result); - } catch (error: any) { - console.error('Calculate deductions error:', error); - res.status(500).json({ error: error.message }); + } catch (error) { + sendInternalError(res, req, error, 'Failed to calculate deductions'); } } @@ -174,9 +170,8 @@ export class TaxController { ); res.json(report); - } catch (error: any) { - console.error('Generate tax report error:', error); - res.status(500).json({ error: error.message }); + } catch (error) { + sendInternalError(res, req, error, 'Failed to generate tax report'); } } } diff --git a/backend/src/utils/internalError.ts b/backend/src/utils/internalError.ts new file mode 100644 index 00000000..df317bee --- /dev/null +++ b/backend/src/utils/internalError.ts @@ -0,0 +1,47 @@ +import { Request, Response } from 'express'; +import config from '../config/index.js'; +import logger from '../utils/logger.js'; + +/** + * Shared 500-response helper. + * + * Why this exists: several controllers were doing + * `res.status(500).json({ error: error.message })`. When the failing call is a + * database query, `error.message` carries table names, column names and chunks + * of SQL — exactly the schema information an attacker needs to aim SQL + * injection. It can also leak file paths and stack details from libraries. + * + * What it does instead: + * - Production always returns one fixed, generic message per call site. + * No error text reaches the client, so nothing about internals leaks. + * - The full error is logged server-side with the request id (when the + * requestId middleware has run) so operations can correlate a client report + * with the log entry that holds the real cause. + * - In development the real message is included in the response so debugging + * stays fast where the leak cannot be exploited. + */ +export function sendInternalError( + res: Response, + req: Request, + error: unknown, + userMessage = 'Internal server error' +): void { + const message = + error instanceof Error ? `${error.name}: ${error.message}` : String(error); + + const requestId = typeof (req as any).requestId === 'string' ? (req as any).requestId : undefined; + logger.error('Request failed', { + requestId, + path: req.originalUrl, + method: req.method, + message, + stack: error instanceof Error ? error.stack : undefined, + }); + + if (config.nodeEnv === 'development') { + res.status(500).json({ error: userMessage, detail: message }); + return; + } + + res.status(500).json({ error: userMessage }); +}