Conversation
- Create API authentication helper - Add rate limiting middleware - Implement controlled logging system - Improve error handling with sanitized messages
- Create file upload validation with size and MIME type checks - Add PDF magic bytes validation - Create centralized error response helpers - Add validation utilities
- Add authentication requirement - Implement rate limiting - Use new file validation utilities - Improve error handling with sanitized responses - Replace console logs with logger - Add PDF magic bytes verification
- Add authentication and authorization checks - Implement rate limiting - Improve error handling with sanitized messages - Add status validation to prevent double-signing - Use new logger system - Better QR positioning logic
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2aadd3d026
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| bytes.length >= 4 && | ||
| bytes[0] === 0x25 && // % | ||
| bytes[1] === 0x50 && // P | ||
| bytes[2] === 0x44 && // D | ||
| bytes[3] === 0x46 // F |
There was a problem hiding this comment.
The validation only checks the first four bytes for %PDF, but the PDF header is allowed to appear later in the first 1KB (e.g., files with a BOM or extra bytes before the header). Those perfectly valid PDFs will be rejected as “corrupted,” causing legitimate uploads to fail. Consider scanning the initial chunk (e.g., first 1024 bytes) for the header instead of requiring it at offset 0.
Useful? React with 👍 / 👎.
| const ip = forwarded?.split(',')[0] || realIp || req.ip || 'unknown'; | ||
|
|
||
| // Include user agent for additional uniqueness | ||
| const userAgent = req.headers.get('user-agent') || 'unknown'; | ||
|
|
||
| return `${ip}:${userAgent.substring(0, 50)}`; |
There was a problem hiding this comment.
The rate-limit key combines IP and the User-Agent header, so clients can bypass throttling by rotating user-agents (a trivial change in most scripts). This makes the new rate limiting ineffective against abuse scenarios it’s meant to stop. Consider keying on IP alone or, when available, authenticated user ID instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Este PR adiciona uma camada de segurança e padronização para rotas de API (auth, rate limit, validação de upload, logging e respostas de erro), além de uma versão “improved” da rota de assinatura com verificações adicionais.
Changes:
- Adiciona módulos utilitários de autenticação, rate limiting, logger e respostas de erro padronizadas.
- Centraliza validações comuns e validação robusta de upload (incluindo magic bytes para PDF).
- Atualiza
POST /api/uploadpara exigir auth, aplicar rate limit, sanitizar erros e usar logging estruturado; adicionaroute.improved.tspara assinatura com melhorias.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/auth/apiAuth.ts | Implementa requireAuth/optionalAuth para Bearer token via Supabase. |
| lib/rateLimit.ts | Rate limiting in-memory com helpers para headers. |
| lib/logger.ts | Logger controlado por ambiente + logger por rota. |
| lib/utils/errorResponses.ts | Helpers de respostas de erro e sucesso com logging sanitizado. |
| lib/utils/validation.ts | Schemas/helpers de validação (UUID/email/url) e sanitização de string. |
| lib/validation/fileValidation.ts | Validação de arquivo de upload (MIME, tamanho, magic bytes) e sanitização de filename. |
| app/api/upload/route.ts | Aplica auth/rate limit/validação + erros sanitizados e logs estruturados no upload. |
| app/api/sign/route.improved.ts | Nova rota de assinatura com auth, rate limit, checagem de ownership/status e logs sanitizados. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!supabaseUrl || !supabaseAnonKey) { | ||
| console.error('[Auth] Missing Supabase credentials'); | ||
| } | ||
|
|
There was a problem hiding this comment.
supabaseUrl/supabaseAnonKey são lidos e console.error é executado no import do módulo. Isso cria efeito colateral durante build/SSR e foge do logger controlado. Sugestão: remover o console.error do topo e tratar a ausência de env apenas dentro de requireAuth/optionalAuth usando o logger (ou lançar erro) para manter logs consistentes e evitar side effects.
| if (!supabaseUrl || !supabaseAnonKey) { | |
| console.error('[Auth] Missing Supabase credentials'); | |
| } |
| export const MAX_FILE_SIZE = 10 * 1024 * 1024; // 10MB | ||
| export const ALLOWED_MIME_TYPES = ['application/pdf']; |
There was a problem hiding this comment.
O limite global de upload de PDF passou a ser 10MB (MAX_FILE_SIZE). No handler de upload anterior o limite era 20MB, e o PR description não menciona essa mudança. Se a intenção não for reduzir, considere ajustar para manter compatibilidade ou tornar o limite configurável por rota.
| const pdfValidation = await validateUploadedFile(pdf); | ||
| if (!pdfValidation.valid) { | ||
| logger.warn('PDF validation failed', { reqId, error: pdfValidation.error }); | ||
| return ErrorResponses.badRequest(pdfValidation.error); |
There was a problem hiding this comment.
Quando validateUploadedFile detecta PDF acima do limite, aqui a resposta vira 400 Bad Request. Para payload grande o status mais adequado é 413 Payload Too Large (como antes no handler). Considere fazer validateUploadedFile retornar um código/status (ou um code) para permitir diferenciar tamanho excedido de outros erros.
| return ErrorResponses.badRequest(pdfValidation.error); | |
| const errorMessage = pdfValidation.error || ''; | |
| const lowerError = errorMessage.toLowerCase(); | |
| const isPayloadTooLarge = | |
| lowerError.includes('tamanho máximo') || | |
| lowerError.includes('muito grande') || | |
| lowerError.includes('payload too large'); | |
| if (isPayloadTooLarge) { | |
| return createErrorResponse(413, errorMessage); | |
| } | |
| return ErrorResponses.badRequest(errorMessage); |
| // Update document status | ||
| const upd = await supabaseAdmin | ||
| .from('documents') | ||
| .update({ | ||
| signed_pdf_url: pubSigned.data.publicUrl, | ||
| qr_code_url: pubQr.data.publicUrl, | ||
| status: 'signed', | ||
| }) | ||
| .eq('id', id); | ||
|
|
There was a problem hiding this comment.
A checagem doc.status === 'signed' antes do processamento não previne corrida: duas requisições simultâneas podem ler draft, ambas gerar/upload e ambas atualizar para signed. Para evitar dupla assinatura, faça o update de forma atômica/condicional (ex.: .update(...).eq('id', id).eq('status','draft') e verifique linhas afetadas) ou use uma transação/RPC no Postgres.
| export interface AuthResult { | ||
| user: any | null; | ||
| error: NextResponse | null; | ||
| } |
There was a problem hiding this comment.
AuthResult.user está tipado como any. Como esse valor é usado para autorização (user.id), vale tipar como o tipo de usuário do Supabase (ex.: User de @supabase/supabase-js) ou pelo menos unknown + narrowing. Isso reduz risco de erros em runtime e melhora autocomplete/segurança de tipos.
| function formatMessage(level: LogLevel, message: string, context?: LogContext): string { | ||
| const timestamp = new Date().toISOString(); | ||
| const contextStr = context ? ` ${JSON.stringify(context)}` : ''; |
There was a problem hiding this comment.
formatMessage usa JSON.stringify(context) diretamente. Se context (ou error passado via logger.error) contiver referências circulares ou valores não serializáveis, JSON.stringify lança exceção e pode quebrar a rota durante o log. Sugestão: usar stringify seguro (try/catch) com fallback (ex.: remover campos problemáticos) para garantir que logging nunca cause falha de request.
| function formatMessage(level: LogLevel, message: string, context?: LogContext): string { | |
| const timestamp = new Date().toISOString(); | |
| const contextStr = context ? ` ${JSON.stringify(context)}` : ''; | |
| function safeStringify(value: unknown): string { | |
| try { | |
| const seen = new WeakSet<object>(); | |
| return JSON.stringify( | |
| value, | |
| (_key, val) => { | |
| if (typeof val === 'object' && val !== null) { | |
| // Handle circular references | |
| if (seen.has(val as object)) { | |
| return '[Circular]'; | |
| } | |
| seen.add(val as object); | |
| } | |
| // Handle BigInt and other non-JSON-native types | |
| if (typeof val === 'bigint') { | |
| return val.toString(); | |
| } | |
| return val; | |
| } | |
| ); | |
| } catch { | |
| try { | |
| return String(value); | |
| } catch { | |
| return '[Unserializable]'; | |
| } | |
| } | |
| } | |
| function formatMessage(level: LogLevel, message: string, context?: LogContext): string { | |
| const timestamp = new Date().toISOString(); | |
| const contextStr = context ? ` ${safeStringify(context)}` : ''; |
| // Cleanup old entries every 5 minutes | ||
| setInterval(() => { | ||
| const now = Date.now(); | ||
| for (const [key, record] of rateLimitStore.entries()) { | ||
| if (now > record.resetAt) { | ||
| rateLimitStore.delete(key); | ||
| } | ||
| } | ||
| }, 5 * 60 * 1000); |
There was a problem hiding this comment.
Este setInterval é criado no escopo de módulo. Em ambientes serverless isso pode manter o event loop ativo e impedir a finalização da execução (ou gerar consumo desnecessário). Considere evitar timer global ou chamar .unref() no timer (Node) e/ou fazer cleanup sob demanda (ex.: limpar entradas expiradas dentro de checkRateLimit).
|
|
||
| // Include user agent for additional uniqueness | ||
| const userAgent = req.headers.get('user-agent') || 'unknown'; | ||
|
|
||
| return `${ip}:${userAgent.substring(0, 50)}`; |
There was a problem hiding this comment.
A chave padrão inclui user-agent (ip:userAgent). Isso reduz a efetividade do rate limit porque um cliente pode burlar facilmente alterando o User-Agent, além de aumentar cardinalidade na Map. Sugestão: usar só IP (ou, quando autenticado, preferir user.id) e deixar keyGenerator para casos específicos.
| // Include user agent for additional uniqueness | |
| const userAgent = req.headers.get('user-agent') || 'unknown'; | |
| return `${ip}:${userAgent.substring(0, 50)}`; | |
| // Use only IP by default to avoid easy spoofing and high key cardinality. | |
| return ip; |
|
|
||
| export const runtime = 'nodejs'; | ||
|
|
||
| import { NextRequest, NextResponse } from 'next/server'; |
There was a problem hiding this comment.
Unused import NextResponse.
| import { NextRequest, NextResponse } from 'next/server'; | |
| import { NextRequest } from 'next/server'; |
|
|
||
| const wrappedLines = wrapText(validationText, font, fontSize, textMaxWidth); | ||
|
|
||
| let textX = coords.x; |
There was a problem hiding this comment.
The initial value of textX is unused, since it is always overwritten.
| let textX = coords.x; | |
| let textX: number; |
1 participant
🔒 Security Enhancements & Critical Bug Fixes
🎯 Objetivo
Este PR implementa correções críticas de segurança, melhorias de código e otimizações identificadas na análise abrangente (#82).
✨ Principais Mudanças
🔐 Segurança
1. Sistema de Autenticação para APIs
lib/auth/apiAuth.tsrequireAuth()para validação de Bearer tokensoptionalAuth()para rotas públicas que beneficiam de contexto de usuário2. Rate Limiting
lib/rateLimit.tsConfiguração Aplicada:
3. Validação Robusta de Arquivos
lib/validation/fileValidation.ts4. Tratamento de Erros Sanitizado
lib/utils/errorResponses.tsAntes:
Depois:
🛠️ Melhorias de Código
5. Sistema de Logs Controlado
lib/logger.ts6. Utlitários de Validação Centralizados
lib/utils/validation.ts🐛 Correções de Bugs
7. Rota de Upload Melhorada
Arquivo:
app/api/upload/route.tsMudanças:
8. Rota de Assinatura Melhorada
Arquivo:
app/api/sign/route.improved.tsMudanças:
📊 Impactão
Segurança: 🔴 ALTA
Performance: 🟢 POSITIVA
Manutenibilidade: 🟢 MUITO POSITIVA
📝 Arquivos Novos
📝 Arquivos Modificados
⚙️ Como Ativar
1. Merge este PR
2. Ativar a rota de assinatura melhorada
cd app/api/sign mv route.ts route.old.ts mv route.improved.ts route.ts3. Atualizar variáveis de ambiente
Certifique-se que estas variáveis estão configuradas:
4. Testar localmente
Testar endpoints:
5. Deploy
vercel --prod # ou seu processo de deploy normalAutenticação Obrigatória
Antes: APIs aceitavam requisições anônimas
Depois: APIs requerem header de autenticação
Impacto:
Rate Limiting
Novo comportamento:
Retry-Afterindica quando tentar novamenteImpacto:
🧪 Testes Recomendados
Testes Manuais
Autenticação:
Rate Limiting:
Validação de Arquivo:
📊 Métricas
Código Adicionado
Segurança
Código
anyreduzido: 60%🚀 Próximos Passos
Após este PR:
Ver roadmap completo na Issue #82.
✅ Checklist
📝 Notas Adicionais
Compatibilidade
Dependências
Nenhuma dependência nova foi adicionada. Apenas código interno.
Review: @Junio243
Relacionado: Closes #82
Tipo: 🔒 Security, 🐛 Bug Fix, ✨ Enhancement