Skip to content

Commit c6dcf1f

Browse files
authored
Merge pull request #96 from soranjiro/copilot/fix-security-risks
fix: Address critical security vulnerabilities in API
2 parents aca5ec0 + 7dbe447 commit c6dcf1f

10 files changed

Lines changed: 270 additions & 33 deletions

File tree

apps/api/package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
},
1313
"dependencies": {
1414
"@tsndr/cloudflare-worker-jwt": "^3.2.0",
15+
"bcryptjs": "^2.4.3",
1516
"hono": "^4.6.14"
1617
},
1718
"devDependencies": {

apps/api/src/routes/auth.ts

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { Hono } from 'hono';
22
import { Env } from '../utils';
33
import { ItineraryService } from '../services/itinerary.service';
44
import { generateToken, verifyToken, extractBearerToken } from '../utils/jwt';
5+
import { verifyPassword } from '../utils/password';
56
import type { PasswordAuthRequest } from '@tabitabi/types';
67

78
const auth = new Hono<{ Bindings: Env }>();
@@ -52,17 +53,20 @@ auth.post('/password', async (c) => {
5253
}, 404);
5354
}
5455

55-
// If itinerary has a password, verify it
56-
if (itinerary.password && itinerary.password !== password) {
57-
return c.json({
58-
success: false,
59-
error: { code: 'UNAUTHORIZED', message: 'Invalid password' }
60-
}, 401);
61-
}
62-
63-
// If itinerary has no password, allow access (password can be anything or empty)
64-
if (!itinerary.password) {
65-
// No check needed
56+
if (itinerary.password) {
57+
if (!password) {
58+
return c.json({
59+
success: false,
60+
error: { code: 'UNAUTHORIZED', message: 'Invalid password' }
61+
}, 401);
62+
}
63+
const isValid = await verifyPassword(password, itinerary.password);
64+
if (!isValid) {
65+
return c.json({
66+
success: false,
67+
error: { code: 'UNAUTHORIZED', message: 'Invalid password' }
68+
}, 401);
69+
}
6670
}
6771

6872
const token = await generateToken(shioriId, c.env.JWT_SECRET);

apps/api/src/services/itinerary.service.ts

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import type { Itinerary, CreateItineraryInput, UpdateItineraryInput } from '@tab
22
import type { D1Database } from '@cloudflare/workers-types';
33
import { generateId, getCurrentTimestamp } from '../utils';
44
import { validateMemoJson } from '../utils/memo';
5+
import { hashPassword } from '../utils/password';
56

67
const DEFAULT_THEME_ID = 'standard-autumn';
78

@@ -42,7 +43,7 @@ export class ItineraryService {
4243
}
4344

4445
async create(input: CreateItineraryInput): Promise<Itinerary> {
45-
const id = generateId(32);
46+
const id = generateId();
4647
const now = getCurrentTimestamp();
4748

4849
const memo = input.memo ?? '{"text":""}';
@@ -51,13 +52,15 @@ export class ItineraryService {
5152
throw new Error(validation.error);
5253
}
5354

55+
const hashedPassword = input.password ? await hashPassword(input.password) : null;
56+
5457
const itinerary: Itinerary = {
5558
id,
5659
title: input.title,
5760
theme_id: input.theme_id || DEFAULT_THEME_ID,
5861
memo,
5962
walica_id: input.walica_id ?? null,
60-
password: input.password ?? null,
63+
password: hashedPassword,
6164
secret_settings: input.secret_settings ? {
6265
enabled: input.secret_settings.enabled,
6366
offset_minutes: input.secret_settings.offset_minutes
@@ -103,7 +106,7 @@ export class ItineraryService {
103106

104107
const now = getCurrentTimestamp();
105108
const fields = ['updated_at = ?'];
106-
const values: any[] = [now];
109+
const values: (string | number | null)[] = [now];
107110

108111
if (input.title !== undefined) {
109112
fields.push('title = ?');
@@ -123,7 +126,8 @@ export class ItineraryService {
123126
}
124127
if (input.password !== undefined) {
125128
fields.push('password = ?');
126-
values.push(input.password);
129+
const hashedPassword = input.password ? await hashPassword(input.password) : null;
130+
values.push(hashedPassword);
127131
}
128132

129133
if (fields.length > 1) {
@@ -200,7 +204,7 @@ export class ItineraryService {
200204
return result.success;
201205
}
202206

203-
private mapToItinerary(row: any): Itinerary {
207+
private mapToItinerary(row: Record<string, unknown>): Itinerary {
204208
const itinerary: Itinerary = {
205209
id: row.id,
206210
title: row.title,

apps/api/src/services/step.service.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ export class StepService {
88

99
async list(itineraryId: string, options?: { currentTime?: string; offsetMinutes?: number; maskSecrets?: boolean }): Promise<Step[]> {
1010
let query = 'SELECT * FROM steps WHERE itinerary_id = ?';
11-
const bindings: any[] = [itineraryId];
11+
const bindings: (string | number)[] = [itineraryId];
1212

1313
// If time filtering is enabled (secret mode)
1414
if (options?.currentTime && options?.offsetMinutes !== undefined) {
@@ -48,7 +48,7 @@ export class StepService {
4848
}
4949

5050
async create(input: CreateStepInput): Promise<Step> {
51-
const id = generateId(32);
51+
const id = generateId();
5252
const now = getCurrentTimestamp();
5353

5454
const notes = input.notes ?? '{"text":""}';
@@ -95,7 +95,7 @@ export class StepService {
9595

9696
const now = getCurrentTimestamp();
9797
const fields = ['updated_at = ?'];
98-
const values: any[] = [now];
98+
const values: (string | number | null)[] = [now];
9999

100100
if (input.title !== undefined) {
101101
fields.push('title = ?');
@@ -141,7 +141,7 @@ export class StepService {
141141
return result.success;
142142
}
143143

144-
private mapToStep(row: any, maskSecrets: boolean = true): Step {
144+
private mapToStep(row: Record<string, unknown>, maskSecrets: boolean = true): Step {
145145
const step: Step = {
146146
id: row.id,
147147
itinerary_id: row.itinerary_id,

apps/api/src/utils/index.ts

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,8 @@ export interface Env {
44
JWT_SECRET?: string;
55
}
66

7-
export function generateId(length: number = 32): string {
8-
const chars = 'abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ';
9-
let result = '';
10-
for (let i = 0; i < length; i++) {
11-
result += chars.charAt(Math.floor(Math.random() * chars.length));
12-
}
13-
return result;
7+
export function generateId(): string {
8+
return crypto.randomUUID();
149
}
1510

1611
export function getCurrentTimestamp(): string {

apps/api/src/utils/jwt.ts

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,26 +6,28 @@ export interface JwtPayload {
66
exp: number;
77
}
88

9-
const DEFAULT_SECRET = 'tabitabi-default-secret-change-in-production';
109
const TOKEN_EXPIRY = 30 * 24 * 60 * 60;
1110

1211
export async function generateToken(shioriId: string, secret?: string): Promise<string> {
12+
if (!secret) {
13+
throw new Error('JWT_SECRET is required');
14+
}
1315
const now = Math.floor(Date.now() / 1000);
1416
const payload: JwtPayload = {
1517
shioriId,
1618
iat: now,
1719
exp: now + TOKEN_EXPIRY,
1820
};
1921

20-
return await sign(payload, secret || DEFAULT_SECRET);
22+
return await sign(payload, secret);
2123
}
2224

2325
export async function verifyToken(token: string, secret?: string): Promise<JwtPayload | null> {
26+
if (!secret) {
27+
throw new Error('JWT_SECRET is required');
28+
}
2429
try {
25-
const isValid = await verify(token, secret || DEFAULT_SECRET);
26-
if (!isValid) return null;
27-
28-
const { payload } = await verify(token, secret || DEFAULT_SECRET, { throwError: true }) as { payload: JwtPayload };
30+
const { payload } = await verify(token, secret, { throwError: true }) as { payload: JwtPayload };
2931

3032
if (payload.exp < Math.floor(Date.now() / 1000)) {
3133
return null;

apps/api/src/utils/password.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
import bcrypt from 'bcryptjs';
2+
3+
const SALT_ROUNDS = 10;
4+
5+
export function hashPassword(password: string): Promise<string> {
6+
return bcrypt.hash(password, SALT_ROUNDS);
7+
}
8+
9+
export function verifyPassword(password: string, hash: string): Promise<boolean> {
10+
return bcrypt.compare(password, hash);
11+
}

apps/api/test/auth.test.ts

Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,163 @@
1+
import { env } from 'cloudflare:test';
2+
import { describe, it, expect, beforeEach } from 'vitest';
3+
import app from '../src/index';
4+
5+
async function applyMigrations(db: D1Database) {
6+
const migrations = [
7+
`CREATE TABLE IF NOT EXISTS itineraries (
8+
id TEXT PRIMARY KEY,
9+
title TEXT NOT NULL,
10+
theme_id TEXT NOT NULL DEFAULT 'standard-autumn',
11+
memo TEXT,
12+
password TEXT,
13+
created_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP,
14+
updated_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP
15+
);`,
16+
`CREATE TABLE IF NOT EXISTS steps (
17+
id TEXT PRIMARY KEY,
18+
itinerary_id TEXT NOT NULL,
19+
title TEXT NOT NULL,
20+
date TEXT NOT NULL,
21+
time TEXT NOT NULL,
22+
location TEXT,
23+
notes TEXT,
24+
created_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP,
25+
updated_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP,
26+
FOREIGN KEY (itinerary_id) REFERENCES itineraries(id) ON DELETE CASCADE
27+
);`,
28+
`CREATE INDEX IF NOT EXISTS idx_steps_itinerary ON steps(itinerary_id);`,
29+
`CREATE TABLE IF NOT EXISTS itinerary_secrets (
30+
itinerary_id TEXT PRIMARY KEY,
31+
enabled BOOLEAN DEFAULT FALSE,
32+
offset_minutes INTEGER DEFAULT 60,
33+
created_at TEXT NOT NULL,
34+
updated_at TEXT NOT NULL,
35+
FOREIGN KEY (itinerary_id) REFERENCES itineraries(id) ON DELETE CASCADE
36+
);`,
37+
`CREATE TABLE IF NOT EXISTS itinerary_walica_settings (
38+
itinerary_id TEXT PRIMARY KEY,
39+
walica_id TEXT NOT NULL,
40+
created_at TEXT NOT NULL,
41+
updated_at TEXT NOT NULL,
42+
FOREIGN KEY (itinerary_id) REFERENCES itineraries(id) ON DELETE CASCADE
43+
);`,
44+
];
45+
46+
for (const sql of migrations) {
47+
await db.prepare(sql).run();
48+
}
49+
}
50+
51+
describe('Password Authentication', () => {
52+
beforeEach(async () => {
53+
await applyMigrations(env.DB);
54+
await env.DB.prepare('DELETE FROM steps').run();
55+
await env.DB.prepare('DELETE FROM itinerary_secrets').run();
56+
await env.DB.prepare('DELETE FROM itinerary_walica_settings').run();
57+
await env.DB.prepare('DELETE FROM itineraries').run();
58+
});
59+
60+
describe('Itinerary with password', () => {
61+
it('creates itinerary with hashed password', async () => {
62+
const request = new Request('http://localhost/api/v1/itineraries', {
63+
method: 'POST',
64+
headers: { 'Content-Type': 'application/json' },
65+
body: JSON.stringify({ title: 'Protected Trip', password: 'secret123' }),
66+
});
67+
68+
const response = await app.fetch(request, env);
69+
expect(response.status).toBe(201);
70+
71+
const { success, data } = await response.json() as { success: boolean; data: { id: string; is_password_protected: boolean } };
72+
expect(success).toBe(true);
73+
expect(data.is_password_protected).toBe(true);
74+
75+
const dbResult = await env.DB.prepare('SELECT password FROM itineraries WHERE id = ?').bind(data.id).first() as { password: string } | null;
76+
expect(dbResult).not.toBeNull();
77+
expect(dbResult!.password).not.toBe('secret123');
78+
expect(dbResult!.password).toMatch(/^\$2[aby]\$\d{2}\$.{53}$/);
79+
});
80+
});
81+
82+
describe('POST /api/v1/auth/password', () => {
83+
it('authenticates with correct password', async () => {
84+
const createRequest = new Request('http://localhost/api/v1/itineraries', {
85+
method: 'POST',
86+
headers: { 'Content-Type': 'application/json' },
87+
body: JSON.stringify({ title: 'Protected Trip', password: 'secret123' }),
88+
});
89+
const createResponse = await app.fetch(createRequest, env);
90+
const { data: created } = await createResponse.json() as { data: { id: string } };
91+
92+
const authRequest = new Request('http://localhost/api/v1/auth/password', {
93+
method: 'POST',
94+
headers: { 'Content-Type': 'application/json' },
95+
body: JSON.stringify({ shioriId: created.id, password: 'secret123' }),
96+
});
97+
const response = await app.fetch(authRequest, env);
98+
99+
expect(response.status).toBe(200);
100+
const { success, data } = await response.json() as { success: boolean; data: { token: string } };
101+
expect(success).toBe(true);
102+
expect(data.token).toBeDefined();
103+
});
104+
105+
it('rejects authentication with incorrect password', async () => {
106+
const createRequest = new Request('http://localhost/api/v1/itineraries', {
107+
method: 'POST',
108+
headers: { 'Content-Type': 'application/json' },
109+
body: JSON.stringify({ title: 'Protected Trip', password: 'secret123' }),
110+
});
111+
const createResponse = await app.fetch(createRequest, env);
112+
const { data: created } = await createResponse.json() as { data: { id: string } };
113+
114+
const authRequest = new Request('http://localhost/api/v1/auth/password', {
115+
method: 'POST',
116+
headers: { 'Content-Type': 'application/json' },
117+
body: JSON.stringify({ shioriId: created.id, password: 'wrongpassword' }),
118+
});
119+
const response = await app.fetch(authRequest, env);
120+
121+
expect(response.status).toBe(401);
122+
const { success, error } = await response.json() as { success: boolean; error: { code: string } };
123+
expect(success).toBe(false);
124+
expect(error.code).toBe('UNAUTHORIZED');
125+
});
126+
127+
it('allows access to itinerary without password', async () => {
128+
const createRequest = new Request('http://localhost/api/v1/itineraries', {
129+
method: 'POST',
130+
headers: { 'Content-Type': 'application/json' },
131+
body: JSON.stringify({ title: 'Public Trip' }),
132+
});
133+
const createResponse = await app.fetch(createRequest, env);
134+
const { data: created } = await createResponse.json() as { data: { id: string } };
135+
136+
const authRequest = new Request('http://localhost/api/v1/auth/password', {
137+
method: 'POST',
138+
headers: { 'Content-Type': 'application/json' },
139+
body: JSON.stringify({ shioriId: created.id }),
140+
});
141+
const response = await app.fetch(authRequest, env);
142+
143+
expect(response.status).toBe(200);
144+
const { success, data } = await response.json() as { success: boolean; data: { token: string } };
145+
expect(success).toBe(true);
146+
expect(data.token).toBeDefined();
147+
});
148+
149+
it('returns 404 for non-existent itinerary', async () => {
150+
const authRequest = new Request('http://localhost/api/v1/auth/password', {
151+
method: 'POST',
152+
headers: { 'Content-Type': 'application/json' },
153+
body: JSON.stringify({ shioriId: 'nonexistent', password: 'secret' }),
154+
});
155+
const response = await app.fetch(authRequest, env);
156+
157+
expect(response.status).toBe(404);
158+
const { success, error } = await response.json() as { success: boolean; error: { code: string } };
159+
expect(success).toBe(false);
160+
expect(error.code).toBe('NOT_FOUND');
161+
});
162+
});
163+
});

0 commit comments

Comments
 (0)