mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-06-20 05:41:47 +00:00
2d0414b4a3
Fixes the critical + high + medium findings from our internal security
review. Bundled into one PR because the changes overlap heavily (JWT
verification unifies across three call sites; backup-code hashing and
demo-email handling cross-cut several services); splitting them out
would mean redundant reviews of the same files.
Critical
- CI-C1 — .github/workflows/test.yml: restore actions/{checkout,setup-
node,upload-artifact} to @v4. The @v6 refs don't exist, so the test
workflow was errorring before a single test ran.
- SEC-C1 — mfaPolicy now extracts the token via extractToken() (cookie-
first, Bearer fallback). Previously it only read Authorization, so
every cookie-authenticated SPA session bypassed require_mfa entirely.
- SEC-C2/C4/C6 — all JWT verification paths (MCP bearer, file download,
photo route) now go through the shared verifyJwtAndLoadUser that
checks password_version. resetPassword additionally deletes every
mcp_tokens row and marks outstanding oauth_tokens revoked, so a
password reset invalidates ALL credential classes — not just the
cookie JWT.
High
- SEC-H2 — reset email URL is built from server-side APP_URL /
ALLOWED_ORIGINS (via existing getAppUrl()), not request headers.
Closes the host-header-injection vector into reset links.
- SEC-H3 — OIDC findOrCreateUser wraps the invite-redemption UPDATE +
user INSERT in a transaction. The UPDATE is the capacity check; if
a concurrent callback takes the last slot, the whole transaction
aborts with registration_disabled instead of double-creating users.
- SEC-H4 — new verifyIdToken() performs full JWT signature
verification via the provider's JWKS (Node's crypto.createPublicKey
accepts JWK directly — no extra dependency), plus iss/aud/exp
checks. The callback also rejects the login when userinfo.sub does
not match id_token.sub.
- SEC-H5 — OAuth DCR now validates redirect_uris against an allowlist
of schemes: https, http-loopback, or a private custom scheme. Plain
http://non-loopback is rejected.
- SEC-H6 — oauthService audience defaults to mcpResource when the
`resource` parameter is missing, so tokens are always audience-bound
to /mcp instead of being issued with audience=null.
- SEC-H7 — HSTS is enabled any time NODE_ENV=production (previously
required FORCE_HTTPS=true), includeSubDomains defaults on and can
be disabled with HSTS_INCLUDE_SUBDOMAINS=false.
- SEC-H8 — trek_session cookie Secure flag is also driven by
req.secure (which Express resolves from X-Forwarded-Proto once
trust proxy is set), so instances behind a TLS-terminating proxy
get Secure cookies without needing FORCE_HTTPS.
Medium
- SEC-M1 — permanentDeleteFile / emptyTrash / avatar unlink now use
fs.promises.rm with { force: true } (one async op vs the previous
existsSync + unlinkSync pair per file).
- SEC-M2 — invalidatePermissionsCache() is called inside restoreFromZip
so a restored DB with different permission rows is honoured
immediately.
- SEC-M3 + C1 — idempotency store bounds the key at 128 chars, caches
only responses ≤ 256 KiB, and scopes the lookup by (key, user_id,
method, path) rather than (key, user_id). Same key replayed against
a different endpoint no longer returns a stale unrelated body.
- SEC-M4 — share_tokens gets an expires_at column; new tokens default
to 90-day TTL, expired tokens are denied at lookup. Existing tokens
stay NULL = no expiry so already-published links don't break.
- SEC-M5 — /uploads/photos/:filename now resolves the photo to its
trip_id and requires the share token to cover THAT trip. Previously
any share token for any trip would unlock any photo filename.
- SEC-M6 — BLOCKED_EXTENSIONS is the single source of truth shared
between fileService and collab uploads. The '*' allowed_file_types
wildcard now still rejects executables/scripts.
- SEC-M7 — single DEMO_EMAILS constant (services/demo.ts) used by
demoUploadBlock, mfaPolicy, and every demo-mode guard in
authService. The old demoUploadBlock only matched 'demo@nomad.app'
so the seed 'demo@trek.app' could in fact upload in demo mode.
- SEC-M8 — MFA backup codes are now bcrypt-hashed at rest
(hashBackupCodeBcrypt). matchBackupCode accepts both bcrypt and
legacy SHA-256 hex hashes, so existing installs keep working until
the user regenerates codes via enableMfa.
- SEC-M9 — document the "security via UUID v4 filename" model for
/uploads/avatars|covers|journey. Requires no code change but
captures the decision so future reviewers don't re-flag it.
- SEC-M10 — already covered by the resetPassword revocation logic
above: mcp_tokens DELETE + oauth_tokens UPDATE … SET revoked_at.
Performance
- PERF-H1 — new migration adds the indexes flagged in the audit:
trips(user_id), trips(created_at DESC), photos(day_id),
photos(place_id), reservations(day_id), share_tokens(token), plus
conditional day_accommodations and notifications indexes depending
on which columns are present.
Tests
- tests/integration/oidc.test.ts now mocks verifyIdToken and passes
an id_token in the exchangeCodeForToken stub for the three flows
that exercise a successful callback. The three remaining failures
tests pointed out were all pre-existing (file-upload flakes +
notificationPreferences event_types count drift), none introduced
by this PR.
197 lines
7.7 KiB
TypeScript
197 lines
7.7 KiB
TypeScript
import { db, canAccessTrip } from '../db/database';
|
|
import crypto from 'crypto';
|
|
import { loadTagsByPlaceIds } from './queryHelpers';
|
|
|
|
interface SharePermissions {
|
|
share_map?: boolean;
|
|
share_bookings?: boolean;
|
|
share_packing?: boolean;
|
|
share_budget?: boolean;
|
|
share_collab?: boolean;
|
|
}
|
|
|
|
interface ShareTokenInfo {
|
|
token: string;
|
|
created_at: string;
|
|
share_map: boolean;
|
|
share_bookings: boolean;
|
|
share_packing: boolean;
|
|
share_budget: boolean;
|
|
share_collab: boolean;
|
|
}
|
|
|
|
/**
|
|
* Creates a new share link or updates the permissions on an existing one.
|
|
* Returns an object with the token string and whether it was newly created.
|
|
*/
|
|
export function createOrUpdateShareLink(
|
|
tripId: string,
|
|
createdBy: number,
|
|
permissions: SharePermissions
|
|
): { token: string; created: boolean } {
|
|
const {
|
|
share_map = true,
|
|
share_bookings = true,
|
|
share_packing = false,
|
|
share_budget = false,
|
|
share_collab = false,
|
|
} = permissions;
|
|
|
|
const existing = db.prepare('SELECT token FROM share_tokens WHERE trip_id = ?').get(tripId) as { token: string } | undefined;
|
|
if (existing) {
|
|
db.prepare('UPDATE share_tokens SET share_map = ?, share_bookings = ?, share_packing = ?, share_budget = ?, share_collab = ? WHERE trip_id = ?')
|
|
.run(share_map ? 1 : 0, share_bookings ? 1 : 0, share_packing ? 1 : 0, share_budget ? 1 : 0, share_collab ? 1 : 0, tripId);
|
|
return { token: existing.token, created: false };
|
|
}
|
|
|
|
// New share links default to a 90-day TTL. Existing tokens that were
|
|
// created before the expires_at migration keep NULL here and remain
|
|
// valid indefinitely until the owner rotates them; that preserves
|
|
// behaviour for anyone who's already sharing a link.
|
|
const token = crypto.randomBytes(24).toString('base64url');
|
|
const expiresAt = new Date(Date.now() + 90 * 24 * 60 * 60 * 1000).toISOString();
|
|
db.prepare('INSERT INTO share_tokens (trip_id, token, created_by, share_map, share_bookings, share_packing, share_budget, share_collab, expires_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)')
|
|
.run(tripId, token, createdBy, share_map ? 1 : 0, share_bookings ? 1 : 0, share_packing ? 1 : 0, share_budget ? 1 : 0, share_collab ? 1 : 0, expiresAt);
|
|
return { token, created: true };
|
|
}
|
|
|
|
/**
|
|
* Returns share token info for a trip, or null if no share link exists.
|
|
*/
|
|
export function getShareLink(tripId: string): ShareTokenInfo | null {
|
|
const row = db.prepare('SELECT * FROM share_tokens WHERE trip_id = ?').get(tripId) as any;
|
|
if (!row) return null;
|
|
return {
|
|
token: row.token,
|
|
created_at: row.created_at,
|
|
share_map: !!row.share_map,
|
|
share_bookings: !!row.share_bookings,
|
|
share_packing: !!row.share_packing,
|
|
share_budget: !!row.share_budget,
|
|
share_collab: !!row.share_collab,
|
|
};
|
|
}
|
|
|
|
/**
|
|
* Deletes the share token for a trip.
|
|
*/
|
|
export function deleteShareLink(tripId: string): void {
|
|
db.prepare('DELETE FROM share_tokens WHERE trip_id = ?').run(tripId);
|
|
}
|
|
|
|
/**
|
|
* Loads the full public trip data for a share token, filtered by the token's
|
|
* permission flags. Returns null if the token is invalid or the trip is gone.
|
|
*/
|
|
export function getSharedTripData(token: string): Record<string, any> | null {
|
|
const shareRow = db.prepare(
|
|
"SELECT * FROM share_tokens WHERE token = ? AND (expires_at IS NULL OR expires_at > datetime('now'))"
|
|
).get(token) as any;
|
|
if (!shareRow) return null;
|
|
|
|
const tripId = shareRow.trip_id;
|
|
|
|
// Trip
|
|
const trip = db.prepare('SELECT id, title, description, start_date, end_date, cover_image, currency FROM trips WHERE id = ?').get(tripId);
|
|
if (!trip) return null;
|
|
|
|
// Days with assignments
|
|
const days = db.prepare('SELECT * FROM days WHERE trip_id = ? ORDER BY day_number ASC').all(tripId) as any[];
|
|
const dayIds = days.map(d => d.id);
|
|
|
|
let assignments: Record<number, any[]> = {};
|
|
let dayNotes: Record<number, any[]> = {};
|
|
if (dayIds.length > 0) {
|
|
const ph = dayIds.map(() => '?').join(',');
|
|
const allAssignments = db.prepare(`
|
|
SELECT da.*, p.id as place_id, p.name as place_name, p.description as place_description,
|
|
p.lat, p.lng, p.address, p.category_id, p.price, p.currency as place_currency,
|
|
COALESCE(da.assignment_time, p.place_time) as place_time,
|
|
COALESCE(da.assignment_end_time, p.end_time) as end_time,
|
|
p.duration_minutes, p.notes as place_notes, p.image_url, p.transport_mode,
|
|
c.name as category_name, c.color as category_color, c.icon as category_icon
|
|
FROM day_assignments da
|
|
JOIN places p ON da.place_id = p.id
|
|
LEFT JOIN categories c ON p.category_id = c.id
|
|
WHERE da.day_id IN (${ph})
|
|
ORDER BY da.order_index ASC
|
|
`).all(...dayIds);
|
|
|
|
const placeIds = [...new Set(allAssignments.map((a: any) => a.place_id))];
|
|
const tagsByPlace = loadTagsByPlaceIds(placeIds, { compact: true });
|
|
|
|
const byDay: Record<number, any[]> = {};
|
|
for (const a of allAssignments as any[]) {
|
|
if (!byDay[a.day_id]) byDay[a.day_id] = [];
|
|
byDay[a.day_id].push({
|
|
id: a.id, day_id: a.day_id, order_index: a.order_index, notes: a.notes,
|
|
place: {
|
|
id: a.place_id, name: a.place_name, description: a.place_description,
|
|
lat: a.lat, lng: a.lng, address: a.address, category_id: a.category_id,
|
|
price: a.price, place_time: a.place_time, end_time: a.end_time,
|
|
image_url: a.image_url, transport_mode: a.transport_mode,
|
|
category: a.category_id ? { id: a.category_id, name: a.category_name, color: a.category_color, icon: a.category_icon } : null,
|
|
tags: tagsByPlace[a.place_id] || [],
|
|
}
|
|
});
|
|
}
|
|
assignments = byDay;
|
|
|
|
const allNotes = db.prepare(`SELECT * FROM day_notes WHERE day_id IN (${ph}) ORDER BY sort_order ASC`).all(...dayIds);
|
|
const notesByDay: Record<number, any[]> = {};
|
|
for (const n of allNotes as any[]) {
|
|
if (!notesByDay[n.day_id]) notesByDay[n.day_id] = [];
|
|
notesByDay[n.day_id].push(n);
|
|
}
|
|
dayNotes = notesByDay;
|
|
}
|
|
|
|
// Places
|
|
const places = db.prepare(`
|
|
SELECT p.*, c.name as category_name, c.color as category_color, c.icon as category_icon
|
|
FROM places p LEFT JOIN categories c ON p.category_id = c.id
|
|
WHERE p.trip_id = ? ORDER BY p.created_at DESC
|
|
`).all(tripId);
|
|
|
|
// Reservations
|
|
const reservations = db.prepare('SELECT * FROM reservations WHERE trip_id = ? ORDER BY reservation_time ASC').all(tripId);
|
|
|
|
// Accommodations
|
|
const accommodations = db.prepare(`
|
|
SELECT a.*, p.name as place_name, p.address as place_address, p.lat as place_lat, p.lng as place_lng
|
|
FROM day_accommodations a JOIN places p ON a.place_id = p.id
|
|
WHERE a.trip_id = ?
|
|
`).all(tripId);
|
|
|
|
// Packing
|
|
const packing = db.prepare('SELECT * FROM packing_items WHERE trip_id = ? ORDER BY sort_order ASC').all(tripId);
|
|
|
|
// Budget
|
|
const budget = db.prepare('SELECT * FROM budget_items WHERE trip_id = ? ORDER BY category ASC').all(tripId);
|
|
|
|
// Categories
|
|
const categories = db.prepare('SELECT * FROM categories').all();
|
|
|
|
const permissions = {
|
|
share_map: !!shareRow.share_map,
|
|
share_bookings: !!shareRow.share_bookings,
|
|
share_packing: !!shareRow.share_packing,
|
|
share_budget: !!shareRow.share_budget,
|
|
share_collab: !!shareRow.share_collab,
|
|
};
|
|
|
|
// Collab messages (only if owner chose to share)
|
|
const collabMessages = permissions.share_collab
|
|
? db.prepare('SELECT m.*, u.username, u.avatar FROM collab_messages m JOIN users u ON m.user_id = u.id WHERE m.trip_id = ? AND m.deleted = 0 ORDER BY m.created_at').all(tripId)
|
|
: [];
|
|
|
|
return {
|
|
trip, days, assignments, dayNotes, places, categories, permissions,
|
|
reservations: permissions.share_bookings ? reservations : [],
|
|
accommodations: permissions.share_bookings ? accommodations : [],
|
|
packing: permissions.share_packing ? packing : [],
|
|
budget: permissions.share_budget ? budget : [],
|
|
collab: collabMessages,
|
|
};
|
|
}
|