Conversation
Features implemented: - One-click email approve/reject with magic links - Mobile-first review page with fixed bottom action bar - Slack interactive buttons with signature verification - Dashboard search/filter and review templates - Toast notification system (Radix-based) - Content format support (PLAIN_TEXT, MARKDOWN, HTML) - ContentRenderer component with DOMPurify sanitization Security improvements (50 issues fixed across 3 code reviews): - HTML sanitization with DOMPurify - Slack webhook HMAC-SHA256 signature verification - Race conditions fixed with Prisma transactions - XSS prevention in markdown link rendering - Input validation and sanitization throughout Accessibility improvements: - ARIA labels on all interactive elements - Role attributes for status indicators and dialogs - Full i18n support with next-intl 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This pull request represents a significant product pivot and major feature expansion, transforming the approval tool into "SendVelo" - a comprehensive ChatGPT-native approval workflow platform. The changes include substantial infrastructure updates, multi-reviewer support, organization management, and enhanced integrations.
Key Changes:
- Complete product rebranding from generic approval tool to SendVelo with detailed implementation and strategy documentation
- Multi-reviewer workflow system with parallel, sequential, and any-one approval modes
- Organization/team management using Better Auth plugin
- Enhanced MCP server with 5 new tools for comprehensive ChatGPT integration
- Slack integration with interactive approvals
- Version control and content format support (Markdown, HTML, plain text)
Reviewed changes
Copilot reviewed 63 out of 92 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/CRITICAL_REVIEW.md | Removed critical review document (cleanup) |
| docs/IMPLEMENTATION_PLAN.md | Added comprehensive SendVelo implementation roadmap with Better Auth integration |
| docs/CHATGPT_APP_STORE_STRATEGY.md | Added ChatGPT App Store optimization strategy with golden prompts |
| baseUrl.ts | Removed file (architecture cleanup) |
| approval-tool/src/server/api/routers/user.ts | Updated subscription tier checks from string to enum |
| approval-tool/src/server/api/routers/review.ts | Major expansion: multi-reviewer support, workflows, version control, activity logs |
| approval-tool/src/server/api/routers/organization.ts | Added complete organization management router |
| approval-tool/src/server/api/root.ts | Added organization router to API |
| approval-tool/src/lib/templates.ts | Added review templates for common use cases |
| approval-tool/src/lib/slack.ts | Added Slack integration with retry logic and interactive notifications |
| approval-tool/src/lib/email.ts | Enhanced email templates with one-click approve/reject links |
| approval-tool/src/lib/auth.ts | Added organization plugin to Better Auth configuration |
| approval-tool/src/lib/auth-client.ts | Added organization client plugin |
| approval-tool/src/env.ts | Added Slack and additional Stripe environment variables |
| approval-tool/src/components/ui/toast.tsx | Added toast notification system |
| approval-tool/src/components/content-renderer.tsx | Added content renderer with XSS protection |
| approval-tool/prisma/schema.prisma | Major schema expansion: organizations, teams, reviewers, versions, activity logs |
| approval-tool/package.json | Updated dependencies for Slack, DOMPurify, diff |
| approval-tool/messages/en.json | Added translations for new features |
| approval-tool/app/mcp/route.ts | Major MCP server expansion with 5 new tools |
| approval-tool/app/layout.tsx | Simplified layout (removed i18n/tRPC from root) |
| approval-tool/app/globals.css | Added mobile-friendly styles and animations |
| approval-tool/app/api/webhooks/stripe/route.ts | Updated subscription tier enum values |
| approval-tool/app/api/slack/interactions/route.ts | Added Slack webhook handler with security validation |
| approval-tool/app/[locale]/layout.tsx | Added locale-specific layout with providers |
| approval-tool/app/[locale]/review/[slug]/page.tsx | Complete rewrite with mobile-first design and multi-reviewer UI |
| approval-tool/app/[locale]/approve/[reviewId]/[reviewerId]/page.tsx | Added one-click email approval page |
| approval-tool/app/[locale]/(app)/dashboard/page.tsx | Enhanced dashboard with search, filters, templates |
| Multiple legacy files | Removed old route structures and pages |
Files not reviewed (5)
- .idea/modules.xml: Language not supported
- .idea/sendvelo.iml: Language not supported
- .idea/vcs.xml: Language not supported
- .idea/workspace.xml: Language not supported
- approval-tool/pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Exponential backoff: 1s, 2s, 4s | ||
| const delay = baseDelayMs * Math.pow(2, attempt); | ||
| await new Promise(resolve => setTimeout(resolve, delay)); |
There was a problem hiding this comment.
Missing 'void' operator before Promise in setTimeout. In TypeScript, floating promises should be explicitly marked. Add void before new Promise to indicate intentional fire-and-forget: await new Promise<void>(resolve => setTimeout(resolve, delay));
| await new Promise(resolve => setTimeout(resolve, delay)); | |
| await new Promise<void>(resolve => setTimeout(resolve, delay)); |
| const approveUrl = canBuildOneClickLinks | ||
| ? `${baseUrl}/en/approve/${reviewId}/${reviewerId}?token=${encodeURIComponent(token)}&decision=approve` | ||
| : reviewUrl; | ||
| const rejectUrl = canBuildOneClickLinks | ||
| ? `${baseUrl}/en/approve/${reviewId}/${reviewerId}?token=${encodeURIComponent(token)}&decision=reject` |
There was a problem hiding this comment.
Hardcoded locale 'en' in URL construction. This should use a locale parameter or constant to support internationalization properly, as the app appears to support multiple locales based on the [locale] route structure.
| const approveUrl = canBuildOneClickLinks | |
| ? `${baseUrl}/en/approve/${reviewId}/${reviewerId}?token=${encodeURIComponent(token)}&decision=approve` | |
| : reviewUrl; | |
| const rejectUrl = canBuildOneClickLinks | |
| ? `${baseUrl}/en/approve/${reviewId}/${reviewerId}?token=${encodeURIComponent(token)}&decision=reject` | |
| // Derive locale from the reviewUrl path (e.g., /en/... or /de/...) | |
| let locale = "en"; | |
| try { | |
| const parsedReviewUrl = new URL(reviewUrl); | |
| const pathSegments = parsedReviewUrl.pathname.split("/").filter(Boolean); | |
| if (pathSegments.length > 0 && pathSegments[0]) { | |
| locale = pathSegments[0]; | |
| } | |
| } catch { | |
| // If parsing fails, fall back to the default locale ("en") | |
| } | |
| const approveUrl = canBuildOneClickLinks | |
| ? `${baseUrl}/${locale}/approve/${reviewId}/${reviewerId}?token=${encodeURIComponent(token)}&decision=approve` | |
| : reviewUrl; | |
| const rejectUrl = canBuildOneClickLinks | |
| ? `${baseUrl}/${locale}/approve/${reviewId}/${reviewerId}?token=${encodeURIComponent(token)}&decision=reject` |
| if (env.SLACK_SIGNING_SECRET) { | ||
| const crypto = await import("crypto"); | ||
| const sigBasestring = `v0:${timestamp}:${rawBody}`; | ||
| const mySignature = 'v0=' + crypto.createHmac('sha256', env.SLACK_SIGNING_SECRET).update(sigBasestring, 'utf8').digest('hex'); |
There was a problem hiding this comment.
Use template literals for string concatenation for consistency: const mySignature = \v0=${crypto.createHmac('sha256', env.SLACK_SIGNING_SECRET).update(sigBasestring, 'utf8').digest('hex')}`;`
| const mySignature = 'v0=' + crypto.createHmac('sha256', env.SLACK_SIGNING_SECRET).update(sigBasestring, 'utf8').digest('hex'); | |
| const mySignature = `v0=${crypto.createHmac('sha256', env.SLACK_SIGNING_SECRET).update(sigBasestring, 'utf8').digest('hex')}`; |
| // Sanitize HTML content with DOMPurify to prevent XSS attacks | ||
| const sanitizedHTML = DOMPurify.sanitize(content, { | ||
| ALLOWED_TAGS: ['p', 'br', 'strong', 'em', 'u', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'ul', 'ol', 'li', 'a', 'code', 'pre', 'blockquote', 'hr', 'img', 'div', 'span'], | ||
| ALLOWED_ATTR: ['href', 'src', 'alt', 'title', 'class', 'id'], |
There was a problem hiding this comment.
Allowing 'id' and 'class' attributes in sanitized HTML could enable CSS-based attacks or DOM clobbering. Consider removing these attributes from ALLOWED_ATTR unless specifically required for functionality.
| ALLOWED_ATTR: ['href', 'src', 'alt', 'title', 'class', 'id'], | |
| ALLOWED_ATTR: ['href', 'src', 'alt', 'title'], |
| try { | ||
| switch (name) { | ||
| case "send_for_review": { | ||
| const input = SendForReviewSchema.parse(args); |
There was a problem hiding this comment.
Schema validation errors from Zod will be caught and handled generically at line 718-729, but users would benefit from more specific error messages about which field failed validation (e.g., 'Invalid email format for reviewer at index 2'). Consider enhancing the error handling to provide field-specific context.
| <td className="px-6 py-4 whitespace-nowrap text-right text-sm font-medium"> | ||
| <button | ||
| onClick={() => { | ||
| if (confirm(t("reviews.deleteConfirm"))) { |
There was a problem hiding this comment.
Using native confirm() dialog is not accessible and doesn't match the modern UI design. Consider implementing a custom confirmation modal component that matches the app's design system and provides better accessibility support (keyboard navigation, screen reader support).
| </span> | ||
| <button | ||
| onClick={() => { | ||
| if (confirm(t("reviews.deleteConfirm"))) { |
There was a problem hiding this comment.
Using native confirm() dialog is not accessible and doesn't match the modern UI design. Consider implementing a custom confirmation modal component that matches the app's design system and provides better accessibility support (keyboard navigation, screen reader support).
Code Review: Feature/Brand Naming Exploration PROverall Assessment: This is a well-architected greenfield application with solid foundations, but contains several security vulnerabilities that need addressing before production deployment. Critical Security Issues (Must Fix Before Merge)1. XSS Vulnerability in MCP Route - approval-tool/app/mcp/route.ts:167-169Only trims/limits length - no HTML sanitization. When contentFormat is HTML, this allows XSS attacks. Fix: The ContentRenderer component already has proper DOMPurify sanitization - use the same approach in the MCP route before storing. 2. CSRF Vulnerability in One-Click Email Actions - approval-tool/app/[locale]/approve/[reviewId]/[reviewerId]/page.tsx:76-82Clicking email link auto-submits decision with no confirmation - vulnerable to CSRF attacks via email client prefetching or malicious forwards. Fix: Require user to click a confirmation button instead of auto-submitting on page load. 3. Missing Slack Signature Verification Enforcement - approval-tool/app/api/slack/interactions/route.ts:66Logs warning but continues processing - allows unauthenticated requests. Fix: Return 401 when SLACK_SIGNING_SECRET is missing. 4. XSS in Email Templates - approval-tool/src/lib/email.ts:99User-provided title inserted directly into HTML without escaping. Fix: HTML-escape user content or use a templating library. High Priority Issues5. Insecure Environment Variable Handling - approval-tool/src/env.ts:14-40All security-critical variables are .optional() - app builds successfully without proper config, then fails at runtime. Fix: Make required variables required at build time. 6. Access Token Security - approval-tool/prisma/schema.prisma:276-277
Fix: Make accessExpires required with short lifetime (1-24 hours), enforce expiration in all queries. Good Security Practices✅ Slack Integration - Excellent signature verification with timing-safe comparison Additional ObservationsPerformance: Good use of database indexes, pagination implemented in tRPC Code Quality: Follows CLAUDE.md conventions well, good TypeScript type safety Breaking Changes: This is a greenfield app in approval-tool/ subdirectory Recommended Action PlanBefore Merging:
Follow-up PR: SummaryThis is a professional, well-structured application following modern Next.js 15, tRPC, and Prisma patterns. The Slack integration and content rendering show good security awareness. However, the XSS vulnerabilities and CSRF issues are blockers for production deployment. Recommendation: Request fixes for issues #1-6, then approve for merge. |
1 participant
No description provided.