feat(feedback): in‑app feedback with delegated SMTP OAuth; navbar icon order; seed + test scripts #7
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds an in-app feedback system with SMTP OAuth email functionality. Users can submit feedback through a modal in the navbar, which triggers automated email notifications to the team and an acknowledgement to the submitter.
- Implements Office 365 SMTP with OAuth2 using MSAL device code flow for delegated authentication
- Adds a feedback modal UI component accessible from a new bug report icon in the navbar
- Creates feedback API endpoint that validates input and sends emails asynchronously
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| server/utils/email.js | Core email utility using MSAL OAuth and nodemailer for feedback notifications and acknowledgements |
| server/routes/api.js | POST /api/feedback endpoint with input validation and fire-and-forget email sending |
| server/scripts/seed_smtp_oauth_device_login.js | Device code authentication script to seed OAuth token cache |
| server/scripts/test_feedback_email.js | Manual testing script for feedback email flow |
| client/src/components/FeedbackModal.jsx | React modal component for feedback submission form |
| client/src/components/FeedbackModal.css | Styling for feedback modal |
| client/src/components/Navbar.jsx | Adds feedback icon button and modal integration |
| client/src/components/Navbar.css | Styling for feedback icon button |
| package.json | Adds @azure/msal-node and nodemailer dependencies |
| .env.example | Documents new environment variables for SMTP OAuth and feedback emails |
| .gitignore | Excludes OAuth token cache file from version control |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
server/utils/email.js
Outdated
| // quick email regex | ||
| const emailRe = /[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,}/i; |
There was a problem hiding this comment.
The email regex is defined after it's used in sendFeedbackNotification (lines 118-120). JavaScript hoisting allows this to work, but it reduces code readability. Consider moving the regex definition before the functions that use it.
server/utils/email.js
Outdated
| if (contactInfo && emailRe.test(contactInfo)) { | ||
| const m = contactInfo.match(emailRe); | ||
| if (m) replyTo = m[0]; | ||
| } |
There was a problem hiding this comment.
The regex test is performed twice: once with .test() and again with .match(). This is inefficient. Use .match() directly and check if the result is truthy: const m = contactInfo?.match(emailRe); if (m) replyTo = m[0];
server/utils/email.js
Outdated
| if (contactInfo && emailRe.test(contactInfo)) { | ||
| const m = contactInfo.match(emailRe); | ||
| if (m) return m[0]; | ||
| } |
There was a problem hiding this comment.
The regex test is performed twice: once with .test() and again with .match(). This is inefficient. Use .match() directly and check if the result is truthy: const m = contactInfo?.match(emailRe); if (m) return m[0];
server/routes/api.js
Outdated
| const { category = 'feedback', message, contactInfo, pagePath } = req.body || {}; | ||
|
|
||
| if (!message || typeof message !== 'string' || message.trim().length < 10) { | ||
| return res.status(400).json({ error: 'Please include a short description so we can help (10+ characters).' }); |
There was a problem hiding this comment.
The error message says 'short description' which is confusing since the requirement is actually for a longer message (10+ characters). Consider changing to 'Please include a description with at least 10 characters so we can help.' for clarity.
| return res.status(400).json({ error: 'Please include a short description so we can help (10+ characters).' }); | |
| return res.status(400).json({ error: 'Please include a description with at least 10 characters so we can help.' }); |
client/src/components/Navbar.jsx
Outdated
| setPlayerReady, | ||
| resetRace | ||
| } = useRace(); | ||
| const { raceState, typingState, setPlayerReady, resetRace } = useRace(); |
There was a problem hiding this comment.
Unused variable raceState.
| const { raceState, typingState, setPlayerReady, resetRace } = useRace(); | |
| const { typingState, setPlayerReady, resetRace } = useRace(); |
client/src/components/Navbar.jsx
Outdated
| setPlayerReady, | ||
| resetRace | ||
| } = useRace(); | ||
| const { raceState, typingState, setPlayerReady, resetRace } = useRace(); |
There was a problem hiding this comment.
Unused variable typingState.
| const { raceState, typingState, setPlayerReady, resetRace } = useRace(); | |
| const { raceState, setPlayerReady, resetRace } = useRace(); |
client/src/components/Navbar.jsx
Outdated
| setPlayerReady, | ||
| resetRace | ||
| } = useRace(); | ||
| const { raceState, typingState, setPlayerReady, resetRace } = useRace(); |
There was a problem hiding this comment.
Unused variable setPlayerReady.
| const { raceState, typingState, setPlayerReady, resetRace } = useRace(); | |
| const { raceState, typingState, resetRace } = useRace(); |
| }); | ||
|
|
||
| (async () => { | ||
| const result = await pca.acquireTokenByDeviceCode({ |
There was a problem hiding this comment.
Unused variable result.
![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.
ℹ️ 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".
| router.post('/feedback', async (req, res) => { | ||
| try { | ||
| const { category = 'feedback', message, contactInfo, pagePath } = req.body || {}; | ||
|
|
||
| if (!message || typeof message !== 'string' || message.trim().length < 10) { | ||
| return res.status(400).json({ error: 'Please include a short description so we can help (10+ characters).' }); | ||
| } | ||
|
|
||
| const sanitizedMessage = message.trim().slice(0, 4000); | ||
| const sanitizedContact = typeof contactInfo === 'string' ? contactInfo.trim().slice(0, 255) : null; | ||
| const sanitizedPagePath = typeof pagePath === 'string' ? pagePath.trim().slice(0, 255) : null; | ||
|
|
||
| let netid = null; | ||
| if (isAuthenticated(req)) { | ||
| if (!req.user && req.session?.userInfo) { | ||
| req.user = { | ||
| id: req.session.userInfo.userId, | ||
| netid: req.session.userInfo.user | ||
| }; | ||
| } | ||
| netid = req.user?.netid || req.session?.userInfo?.user || null; | ||
| } | ||
|
|
||
| const userAgent = req.get('user-agent')?.slice(0, 500) || null; | ||
|
|
||
| // fire and forget to avoid hanging if mail provider is slow | ||
| sendFeedbackEmails({ | ||
| category, | ||
| message: sanitizedMessage, | ||
| contactInfo: sanitizedContact, |
There was a problem hiding this comment.
The new /api/feedback route accepts arbitrary contactInfo from unauthenticated clients and immediately calls sendFeedbackEmails with that value, which sends an acknowledgement email to the derived address (sendFeedbackAcknowledgement in server/utils/email.js). Because there is no authentication, rate limiting, or verification that the email belongs to the submitter, this endpoint can be scripted to send TigerType-branded emails with attacker-controlled content to any address. That effectively turns the feedback API into an open mail relay and risks the SMTP account being abused or blocked. The handler should require a logged-in user, enforce CAPTCHA/rate limits, or suppress acknowledgements for untrusted addresses.
Useful? React with 👍 / 👎.
…lishing feedback sent modal
|
@codex re-review |
Ammaar-Alam
left a comment
Ammaar-Alam
left a comment
There was a problem hiding this comment.
secured api endpoint
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
server/utils/email.js
Outdated
| // Validate required email configuration | ||
| if (!process.env.FEEDBACK_EMAIL_FROM) { | ||
| throw new Error('Missing required env var: FEEDBACK_EMAIL_FROM'); | ||
| } | ||
| if (!process.env.FEEDBACK_EMAIL_TO_TEAM) { | ||
| throw new Error('Missing required env var: FEEDBACK_EMAIL_TO_TEAM'); |
There was a problem hiding this comment.
The new feedback email helper throws during module load if FEEDBACK_EMAIL_FROM or FEEDBACK_EMAIL_TO_TEAM are not defined. Because server/routes/api.js imports this module at the top level, the entire Express app will fail to boot in any environment where those secrets are not configured (including local development and tests) even if the /api/feedback route is never hit. Consider deferring validation until the first send attempt or providing a no‑op implementation when the variables are absent so that the rest of the server can still run.
Useful? React with 👍 / 👎.
|
@codex re-review one more time pls 🥺 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
server/utils/email.js
Outdated
| const pca = new PublicClientApplication({ | ||
| auth: { | ||
| clientId, | ||
| authority: `https://login.microsoftonline.com/${tenantId}` | ||
| }, | ||
| cache: { cachePlugin: tokenCachePlugin } | ||
| }); |
There was a problem hiding this comment.
The helper initializes new PublicClientApplication at module load using AZURE_CLIENT_ID and AZURE_TENANT_ID. If those variables are unset (the common case for local dev or CI that hasn’t seeded OAuth credentials), @azure/msal-node throws a ClientConfigurationError immediately and the server process fails to start, despite the later checks in sendMailGeneric that try to no-op when SMTP isn’t configured. This makes the entire backend depend on optional secrets. Instantiate the MSAL client only after verifying the env vars are present or wrap the constructor in a conditional so the app can boot without SMTP configured.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
server/routes/api.js
Outdated
| router.post('/feedback', async (req, res) => { | ||
| try { | ||
| const ip = (req.headers['x-forwarded-for'] || req.ip || req.connection?.remoteAddress || '').toString(); | ||
| if (isRateLimited(ip)) { |
There was a problem hiding this comment.
The new feedback endpoint uses (req.headers['x-forwarded-for'] || req.ip …) to identify the caller’s IP before applying the in-memory rate limit. Because the raw X-Forwarded-For header is checked before req.ip and there is no verification that the header comes from a trusted proxy, any client can send arbitrary header values to reset the rate limit on every request and spam the feedback API. This leaves the endpoint effectively unprotected against abuse. Consider relying on req.ip (which honors Express’s trust proxy setting) or explicitly parsing X-Forwarded-For only when the proxy is known to be trusted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (isRateLimited(ip)) { | ||
| return res.status(429).json({ error: 'Too many feedback submissions. Please try again later.' }); | ||
| } | ||
| const { category = 'feedback', message, contactInfo, pagePath } = req.body || {}; |
There was a problem hiding this comment.
The category parameter is not validated against a whitelist of allowed values. While the frontend uses a dropdown with specific options ('feedback', 'bug', 'idea', 'other'), a malicious client could send arbitrary values via direct API calls.
Suggestion: Add validation to ensure category is one of the allowed values:
const ALLOWED_CATEGORIES = ['feedback', 'bug', 'idea', 'other'];
const category = ALLOWED_CATEGORIES.includes(req.body.category) ? req.body.category : 'feedback';| const { category = 'feedback', message, contactInfo, pagePath } = req.body || {}; | |
| const ALLOWED_CATEGORIES = ['feedback', 'bug', 'idea', 'other']; | |
| const { category: rawCategory = 'feedback', message, contactInfo, pagePath } = req.body || {}; | |
| const category = ALLOWED_CATEGORIES.includes(rawCategory) ? rawCategory : 'feedback'; |
| const html = ` | ||
| <div style="font-family:-apple-system,BlinkMacSystemFont,Segoe UI,Roboto,Helvetica,Arial,sans-serif;max-width:720px;margin:0 auto;padding:32px 24px;color:#333;background:#ffffff;"> | ||
| <div style=\"margin:0 0 32px 0;text-align:left;\"> | ||
| <a href=\"${siteUrl}\" target=\"_blank\" rel=\"noopener noreferrer\" style=\"display:inline-block;\"> |
There was a problem hiding this comment.
The siteUrl variable should be escaped when used in HTML to prevent potential XSS vulnerabilities. Even though this comes from environment variables, it's a best practice to escape all dynamic content in HTML.
Suggestion: Change ${siteUrl} to ${escapeHtml(siteUrl)} on this line and line 205.
| }) => { | ||
| const fromAddr = from || process.env.FEEDBACK_EMAIL_FROM; | ||
|
|
||
| const subject = `[TigerType Feedback] ${category.toUpperCase()} from ${netid || 'anonymous user'}`; |
There was a problem hiding this comment.
The email subject line uses unsanitized user input (category and netid) which could potentially be exploited for email header injection attacks. While these values are somewhat controlled (category from a dropdown and netid from authentication), they should be sanitized to prevent newline characters and other potentially dangerous characters.
Suggestion: Sanitize these values or at minimum remove newline characters before using in the subject line. For example:
const subject = `[TigerType Feedback] ${category.replace(/[\r\n]/g, '').toUpperCase()} from ${(netid || 'anonymous user').replace(/[\r\n]/g, '')}`;| const subject = `[TigerType Feedback] ${category.toUpperCase()} from ${netid || 'anonymous user'}`; | |
| const subject = `[TigerType Feedback] ${category.replace(/[\r\n]/g, '').toUpperCase()} from ${(netid || 'anonymous user').replace(/[\r\n]/g, '')}`; |
server/routes/api.js
Outdated
| // simple in-memory rate limiting for feedback (per IP) | ||
| const FEEDBACK_LIMIT_WINDOW_MS = 10 * 60 * 1000; // 10 minutes | ||
| const FEEDBACK_LIMIT_COUNT = 5; // max 5 submissions per window | ||
| const feedbackRate = new Map(); // ip -> { windowStart, count } | ||
|
|
||
| function isRateLimited(ip) { | ||
| const now = Date.now(); | ||
| const entry = feedbackRate.get(ip); | ||
| if (!entry || (now - entry.windowStart) > FEEDBACK_LIMIT_WINDOW_MS) { | ||
| feedbackRate.set(ip, { windowStart: now, count: 1 }); | ||
| return false; | ||
| } | ||
| entry.count += 1; | ||
| if (entry.count > FEEDBACK_LIMIT_COUNT) return true; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The in-memory rate limiting will not work correctly in multi-instance deployments (e.g., when horizontally scaled on Heroku). Each instance will maintain its own rate limit map, allowing users to bypass the rate limit by hitting different instances. Consider using a shared store like Redis for rate limiting, or accept that this is a best-effort rate limit for single-instance deployments.
Additionally, the Map will grow indefinitely as IPs are added and never cleaned up, which could lead to a memory leak over time. Consider implementing periodic cleanup of old entries or using a package like express-rate-limit which handles this properly.
| const text = [ | ||
| 'Thanks for your feedback!', | ||
| '', | ||
| `We received your ${category || 'feedback'} and will look into it shortly.`, | ||
| '', | ||
| 'Summary:', | ||
| pagePath ? `• Page: ${pagePath}` : null, | ||
| contactInfo ? `• Contact: ${contactInfo}` : null, | ||
| `• Submitted: ${submittedLocal} ET`, | ||
| '', | ||
| safeMessage ? 'Your message:' : null, | ||
| safeMessage || null, | ||
| '', | ||
| '— TigerType Team' | ||
| ].filter(Boolean).join('\n'); |
There was a problem hiding this comment.
[nitpick] The submittedLocal variable is used in the plain text email body without escaping. While this is plain text and not HTML, category, pagePath, and contactInfo could potentially contain control characters or formatting that might cause issues in email clients.
For consistency and safety, consider sanitizing these values to remove control characters, especially newlines that could break the email formatting.
| AZURE_TENANT_ID= | ||
| AZURE_CLIENT_ID= | ||
| SMTP_SENDER=[email protected] | ||
| SMTP_OAUTH_CACHE= (optional; JSON blob of MSAL cache for headless servers) |
There was a problem hiding this comment.
The SMTP_OAUTH_CACHE environment variable has a comment in the value portion which is not valid .env syntax. Comments should be on their own line or after the value with a # separator.
Suggestion: Change to:
# Optional: JSON blob of MSAL cache for headless servers
SMTP_OAUTH_CACHE=
| SMTP_OAUTH_CACHE= (optional; JSON blob of MSAL cache for headless servers) | |
| # Optional: JSON blob of MSAL cache for headless servers | |
| SMTP_OAUTH_CACHE= |
server/routes/api.js
Outdated
|
|
||
| router.post('/feedback', async (req, res) => { | ||
| try { | ||
| const ip = (req.headers['x-forwarded-for'] || req.ip || req.connection?.remoteAddress || '').toString(); |
There was a problem hiding this comment.
The IP address extraction could result in an empty string when behind certain proxies, which would cause all anonymous requests to share the same rate limit entry. The x-forwarded-for header can also contain multiple IPs (comma-separated), and only the first (client) IP should be used for rate limiting.
Suggestion: Parse the x-forwarded-for header properly and provide a fallback:
const forwardedFor = req.headers['x-forwarded-for'];
const ip = forwardedFor ? forwardedFor.split(',')[0].trim() : (req.ip || req.connection?.remoteAddress || 'unknown');| const ip = (req.headers['x-forwarded-for'] || req.ip || req.connection?.remoteAddress || '').toString(); | |
| const forwardedFor = req.headers['x-forwarded-for']; | |
| const ip = forwardedFor | |
| ? forwardedFor.split(',')[0].trim() | |
| : (req.ip || req.connection?.remoteAddress || 'unknown'); |
| console.error(' node server/scripts/test_feedback_email.js [email protected] bug'); | ||
| console.error(' node server/scripts/test_feedback_email.js [email protected] feature "I would like dark mode"'); | ||
| console.error(''); | ||
| console.error('Available categories: bug, feature, feedback, question, test'); |
There was a problem hiding this comment.
The available categories listed in the usage documentation don't match the actual categories defined in the FeedbackModal component. The script lists "bug, feature, feedback, question, test" but the modal uses "feedback, bug, idea, other".
Suggestion: Update the documentation to match:
console.error('Available categories: feedback, bug, idea, other');| console.error('Available categories: bug, feature, feedback, question, test'); | |
| console.error('Available categories: feedback, bug, idea, other'); |
| const html = ` | ||
| <div style=\"font-family:-apple-system,BlinkMacSystemFont,Segoe UI,Roboto,Helvetica,Arial,sans-serif;max-width:640px;margin:0 auto;padding:32px 24px;color:#333;background:#ffffff;\"> | ||
| <div style=\"margin:0 0 32px 0;text-align:left;\"> | ||
| <a href=\"${siteUrl}\" target=\"_blank\" rel=\"noopener noreferrer\" style=\"display:inline-block;\"> |
There was a problem hiding this comment.
The siteUrl variable should be escaped when used in HTML to prevent potential XSS vulnerabilities. Even though this comes from environment variables, it's a best practice to escape all dynamic content in HTML.
Suggestion: Change ${siteUrl} to ${escapeHtml(siteUrl)}.
| <a href=\"${siteUrl}\" target=\"_blank\" rel=\"noopener noreferrer\" style=\"display:inline-block;\"> | |
| <a href=\"${escapeAttr(siteUrl)}\" target=\"_blank\" rel=\"noopener noreferrer\" style=\"display:inline-block;\"> |
| value={contactInfo} | ||
| onChange={(event) => setContactInfo(event.target.value)} | ||
| disabled={submitting} | ||
| placeholder="we'll follow up here if we need more info" |
There was a problem hiding this comment.
The placeholder text "we'll follow up here if we need more info" should start with a capital letter for consistency with other UI text in the application.
Suggestion: Change to "We'll follow up here if we need more info"
| placeholder="we'll follow up here if we need more info" | |
| placeholder="We'll follow up here if we need more info" |
|
@codex re-re-review last time promise 🙏🏼 |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
consolidating ports into one npm command, fixing conflicting ports, fixing race results screen not showing, fixing cas login button redirect, fixing backend serving static files
Summary
TLDR: Feedback button added in the navbar, feel free to report bugs, give suggestions, talk, etc.
Implementation Notes
and returns { success: true } immediately.
same JSON so it can be set as SMTP_OAUTH_CACHE in Heroku.