Skip to content

Commit e0bc020

Browse files
devsimzeclaude
andcommitted
Fix TOTP brute-force, broken deploy pipeline, inconsistent error shapes, and stored XSS gaps
- Tighten 2FA challenge rate limiting (3/30s per IP + per account), add account lockout after 10 consecutive failures, and audit-log failed attempts (closes Savitura#378) - Replace placeholder echo deploy steps with real SSH + docker compose deploy, DB migrations, and a post-deploy health check (closes Savitura#381) - Normalize express-validator errors into the canonical { error: { code, message, fields } } shape and document it in the OpenAPI specs (closes Savitura#382) - Sanitize milestone and reward-tier title/description on write (they previously bypassed stripHtml) and backfill any HTML already stored in campaigns/milestones/reward_tiers (closes Savitura#383) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent a41af65 commit e0bc020

9 files changed

Lines changed: 205 additions & 47 deletions

File tree

.github/workflows/deploy.yml

Lines changed: 50 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -6,52 +6,66 @@ on:
66
branches: [main]
77
types:
88
- completed
9+
workflow_dispatch: {}
910

1011
jobs:
1112
deploy:
1213
name: Deploy to Production
1314
runs-on: ubuntu-latest
14-
if: ${{ github.event.workflow_run.conclusion == 'success' }}
15-
16-
steps:
17-
- name: Checkout code
18-
uses: actions/checkout@v4
19-
20-
- name: Log in to GitHub Container Registry
21-
uses: docker/login-action@v3
22-
with:
23-
registry: ghcr.io
24-
username: ${{ github.actor }}
25-
password: ${{ secrets.GITHUB_TOKEN }}
15+
if: ${{ github.event_name == 'workflow_dispatch' || github.event.workflow_run.conclusion == 'success' }}
16+
environment: production
2617

27-
- name: Build and push backend image
28-
uses: docker/build-push-action@v5
18+
steps:
19+
- name: Write production env file and deploy via docker compose
20+
uses: appleboy/ssh-action@v1.0.3
2921
with:
30-
context: ./backend
31-
push: true
32-
tags: ghcr.io/${{ github.repository }}/backend:latest
22+
host: ${{ secrets.DEPLOY_HOST }}
23+
username: ${{ secrets.DEPLOY_USER }}
24+
key: ${{ secrets.DEPLOY_SSH_KEY }}
25+
envs: DATABASE_URL,JWT_SECRET,API_KEY_PEPPER,PLATFORM_SECRET_KEY,USDC_ISSUER,STELLAR_NETWORK,STELLAR_HORIZON_URL,FRONTEND_URL,DEPLOY_PATH
26+
env:
27+
DATABASE_URL: ${{ secrets.DATABASE_URL }}
28+
JWT_SECRET: ${{ secrets.JWT_SECRET }}
29+
API_KEY_PEPPER: ${{ secrets.API_KEY_PEPPER }}
30+
PLATFORM_SECRET_KEY: ${{ secrets.PLATFORM_SECRET_KEY }}
31+
USDC_ISSUER: ${{ secrets.USDC_ISSUER }}
32+
STELLAR_NETWORK: ${{ secrets.STELLAR_NETWORK }}
33+
STELLAR_HORIZON_URL: ${{ secrets.STELLAR_HORIZON_URL }}
34+
FRONTEND_URL: ${{ secrets.FRONTEND_URL }}
35+
DEPLOY_PATH: ${{ secrets.DEPLOY_PATH }}
36+
script: |
37+
set -euo pipefail
38+
cd "$DEPLOY_PATH"
39+
git fetch origin main
40+
git reset --hard origin/main
3341
34-
- name: Deploy backend
35-
run: echo "Deploying backend to hosting provider..."
36-
# Replace with actual deploy command, e.g., Fly.io:
37-
# flyctl deploy --image ghcr.io/${{ github.repository }}/backend:latest
38-
env:
39-
DEPLOY_TOKEN: ${{ secrets.DEPLOY_TOKEN }}
40-
41-
- name: Setup Node.js for migrations
42-
uses: actions/setup-node@v4
43-
with:
44-
node-version: '20'
42+
cat > backend/.env <<EOF
43+
NODE_ENV=production
44+
DATABASE_URL=${DATABASE_URL}
45+
JWT_SECRET=${JWT_SECRET}
46+
API_KEY_PEPPER=${API_KEY_PEPPER}
47+
PLATFORM_SECRET_KEY=${PLATFORM_SECRET_KEY}
48+
USDC_ISSUER=${USDC_ISSUER}
49+
STELLAR_NETWORK=${STELLAR_NETWORK}
50+
STELLAR_HORIZON_URL=${STELLAR_HORIZON_URL}
51+
FRONTEND_URL=${FRONTEND_URL}
52+
EOF
4553
46-
- name: Run DB migrations
47-
env:
48-
DATABASE_URL: ${{ secrets.DATABASE_URL }}
49-
run: node migrate.js
54+
docker compose -f docker-compose.prod.yml up -d --build
55+
docker compose -f docker-compose.prod.yml exec -T backend node db/migrate.js
5056
51-
- name: Deploy frontend
52-
run: echo "Deploying frontend to static hosting..."
53-
# Replace with actual deploy action, e.g., Vercel:
54-
# npx vercel --prod --token ${{ secrets.VERCEL_TOKEN }}
57+
- name: Wait for backend to become healthy
58+
run: |
59+
for i in $(seq 1 10); do
60+
if curl -fsS "${{ secrets.DEPLOY_HEALTH_URL }}/health"; then
61+
echo "Health check passed"
62+
exit 0
63+
fi
64+
echo "Health check attempt $i failed, retrying in 5s..."
65+
sleep 5
66+
done
67+
echo "Health check failed after 10 attempts"
68+
exit 1
5569
5670
- name: Post deploy status to Slack
5771
uses: 8398a7/action-slack@v3
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
ALTER TABLE users
2+
ADD COLUMN totp_failed_attempts INTEGER NOT NULL DEFAULT 0,
3+
ADD COLUMN totp_locked_until TIMESTAMPTZ;
Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
-- Backfill: strip any HTML tags left over from before input sanitization was
2+
-- applied to campaign/milestone/reward-tier text fields (issue #383).
3+
UPDATE campaigns
4+
SET description = TRIM(regexp_replace(description, '<[^>]*>', '', 'g'))
5+
WHERE description ~ '<[^>]*>';
6+
7+
UPDATE campaigns
8+
SET title = TRIM(regexp_replace(title, '<[^>]*>', '', 'g'))
9+
WHERE title ~ '<[^>]*>';
10+
11+
UPDATE milestones
12+
SET title = TRIM(regexp_replace(title, '<[^>]*>', '', 'g'))
13+
WHERE title ~ '<[^>]*>';
14+
15+
UPDATE milestones
16+
SET description = TRIM(regexp_replace(description, '<[^>]*>', '', 'g'))
17+
WHERE description ~ '<[^>]*>';
18+
19+
UPDATE reward_tiers
20+
SET title = TRIM(regexp_replace(title, '<[^>]*>', '', 'g'))
21+
WHERE title ~ '<[^>]*>';
22+
23+
UPDATE reward_tiers
24+
SET description = TRIM(regexp_replace(description, '<[^>]*>', '', 'g'))
25+
WHERE description ~ '<[^>]*>';

backend/src/index.js

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,33 @@ const openApiSpec = swaggerJsdoc({
127127
bearerFormat: "JWT",
128128
},
129129
},
130+
schemas: {
131+
Error: {
132+
type: "object",
133+
properties: {
134+
error: {
135+
type: "object",
136+
properties: {
137+
code: { type: "string", example: "VALIDATION_ERROR" },
138+
message: { type: "string", example: "Invalid email format" },
139+
fields: {
140+
type: "array",
141+
nullable: true,
142+
items: {
143+
type: "object",
144+
properties: {
145+
field: { type: "string" },
146+
message: { type: "string" },
147+
},
148+
},
149+
},
150+
},
151+
required: ["code", "message"],
152+
},
153+
},
154+
required: ["error"],
155+
},
156+
},
130157
},
131158
},
132159
apis: ["./src/routes/*.js"],
@@ -150,6 +177,33 @@ const v1OpenApiSpec = swaggerJsdoc({
150177
bearerFormat: "cp_live_…",
151178
},
152179
},
180+
schemas: {
181+
Error: {
182+
type: "object",
183+
properties: {
184+
error: {
185+
type: "object",
186+
properties: {
187+
code: { type: "string", example: "VALIDATION_ERROR" },
188+
message: { type: "string", example: "Invalid email format" },
189+
fields: {
190+
type: "array",
191+
nullable: true,
192+
items: {
193+
type: "object",
194+
properties: {
195+
field: { type: "string" },
196+
message: { type: "string" },
197+
},
198+
},
199+
},
200+
},
201+
required: ["code", "message"],
202+
},
203+
},
204+
required: ["error"],
205+
},
206+
},
153207
},
154208
},
155209
apis: ["./src/routes/v1.js"],

backend/src/middleware/validation.js

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -321,7 +321,18 @@ function validateRequest(req, res, next) {
321321
const result = validationResult(req);
322322
if (result.isEmpty()) return next();
323323

324-
return res.status(400).json({ errors: result.array() });
324+
const fields = result.array().map((e) => ({
325+
field: e.path || e.param,
326+
message: e.msg,
327+
}));
328+
329+
return res.status(400).json({
330+
error: {
331+
code: 'VALIDATION_ERROR',
332+
message: fields[0]?.message || 'Validation failed',
333+
fields,
334+
},
335+
});
325336
}
326337

327338
function validateRequestAsError(req, res, next) {

backend/src/routes/auth.js

Lines changed: 51 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -101,15 +101,30 @@ const loginLimiter = rateLimit({
101101
skip: () => isTest,
102102
});
103103

104+
// Per-IP: 3 TOTP attempts per 30 seconds.
104105
const totpChallengeLimiter = rateLimit({
105-
windowMs: 15 * 60 * 1000,
106-
max: isTest ? 100000 : 10,
106+
windowMs: 30 * 1000,
107+
max: isTest ? 100000 : 3,
108+
message: { error: 'Too many 2FA attempts, please try again later.' },
109+
standardHeaders: true,
110+
legacyHeaders: false,
111+
skip: () => isTest,
112+
});
113+
114+
// Per-account (by email): 3 TOTP attempts per 30 seconds, independent of IP.
115+
const totpChallengeEmailLimiter = rateLimit({
116+
windowMs: 30 * 1000,
117+
max: isTest ? 100000 : 3,
107118
message: { error: 'Too many 2FA attempts, please try again later.' },
108119
standardHeaders: true,
109120
legacyHeaders: false,
110121
skip: () => isTest,
122+
keyGenerator: (req) => String((req.body?.email || '').trim().toLowerCase()),
111123
});
112124

125+
const TOTP_MAX_CONSECUTIVE_FAILURES = 10;
126+
const TOTP_LOCKOUT_MS = 15 * 60 * 1000;
127+
113128
function hashToken(token) {
114129
return crypto.createHash('sha256').update(token, 'utf8').digest('hex');
115130
}
@@ -438,7 +453,7 @@ router.post('/login', loginLimiter, loginValidation, validateRequest, async (req
438453
});
439454
});
440455

441-
router.post('/2fa/challenge', totpChallengeLimiter, validateRequest, async (req, res) => {
456+
router.post('/2fa/challenge', totpChallengeLimiter, totpChallengeEmailLimiter, validateRequest, async (req, res) => {
442457
const { email, password, code } = req.body;
443458
if (!email || !password || !code) {
444459
return res.status(400).json({ error: 'Email, password, and code are required' });
@@ -455,6 +470,15 @@ router.post('/2fa/challenge', totpChallengeLimiter, validateRequest, async (req,
455470
return res.status(400).json({ error: '2FA is not enabled for this account' });
456471
}
457472

473+
if (user.totp_locked_until && new Date(user.totp_locked_until) > new Date()) {
474+
logger.warn('TOTP challenge blocked: account locked', {
475+
event: 'totp_locked',
476+
userId: user.id,
477+
ip: req.ip,
478+
});
479+
return res.status(423).json({ error: 'Too many failed 2FA attempts. Try again later.' });
480+
}
481+
458482
let codeValid = false;
459483

460484
if (code.length === 6) {
@@ -472,9 +496,33 @@ router.post('/2fa/challenge', totpChallengeLimiter, validateRequest, async (req,
472496
}
473497

474498
if (!codeValid) {
499+
const failedAttempts = (user.totp_failed_attempts || 0) + 1;
500+
const lockingOut = failedAttempts >= TOTP_MAX_CONSECUTIVE_FAILURES;
501+
await db.query(
502+
'UPDATE users SET totp_failed_attempts = $1, totp_locked_until = $2 WHERE id = $3',
503+
[
504+
lockingOut ? 0 : failedAttempts,
505+
lockingOut ? new Date(Date.now() + TOTP_LOCKOUT_MS) : null,
506+
user.id,
507+
]
508+
);
509+
logger.warn('Failed 2FA attempt', {
510+
event: 'totp_failed_attempt',
511+
userId: user.id,
512+
ip: req.ip,
513+
consecutiveFailures: failedAttempts,
514+
lockedOut: lockingOut,
515+
});
475516
return res.status(401).json({ error: 'Invalid 2FA code' });
476517
}
477518

519+
if (user.totp_failed_attempts) {
520+
await db.query(
521+
'UPDATE users SET totp_failed_attempts = 0, totp_locked_until = NULL WHERE id = $1',
522+
[user.id]
523+
);
524+
}
525+
478526
const { accessToken } = generateTokens(user);
479527
const { token: refreshToken, expiresAt } = await createRefreshToken(user.id);
480528

backend/src/routes/campaigns.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -134,8 +134,8 @@ function normalizeMilestonesInput(input) {
134134
}
135135

136136
const normalized = input.map((milestone, index) => {
137-
const title = String(milestone?.title || '').trim();
138-
const description = String(milestone?.description || '').trim();
137+
const title = stripHtml(milestone?.title || '');
138+
const description = stripHtml(milestone?.description || '');
139139
if (!title) {
140140
throw new Error(`Milestone ${index + 1} title is required`);
141141
}

backend/src/routes/users.test.js

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -106,8 +106,9 @@ test('POST /api/auth/register returns 400 with validation errors for invalid inp
106106
.send({ email: 'not-an-email', password: 'short', name: '' });
107107

108108
assert.equal(res.status, 400);
109-
assert.ok(Array.isArray(res.body.errors));
110-
assert.ok(res.body.errors.length >= 1);
109+
assert.equal(res.body.error.code, 'VALIDATION_ERROR');
110+
assert.ok(Array.isArray(res.body.error.fields));
111+
assert.ok(res.body.error.fields.length >= 1);
111112
});
112113

113114
test('POST /api/auth/login returns 400 with validation errors for invalid email', async () => {
@@ -120,7 +121,8 @@ test('POST /api/auth/login returns 400 with validation errors for invalid email'
120121
.send({ email: 'bad-email', password: '' });
121122

122123
assert.equal(res.status, 400);
123-
assert.ok(Array.isArray(res.body.errors));
124+
assert.equal(res.body.error.code, 'VALIDATION_ERROR');
125+
assert.ok(Array.isArray(res.body.error.fields));
124126
});
125127

126128
test('POST /api/auth/forgot-password returns generic message for unknown email', async () => {

backend/src/services/rewardTierService.js

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
const db = require('../config/database');
2+
const { stripHtml } = require('../lib/sanitize');
23

34
const MAX_TIERS_PER_CAMPAIGN = 10;
45

@@ -24,7 +25,7 @@ function validateTiersInput(tiers, campaignAssetType) {
2425
return tiers.map((tier, index) => {
2526
const label = `reward_tiers[${index}]`;
2627

27-
const title = typeof tier.title === 'string' ? tier.title.trim() : '';
28+
const title = stripHtml(tier.title || '');
2829
if (!title) throw new Error(`${label}: title is required`);
2930

3031
const minAmount = Number(tier.min_amount);
@@ -57,7 +58,7 @@ function validateTiersInput(tiers, campaignAssetType) {
5758

5859
return {
5960
title,
60-
description: typeof tier.description === 'string' ? tier.description.trim() : null,
61+
description: typeof tier.description === 'string' ? stripHtml(tier.description) || null : null,
6162
min_amount: minAmount,
6263
asset_type: assetType,
6364
tier_limit: tierLimit,

0 commit comments

Comments
 (0)