Add RAG sensitive data exposure - #19
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughA new four-level RAG Sensitive Data Exposure lab adds FAISS/SQLite retrieval, sensitivity scanning, secret validation, FastAPI endpoints, document corpora, an interactive frontend, tests, and local Compose support. ChangesRAG Sensitive Data Exposure Lab
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant Controller
participant Lab as RAG Lab
participant Store as FAISS SQLite Store
participant Embedder
participant LLM
Browser->>Controller: POST generate request
Controller->>Lab: evaluate level and input
Lab->>Store: ensure index and retrieve matches
Store->>Embedder: embed documents and query
Embedder->>Store: return vectors
Store->>Lab: return filtered documents
Lab->>LLM: send prompt with retrieved context
LLM->>Lab: return assistant output
Lab->>Controller: return evaluation response
Controller->>Browser: return output and retrieved documents
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
luks-santos
changed the title
Lexical denylist (password, secret, admin) bypassed by semantic paraphrase retrieval. - Add L2 corpus and denylist input handler - Add level2 endpoint and locale entries - Make the facade level-aware
…tive-data-exposure
d3b5626 to
5ef3d01
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@src/controllers/rag_data_exposure_controller.py`:
- Around line 47-53: The controller currently treats any non-"validate" action
as "generate"; add an explicit allowlist check for action values ("validate" or
"generate") and reject others with an error response. In the handler around the
action variable, validate that action is one of {"validate","generate"} and
return a 4xx error (or raise the appropriate HTTPException) for invalid values;
keep existing calls to validate_rag_data_exposure_secret when action ==
"validate" and to evaluate_rag_data_exposure_level when action == "generate"
(use the same user_input/model extraction). Ensure you reference the action
variable, validate_rag_data_exposure_secret, and
evaluate_rag_data_exposure_level when implementing this guard so bad clients
fail fast instead of following the wrong path.
- Around line 42-44: In the _handle_level method move the JSON parsing (await
request.json()) inside the existing try/except so malformed JSON can't bubble
out; specifically, call await request.json() within the try block at the start
of _handle_level, validate/normalize action = str(data.get("action",
"generate")).strip().lower() there, and in the except handle
JSONDecodeError/ValueError by returning a controlled error dict (and/or
appropriate status payload) instead of letting an unhandled 500 occur.
In `@src/framework/registry.py`:
- Around line 99-100: The branch comparing controller_name uses a hyphenated
string ("rag-sensitive-data-exposure") but other dispatches expect underscored
names (e.g., "rag_sensitive_data_exposure"), causing mismatches; update the
dispatch logic in the function that handles controller_name (the branch that
calls validate_rag_data_exposure_secret) to normalize controller_name (e.g.,
controller_name = controller_name.replace('-', '_') or compare against both
forms) before the if checks so the compare will match and
validate_rag_data_exposure_secret(level_number, candidate_secret) is reached for
registrations named rag_sensitive_data_exposure.
In `@src/service/vulnerabilities/rag_data_exposure_lab.py`:
- Around line 195-231: The current flow appends vectors to FAISS before
inserting rows into SQLite which can cause FAISS/DB divergence on
unique-constraint failures; change the order so you build the DB rows and
perform the INSERT (using self._connect() and handling
sqlite3.IntegrityError/unique-constraint races by skipping or deduping duplicate
rows) before mutating FAISS, then compute start_vector_id = int(index.ntotal),
call faiss.normalize_L2(matrix) and index.add(matrix); apply the same
swap-and-tolerate-duplicate-insert logic to the other identical block referenced
around lines 461-471 (use symbols: index.add, faiss.normalize_L2,
start_vector_id, self._connect, rows, documents).
In `@src/static/facade/rag_data_exposure_template.html`:
- Around line 11-21: The textarea (id="ragExposurePrompt") and the secret input
lack programmatic labels; add accessible labels by either inserting <label
for="ragExposurePrompt">Prompt</label> tied to the textarea and a corresponding
<label for="..."> for the secret input, or by adding clear aria-label attributes
to those elements, and if needed use a visually-hidden utility class to keep
visual layout unchanged; ensure the labels reference the exact element ids used
in this template so screen readers can announce the fields.
In `@src/static/facade/rag_data_exposure_template.js`:
- Around line 72-78: The UI currently accepts any integer level >=1 (from the
level parsing code) but endpointForLevel only implements level1..level3, causing
404s for higher levels; clamp the parsed level to the supported range (1..3) or
clamp inside endpointForLevel so that any incoming level is reduced to
Math.min(Math.max(level, 1), 3) before building the URL. Update the level
parsing function or endpointForLevel (referencing endpointForLevel and the
level-parsing code that sets const level = Number(match[1])) to enforce this
clamp so requests always target /level1, /level2, or /level3.
🪄 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: 14462f52-df10-4037-8223-dc7e7b71afe4
📒 Files selected for processing (13)
locale/messages_us.propertiessrc/app.pysrc/controllers/rag_data_exposure_controller.pysrc/framework/decorators.pysrc/framework/registry.pysrc/service/vulnerabilities/__init__.pysrc/service/vulnerabilities/docs/RAG_DATA_EXPOSURE/LEVEL1/documents.jsonsrc/service/vulnerabilities/docs/RAG_DATA_EXPOSURE/LEVEL2/documents.jsonsrc/service/vulnerabilities/docs/RAG_DATA_EXPOSURE/LEVEL3/documents.jsonsrc/service/vulnerabilities/rag_data_exposure_lab.pysrc/static/facade/rag_data_exposure_template.csssrc/static/facade/rag_data_exposure_template.htmlsrc/static/facade/rag_data_exposure_template.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/controllers/rag_data_exposure_controller.py`:
- Around line 44-47: The handler assumes request.json() returns a dict and calls
data.get(), which will raise if JSON root is a list/string; update the code
around data = await request.json() to validate that data is a dict (e.g.,
isinstance(data, dict)) before using data.get("action", ...), and if it isn't,
return a structured error (e.g., {"error": "Invalid JSON root type: expected
object"}) so the existing action handling (action variable and the action not in
{"generate","validate"} check) only runs on valid input.
🪄 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: 10128d38-5302-4a02-9134-8cdfeb79cb2d
📒 Files selected for processing (5)
src/controllers/rag_data_exposure_controller.pysrc/framework/registry.pysrc/service/vulnerabilities/rag_data_exposure_lab.pysrc/static/facade/rag_data_exposure_template.htmlsrc/static/facade/rag_data_exposure_template.js
🚧 Files skipped from review as they are similar to previous changes (4)
- src/static/facade/rag_data_exposure_template.html
- src/framework/registry.py
- src/static/facade/rag_data_exposure_template.js
- src/service/vulnerabilities/rag_data_exposure_lab.py
|
I tried to run this PR and following are my observations which I think should be corrected:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
preetkaran20
left a comment
preetkaran20
left a comment
There was a problem hiding this comment.
Thank you for the PR. I really liked the idea. Added a few comments.
|
@luks-santos pinging again in case notification was missed. |
|
|
@preetkaran20 Thanks for the detailed feedback! These are great suggestions. I'll implement the suggested changes |
|
I'll try to work on this over the weekend. Thanks! |
|
Thanks for the detailed review, it was really helpful. I'm actively working through all of the feedback, including restructuring the Acme Corp narrative and the other suggested improvements. I'll push the changes soon for another review. Thanks again! |
…ts, progressive hints, hardened L4 Addresses review feedback on the RAG Sensitive Data Exposure lab: - Replace guessable rag_lN_secret values with opaque, non-descriptive secrets - Rewrite L1-L3 corpora around a consistent Acme Corp narrative (no security controls -> keyword denylist -> mistagged document sensitivity) - Remove one-click auto-solve: blank input no longer runs the winning prompt, and the facade now offers progressive hints instead of a pre-filled payload - Add a hardened Level 4 that scans each chunk at ingest and reclassifies sensitivity automatically, demonstrating the correct chunk-level control versus the human-typed document tag that caused the L3 bypass - Add a pytest harness with unit coverage for secret handling, input validation, the ingest-time scanner, and endpoint registration Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ata exposure lab
Replace the custom hint/solution buttons with tiered @attack_vector
hints (narrative context -> nudge -> real payload) surfaced through
the existing VulnerabilityDefinitions mechanism, matching the pattern
used by prompt_injection and CachePoisoning. Also give each level a
story title ("Level 1 - The Break-Glass Leak") and a fixed Acme Corp
premise in the facade header.
The llmforge service pointed at ../llmforge as its build context, which only works if a sibling directory happens to be named that way; fix it to use the repo root (.) so a plain clone works. Also picks up the mailpit service and ENABLE_VULNERABLEAPP_* facade flags already present on origin/main, which this branch predates. Ignore the FAISS/SQLite data the RAG Sensitive Data Exposure lab generates at runtime so it never lands in git or the Docker build context.
Each level now shows a one-sentence, spoiler-free story beat in the header explaining what Acme changed at that point (denylist, sensitivity tags, ingest scanner) without needing to click through a hint. Since that context previously lived only in the first framework hint tier, remove it there to avoid repeating the same sentence twice: L1-L3 go from 3 hint tiers to 2 (nudge, then payload), L4 from 2 to 1 (verification guidance only).
|
Thanks for the detailed review! Here's what I changed to address the feedback:
Each level's UI now shows a chapter title and a short, always-visible summary, so the overall story is clear without needing to dig into the hints.
Let me know if you'd like any adjustments before another pass. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.dockerignore (1)
12-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSync ignored paths with
.gitignore.To ensure locally generated vector store databases aren't accidentally copied into the Docker image build context (which could cause stale container state or inflate image size), consider adding
data/rag_data_exposure/here so it matches your.gitignore.💡 Proposed fix
data/*.sqlite3 data/faiss.index data/faiss-docstore.json +data/rag_data_exposure/🤖 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 @.dockerignore around lines 12 - 14, Add data/rag_data_exposure/ to .dockerignore alongside the existing data exclusions so Docker build contexts match the repository’s ignored paths and omit locally generated vector-store data.src/service/vulnerabilities/rag_data_exposure_lab.py (1)
424-427: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDynamically list valid levels in the error message.
Hardcoding the level numbers in the error message will require manual updates if new levels are added. Consider using the keys of the
LEVELSdictionary to dynamically generate the message.♻️ Proposed refactor
def _challenge_for(level: int) -> RagDataExposureLevel: if level not in LEVELS: - raise ValueError("level must be 1, 2, 3 or 4") + valid_levels = ", ".join(str(k) for k in sorted(LEVELS.keys())) + raise ValueError(f"level must be one of: {valid_levels}") return LEVELS[level]🤖 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 `@src/service/vulnerabilities/rag_data_exposure_lab.py` around lines 424 - 427, Update _challenge_for so its ValueError message derives the valid level values from LEVELS.keys() instead of hardcoding “1, 2, 3 or 4”; preserve the existing validation and LEVELS lookup behavior.
🤖 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 `@src/service/vulnerabilities/rag_data_exposure_lab.py`:
- Around line 663-664: Update the secret comparison in the challenge validation
flow to UTF-8 encode both the stripped candidate value and
challenge.secret_token before passing them to hmac.compare_digest. Preserve
false-result behavior for invalid or non-ASCII input instead of allowing a
TypeError to escape.
---
Nitpick comments:
In @.dockerignore:
- Around line 12-14: Add data/rag_data_exposure/ to .dockerignore alongside the
existing data exclusions so Docker build contexts match the repository’s ignored
paths and omit locally generated vector-store data.
In `@src/service/vulnerabilities/rag_data_exposure_lab.py`:
- Around line 424-427: Update _challenge_for so its ValueError message derives
the valid level values from LEVELS.keys() instead of hardcoding “1, 2, 3 or 4”;
preserve the existing validation and LEVELS lookup behavior.
🪄 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: ced83e06-76a2-4c25-9fd8-97126a8f7799
📒 Files selected for processing (18)
.dockerignore.gitignoredocker-compose.ymllocale/messages_us.propertiespytest.inirequirements-dev.txtsrc/controllers/rag_data_exposure_controller.pysrc/service/vulnerabilities/docs/RAG_DATA_EXPOSURE/LEVEL1/documents.jsonsrc/service/vulnerabilities/docs/RAG_DATA_EXPOSURE/LEVEL2/documents.jsonsrc/service/vulnerabilities/docs/RAG_DATA_EXPOSURE/LEVEL3/documents.jsonsrc/service/vulnerabilities/docs/RAG_DATA_EXPOSURE/LEVEL4/documents.jsonsrc/service/vulnerabilities/rag_data_exposure_lab.pysrc/static/facade/rag_data_exposure_template.csssrc/static/facade/rag_data_exposure_template.htmlsrc/static/facade/rag_data_exposure_template.jstests/conftest.pytests/test_rag_data_exposure_lab.pytests/test_rag_data_exposure_registry.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/service/vulnerabilities/docs/RAG_DATA_EXPOSURE/LEVEL2/documents.json
- src/static/facade/rag_data_exposure_template.css
- src/service/vulnerabilities/docs/RAG_DATA_EXPOSURE/LEVEL1/documents.json
| "was logged and never redacted. Document-level tags are too coarse - the bypass " | ||
| "is to ask about content that lives inside the misclassified low-sensitivity doc." | ||
| ), | ||
| metadata_filter=L3_METADATA_FILTER, |
There was a problem hiding this comment.
I think adding deny list to Level 3 and Level 4 is really important. We need defence in depth at all the layers so security of deny list at application layer is good for level 3 and level 4.
preetkaran20
left a comment
preetkaran20
left a comment
There was a problem hiding this comment.
@luks-santos this is a great great great contribution. just a small feedback.
- I think when something is marked as sensitive high, we should not return that document to the UI. That way we have made secure level as secure.
- Adding deny list to level 3 and level 4
|
@preetkaran20 Regarding the comments in Portuguese, they actually slipped past me while I was reviewing the code, haha. I've updated them to English now. As for the other points, the sensitivity-high filtering is actually already how L4 works today: once a chunk gets reclassified, it's excluded from the search results before it ever reaches the retrieved-documents panel, so it never shows up in the UI. The denylist suggestion for L3 and L4 is a good one though, I'll add that. I'm working on these improvements throughout the week and will push them soon. |
@luks-santos I think it is good to merge it now and have deny list added as a separate PR. |
2 participants
Overview
Adds a RAG Sensitive Data Exposure lab (OWASP LLM02) built around Acme Corp's internal wiki assistant. Four levels tell a chronological story: each one adds a defense, and the next shows why it isn't enough — until the hardened level fixes the root cause.
Levels
sensitivity: lowtag on the whole document misses a sensitive chunk buried inside it.Variant.SECURE, no secret to capture).How it works
RagDataExposureVectorStorecombines FAISS (per-namespace index) with SQLite (chunk text + metadata), using Ollama for embeddings and chat. The facade shows retrieved chunks, offers progressive hints instead of a pre-filled winning prompt, and validates the captured secret with a constant-time comparison. Secrets are fixed but opaque values, obtainable only through retrieval.Closes #8
Summary by CodeRabbit