From 23edfe3dfc4f7822fed51f17739113d6f4e8a770 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?G=C3=A9rnyi=20M=C3=A1rk?= <gernyimark@gmail.com>
Date: Tue, 31 Mar 2026 23:33:27 +0200
Subject: [PATCH] fix: harden permissions system after code review

- Gate permissions in /app-config behind optionalAuth so unauthenticated
  requests don't receive admin configuration
- Fix trip_delete isMember parameter (was hardcoded false)
- Return skipped keys from savePermissions for admin visibility
- Add disabled prop to CustomSelect, use in BudgetPanel currency picker
- Fix CollabChat reaction handler returning false instead of void
- Pass canUploadFiles as prop to NoteFormModal instead of internal store read
- Make edit-only NoteFormModal props optional (onDeleteFile, note, tripId)
- Add missing trailing newlines to .gitignore and it.ts
---
 .gitignore                                    |  2 +-
 client/src/components/Budget/BudgetPanel.tsx  |  3 ++-
 client/src/components/Collab/CollabChat.tsx   |  2 +-
 client/src/components/Collab/CollabNotes.tsx  | 15 ++++++++-------
 client/src/components/shared/CustomSelect.tsx | 10 +++++++---
 client/src/i18n/translations/it.ts            |  2 +-
 server/src/routes/admin.ts                    |  4 ++--
 server/src/routes/auth.ts                     |  8 ++++----
 server/src/routes/trips.ts                    |  3 ++-
 server/src/services/permissions.ts            | 10 +++++++---
 10 files changed, 35 insertions(+), 24 deletions(-)

diff --git a/.gitignore b/.gitignore
index 57a3d9e..24ca73e 100644
--- a/.gitignore
+++ b/.gitignore
@@ -55,4 +55,4 @@ coverage
 .eslintcache
 .cache
 *.tsbuildinfo
-*.tgz
\ No newline at end of file
+*.tgz
diff --git a/client/src/components/Budget/BudgetPanel.tsx b/client/src/components/Budget/BudgetPanel.tsx
index 349af9c..e95d990 100644
--- a/client/src/components/Budget/BudgetPanel.tsx
+++ b/client/src/components/Budget/BudgetPanel.tsx
@@ -649,7 +649,8 @@ export default function BudgetPanel({ tripId, tripMembers = [] }: BudgetPanelPro
           <div style={{ marginBottom: 12 }}>
             <CustomSelect
               value={currency}
-              onChange={canEdit ? setCurrency : () => {}}
+              onChange={setCurrency}
+              disabled={!canEdit}
               options={CURRENCIES.map(c => ({ value: c, label: `${c} (${SYMBOLS[c] || c})` }))}
               searchable
             />
diff --git a/client/src/components/Collab/CollabChat.tsx b/client/src/components/Collab/CollabChat.tsx
index 2afa755..251a443 100644
--- a/client/src/components/Collab/CollabChat.tsx
+++ b/client/src/components/Collab/CollabChat.tsx
@@ -740,7 +740,7 @@ export default function CollabChat({ tripId, currentUser }: CollabChatProps) {
                           {msg.reactions.map(r => {
                             const myReaction = r.users.some(u => String(u.user_id) === String(currentUser.id))
                             return (
-                              <ReactionBadge key={r.emoji} reaction={r} currentUserId={currentUser.id} onReact={() => canEdit && handleReact(msg.id, r.emoji)} />
+                              <ReactionBadge key={r.emoji} reaction={r} currentUserId={currentUser.id} onReact={() => { if (canEdit) handleReact(msg.id, r.emoji) }} />
                             )
                           })}
                         </div>
diff --git a/client/src/components/Collab/CollabNotes.tsx b/client/src/components/Collab/CollabNotes.tsx
index ccf6b7b..bee4e0d 100644
--- a/client/src/components/Collab/CollabNotes.tsx
+++ b/client/src/components/Collab/CollabNotes.tsx
@@ -218,19 +218,17 @@ function UserAvatar({ user, size = 14 }: UserAvatarProps) {
 interface NoteFormModalProps {
   onClose: () => void
   onSubmit: (data: { title: string; content: string; category: string; website: string; files?: File[] }) => Promise<void>
-  onDeleteFile: (noteId: number, fileId: number) => Promise<void>
+  onDeleteFile?: (noteId: number, fileId: number) => Promise<void>
   existingCategories: string[]
   categoryColors: Record<string, string>
   getCategoryColor: (category: string) => string
-  note: CollabNote | null
-  tripId: number
+  note?: CollabNote | null
+  tripId?: number
   t: (key: string) => string
+  canUploadFiles?: boolean
 }
 
-function NoteFormModal({ onClose, onSubmit, onDeleteFile, existingCategories, categoryColors, getCategoryColor, note, tripId, t }: NoteFormModalProps) {
-  const can = useCanDo()
-  const tripObj = useTripStore((s) => s.trip)
-  const canUploadFiles = can('file_upload', tripObj)
+function NoteFormModal({ onClose, onSubmit, onDeleteFile, existingCategories, categoryColors, getCategoryColor, note, tripId, t, canUploadFiles = true }: NoteFormModalProps) {
   const isEdit = !!note
   const allCategories = [...new Set([...existingCategories, ...Object.keys(categoryColors || {})])].filter(Boolean)
 
@@ -889,6 +887,7 @@ export default function CollabNotes({ tripId, currentUser }: CollabNotesProps) {
   const can = useCanDo()
   const trip = useTripStore((s) => s.trip)
   const canEdit = can('collab_edit', trip)
+  const canUploadFiles = can('file_upload', trip)
   const [notes, setNotes] = useState([])
   const [loading, setLoading] = useState(true)
   const [showNewModal, setShowNewModal] = useState(false)
@@ -1343,6 +1342,7 @@ export default function CollabNotes({ tripId, currentUser }: CollabNotesProps) {
           existingCategories={categories}
           categoryColors={categoryColors}
           getCategoryColor={getCategoryColor}
+          canUploadFiles={canUploadFiles}
           t={t}
         />
       )}
@@ -1358,6 +1358,7 @@ export default function CollabNotes({ tripId, currentUser }: CollabNotesProps) {
           existingCategories={categories}
           categoryColors={categoryColors}
           getCategoryColor={getCategoryColor}
+          canUploadFiles={canUploadFiles}
           t={t}
         />
       )}
diff --git a/client/src/components/shared/CustomSelect.tsx b/client/src/components/shared/CustomSelect.tsx
index 2df9c5e..2526c8a 100644
--- a/client/src/components/shared/CustomSelect.tsx
+++ b/client/src/components/shared/CustomSelect.tsx
@@ -19,6 +19,7 @@ interface CustomSelectProps {
   searchable?: boolean
   style?: React.CSSProperties
   size?: 'sm' | 'md'
+  disabled?: boolean
 }
 
 export default function CustomSelect({
@@ -29,6 +30,7 @@ export default function CustomSelect({
   searchable = false,
   style = {},
   size = 'md',
+  disabled = false,
 }: CustomSelectProps) {
   const [open, setOpen] = useState(false)
   const [search, setSearch] = useState('')
@@ -83,17 +85,19 @@ export default function CustomSelect({
       {/* Trigger */}
       <button
         type="button"
-        onClick={() => { setOpen(o => !o); setSearch('') }}
+        disabled={disabled}
+        onClick={() => { if (!disabled) { setOpen(o => !o); setSearch('') } }}
         style={{
           width: '100%', display: 'flex', alignItems: 'center', gap: 8,
           padding: sm ? '8px 12px' : '8px 14px', borderRadius: 10,
           border: '1px solid var(--border-primary)',
           background: 'var(--bg-input)', color: 'var(--text-primary)',
           fontSize: 13, fontWeight: 500, fontFamily: 'inherit',
-          cursor: 'pointer', outline: 'none', textAlign: 'left',
+          cursor: disabled ? 'default' : 'pointer', outline: 'none', textAlign: 'left',
           transition: 'border-color 0.15s', overflow: 'hidden', minWidth: 0,
+          opacity: disabled ? 0.5 : 1,
         }}
-        onMouseEnter={e => e.currentTarget.style.borderColor = 'var(--text-faint)'}
+        onMouseEnter={e => { if (!disabled) e.currentTarget.style.borderColor = 'var(--text-faint)' }}
         onMouseLeave={e => { if (!open) e.currentTarget.style.borderColor = 'var(--border-primary)' }}
       >
         {selected?.icon && <span style={{ display: 'flex', flexShrink: 0 }}>{selected.icon}</span>}
diff --git a/client/src/i18n/translations/it.ts b/client/src/i18n/translations/it.ts
index 530ff91..53acacc 100644
--- a/client/src/i18n/translations/it.ts
+++ b/client/src/i18n/translations/it.ts
@@ -1470,4 +1470,4 @@ const it: Record<string, string | { name: string; category: string }[]> = {
   'perm.actionHint.share_manage': 'Chi può creare o eliminare link di condivisione pubblici',
 }
 
-export default it
\ No newline at end of file
+export default it
diff --git a/server/src/routes/admin.ts b/server/src/routes/admin.ts
index 1988680..674aa31 100644
--- a/server/src/routes/admin.ts
+++ b/server/src/routes/admin.ts
@@ -177,7 +177,7 @@ router.put('/permissions', (req: Request, res: Response) => {
   if (!permissions || typeof permissions !== 'object') {
     return res.status(400).json({ error: 'permissions object required' });
   }
-  savePermissions(permissions);
+  const { skipped } = savePermissions(permissions);
   writeAudit({
     userId: authReq.user.id,
     action: 'admin.permissions_update',
@@ -185,7 +185,7 @@ router.put('/permissions', (req: Request, res: Response) => {
     ip: getClientIp(req),
     details: permissions,
   });
-  res.json({ success: true, permissions: getAllPermissions() });
+  res.json({ success: true, permissions: getAllPermissions(), ...(skipped.length ? { skipped } : {}) });
 });
 
 router.get('/audit-log', (req: Request, res: Response) => {
diff --git a/server/src/routes/auth.ts b/server/src/routes/auth.ts
index abb871d..679d20d 100644
--- a/server/src/routes/auth.ts
+++ b/server/src/routes/auth.ts
@@ -10,13 +10,13 @@ import fetch from 'node-fetch';
 import { authenticator } from 'otplib';
 import QRCode from 'qrcode';
 import { db } from '../db/database';
-import { authenticate, demoUploadBlock } from '../middleware/auth';
+import { authenticate, optionalAuth, demoUploadBlock } from '../middleware/auth';
 import { JWT_SECRET } from '../config';
 import { encryptMfaSecret, decryptMfaSecret } from '../services/mfaCrypto';
 import { getAllPermissions } from '../services/permissions';
 import { randomBytes, createHash } from 'crypto';
 import { revokeUserSessions } from '../mcp';
-import { AuthRequest, User } from '../types';
+import { AuthRequest, OptionalAuthRequest, User } from '../types';
 import { writeAudit, getClientIp } from '../services/auditLog';
 import { decrypt_api_key, maybe_encrypt_api_key } from '../services/apiKeyCrypto';
 import { startTripReminders } from '../scheduler';
@@ -171,7 +171,7 @@ function generateToken(user: { id: number | bigint }) {
   );
 }
 
-router.get('/app-config', (_req: Request, res: Response) => {
+router.get('/app-config', optionalAuth, (req: Request, res: Response) => {
   const userCount = (db.prepare('SELECT COUNT(*) as count FROM users').get() as { count: number }).count;
   const setting = db.prepare("SELECT value FROM app_settings WHERE key = 'allow_registration'").get() as { value: string } | undefined;
   const allowRegistration = userCount === 0 || (setting?.value ?? 'true') === 'true';
@@ -210,7 +210,7 @@ router.get('/app-config', (_req: Request, res: Response) => {
     timezone: process.env.TZ || Intl.DateTimeFormat().resolvedOptions().timeZone || 'UTC',
     notification_channel: notifChannel,
     trip_reminders_enabled: tripRemindersEnabled,
-    permissions: getAllPermissions(),
+    permissions: (req as OptionalAuthRequest).user ? getAllPermissions() : undefined,
   });
 });
 
diff --git a/server/src/routes/trips.ts b/server/src/routes/trips.ts
index 07a130b..f400a65 100644
--- a/server/src/routes/trips.ts
+++ b/server/src/routes/trips.ts
@@ -294,7 +294,8 @@ router.delete('/:id', authenticate, (req: Request, res: Response) => {
   const trip = db.prepare('SELECT user_id FROM trips WHERE id = ?').get(req.params.id) as { user_id: number } | undefined;
   if (!trip) return res.status(404).json({ error: 'Trip not found' });
   const tripOwnerId = trip.user_id;
-  if (!checkPermission('trip_delete', authReq.user.role, tripOwnerId, authReq.user.id, false))
+  const isMemberDel = tripOwnerId !== authReq.user.id;
+  if (!checkPermission('trip_delete', authReq.user.role, tripOwnerId, authReq.user.id, isMemberDel))
     return res.status(403).json({ error: 'No permission to delete this trip' });
   const deletedTripId = Number(req.params.id);
   const delTrip = db.prepare('SELECT title, user_id FROM trips WHERE id = ?').get(req.params.id) as { title: string; user_id: number } | undefined;
diff --git a/server/src/services/permissions.ts b/server/src/services/permissions.ts
index 46adc6a..4912ac9 100644
--- a/server/src/services/permissions.ts
+++ b/server/src/services/permissions.ts
@@ -95,18 +95,22 @@ export function getAllPermissions(): Record<string, PermissionLevel> {
   return result;
 }
 
-export function savePermissions(settings: Record<string, string>): void {
+export function savePermissions(settings: Record<string, string>): { skipped: string[] } {
+  const skipped: string[] = [];
   const upsert = db.prepare("INSERT OR REPLACE INTO app_settings (key, value) VALUES (?, ?)");
   const txn = db.transaction(() => {
     for (const [actionKey, level] of Object.entries(settings)) {
       const action = ACTIONS_MAP.get(actionKey);
-      if (!action) continue;
-      if (!action.allowedLevels.includes(level as PermissionLevel)) continue;
+      if (!action || !action.allowedLevels.includes(level as PermissionLevel)) {
+        skipped.push(actionKey);
+        continue;
+      }
       upsert.run(`perm_${actionKey}`, level);
     }
   });
   txn();
   invalidatePermissionsCache();
+  return { skipped };
 }
 
 /**