From d70d37a4679a87cb5dfd4acd5aa741791ca7cbc2 Mon Sep 17 00:00:00 2001 From: Arthur Miranda Sales Date: Wed, 27 May 2026 23:51:15 -0300 Subject: [PATCH] docs: add code review with 23 comments on architecture, security and code smells --- CODE_REVIEW.md | 465 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 465 insertions(+) create mode 100644 CODE_REVIEW.md diff --git a/CODE_REVIEW.md b/CODE_REVIEW.md new file mode 100644 index 0000000..f99952d --- /dev/null +++ b/CODE_REVIEW.md @@ -0,0 +1,465 @@ +# Code Review — RevoAPP + +Revisão de código realizada como atividade da disciplina. Os comentários abaixo cobrem segurança, arquitetura, padrões de projeto, code smells e boas práticas. + +--- + +## 🔴 Segurança Crítica + +### [1] Senha armazenada em texto puro — `src/backend/src/routes/user.ts:86` + +```ts +password: body.password, +``` + +**Problema:** A senha do usuário é salva diretamente no banco sem nenhum processo de hash. Se o banco de dados for comprometido, todas as senhas ficam expostas em texto legível. + +**Sugestão:** Aplicar hash com `bcrypt` ou `argon2` antes de persistir: + +```ts +import bcrypt from "bcrypt" +const hashedPassword = await bcrypt.hash(body.password, 12) +// usar hashedPassword no prisma.user.create +``` + +Na comparação do login, substituir a igualdade direta por: +```ts +await bcrypt.compare(body.password, user.password) +``` + +--- + +### [2] `@ts-ignore` mascarando falha de segurança — `user.ts:130` + +```ts +//@ts-ignore +if(user.password != body.password) +``` + +**Problema:** O TypeScript está apontando um erro legítimo: `password` não está incluído no `select` da query de login, então `user.password` é `undefined` em runtime — a comparação nunca funciona corretamente. O `@ts-ignore` suprime o alerta em vez de corrigir o problema. + +**Sugestão:** Incluir `password` explicitamente no `include`/`select` e usar `bcrypt.compare`. + +--- + +### [3] Log de credenciais em produção — `user.ts:69` + +```ts +console.log(body) +``` + +**Problema:** O corpo da requisição de cadastro (contendo email e senha em texto puro) é logado no servidor em produção. Viola privacidade do usuário e pode expor credenciais nos logs. + +**Sugestão:** Remover todos os `console.log` de debug antes de qualquer deploy em produção. + +--- + +### [4] URL de produção hardcoded — `user.ts:7`, `event.ts:11` + +```ts +const productionURL = "https://revoapp.up.railway.app" +``` + +**Problema:** URL fixa no código-fonte. Em ambientes de desenvolvimento ou staging, os links gerados (convites, notificações) apontam para produção — comportamento incorreto. Além disso, está duplicada em dois arquivos diferentes. + +**Sugestão:** Usar variável de ambiente: + +```ts +const productionURL = process.env.BASE_URL ?? "http://localhost:3333" +``` + +--- + +### [5] Ausência de autenticação nas rotas — `routes/user.ts`, `routes/event.ts` + +**Problema:** Qualquer pessoa pode chamar `PUT /user/:userId` e alterar os dados de qualquer usuário. Não há middleware verificando se quem faz a requisição tem autorização para operar sobre aquele recurso. + +**Sugestão:** Implementar autenticação com JWT: + +```ts +// middleware/authenticate.ts +export function authenticate(req, res, next) { + const token = req.headers.authorization?.split(" ")[1] + if (!token) return res.status(401).json({ message: "Não autenticado" }) + try { + const payload = jwt.verify(token, process.env.JWT_SECRET) + req.userId = payload.sub + next() + } catch { + res.status(401).json({ message: "Token inválido" }) + } +} +``` + +Aplicar o middleware nas rotas que exigem autenticação. + +--- + +## 🟠 Arquitetura e Padrões de Projeto + +### [6] Ausência de camada de serviço — Padrão: Service Layer + +**Problema:** Toda a lógica de negócio está embutida diretamente nos handlers de rota — verificação de existência de usuário, criação de notificações, regras de amizade e convites. Isso viola o **Princípio da Responsabilidade Única (SRP)** e torna o código impossível de testar unitariamente sem subir um servidor HTTP. + +**Padrão recomendado:** Service Layer + +``` +routes/ → recebe requisição, delega, retorna resposta +services/ → lógica de negócio (UserService, EventService) +repositories/ → acesso ao banco de dados (UserRepository, EventRepository) +``` + +Exemplo: +```ts +// services/UserService.ts +export class UserService { + constructor(private userRepo: UserRepository) {} + + async addFriend(userId: string, friendId: string): Promise { + const user = await this.userRepo.findById(userId) + if (!user) throw new NotFoundError("Usuário não encontrado") + // ... lógica completa aqui + } +} +``` + +**Benefícios:** +- Lógica de negócio testável sem depender de HTTP +- Separação clara de responsabilidades +- Fácil substituição da camada de persistência + +--- + +### [7] PrismaClient instanciado múltiplas vezes — Padrão: Singleton + +**Problema:** `new PrismaClient()` é chamado em `user.ts`, `event.ts` e `indicadores.ts`. Cada instância abre seu próprio connection pool com o banco. Em produção com hot-reload isso causa **connection pool exhaustion**. + +**Sugestão:** Singleton compartilhado: + +```ts +// src/lib/prisma.ts +import { PrismaClient } from "@prisma/client" +export const prisma = new PrismaClient() +``` + +Importar `prisma` deste arquivo em todos os módulos que precisarem. + +--- + +### [8] Duplicação de `productionURL` — Violação do princípio DRY + +**Problema:** A constante `productionURL` está declarada em `user.ts:7` e novamente em `event.ts:11`. Se a URL mudar, precisa ser atualizada em dois lugares — fonte de bugs. + +**Sugestão:** Centralizar em `src/config/env.ts`: + +```ts +export const config = { + baseUrl: process.env.BASE_URL ?? "https://revoapp.up.railway.app", + port: process.env.PORT ?? 3333, +} +``` + +--- + +### [9] Ausência de tratamento de erros (try/catch) — todas as rotas + +**Problema:** Nenhuma rota possui `try/catch`. Se o banco de dados cair ou o Prisma lançar uma exceção inesperada, o Express não captura a promise rejeitada e o servidor derruba com `UnhandledPromiseRejection`. + +**Sugestão:** Wrapper de async + middleware global de erro: + +```ts +// utils/asyncHandler.ts +export const asyncHandler = (fn: RequestHandler): RequestHandler => + (req, res, next) => Promise.resolve(fn(req, res, next)).catch(next) + +// src/index.ts — registrar por último +app.use((err: Error, req: Request, res: Response, next: NextFunction) => { + console.error(err) + res.status(500).json({ message: "Erro interno do servidor" }) +}) +``` + +--- + +### [10] Ausência de validação de input — rotas POST e PUT + +**Problema:** Campos como `body.email`, `body.password` e `body.name` são usados diretamente sem qualquer validação. Um cliente malicioso pode enviar campos ausentes ou com tipos errados, causando erros internos inesperados. + +**Padrão recomendado:** Validação com schema usando `zod`: + +```ts +import { z } from "zod" + +const SignupSchema = z.object({ + email: z.string().email("Email inválido"), + name: z.string().min(2, "Nome muito curto"), + password: z.string().min(8, "Senha deve ter ao menos 8 caracteres"), +}) + +// Na rota: +const result = SignupSchema.safeParse(request.body) +if (!result.success) + return response.status(400).json(result.error.flatten()) +``` + +--- + +### [11] Padrão Repository ausente — acoplamento direto com Prisma + +**Problema:** As rotas chamam o Prisma diretamente em todos os lugares. Se a tecnologia de banco mudar (ex.: de PostgreSQL para MongoDB, ou de Prisma para outro ORM), cada rota precisaria ser reescrita. + +**Sugestão:** Padrão Repository: + +```ts +// repositories/UserRepository.ts +export class UserRepository { + constructor(private prisma: PrismaClient) {} + + findById(id: string) { + return this.prisma.user.findUnique({ where: { id } }) + } + + findByEmail(email: string) { + return this.prisma.user.findUnique({ where: { email } }) + } + + create(data: CreateUserInput) { + return this.prisma.user.create({ data }) + } +} +``` + +**Benefícios:** +- Desacoplamento entre lógica de negócio e persistência +- Mock simples para testes unitários +- Substituição de ORM sem impacto nas camadas superiores + +--- + +## 🟡 Bugs e HTTP Semântico + +### [12] Status HTTP semanticamente incorretos — múltiplos arquivos + +| Rota | Status atual | Status correto | Motivo | +|---|---|---|---| +| `GET /event/` | `302` (redirect) | `200` (ok) | 302 significa redirecionamento | +| `GET /event/:id` | `302` (redirect) | `200` (ok) | Idem | +| `POST /user/search` | `201` (created) | `200` (ok) | Search não cria recurso | +| `GET accept_friend` já amigo | `404` (not found) | `409` (conflict) | Conflito de estado, não ausência | +| `PUT /user/:id` retorno | `302` | `200` | Retorno de dados não é redirect | + +Usar status HTTP incorretos confunde clientes e ferramentas de monitoramento. + +--- + +### [13] Bug: `Array.forEach` atribuído a variável — `event.ts:329` + +```ts +const goalId = event.recevied_goal.forEach((goal) => { + // ... + return goal.id // não funciona — forEach ignora o retorno +}) +// goalId === undefined sempre +``` + +**Problema:** `Array.forEach` sempre retorna `undefined`. O `return goal.id` dentro do callback não retorna valor nenhum para `goalId`. A URL de recusa do patrocínio (`recuse_route`) sempre terá `goalId = undefined`. + +**Sugestão:** Usar `Array.find`: + +```ts +const matchingGoal = event.recevied_goal.find((goal) => + goal.money === body.money && goal.userId === params.authorId +) +const goalId = matchingGoal?.id +``` + +--- + +### [14] `const` reatribuída em runtime — `Login/index.js:21-24` + +```js +const userData = JSON.parse(localStorage.getItem("user")) +if (userData == null) { + userData = [] // TypeError: Assignment to constant variable +} +``` + +**Problema:** `const` não pode ser reatribuída. Este código lança `TypeError` em runtime quando o usuário não tem dados no localStorage. + +**Sugestão:** +```js +const userData = JSON.parse(localStorage.getItem("user")) ?? [] +``` + +--- + +### [15] Comparação de resposta por string JSON — `Login/index.js:13` + +```js +if (teste === '{"Message":"Usuário não existente"}') { +``` + +**Problema:** Comparação frágil. Se a API adicionar um campo, mudar a ordem das chaves ou espaçamento, a comparação quebra silenciosamente — o usuário não vê o alerta de erro mesmo quando a autenticação falha. + +**Sugestão:** Parsear o JSON e verificar por status HTTP ou campo específico: + +```js +const data = JSON.parse(this.responseText) +if (this.status === 404) alert(data.Message) +else if (this.status === 409) alert(data.Message) +else window.location.href = './pages/user.html' +``` + +--- + +### [16] `Date()` sem `new` — `createEvent/index.js:14` + +```js +var date = Date(document.getElementById('data').value) +``` + +**Problema:** `Date(valor)` sem `new` ignora completamente o argumento e retorna a data atual como string legível. Todo evento criado usará a data de criação, não a data escolhida pelo usuário. + +**Sugestão:** +```js +const date = new Date(document.getElementById('data').value) +``` + +--- + +### [17] Rotas GET com efeitos colaterais — `user.ts:200`, `user.ts:375` + +```ts +userRoutes.get("/:userId/invite/:friendId/to/:eventId", ...) +userRoutes.get("/:userId/add_friend/:friendId", ...) +``` + +**Problema:** Rotas GET devem ser idempotentes e sem efeitos colaterais (RFC 7231). Estas rotas criam notificações e registros no banco — ações de escrita nunca devem estar em GET. Um link pré-carregado pelo browser ou um bot de preview poderia disparar convites involuntariamente. + +**Sugestão:** Mover para `POST`: +- `POST /user/:userId/invites` — body `{ friendId, eventId }` +- `POST /user/:userId/friends` — body `{ friendId }` + +--- + +## 🔵 Code Smells e Qualidade + +### [18] Typos generalizados em nomes de modelo e campo — `schema.prisma` + +| Nome atual | Nome correto | +|---|---| +| `sponsership` | `sponsorship` | +| `recevied` / `ReceviedGoal` | `received` / `ReceivedGoal` | +| `accepte_route` | `accept_route` | +| `recuse_route` | `refuse_route` | +| `"unkown"` (default de `Friend.friendId`) | `"unknown"` | +| `exixst` (mensagem de erro) | `exist` | + +Typos em nomes de modelos do banco propagam-se por toda a codebase e são difíceis de corrigir depois que dados existem em produção (exigem migrações). + +--- + +### [19] Mensagens de erro em português e inglês misturadas — `user.ts` + +```ts +// Inglês: +{"Message": "User doesn't exixst"} +{"Message": "Friend doesn't exixst"} + +// Português: +{"Message": "Usuário não encontrado"} +``` + +**Problema:** Inconsistência de idioma nas mensagens retornadas pela API. Dificulta tanto o uso por clientes quanto a manutenção. + +**Sugestão:** Padronizar todas as mensagens em português (idioma já predominante no projeto) e criar um arquivo de constantes: + +```ts +// constants/messages.ts +export const MSG = { + USER_NOT_FOUND: "Usuário não encontrado", + FRIEND_NOT_FOUND: "Amigo não encontrado", + // ... +} +``` + +--- + +### [20] Ausência de paginação em `findMany` — `user.ts:11`, `event.ts:16` + +```ts +const users = await prisma.user.findMany({ ... }) +const events = await prisma.event.findMany({ ... }) +``` + +**Problema:** Sem limite, uma query pode retornar milhares de registros, causando timeout, consumo excessivo de memória e resposta lenta. + +**Sugestão:** Cursor-based pagination: + +```ts +const { cursor, take = "20" } = request.query +const events = await prisma.event.findMany({ + take: Number(take), + ...(cursor ? { cursor: { id: cursor as string }, skip: 1 } : {}), + orderBy: { date: "desc" }, +}) +``` + +--- + +### [21] `console.log` de debug esquecidos em produção — `user.ts:454`, `event.ts:345` + +```ts +console.log(isAlreadyFriend) +console.log(goalId) +``` + +Logs de debug poluem os logs de produção e podem expor dados sensíveis de usuários. Remover antes de qualquer deploy. + +--- + +### [22] XMLHttpRequest vs Fetch — inconsistência no frontend + +`Login/index.js` usa `XMLHttpRequest` (API antiga e verbosa). Todos os outros scripts (`createEvent`, `search`, `Home`) usam `fetch` com `async/await`. Inconsistência tecnológica sem justificativa no mesmo projeto. + +**Sugestão:** Padronizar todo o frontend em `fetch + async/await`: + +```js +async function fazPost(url, body) { + const response = await fetch(url, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify(body), + }) + if (!response.ok) throw new Error(await response.text()) + return response.json() +} +``` + +--- + +### [23] `indicadores.ts` é código morto + +```ts +// src/backend/src/indicadores.ts +indicadorRoutes.get("/", async (request, response) => { + const users = await prisma.user.findMany(...) + return response.status(200).json(users) +}) +``` + +Funcionalidade idêntica a `GET /user/`. Nenhuma lógica de "indicadores" implementada. Código morto aumenta a carga cognitiva e deve ser removido ou implementado de fato. + +--- + +## Resumo + +| Categoria | Qtd | +|---|---| +| Segurança crítica | 5 | +| Arquitetura / Padrões | 6 | +| Bugs / HTTP semântico | 6 | +| Code smells / Qualidade | 6 | +| **Total** | **23** | + +**Padrões de projeto sugeridos:** Service Layer, Repository Pattern, Singleton (PrismaClient), middleware de autenticação JWT, validação de schema com Zod.