fix: relax role sanitization instead of strict allow-list - #1645
fix: relax role sanitization instead of strict allow-list#1645ionfwsrijan wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughRole validation now accepts normalized free-form roles up to 100 characters. It removes the fixed allow-list and its public export while retaining ChangesRole validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/utils/prompts.js`:
- Around line 3-11: Update sanitizeRole and the
questionAnswerPrompt/interviewTipsPrompt call sites so the user-controlled role
is passed as structured data or a separate message rather than interpolated into
the model instruction text. Preserve the role as data, including delimiter-like
content such as >>> and embedded instructions, and ensure the existing JSON-only
response contract remains enforced; if the API only supports one string, use a
safe data encoding and add adversarial delimiter/instruction coverage.
- Around line 8-14: Update sanitizeRole so control characters that represent
whitespace are normalized to spaces rather than removed, preserving boundaries
such as the separator in “Data Analyst”; perform whitespace collapsing and a
final trim after all replacements, then keep the existing empty-role validation
based on the fully normalized result.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9da15c08-18b0-4ce3-a074-c78687a5fd94
📒 Files selected for processing (1)
backend/utils/prompts.js
| const sanitizeRole = (role) => { | ||
| if (!role || typeof role !== 'string') { | ||
| throw new Error('Role must be a non-empty string'); | ||
| } | ||
| const trimmedRole = role.trim().toLowerCase(); | ||
| if (!ALLOWED_ROLES.has(trimmedRole)) { | ||
| throw new Error(`Invalid role. Allowed roles: ${Array.from(ALLOWED_ROLES).join(', ')}`); | ||
| const trimmedRole = role | ||
| .trim() | ||
| .toLowerCase() | ||
| .replace(/[\u0000-\u001f\u007f]/g, '') | ||
| .replace(/\s+/g, ' '); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Keep free-form role text out of the model instruction channel.
Removing the allow-list makes the role an arbitrary user-controlled string. sanitizedRole is interpolated directly into questionAnswerPrompt at Line 36 and interviewTipsPrompt at Line 86. The <<<...>>> markers are not a trust boundary, and a role can contain >>> or plain instructions. This lets callers alter model behavior and violate the JSON-only response contract.
Pass the role through a separate structured data field or message. If the API only accepts one string, encode the value as data and add adversarial delimiter and instruction tests. Do not rely on lowercasing or control-character removal as a prompt-injection defense.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/utils/prompts.js` around lines 3 - 11, Update sanitizeRole and the
questionAnswerPrompt/interviewTipsPrompt call sites so the user-controlled role
is passed as structured data or a separate message rather than interpolated into
the model instruction text. Preserve the role as data, including delimiter-like
content such as >>> and embedded instructions, and ensure the existing JSON-only
response contract remains enforced; if the API only supports one string, use a
safe data encoding and add adversarial delimiter/instruction coverage.
| .trim() | ||
| .toLowerCase() | ||
| .replace(/[\u0000-\u001f\u007f]/g, '') | ||
| .replace(/\s+/g, ' '); | ||
|
|
||
| if (trimmedRole.length === 0) { | ||
| throw new Error('Role must be a non-empty string'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Trim after all replacements and preserve whitespace boundaries.
Line 8 trims before Line 10 removes controls. sanitizeRole('\u0000 \u0000') therefore returns ' ' and passes the empty check. sanitizeRole('Data\tAnalyst') returns dataanalyst because the tab is removed before \s+ runs. Map whitespace controls to spaces, collapse whitespace, and trim again after the replacements.
Suggested normalization
const trimmedRole = role
.trim()
.toLowerCase()
- .replace(/[\u0000-\u001f\u007f]/g, '')
- .replace(/\s+/g, ' ');
+ .replace(/[\u0000-\u001f\u007f]/g, (character) => /\s/.test(character) ? ' ' : '')
+ .replace(/\s+/g, ' ')
+ .trim();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .trim() | |
| .toLowerCase() | |
| .replace(/[\u0000-\u001f\u007f]/g, '') | |
| .replace(/\s+/g, ' '); | |
| if (trimmedRole.length === 0) { | |
| throw new Error('Role must be a non-empty string'); | |
| .trim() | |
| .toLowerCase() | |
| .replace(/[\u0000-\u001f\u007f]/g, (character) => /\s/.test(character) ? ' ' : '') | |
| .replace(/\s+/g, ' ') | |
| .trim(); | |
| if (trimmedRole.length === 0) { | |
| throw new Error('Role must be a non-empty string'); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/utils/prompts.js` around lines 8 - 14, Update sanitizeRole so control
characters that represent whitespace are normalized to spaces rather than
removed, preserving boundaries such as the separator in “Data Analyst”; perform
whitespace collapsing and a final trim after all replacements, then keep the
existing empty-role validation based on the fully normalized result.
1 participant
Problem
sanitizeRoleinbackend/utils/prompts.jsrejects any role not in a hard-coded 23-entryALLOWED_ROLESset, even though the Create Session form is a free-text input whose placeholder suggests roles like "UI/UX Designer" and "Frontend Developer". Users following the placeholder get400 { message: "Invalid role. Allowed roles: ..." }.Fix
Relax
sanitizeRoleto a length/sanitization check instead of an exact allow-list match (the third option suggested in the issue):The now-unused
ALLOWED_ROLESset and its export are removed.Files changed
backend/utils/prompts.jsTesting
sanitizeRole:"UI/UX Designer"→"ui/ux designer"(accepted)"Data Analyst"→"data analyst"(accepted)" Frontend Developer "→"frontend developer"(accepted)Role must be at most 100 charactersRole must be a non-empty stringnpx vitest run): 9 test files, 95 tests, all pass.Closes #1626
Looks good to me. Ready to merge.