fix(#949): defend mock interview role field against prompt injection via enum allowlist and escaping#1000
Conversation
…interview role field
|
@Srejoye is attempting to deploy a commit to the durdana3105's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesPrompt Injection Hardening for Mock Interview Role
CI Cache and Environment Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🤖 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/controllers/aiController.js`:
- Around line 21-28: The escapeForPrompt function currently escapes backticks by
prefixing them with a backslash, but the test expects backticks to be completely
removed from the output. Modify the backtick replacement in the escapeForPrompt
function (the line with .replace(/`/g, ...)) to remove backticks entirely by
replacing them with an empty string instead of escaping them with a backslash.
In `@backend/tests/mockInterview.test.js`:
- Around line 10-13: The "all allowed roles are accepted" test suite stubs the
OPENROUTER_API_KEY environment variable in a beforeAll hook, but the global
afterEach hook clears all stubbed environment variables after each test. Since
the suite uses a for loop to create multiple parametrized tests, only the first
test will have the API key available while subsequent tests will run without it.
Change the beforeAll to beforeEach in the "all allowed roles are accepted" suite
so that the OPENROUTER_API_KEY is stubbed fresh before each parametrized test
runs, ensuring all tests have access to the environment variable.
- Around line 200-203: The test assertions at lines 201-202 in
mockInterview.test.js are checking the entire systemMsg.content for absence of
backticks and newlines, but this includes the multiline system prompt template
which has intentional formatting. Instead of asserting against the full content,
extract and assert only the injected role portion that was processed by the
escapeForPrompt() function. This way the test validates that injection payloads
are properly escaped without being falsely triggered by the prompt template's
static newlines.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2964c71a-2a9c-472f-98d9-f0a48bcaf2c6
📒 Files selected for processing (3)
backend/controllers/aiController.jsbackend/tests/mockInterview.test.jsbackend/validation/schemas.js
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/tests/mockInterview.test.js (2)
114-117: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRedundant API key stubbing due to top-level beforeEach.
The
beforeAllhook at line 115 stubsOPENROUTER_API_KEY, but the top-levelbeforeEachat line 11 already stubs the same environment variable before each test. This makes thebeforeAllredundant and the test setup confusing.Once you resolve the top-level describe block structure issue (lines 10-13), decide on a consistent strategy:
- If you remove the top-level describe/beforeEach, keep this
beforeAlland add similar hooks to the other describe blocks that need the API key.- If you keep the top-level beforeEach, remove this
beforeAllas it's redundant.🤖 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/tests/mockInterview.test.js` around lines 114 - 117, The beforeAll hook at lines 115-117 in the "POST /mock-interview/chat — all allowed roles are accepted" describe block redundantly stubs the OPENROUTER_API_KEY environment variable, which is already handled by the top-level beforeEach hook at line 11. Remove this redundant beforeAll hook from the describe block to maintain a single consistent API key stubbing strategy at the top level. If there are other describe blocks with similar redundant API key stubbing in beforeAll hooks, remove those as well to keep the test setup clear and non-repetitive.
10-13:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical syntax error: missing closing brace for top-level describe block.
The describe block opened on line 10 is never closed. The file ends at line 209 with a closing brace that closes the "role escaping in system prompt" describe block, but there's no subsequent closing brace for the top-level describe block. This causes the ESLint parsing error reported in the pipeline.
Additionally, this top-level describe block has the same name as the nested describe block at line 114, which is confusing and makes the test structure unclear.
🔧 Recommended fix
Option 1 (Recommended): Remove the redundant top-level describe block
Since the
beforeEachon line 11 is redundant with test-specific setup in nested describe blocks, remove the top-level describe wrapper entirely:-describe("POST /mock-interview/chat — all allowed roles are accepted", () => { - beforeEach(() => { - vi.stubEnv("OPENROUTER_API_KEY", "test-key-949"); - }); - // ── Shared app fixture ─────────────────────────────────────────────────────────────Then add a closing brace after line 209:
expect(sanitisedRole).not.toContain("\n"); }); }); -});Option 2: Rename and properly close the top-level describe block
If you want to keep the top-level wrapper, rename it to avoid duplication and add the closing brace:
-describe("POST /mock-interview/chat — all allowed roles are accepted", () => { +describe("Mock Interview Endpoint Tests", () => { beforeEach(() => { vi.stubEnv("OPENROUTER_API_KEY", "test-key-949"); });And after line 209:
expect(sanitisedRole).not.toContain("\n"); }); }); +});🤖 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/tests/mockInterview.test.js` around lines 10 - 13, The top-level describe block opened at line 10 is never closed, causing a parsing error. The file contains a redundant beforeEach hook in this top-level describe that duplicates setup already handled in nested describe blocks. Remove the entire top-level describe block wrapper (starting at line 10 with "POST /mock-interview/chat — all allowed roles are accepted") along with its beforeEach hook, and add a closing brace after line 209 to properly close any remaining test structure. This removes the duplicate describe block name and resolves the syntax error.
🧹 Nitpick comments (1)
backend/tests/mockInterview.test.js (1)
3-3: 💤 Low valueRemove unused
afterEachimport.The
afterEachfunction is imported but never used in this file. While this doesn't affect functionality, removing it improves code clarity.♻️ Cleanup
-import { vi, describe, it, expect, afterEach, beforeAll } from "vitest"; +import { vi, describe, it, expect, beforeAll, beforeEach } from "vitest";🤖 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/tests/mockInterview.test.js` at line 3, Remove the unused `afterEach` import from the vitest import statement at the top of the file. The import currently includes `afterEach` in its destructured list, but this function is never called anywhere in the test file, so it should be removed from the import to clean up the code and improve clarity.
🤖 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.
Outside diff comments:
In `@backend/tests/mockInterview.test.js`:
- Around line 114-117: The beforeAll hook at lines 115-117 in the "POST
/mock-interview/chat — all allowed roles are accepted" describe block
redundantly stubs the OPENROUTER_API_KEY environment variable, which is already
handled by the top-level beforeEach hook at line 11. Remove this redundant
beforeAll hook from the describe block to maintain a single consistent API key
stubbing strategy at the top level. If there are other describe blocks with
similar redundant API key stubbing in beforeAll hooks, remove those as well to
keep the test setup clear and non-repetitive.
- Around line 10-13: The top-level describe block opened at line 10 is never
closed, causing a parsing error. The file contains a redundant beforeEach hook
in this top-level describe that duplicates setup already handled in nested
describe blocks. Remove the entire top-level describe block wrapper (starting at
line 10 with "POST /mock-interview/chat — all allowed roles are accepted") along
with its beforeEach hook, and add a closing brace after line 209 to properly
close any remaining test structure. This removes the duplicate describe block
name and resolves the syntax error.
---
Nitpick comments:
In `@backend/tests/mockInterview.test.js`:
- Line 3: Remove the unused `afterEach` import from the vitest import statement
at the top of the file. The import currently includes `afterEach` in its
destructured list, but this function is never called anywhere in the test file,
so it should be removed from the import to clean up the code and improve
clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8472f7fa-8b22-44a5-9724-79ac0cdd7239
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
backend/controllers/aiController.jsbackend/tests/mockInterview.test.js
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
10-34: ⚡ Quick winAdd explicit minimal permissions to the workflow.
The workflow uses default permissions, which are overly broad. Since this job only performs read-only operations (checkout, install, lint, test), it should explicitly declare minimal permissions following the principle of least privilege.
🔒 Proposed fix to add explicit permissions
jobs: test: runs-on: ubuntu-latest + permissions: + contents: read steps: - name: Checkout RepositoryAlternatively, if no permissions are needed beyond the implicit checkout access, you can use an empty block:
jobs: test: runs-on: ubuntu-latest + permissions: {} steps: - name: Checkout RepositoryAs per coding guidelines, the static analysis tool zizmor flagged: "overly broad permissions (excessive-permissions): default permissions used due to no permissions: block".
🤖 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 @.github/workflows/ci.yml around lines 10 - 34, The test job in the GitHub Actions workflow uses default permissions which are overly broad. Add an explicit permissions block at the job level for the test job that declares only the minimal permissions needed. Since this job only performs read-only operations (checkout, install, lint, test), add permissions with contents set to read, or use an empty permissions block if no special permissions are required beyond the implicit checkout access. This follows the principle of least privilege and will resolve the zizmor static analysis warning about excessive-permissions.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 10-34: The test job in the GitHub Actions workflow uses default
permissions which are overly broad. Add an explicit permissions block at the job
level for the test job that declares only the minimal permissions needed. Since
this job only performs read-only operations (checkout, install, lint, test), add
permissions with contents set to read, or use an empty permissions block if no
special permissions are required beyond the implicit checkout access. This
follows the principle of least privilege and will resolve the zizmor static
analysis warning about excessive-permissions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d886d84-9cd0-46df-bad5-9639e90ea3ec
📒 Files selected for processing (1)
.github/workflows/ci.yml
|
@durdana3105 These 6 CI failures ( All 75 passing tests continue to pass. My changes introduce no new failures. |
|
@durdana3105 You could just check and merge the PR! Thanks. |
1 participant
Closes #949
Summary
conductMockInterviewinterpolatedreq.body.roleverbatim into the OpenRouter system prompt. The only validation wasz.string().max(200), which permitted backticks, newlines, and instruction-override sequences. An authenticated user could redirect the model's behaviour and exfiltrate prompting strategy — all at the platform's API cost.This PR applies two independent layers of defence.
Defence layers
Layer 1 — Schema allowlist (primary gate,
backend/validation/schemas.js)A new exported constant
ALLOWED_INTERVIEW_ROLESenumerates 15 canonical job titles. ThemockInterviewChatZod schema now applies:Any request with a role outside this set, or containing backticks, newlines, angle brackets,
${}, or other injection characters, isrejected at the validation middleware with HTTP 400 before the controller is reached.
Layer 2 — Controller escaping (defence-in-depth,
backend/controllers/aiController.js)A new
escapeForPrompt()helper escapes`,\,$,",\n, and\rbefore the role is interpolated into the system message. This ensures that even if the schema allowlist is inadvertently widened in a future change, raw user input will not reach the LLM verbatim.Changes
backend/validation/schemas.jsALLOWED_INTERVIEW_ROLES; tightenmockInterviewChat.rolewith regex + refinebackend/controllers/aiController.jsescapeForPrompt(); apply it inconductMockInterviewbackend/tests/mockInterview.test.jsTest coverage
Schema rejection (400): backtick injection · newline injection · angle brackets · whitespace-only · free-form unlisted role · role > 100 chars · empty messages · message > 2000 chars
Schema acceptance (200): parametric loop over all 15
ALLOWED_INTERVIEW_ROLESController unit tests: correct role appears in system prompt · backtick and newline absent from prompt even when schema is bypassed
Reproduction check
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features
Tests
Chores