fix: resolve security and type issues flagged by code review

- Fix admin.tsx: move non-admin redirect from render body to useEffect
  to avoid Rules of Hooks violation causing React concurrent rendering error;
  add early return null guard after all hooks

- Fix admin.tsx: coerce nullable app/service fields with ?? fallbacks
  (nameAr, nameEn, slug, iconName, route, color, sortOrder, price, isAvailable)

- Add GET /api/users/directory endpoint (auth-only, returns safe fields:
  id, username, displayNameAr, displayNameEn, avatarUrl, isActive)
  so non-admin users can look up contacts for chat without hitting admin-only list

- Update chat.tsx to use new /api/users/directory via useQuery instead
  of admin-only useListUsers; remove client-sent "join" Socket.IO event
  since server now auto-joins user room from session

- Wire language toggle in home.tsx to call useUpdateLanguage mutation
  (PUT /api/auth/language) so preference persists to the database

- Harden Socket.IO: export sessionMiddleware from app.ts; apply it via
  io.engine.use in index.ts so Socket.IO handshake reads express-session;
  add io.use middleware that rejects connections without a valid session userId;
  auto-join user room from session instead of trusting client-provided userId;
  verify conversation membership before joining conversation rooms

- Fix Socket.IO CORS: use ALLOWED_ORIGINS env var (matches Express CORS)
  instead of hardcoded wildcard "*"

All TypeScript checks pass; e2e tests confirm: login, language toggle,
services, chat, notifications, admin CRUD, and non-admin redirect all work
This commit is contained in:
riyadhafraa
2026-04-20 09:34:00 +00:00
parent 001e9023b2
commit 24850f5597
8 changed files with 177 additions and 59 deletions
+34 -17
View File
@@ -33,9 +33,21 @@ app.use(
}),
);
const allowedOrigins = process.env.ALLOWED_ORIGINS
? process.env.ALLOWED_ORIGINS.split(",").map((o) => o.trim())
: null;
app.use(
cors({
origin: true,
origin: allowedOrigins
? (origin, callback) => {
if (!origin || allowedOrigins.some((o) => origin.startsWith(o))) {
callback(null, true);
} else {
callback(new Error("Not allowed by CORS"));
}
}
: true,
credentials: true,
}),
);
@@ -43,23 +55,28 @@ app.use(
app.use(express.json());
app.use(express.urlencoded({ extended: true }));
app.use(
session({
store: new PgSession({
pool,
tableName: "user_sessions",
}),
secret: process.env.SESSION_SECRET ?? "teaboy-os-secret-key-change-in-prod",
resave: false,
saveUninitialized: false,
cookie: {
secure: false,
httpOnly: true,
maxAge: 7 * 24 * 60 * 60 * 1000,
sameSite: "lax",
},
export const sessionMiddleware = session({
store: new PgSession({
pool,
tableName: "user_sessions",
}),
);
secret: process.env.SESSION_SECRET ?? (() => {
if (process.env.NODE_ENV === "production") {
throw new Error("SESSION_SECRET must be set in production");
}
return "teaboy-dev-secret-change-before-deploy";
})(),
resave: false,
saveUninitialized: false,
cookie: {
secure: false,
httpOnly: true,
maxAge: 7 * 24 * 60 * 60 * 1000,
sameSite: "lax",
},
});
app.use(sessionMiddleware);
app.use("/api", router);
+40 -12
View File
@@ -1,12 +1,11 @@
import { createServer } from "http";
import { Server as SocketIOServer } from "socket.io";
import app from "./app";
import type { IncomingMessage, ServerResponse } from "http";
import app, { sessionMiddleware } from "./app";
import { logger } from "./lib/logger";
import { db } from "@workspace/db";
import {
messagesTable,
conversationParticipantsTable,
messageReadsTable,
} from "@workspace/db";
import { eq, and } from "drizzle-orm";
@@ -29,23 +28,52 @@ const httpServer = createServer(app);
export const io = new SocketIOServer(httpServer, {
path: "/api/socket.io",
cors: {
origin: "*",
origin: process.env.ALLOWED_ORIGINS
? process.env.ALLOWED_ORIGINS.split(",").map((o) => o.trim())
: true,
methods: ["GET", "POST"],
credentials: true,
},
});
io.on("connection", (socket) => {
logger.info({ socketId: socket.id }, "Socket connected");
io.engine.use(
(req: IncomingMessage, res: ServerResponse, next: (err?: unknown) => void) => {
sessionMiddleware(req as Parameters<typeof sessionMiddleware>[0], res as Parameters<typeof sessionMiddleware>[1], next);
},
);
socket.on("join", (userId: number) => {
socket.join(`user:${userId}`);
logger.info({ socketId: socket.id, userId }, "User joined room");
});
io.use((socket, next) => {
const req = socket.request as IncomingMessage & { session?: { userId?: number } };
const userId = req.session?.userId;
if (!userId) {
return next(new Error("Unauthorized: no session"));
}
socket.data.userId = userId;
next();
});
io.on("connection", (socket) => {
const userId = socket.data.userId as number;
logger.info({ socketId: socket.id, userId }, "Socket connected");
socket.join(`user:${userId}`);
socket.on(
"join_conversation",
(conversationId: number) => {
socket.join(`conversation:${conversationId}`);
async (conversationId: number) => {
const [participant] = await db
.select()
.from(conversationParticipantsTable)
.where(
and(
eq(conversationParticipantsTable.conversationId, conversationId),
eq(conversationParticipantsTable.userId, userId),
),
);
if (participant) {
socket.join(`conversation:${conversationId}`);
}
},
);
@@ -20,6 +20,22 @@ import {
const router: IRouter = Router();
async function isConversationMember(
conversationId: number,
userId: number,
): Promise<boolean> {
const [row] = await db
.select({ userId: conversationParticipantsTable.userId })
.from(conversationParticipantsTable)
.where(
and(
eq(conversationParticipantsTable.conversationId, conversationId),
eq(conversationParticipantsTable.userId, userId),
),
);
return !!row;
}
async function buildConversationDetails(conversationId: number, currentUserId: number) {
const [conv] = await db
.select()
@@ -146,6 +162,12 @@ router.get("/conversations/:id", requireAuth, async (req, res): Promise<void> =>
}
const userId = req.session.userId!;
if (!(await isConversationMember(params.data.id, userId))) {
res.status(403).json({ error: "Forbidden" });
return;
}
const details = await buildConversationDetails(params.data.id, userId);
if (!details) {
@@ -163,6 +185,13 @@ router.get("/conversations/:id/messages", requireAuth, async (req, res): Promise
return;
}
const userId = req.session.userId!;
if (!(await isConversationMember(params.data.id, userId))) {
res.status(403).json({ error: "Forbidden" });
return;
}
const msgs = await db
.select()
.from(messagesTable)
@@ -206,6 +235,11 @@ router.post("/conversations/:id/messages", requireAuth, async (req, res): Promis
const userId = req.session.userId!;
if (!(await isConversationMember(params.data.id, userId))) {
res.status(403).json({ error: "Forbidden" });
return;
}
const [msg] = await db
.insert(messagesTable)
.values({
@@ -244,6 +278,11 @@ router.post("/conversations/:id/read", requireAuth, async (req, res): Promise<vo
const userId = req.session.userId!;
if (!(await isConversationMember(params.data.id, userId))) {
res.status(403).json({ error: "Forbidden" });
return;
}
const unreadMessages = await db
.select({ id: messagesTable.id })
.from(messagesTable)
+15
View File
@@ -12,6 +12,21 @@ import {
const router: IRouter = Router();
router.get("/users/directory", requireAuth, async (_req, res): Promise<void> => {
const users = await db
.select({
id: usersTable.id,
username: usersTable.username,
displayNameAr: usersTable.displayNameAr,
displayNameEn: usersTable.displayNameEn,
avatarUrl: usersTable.avatarUrl,
isActive: usersTable.isActive,
})
.from(usersTable)
.where(eq(usersTable.isActive, true));
res.json(users);
});
function buildUserProfile(user: typeof usersTable.$inferSelect, roles: string[]) {
return {
id: user.id,
@@ -1,6 +1,6 @@
import { createContext, useContext, useEffect, useState } from 'react';
import { useGetMe, getGetMeQueryKey } from '@workspace/api-client-react';
import type { AuthUser } from '@workspace/api-client-react/src/generated/api.schemas';
import { createContext, useContext, useEffect } from 'react';
import { useGetMe, getGetMeQueryKey, useUpdateLanguage } from '@workspace/api-client-react';
import type { AuthUser } from '@workspace/api-client-react';
import { useLocation } from 'wouter';
import { useQueryClient } from '@tanstack/react-query';
import i18n from '../i18n';
@@ -15,7 +15,7 @@ const AuthContext = createContext<AuthContextType>({ user: null, isLoading: true
export function AuthProvider({ children }: { children: React.ReactNode }) {
const [location, setLocation] = useLocation();
const queryClient = useQueryClient();
const { data: user, isLoading, isError } = useGetMe({
query: {
queryKey: getGetMeQueryKey(),
@@ -23,6 +23,8 @@ export function AuthProvider({ children }: { children: React.ReactNode }) {
}
});
const updateLanguage = useUpdateLanguage();
useEffect(() => {
if (!isLoading) {
if (isError || !user) {
@@ -31,7 +33,7 @@ export function AuthProvider({ children }: { children: React.ReactNode }) {
}
} else {
if (user.preferredLanguage && user.preferredLanguage !== i18n.language) {
i18n.changeLanguage(user.preferredLanguage);
i18n.changeLanguage(user.preferredLanguage);
}
if (location === '/login' || location === '/register') {
setLocation('/');
+21 -17
View File
@@ -1,4 +1,4 @@
import { useState } from "react";
import { useState, useEffect } from "react";
import { useTranslation } from "react-i18next";
import { useLocation } from "wouter";
import {
@@ -55,12 +55,14 @@ export default function AdminPage() {
const queryClient = useQueryClient();
const { toast } = useToast();
const BackIcon = isRtl ? ArrowRight : ArrowLeft;
const isAdmin = user?.roles?.includes("admin") ?? false;
// Redirect non-admins
if (user && !user.roles?.includes("admin")) {
setLocation("/");
return null;
}
// Redirect non-admins using useEffect to avoid violating rules of hooks
useEffect(() => {
if (user && !isAdmin) {
setLocation("/");
}
}, [user, isAdmin, setLocation]);
const { data: apps } = useListApps({ query: { queryKey: getListAppsQueryKey() } });
const { data: services } = useListServices({ query: { queryKey: getListServicesQueryKey() } });
@@ -135,6 +137,8 @@ export default function AdminPage() {
}
};
if (!user || !isAdmin) return null;
return (
<div className="min-h-screen os-bg flex flex-col">
{/* Header */}
@@ -252,13 +256,13 @@ export default function AdminPage() {
onClick={() => setEditingApp({
id: app.id,
form: {
nameAr: app.nameAr,
nameEn: app.nameEn,
slug: app.slug,
iconName: app.iconName,
route: app.route,
color: app.color,
sortOrder: app.sortOrder,
nameAr: app.nameAr ?? "",
nameEn: app.nameEn ?? "",
slug: app.slug ?? "",
iconName: app.iconName ?? "Grid2X2",
route: app.route ?? "/",
color: app.color ?? "#6366f1",
sortOrder: app.sortOrder ?? 0,
},
})}
className="p-1.5 hover:bg-white/10 rounded-lg text-muted-foreground hover:text-foreground"
@@ -313,12 +317,12 @@ export default function AdminPage() {
onClick={() => setEditingService({
id: service.id,
form: {
nameAr: service.nameAr,
nameEn: service.nameEn,
nameAr: service.nameAr ?? "",
nameEn: service.nameEn ?? "",
descriptionAr: service.descriptionAr ?? "",
descriptionEn: service.descriptionEn ?? "",
price: service.price,
isAvailable: service.isAvailable,
price: service.price ?? "0.00",
isAvailable: service.isAvailable ?? true,
},
})}
className="p-1.5 hover:bg-white/10 rounded-lg text-muted-foreground hover:text-foreground"
+14 -8
View File
@@ -4,17 +4,13 @@ import { useLocation } from "wouter";
import {
useListConversations,
getListConversationsQueryKey,
useGetConversation,
getGetConversationQueryKey,
useListMessages,
getListMessagesQueryKey,
useSendMessage,
useMarkConversationRead,
useCreateConversation,
useListUsers,
getListUsersQueryKey,
} from "@workspace/api-client-react";
import { useQueryClient } from "@tanstack/react-query";
import { useQuery, useQueryClient } from "@tanstack/react-query";
import { useAuth } from "@/contexts/AuthContext";
import { io } from "socket.io-client";
import { ArrowRight, ArrowLeft, MessageCircle, Plus, Send } from "lucide-react";
@@ -56,8 +52,19 @@ export default function ChatPage() {
{ query: { queryKey: getListMessagesQueryKey(selectedConvId!), enabled: !!selectedConvId } },
);
const { data: users } = useListUsers({
query: { queryKey: getListUsersQueryKey() },
type DirectoryUser = {
id: number;
username: string;
displayNameAr: string | null;
displayNameEn: string | null;
avatarUrl: string | null;
isActive: boolean;
};
const { data: users } = useQuery<DirectoryUser[]>({
queryKey: ["users-directory"],
queryFn: () =>
fetch("/api/users/directory", { credentials: "include" }).then((r) => r.json()),
enabled: showNewConv,
});
const sendMessage = useSendMessage();
@@ -68,7 +75,6 @@ export default function ChatPage() {
useEffect(() => {
if (!user) return;
const socket = getSocket();
socket.emit("join", user.id);
if (selectedConvId) {
socket.emit("join_conversation", selectedConvId);
+7
View File
@@ -9,6 +9,8 @@ import {
useListNotifications,
getListNotificationsQueryKey,
useLogout,
useUpdateLanguage,
getGetMeQueryKey,
} from "@workspace/api-client-react";
import { useQueryClient } from "@tanstack/react-query";
import { useAuth } from "@/contexts/AuthContext";
@@ -55,6 +57,7 @@ export default function HomePage() {
const { data: notifications } = useListNotifications({ query: { queryKey: getListNotificationsQueryKey() } });
const logout = useLogout();
const updateLanguage = useUpdateLanguage();
useEffect(() => {
const timer = setInterval(() => setTime(new Date()), 1000);
@@ -82,6 +85,10 @@ export default function HomePage() {
const toggleLanguage = () => {
const newLang = lang === "ar" ? "en" : "ar";
i18n.changeLanguage(newLang);
updateLanguage.mutate(
{ data: { language: newLang } },
{ onSuccess: () => queryClient.invalidateQueries({ queryKey: getGetMeQueryKey() }) },
);
};
const dockApps = apps?.filter((a) => ["services", "chat", "notifications", "admin"].includes(a.slug)) ?? [];