mirror of
https://github.com/Sea-Haven-Industries/payments-dashboard.git
synced 2026-09-30 11:13:11 +00:00
fix(security): verify Slack signatures on /slack/events and stop logging raw BoA responses (#63)
Some checks failed
Deploy / deploy (push) Has been cancelled
Some checks failed
Deploy / deploy (push) Has been cancelled
Two agentic-review findings on the payments-dashboard security surface: - CRITICAL (CWE-862): the /slack/events endpoint (slackAppHome) performed no Slack signing-secret verification, so an unauthenticated caller could forge app_home_opened/block_actions events and exfiltrate payment data via DynamoDB scans + views.publish. Add HMAC-SHA256 signature verification with a 5-minute replay window over the raw request body, mirroring the existing expenseReceiver pattern. Requests failing verification get a 401 before any body parsing, DynamoDB access, or Slack API call. Adds the SLACK_SIGNING_SECRET_NAME env var and an additive secretsmanager grant for payments-dashboard/slack-signing-secret. - HIGH (CWE-532): full BoA CashPro response bodies + headers were logged to CloudWatch and persisted to DynamoDB (response_body/response_headers), which could expose account numbers/PII. Drop those fields from both boa_txn# records and stop logging raw bodies/headers on both the main submit path and the backfill retry path; log status + txnId only (txnId still correlates to BoA support for the full body). Verification: sam validate, node --check both handlers. /sh-security-review (detector fan-out + verifier) and GPT-4.1 cross-family review run; the cross-family review confirmed the IAM grant is purely additive.
This commit is contained in:
parent
da71c6e154
commit
0eeca1e7a5
4 changed files with 61 additions and 13 deletions
|
|
@ -12,7 +12,7 @@ AWS SAM application that ingests payment CSVs, syncs check data with Bank of Ame
|
|||
- **ProcessPayrollEmail** — Lambda triggered by S3 (inbound email) and SQS (batch timer). SES receives Gusto payroll emails at `payroll@int.seahaven.com`, stores them to S3, and this Lambda parses the email body, extracts financial data, and posts a combined Slack notification (employee payroll + contractor payments) after a 10-minute batching window. Runs outside VPC.
|
||||
- **ProcessPaymentCsv** — Lambda triggered by S3 CSV upload. Parses Stampli payment exports, upserts to DynamoDB, and submits new/cancelled checks to the CashPro Check Management API.
|
||||
- **FetchBoaTransactions** — Scheduled Lambda (weekdays 9am ET). Calls the CashPro Previous Day Transaction Inquiry API and matches cleared/returned checks back to DynamoDB records.
|
||||
- **SlackAppHome** — Lambda behind API Gateway. Renders the payments dashboard on the Slack App Home tab with outstanding aging buckets and drill-down modals.
|
||||
- **SlackAppHome** — Lambda behind API Gateway (`POST /slack/events`). Verifies the Slack signing secret (HMAC-SHA256, 5-minute replay window) before processing, then renders the payments dashboard on the Slack App Home tab with outstanding aging buckets and drill-down modals.
|
||||
|
||||
- **ExpenseReceiver** — Lambda behind API Gateway (`POST /slack/expense-events`). Verifies the Slack signing secret (HMAC-SHA256), handles URL verification challenges, and async-invokes ExpenseProcessor. Runs outside VPC.
|
||||
- **ExpenseProcessor** — Async Lambda invoked by ExpenseReceiver. Processes `:white_check_mark:` reactions to advance expense messages through a four-stage Slack channel pipeline: Submitted → Processed → Authorized → Matched. Runs outside VPC.
|
||||
|
|
@ -80,6 +80,7 @@ All BoA and Slack credentials are stored in AWS Secrets Manager (per `engineerin
|
|||
| Secret | Type | Contents |
|
||||
|--------|------|----------|
|
||||
| `payments-dashboard/slack-bot-token` | plaintext | Slack Bot OAuth token (used by `processPayrollEmail`, `slackAppHome`) |
|
||||
| `payments-dashboard/slack-signing-secret` | plaintext | Slack signing secret for `slackAppHome` request verification |
|
||||
| `payments-dashboard/boa-check-mgmt` | JSON | `appId`, `clientId`, `token`, `accountNumber`, `companyId` — Check Management API (`processPaymentCsv`) |
|
||||
| `payments-dashboard/boa-reporting` | JSON | `appId`, `clientId`, `token`, `accountNumber`, `bankId` — Reporting API (`fetchBoaTransactions`) |
|
||||
|
||||
|
|
|
|||
|
|
@ -160,8 +160,6 @@ async function backfillBoA() {
|
|||
transaction_id: transactionId,
|
||||
check_numbers: items.map((i) => i.checkNumber),
|
||||
total_amount: items.reduce((sum, i) => sum + parseFloat(i.amount), 0).toFixed(2),
|
||||
response_headers: JSON.stringify(headers),
|
||||
response_body: text,
|
||||
success: res.ok,
|
||||
processed_items: data.processedItems || 0,
|
||||
total_items: data.totalItems || 0,
|
||||
|
|
@ -173,7 +171,7 @@ async function backfillBoA() {
|
|||
throw new Error(`Backfill ${label} failed: BoA returned invalid JSON (HTTP ${res.status})`);
|
||||
}
|
||||
|
||||
return { ok: res.ok, status: res.status, data, text, parseError };
|
||||
return { ok: res.ok, status: res.status, data, text, parseError, transactionId };
|
||||
};
|
||||
|
||||
for (let i = 0; i < toSubmit.length; i += BOA_BATCH_SIZE) {
|
||||
|
|
@ -212,10 +210,13 @@ async function backfillBoA() {
|
|||
|
||||
const retryResult = await submitBatch(retryItems, `batch ${batchNum} retry`);
|
||||
if (!retryResult.ok) {
|
||||
console.error(`Backfill batch ${batchNum} retry failed:`, JSON.stringify(retryResult.data));
|
||||
// Do not log/throw the raw BoA response body — it can carry account
|
||||
// numbers/PII. Status + txnId are enough to triage; the full body is
|
||||
// retrievable from BoA support via txnId.
|
||||
const detail = retryResult.parseError
|
||||
? "BoA returned a non-JSON response"
|
||||
: `BoA returned ${JSON.stringify(retryResult.data)}`;
|
||||
: "BoA returned a non-duplicate error";
|
||||
console.error(`Backfill batch ${batchNum} retry failed: HTTP ${retryResult.status}, txnId=${retryResult.transactionId}`);
|
||||
throw new Error(`Backfill batch ${batchNum} retry failed: ${detail} (HTTP ${retryResult.status})`);
|
||||
}
|
||||
totalProcessed += retryResult.data.processedItems;
|
||||
|
|
@ -424,14 +425,14 @@ export const handler = async (event) => {
|
|||
transaction_id: transactionId,
|
||||
check_numbers: checkNumbers,
|
||||
total_amount: totalAmount.toFixed(2),
|
||||
response_headers: JSON.stringify(headers),
|
||||
response_body: text,
|
||||
ttl: Math.floor(Date.now() / 1000) + 90 * 24 * 60 * 60,
|
||||
};
|
||||
|
||||
if (!res.ok) {
|
||||
console.error(`BoA ${action} error ${res.status}:`, text);
|
||||
console.error(`BoA ${action} response headers:`, JSON.stringify(headers));
|
||||
// Do not log the raw response body/headers — the BoA CashPro response
|
||||
// can carry account numbers/PII. Status + correlation id are enough to
|
||||
// triage; the full body is retrievable from BoA support via txnId.
|
||||
console.error(`BoA ${action} failed: HTTP ${res.status}, txnId=${transactionId}`);
|
||||
record.success = false;
|
||||
await ddb.send(new PutCommand({ TableName: TABLE_NAME, Item: record }));
|
||||
throw new Error(`BoA ${action} failed: ${res.status}`);
|
||||
|
|
@ -449,8 +450,6 @@ export const handler = async (event) => {
|
|||
console.log(
|
||||
`BoA ${action}: ${data.processedItems}/${data.totalItems} processed, ${data.unprocessedItems} failed, txnId=${transactionId}`
|
||||
);
|
||||
console.log(`BoA ${action} response headers:`, JSON.stringify(headers));
|
||||
console.log(`BoA ${action} response body:`, text);
|
||||
return data;
|
||||
};
|
||||
|
||||
|
|
|
|||
|
|
@ -1,3 +1,4 @@
|
|||
import crypto from "node:crypto";
|
||||
import { DynamoDBClient } from "@aws-sdk/client-dynamodb";
|
||||
import { DynamoDBDocumentClient, GetCommand, ScanCommand } from "@aws-sdk/lib-dynamodb";
|
||||
import { SecretsManagerClient, GetSecretValueCommand } from "@aws-sdk/client-secrets-manager";
|
||||
|
|
@ -5,6 +6,7 @@ import { SecretsManagerClient, GetSecretValueCommand } from "@aws-sdk/client-sec
|
|||
const ddb = DynamoDBDocumentClient.from(new DynamoDBClient());
|
||||
const secrets = new SecretsManagerClient();
|
||||
const TABLE_NAME = process.env.TABLE_NAME;
|
||||
const SLACK_SIGNING_SECRET_NAME = process.env.SLACK_SIGNING_SECRET_NAME;
|
||||
|
||||
let cachedToken;
|
||||
async function getSlackToken() {
|
||||
|
|
@ -16,6 +18,34 @@ async function getSlackToken() {
|
|||
return cachedToken;
|
||||
}
|
||||
|
||||
let cachedSigningSecret;
|
||||
async function getSigningSecret() {
|
||||
if (cachedSigningSecret) return cachedSigningSecret;
|
||||
const { SecretString } = await secrets.send(
|
||||
new GetSecretValueCommand({ SecretId: SLACK_SIGNING_SECRET_NAME })
|
||||
);
|
||||
cachedSigningSecret = SecretString;
|
||||
return cachedSigningSecret;
|
||||
}
|
||||
|
||||
// Verify the Slack request signature (HMAC-SHA256 over v0:<ts>:<raw body>).
|
||||
// Rejects requests older than 5 minutes to blunt replay attacks.
|
||||
function verifySignature(body, timestamp, signature, secret) {
|
||||
if (!timestamp || !signature) return false;
|
||||
const ts = Number(timestamp);
|
||||
if (!Number.isFinite(ts)) return false;
|
||||
if (Math.abs(Date.now() / 1000 - ts) > 300) return false;
|
||||
|
||||
const base = `v0:${timestamp}:${body}`;
|
||||
const expected =
|
||||
"v0=" + crypto.createHmac("sha256", secret).update(base).digest("hex");
|
||||
|
||||
const expectedBuf = Buffer.from(expected);
|
||||
const signatureBuf = Buffer.from(signature);
|
||||
if (expectedBuf.length !== signatureBuf.length) return false;
|
||||
return crypto.timingSafeEqual(expectedBuf, signatureBuf);
|
||||
}
|
||||
|
||||
// --- Shared helpers used by both home view and modals ---
|
||||
|
||||
const formatCurrency = (value) =>
|
||||
|
|
@ -118,6 +148,21 @@ export const handler = async (event) => {
|
|||
rawBody = Buffer.from(rawBody, "base64").toString("utf-8");
|
||||
}
|
||||
|
||||
// Verify the Slack signature over the raw request body before doing anything
|
||||
// else — without this, an unauthenticated caller could forge events and
|
||||
// exfiltrate payment data via views.publish.
|
||||
const headers = Object.fromEntries(
|
||||
Object.entries(event.headers || {}).map(([k, v]) => [k.toLowerCase(), v])
|
||||
);
|
||||
const timestamp = headers["x-slack-request-timestamp"] || "";
|
||||
const signature = headers["x-slack-signature"] || "";
|
||||
|
||||
const signingSecret = await getSigningSecret();
|
||||
if (!verifySignature(rawBody, timestamp, signature, signingSecret)) {
|
||||
console.log("Signature verification failed");
|
||||
return { statusCode: 401, body: "unauthorized" };
|
||||
}
|
||||
|
||||
// Slack sends block_actions as form-encoded: payload=<JSON>
|
||||
let body;
|
||||
if (rawBody.startsWith("payload=")) {
|
||||
|
|
|
|||
|
|
@ -967,6 +967,7 @@ Resources:
|
|||
Environment:
|
||||
Variables:
|
||||
SLACK_BOT_TOKEN_SECRET_NAME: payments-dashboard/slack-bot-token
|
||||
SLACK_SIGNING_SECRET_NAME: payments-dashboard/slack-signing-secret
|
||||
Events:
|
||||
SlackEvent:
|
||||
Type: HttpApi
|
||||
|
|
@ -987,7 +988,9 @@ Resources:
|
|||
Statement:
|
||||
- Effect: Allow
|
||||
Action: secretsmanager:GetSecretValue
|
||||
Resource: !Sub arn:aws:secretsmanager:${AWS::Region}:${AWS::AccountId}:secret:payments-dashboard/slack-bot-token-*
|
||||
Resource:
|
||||
- !Sub arn:aws:secretsmanager:${AWS::Region}:${AWS::AccountId}:secret:payments-dashboard/slack-bot-token-*
|
||||
- !Sub arn:aws:secretsmanager:${AWS::Region}:${AWS::AccountId}:secret:payments-dashboard/slack-signing-secret-*
|
||||
- Version: "2012-10-17"
|
||||
Statement:
|
||||
- Effect: Allow
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue