-
Notifications
You must be signed in to change notification settings - Fork 802
fix(library): correct fail-open, payload-leak, and manifest defects in built-in rails #2257
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
7e35375
1919b54
bdb9703
24f044c
f99747c
fbc68e4
dd63ce0
39af202
bd694b8
f9d8094
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,8 +59,13 @@ def _injection_detection_outcome( | |
| action_option: str, | ||
| original_text: str, | ||
| ) -> RailOutcome: | ||
| metadata = dict(result) | ||
| metadata["action"] = action_option | ||
| # the checked text is deliberately excluded: outcome metadata reaches | ||
| # processing logs and tracing exporters | ||
| metadata = { | ||
| "is_injection": result["is_injection"], | ||
| "detections": result["detections"], | ||
| "action": action_option, | ||
| } | ||
|
Comment on lines
+62
to
+68
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n -C 3 'metadata.*text|_injection_detection_outcome|_regex_outcome' nemoguardrails tests --glob '*.py'Repository: NVIDIA-NeMo/Guardrails Length of output: 31707 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- repository guidance ---'
find nemoguardrails -name AGENTS.md -print -exec cat {} \;
printf '%s\n' '--- injection detection implementation and tests ---'
sed -n '1,125p' nemoguardrails/library/injection_detection/actions.py
sed -n '1,145p' tests/test_injection_detection.py
printf '%s\n' '--- regex implementation and related tests/docs ---'
sed -n '1,125p' nemoguardrails/library/regex/actions.py
rg -n -C 4 'regex_detection|_regex_outcome|is_match|detections.*source|source.*detections' nemoguardrails tests docs README.md --glob '*.py' --glob '*.md' 2>/dev/null || true
printf '%s\n' '--- exact metadata text consumers ---'
rg -n -C 3 'metadata\s*\[\s*["'\'']text["'\'']\s*\]|metadata\.get\(\s*["'\'']text["'\'']|["'\'']text["'\'']\s*:\s*.*metadata|metadata.*text' nemoguardrails tests --glob '*.py' || true
printf '%s\n' '--- current diff for the two implementations ---'
git diff -- nemoguardrails/library/injection_detection/actions.py nemoguardrails/library/regex/actions.pyRepository: NVIDIA-NeMo/Guardrails Length of output: 50378 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- injection action documentation ---'
rg -n -A35 -B5 'def (detect|check).*injection|InjectionDetectionResult|injection detection' \
nemoguardrails/library/injection_detection/actions.py
printf '%s\n' '--- regex tests and metadata assertions ---'
rg -n -C 5 'detect_regex_pattern|metadata|is_match|detections|source' \
tests/test_regex_detection.py tests/test_runtime_flow_gate_equivalence.py \
tests/recorded --glob '*.py' | head -n 260
printf '%s\n' '--- focused diff status ---'
git diff --stat -- nemoguardrails/library/injection_detection/actions.py nemoguardrails/library/regex/actions.py tests
git diff --unified=15 -- nemoguardrails/library/injection_detection/actions.py nemoguardrails/library/regex/actions.py
printf '%s\n' '--- AST-based metadata consumer and documentation scan ---'
python3 - <<'PY'
import ast
from pathlib import Path
roots = [Path("nemoguardrails"), Path("tests")]
for root in roots:
for path in root.rglob("*.py"):
try:
tree = ast.parse(path.read_text())
except Exception:
continue
for node in ast.walk(tree):
if isinstance(node, ast.FunctionDef) and node.name in {
"detect_regex_pattern",
"_injection_detection_outcome",
"_regex_outcome",
}:
print(f"{path}:{node.lineno} {node.name}")
print(" docstring:", repr(ast.get_docstring(node)))
if isinstance(node, ast.Subscript):
target = node.value
if isinstance(target, ast.Name) and target.id == "metadata":
sl = node.slice
if isinstance(sl, ast.Constant) and sl.value == "text":
print(f"{path}:{node.lineno} metadata['text'] read")
if isinstance(node, ast.Call):
for kw in node.keywords:
if kw.arg == "metadata" and isinstance(kw.value, ast.Dict):
keys = [
k.value for k in kw.value.keys
if isinstance(k, ast.Constant) and isinstance(k.value, str)
]
if "text" in keys:
print(f"{path}:{node.lineno} metadata literal keys={keys}")
PYRepository: NVIDIA-NeMo/Guardrails Length of output: 32083 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- public action docstrings ---'
sed -n '300,350p' nemoguardrails/library/injection_detection/actions.py
sed -n '42,66p' nemoguardrails/library/regex/actions.py
rg -n -F 'The original text that was checked' . --glob '*.py' --glob '*.md' || true
printf '%s\n' '--- equivalence helper and injection/regex cases ---'
sed -n '840,930p' tests/test_runtime_flow_gate_equivalence.py
sed -n '1140,1180p' tests/test_runtime_flow_gate_equivalence.py
sed -n '1428,1490p' tests/test_runtime_flow_gate_equivalence.py
rg -n -A35 -B10 'assert .*metadata|expected_outcome|actual_outcome|RailOutcome' \
tests/test_runtime_flow_gate_equivalence.py | tail -n 180
printf '%s\n' '--- metadata reads in runtime and tests ---'
python3 - <<'PY'
import ast
from pathlib import Path
for path in [*Path("nemoguardrails").rglob("*.py"), *Path("tests").rglob("*.py")]:
try:
tree = ast.parse(path.read_text())
except Exception:
continue
for node in ast.walk(tree):
if not isinstance(node, ast.Subscript):
continue
key = node.slice
is_text = isinstance(key, ast.Constant) and key.value == "text"
if not is_text:
continue
base = ast.unparse(node.value)
if "metadata" in base or "outcome" in base or "result" in base:
print(f"{path}:{node.lineno}: {base}['text']")
PYRepository: NVIDIA-NeMo/Guardrails Length of output: 16631 Update the Remove 📍 Affects 2 files
🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| if action_option == "reject" and result["is_injection"]: | ||
| return RailOutcome.block(metadata=metadata) | ||
| if result["text"] != original_text: | ||
|
|
@@ -193,7 +198,7 @@ def _load_rules( | |
| except yara.SyntaxError as e: | ||
| msg = f"Failed to initialize injection detection due to configuration or YARA rule error: YARA compilation failed: {e}" | ||
| log.error(msg) | ||
| return None | ||
| raise ValueError(msg) from e | ||
|
Comment on lines
198
to
+201
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n -C 3 'yara\.SyntaxError|_load_rules|injection_detection' nemoguardrails tests --glob '*.py'Repository: NVIDIA-NeMo/Guardrails Length of output: 49424 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- AGENTS.md files ---'
find nemoguardrails -name AGENTS.md -print -exec cat {} \;
printf '%s\n' '--- actions.py documentation and exception flow ---'
sed -n '70,215p' nemoguardrails/library/injection_detection/actions.py
sed -n '315,355p' nemoguardrails/library/injection_detection/actions.py
printf '%s\n' '--- exception handling and documentation references ---'
rg -n -C 4 'except (ValueError|yara\.SyntaxError)|raises:|Raises:|YARA compilation|_load_rules\(' nemoguardrails tests --glob '*.py'Repository: NVIDIA-NeMo/Guardrails Length of output: 50378 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- focused injection tests ---'
sed -n '700,770p' tests/test_injection_detection.py
printf '%s\n' '--- action-dispatch files ---'
fd -t f -i 'action.*dispatcher|dispatcher.*action' nemoguardrails
printf '%s\n' '--- injection action references outside its implementation ---'
rg -n 'library\.injection_detection\.actions|actions:injection_detection|_load_rules|yara\.SyntaxError' nemoguardrails tests --glob '*.py' \
| rg -v 'nemoguardrails/library/injection_detection/actions\.py'
printf '%s\n' '--- targeted exception handlers in the runtime ---'
rg -n -C 3 'execute_action|run_action|dispatch.*action|action.*exception|except Exception|except ValueError' nemoguardrails/actions nemoguardrails/colang nemoguardrails/rails --glob '*.py' \
| head -n 240
printf '%s\n' '--- AST-based documentation and raise verifier ---'
python3 - <<'PY'
import ast
from pathlib import Path
path = Path("nemoguardrails/library/injection_detection/actions.py")
tree = ast.parse(path.read_text())
functions = {
node.name: node
for node in ast.walk(tree)
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef))
}
for name in ("_load_rules", "injection_detection"):
node = functions[name]
doc = ast.get_docstring(node) or ""
raises = [n for n in ast.walk(node) if isinstance(n, ast.Raise)]
print(f"{name}: documented_yara_syntax_error={'yara.SyntaxError' in doc}, "
f"documented_value_error={'ValueError' in doc}, raise_count={len(raises)}")
load = functions["_load_rules"]
for node in ast.walk(load):
if isinstance(node, ast.ExceptHandler):
print("load_rules_handler:", ast.unparse(node.type) if node.type else "bare")
for child in ast.walk(node):
if isinstance(child, ast.Raise):
print("load_rules_raise:", ast.unparse(child.exc) if child.exc else "bare")
PYRepository: NVIDIA-NeMo/Guardrails Length of output: 23636 Document YARA compilation failures as
🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| return rules | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,8 +30,9 @@ class RegexDetectionResult(TypedDict): | |
|
|
||
|
|
||
| def _regex_outcome(source: str, result: RegexDetectionResult) -> RailOutcome: | ||
| metadata = dict(result) | ||
| metadata["source"] = source | ||
| # the checked text is deliberately excluded: outcome metadata reaches | ||
| # processing logs and tracing exporters | ||
| metadata = {"is_match": result["is_match"], "detections": result["detections"], "source": source} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The regex and injection-detection outcome helpers now omit checked text, but their action docstrings still advertise Knowledge Base Used: Library Rails Prompt To Fix With AIThis is a comment left during a code review.
Path: nemoguardrails/library/regex/actions.py
Line: 35
Comment:
**Metadata docs retain removed text**
The regex and injection-detection outcome helpers now omit checked text, but their action docstrings still advertise `metadata["text"]`; custom consumers following that documented return shape can access a nonexistent field, and generated documentation continues to promise sensitive data that is deliberately no longer returned.
**Knowledge Base Used:** [Library Rails](https://app.greptile.com/nvidia-public-github/-/custom-context/knowledge-base/nvidia-nemo/guardrails/-/docs/library-rails.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly. |
||
| if result["is_match"] and source == "retrieval": | ||
| return RailOutcome.transform([(TransformTarget.RELEVANT_CHUNKS, "")], metadata=metadata) | ||
| if result["is_match"]: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not cache the environment key in
ClavataClient.AuthHeader.to_headersnow readsCLAVATA_API_KEY, butClavataClient.__init__still stores the environment value inself.api_keyat Line 160._get_headerspasses that cached value at Line 171, so the fallback at Line 66 is not reached after a key rotation. A reused client sends the old credential.Store only an explicitly supplied
api_keyinClavataClient. LetAuthHeader.to_headersresolve the environment variable for each request. This conflicts with the PR objective that the key is read at call time.Proposed fix
🤖 Prompt for AI Agents