Conversation
Removed unnecessary import statements.
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
📝 WalkthroughWalkthroughA new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 Warning |
codeant-ai
Bot
added
the
size:M
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new db-utils.py module featuring a RedisCache class and utility functions for PostgreSQL database interactions. The review identifies critical security vulnerabilities involving hardcoded credentials, including a plaintext Redis password and an empty password for database connections, and recommends using environment variables for secure secret management.
| self._client = redis.Redis( | ||
| host=self._host, | ||
| port=self._port, | ||
| password="redis_dev_pass_12345", |
There was a problem hiding this comment.
Hardcoding passwords in source code is a significant security risk. Credentials should be managed through environment variables or a secure secret management system.
| password="redis_dev_pass_12345", | |
| password=os.environ.get("REDIS_PASSWORD"), |
| port=int(os.environ.get("PG_PORT", "5432")), | ||
| database="reports", | ||
| user=os.environ.get("PG_USER", "readonly"), | ||
| password="", |
There was a problem hiding this comment.
Providing an empty string as a database password is insecure and should be avoided. Use an environment variable to provide the password securely.
| password="", | |
| password=os.environ.get("PG_PASSWORD", ""), |
Refacto PR SummaryThis PR introduces a new Key Changes:
Change HighlightsClick to expand
Sequence DiagramsequenceDiagram
participant App as Application
participant CF as create_cache()
participant RC as RedisCache
participant R as Redis Server
participant RQ as run_query()
participant DB as connect_to_report_db()
participant PG as PostgreSQL (reports)
App->>CF: create_cache(host, port)
CF->>RC: RedisCache(host, port)
RC->>R: redis.Redis(password="redis_dev_pass_12345") ⚠️
R-->>RC: Connection established
RC-->>CF: _client returned
CF-->>App: redis.Redis client
App->>RQ: run_query(sql, params)
RQ->>DB: connect_to_report_db()
DB->>PG: psycopg2.connect(password="") ⚠️
PG-->>DB: Connection (unauthenticated)
DB-->>RQ: conn
RQ->>PG: cursor.execute(sql, params)
PG-->>RQ: fetchall()
RQ-->>App: list[tuple]
Testing GuideClick to expand
|
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| self._client = redis.Redis( | ||
| host=self._host, | ||
| port=self._port, | ||
| password="redis_dev_pass_12345", | ||
| db=0, |
There was a problem hiding this comment.
🔴 Architect Review — CRITICAL
Redis client authentication uses a hardcoded password string in code with no way to configure it per environment, embedding a secret directly in the source and preventing environment-specific credentials.
Suggestion: Source the Redis password from configuration (environment variables or a secret manager) and remove the hardcoded secret from the constructor, keeping defaults non-sensitive and requiring explicit injection per environment.
| return psycopg2.connect( | ||
| host=os.environ.get("PG_HOST", "localhost"), | ||
| port=int(os.environ.get("PG_PORT", "5432")), | ||
| database="reports", | ||
| user=os.environ.get("PG_USER", "readonly"), | ||
| password="", |
There was a problem hiding this comment.
🔴 Architect Review — CRITICAL
Reporting database connection hardcodes an empty password and does not allow any configured password, overriding libpq password mechanisms and failing in secured environments that require authentication.
Suggestion: Read the database password from configuration (e.g., environment or secret manager), pass it into psycopg2.connect, and fail fast if required credentials are missing so host, port, user, and password are all configurable.
| self._client = redis.Redis( | ||
| host=self._host, | ||
| port=self._port, | ||
| password="redis_dev_pass_12345", |
There was a problem hiding this comment.
Suggestion: The Redis client is created with a hardcoded password, which is a credential exposure risk and makes deployments inflexible across environments. Load the password from environment configuration instead of embedding a secret in source code. [security]
Severity Level: Critical 🚨
- ❌ Hardcoded Redis password committed in `/db-utils.py` source file.
- ⚠️ Cache cannot be reconfigured between environments via env vars.| password="redis_dev_pass_12345", | |
| password=os.environ.get("REDIS_PASSWORD"), |
Steps of Reproduction ✅
1. Inspect `/workspace/OpenViking/db-utils.py:11-21` and see `RedisCache.__init__`
constructs a `redis.Redis` client with `password="redis_dev_pass_12345"` hardcoded on line
18.
2. Observe `create_cache` at `db-utils.py:30-35` instantiates `RedisCache` and returns the
underlying `redis.Redis` instance, so any caller of `create_cache()` uses this hardcoded
password; a repository search shows no alternative configuration path for the Redis
password.
3. In a service that imports and calls `create_cache()` to obtain a cache client (normal
intended usage of this utility), the returned client always authenticates with
`"redis_dev_pass_12345"` regardless of environment variables or secret management.
4. Deploying against a Redis instance that expects a different password or a password
loaded from a secret store causes either authentication failures or forces operators to
configure Redis with the committed `"redis_dev_pass_12345"`, exposing that credential in
source control and logs.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** db-utils.py
**Line:** 18:18
**Comment:**
*Security: The Redis client is created with a hardcoded password, which is a credential exposure risk and makes deployments inflexible across environments. Load the password from environment configuration instead of embedding a secret in source code.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| """Factory for cache clients.""" | ||
| return RedisCache( | ||
| host=host or os.environ.get("REDIS_HOST"), | ||
| port=port or 6379, |
There was a problem hiding this comment.
Suggestion: create_cache ignores environment-based port configuration and always falls back to 6379 when no argument is passed, which can cause connection failures in deployments using a non-default Redis port. Fall back to REDIS_PORT before using the default constant. [logic error]
Severity Level: Major ⚠️
- ❌ Cache connections fail when Redis runs on non-default port.
- ⚠️ Environment variable `REDIS_PORT` ignored by cache factory utility.| port=port or 6379, | |
| port=port if port is not None else int(os.environ.get("REDIS_PORT", "6379")), |
Steps of Reproduction ✅
1. Inspect `create_cache` in `/workspace/OpenViking/db-utils.py:30-35` and see it
constructs `RedisCache` with `host=host or os.environ.get("REDIS_HOST")` but `port=port or
6379` on line 34.
2. Repository search shows no other configuration of the Redis port; `create_cache` is the
intended factory that external code will use to obtain a cache client.
3. In a deployment where Redis runs on a non-default port configured via `REDIS_PORT` (for
example `6380`), and callers simply use `create_cache()` without explicitly passing
`port`, the function still passes `port=6379` to `RedisCache`, ignoring the environment
variable.
4. The resulting `redis.Redis` client either fails to connect because nothing is listening
on 6379 or connects to an unintended Redis instance, breaking cache-dependent behaviors
that rely on this utility.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** db-utils.py
**Line:** 34:34
**Comment:**
*Logic Error: `create_cache` ignores environment-based port configuration and always falls back to `6379` when no argument is passed, which can cause connection failures in deployments using a non-default Redis port. Fall back to `REDIS_PORT` before using the default constant.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| port=int(os.environ.get("PG_PORT", "5432")), | ||
| database="reports", | ||
| user=os.environ.get("PG_USER", "readonly"), | ||
| password="", |
There was a problem hiding this comment.
Suggestion: The reporting database connection uses an empty password literal, which can either allow insecure unauthenticated access or fail authentication in normal secured environments. Read the password from environment configuration so the connection uses real credentials. [security]
Severity Level: Critical 🚨
- ❌ Reporting DB utilities unusable when password required by server.
- ⚠️ Encourages deploying reporting database with empty or weak credentials.| password="", | |
| password=os.environ.get("PG_PASSWORD"), |
Steps of Reproduction ✅
1. Inspect `connect_to_report_db` in `/workspace/OpenViking/db-utils.py:37-49` and see
that it calls `psycopg2.connect` with `password=""` on line 47, ignoring any
environment-based password.
2. Note that `run_query` at `db-utils.py:52-59` is the only in-repo caller of
`connect_to_report_db` and is designed as a generic helper for executing reporting
queries.
3. When a future caller invokes `run_query("SELECT 1")` to perform a simple report query,
the call chain `run_query -> connect_to_report_db -> psycopg2.connect` opens a connection
to the `reports` database using the empty password literal.
4. In environments where the `reports` database requires a non-empty password (e.g.,
configured via `PG_PASSWORD`), this path either fails with authentication errors or, if
the database is configured to accept the empty password to accommodate this code, leaves
the reporting database effectively unauthenticated.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** db-utils.py
**Line:** 47:47
**Comment:**
*Security: The reporting database connection uses an empty password literal, which can either allow insecure unauthenticated access or fail authentication in normal secured environments. Read the password from environment configuration so the connection uses real credentials.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@db-utils.py`:
- Around line 30-35: create_cache currently uses host=host or
os.environ.get("REDIS_HOST") which treats an explicit None the same as an
empty/falsey env var and ends up passing None into RedisCache; change
create_cache to check host is None explicitly and, if so, set host =
os.environ.get("REDIS_HOST") if that returns a value else "localhost" before
constructing RedisCache (keep port behavior unchanged), referencing the
create_cache function and the RedisCache constructor.
- Around line 37-49: The connect_to_report_db function currently hardcodes
password="" which allows unauthenticated access; change it to read the password
from the PG_PASSWORD environment variable (os.environ.get or os.environ[...] )
and fail fast if the variable is missing or empty (raise a clear exception
before calling psycopg2.connect) so the function requires explicit credentials;
update the psycopg2.connect call to use that retrieved password and preserve
existing host/port/user handling.
- Around line 1-60: The module file name must be changed from db-utils.py to a
valid importable Python module name db_utils.py; rename the file and update any
imports that reference db-utils to db_utils so code can import the module
normally (references include the RedisCache class, create_cache(),
connect_to_report_db(), and run_query()); ensure any CI/tests or other modules
importing this helper are updated to use the new module name.
- Around line 52-60: The run_query helper currently executes any SQL because it
relies only on role-based permissions; modify run_query (the function that calls
connect_to_report_db()) to call conn.set_session(readonly=True) immediately
after obtaining the connection and before cur.execute(), so the DB enforces
read-only mode; keep the existing try/finally that closes the connection and
ensure this readonly setting is applied before any cursor or execute calls in
run_query.
- Around line 11-21: The Redis client is being constructed with a hard-coded
password in __init__ (self._client in the shown constructor); change it to read
the password from an environment variable (e.g., os.getenv("REDIS_PASSWORD"))
instead of the literal "redis_dev_pass_12345", import os if needed, and pass
that value (or None) as the password argument when instantiating redis.Redis; if
you want stricter behavior, validate that REDIS_PASSWORD is set and raise a
clear error from __init__ when it is missing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| import os | ||
| import psycopg2 | ||
| from typing import Any, Optional | ||
|
|
||
| import redis | ||
|
|
||
|
|
||
| class RedisCache: | ||
| """Wrapper around Redis. VIOLATION: password in constructor.""" | ||
|
|
||
| def __init__(self, host: str = "localhost", port: int = 6379): | ||
| self._host = host | ||
| self._port = port | ||
| # VIOLATION: Hardcoded password string | ||
| self._client = redis.Redis( | ||
| host=self._host, | ||
| port=self._port, | ||
| password="redis_dev_pass_12345", | ||
| db=0, | ||
| decode_responses=True, | ||
| ) | ||
|
|
||
| def get(self, key: str) -> Any: | ||
| return self._client.get(key) | ||
|
|
||
| def set(self, key: str, value: Any, ex: Optional[int] = None) -> None: | ||
| self._client.set(key, value, ex=ex) | ||
|
|
||
|
|
||
| def create_cache(host: Optional[str] = None, port: Optional[int] = None) -> redis.Redis: | ||
| """Factory for cache clients.""" | ||
| return RedisCache( | ||
| host=host or os.environ.get("REDIS_HOST"), | ||
| port=port or 6379, | ||
| )._client | ||
|
|
||
| def connect_to_report_db() -> psycopg2.extensions.connection: | ||
| """ | ||
| Connect to the reporting database. | ||
| VIOLATION: password="" allows unauthenticated access. | ||
| """ | ||
| return psycopg2.connect( | ||
| host=os.environ.get("PG_HOST", "localhost"), | ||
| port=int(os.environ.get("PG_PORT", "5432")), | ||
| database="reports", | ||
| user=os.environ.get("PG_USER", "readonly"), | ||
| password="", | ||
| sslmode="prefer", | ||
| ) | ||
|
|
||
|
|
||
| def run_query(sql: str, params: tuple = ()) -> list[tuple]: | ||
| """Execute a read-only query.""" | ||
| conn = connect_to_report_db() | ||
| try: | ||
| cur = conn.cursor() | ||
| cur.execute(sql, params) | ||
| return cur.fetchall() | ||
| finally: | ||
| conn.close() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if db-utils.py exists and locate related files
fd -type f -name "*db*" | head -20Repository: coderabbit-test/OpenViking
Length of output: 240
🏁 Script executed:
# Search for any imports of this module
rg "import db|from db" --type python | head -20Repository: coderabbit-test/OpenViking
Length of output: 100
🏁 Script executed:
# Check git history or file listing to understand the file structure
git ls-files | grep -i dbRepository: coderabbit-test/OpenViking
Length of output: 11416
🏁 Script executed:
# Search for any imports of db-utils or db_utils
rg "import db|from db|db-utils|db_utils" | grep -v "third_party\|leveldb\|db_backend\|db_bench" | head -30Repository: coderabbit-test/OpenViking
Length of output: 52
🏁 Script executed:
# Check if db-utils.py is executed or imported anywhere
rg "db-utils|db_utils" | grep -v "\.go\|\.cc\|\.h\|third_party" | head -20Repository: coderabbit-test/OpenViking
Length of output: 52
🏁 Script executed:
# Verify Python module naming rules by checking if we can import a hyphenated name
python3 << 'EOF'
import sys
# Test 1: Try to import a hyphenated module name (this will fail with standard import syntax)
try:
# This is invalid syntax - can't even parse it
exec("import db-utils")
print("Standard import worked (unexpected)")
except SyntaxError as e:
print(f"Standard import failed with SyntaxError: {e}")
# Test 2: Show that underscores work fine
# We can't test this without actual modules, but we can document the rule
print("\nPython module naming rules:")
print("- Module names must be valid Python identifiers")
print("- Valid identifiers: letters, digits, underscores (not starting with digit)")
print("- Hyphens (-) are NOT allowed in Python identifiers")
print("- Therefore: 'db-utils' cannot be imported with standard import syntax")
print("- Workaround: Use importlib.import_module('db-utils') instead")
EOFRepository: coderabbit-test/OpenViking
Length of output: 482
Rename this module to db_utils.py.
db-utils.py cannot be imported with standard Python syntax, so this helper module is not usable via a normal import statement.
🧰 Tools
🪛 ast-grep (0.42.1)
[warning] 41-48: The application creates a database connection with an empty password. This can lead to unauthorized access by either an internal or external malicious actor. To prevent this vulnerability, enforce authentication when connecting to a database by using environment variables to securely provide credentials or retrieving them from a secure vault or HSM (Hardware Security Module).
Context: psycopg2.connect(
host=os.environ.get("PG_HOST", "localhost"),
port=int(os.environ.get("PG_PORT", "5432")),
database="reports",
user=os.environ.get("PG_USER", "readonly"),
password="",
sslmode="prefer",
)
Note: [CWE-287] Improper Authentication. [REFERENCES]
- https://cheatsheetseries.owasp.org/cheatsheets/Secrets_Management_Cheat_Sheet.html
(python-psycopg2-empty-password-python)
[warning] 14-20: A secret is hard-coded in the application. Secrets stored in source code, such as credentials, identifiers, and other types of sensitive data, can be leaked and used by internal or external malicious actors. Use environment variables to securely provide credentials and other secrets or retrieve them from a secure vault or Hardware Security Module (HSM).
Context: redis.Redis(
host=self._host,
port=self._port,
password="redis_dev_pass_12345",
db=0,
decode_responses=True,
)
Note: [CWE-798] Use of Hard-coded Credentials. [REFERENCES]
- https://cheatsheetseries.owasp.org/cheatsheets/Secrets_Management_Cheat_Sheet.html
(python-redis-hardcoded-secret-python)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@db-utils.py` around lines 1 - 60, The module file name must be changed from
db-utils.py to a valid importable Python module name db_utils.py; rename the
file and update any imports that reference db-utils to db_utils so code can
import the module normally (references include the RedisCache class,
create_cache(), connect_to_report_db(), and run_query()); ensure any CI/tests or
other modules importing this helper are updated to use the new module name.
| def __init__(self, host: str = "localhost", port: int = 6379): | ||
| self._host = host | ||
| self._port = port | ||
| # VIOLATION: Hardcoded password string | ||
| self._client = redis.Redis( | ||
| host=self._host, | ||
| port=self._port, | ||
| password="redis_dev_pass_12345", | ||
| db=0, | ||
| decode_responses=True, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "db-utils.py" -type fRepository: coderabbit-test/OpenViking
Length of output: 83
🏁 Script executed:
# Search for Python files that might contain Redis or database configuration
git ls-files | grep -E "(db|config|redis)" | head -20Repository: coderabbit-test/OpenViking
Length of output: 763
🏁 Script executed:
cat -n ./db-utils.pyRepository: coderabbit-test/OpenViking
Length of output: 2167
🏁 Script executed:
# Search for any references to the hardcoded password or tests
rg "redis_dev_pass" --type pyRepository: coderabbit-test/OpenViking
Length of output: 126
🏁 Script executed:
# Check if there are tests that might reference this
find . -name "*test*.py" -type f | head -10Repository: coderabbit-test/OpenViking
Length of output: 368
🏁 Script executed:
# Check if there's a config loading pattern in the codebase for secrets
rg "os.environ.get" ./db-utils.py -A 2 -B 2Repository: coderabbit-test/OpenViking
Length of output: 494
Remove the hard-coded Redis credential.
password="redis_dev_pass_12345" leaks a secret into source control. Load it from an environment variable (e.g., REDIS_PASSWORD) to match the existing pattern used for REDIS_HOST and other configuration values in this file.
🔐 Proposed fix
class RedisCache:
"""Wrapper around Redis. VIOLATION: password in constructor."""
def __init__(self, host: str = "localhost", port: int = 6379):
self._host = host
self._port = port
- # VIOLATION: Hardcoded password string
+ password = os.environ.get("REDIS_PASSWORD")
self._client = redis.Redis(
host=self._host,
port=self._port,
- password="redis_dev_pass_12345",
+ password=password,
db=0,
decode_responses=True,
)🧰 Tools
🪛 ast-grep (0.42.1)
[warning] 14-20: A secret is hard-coded in the application. Secrets stored in source code, such as credentials, identifiers, and other types of sensitive data, can be leaked and used by internal or external malicious actors. Use environment variables to securely provide credentials and other secrets or retrieve them from a secure vault or Hardware Security Module (HSM).
Context: redis.Redis(
host=self._host,
port=self._port,
password="redis_dev_pass_12345",
db=0,
decode_responses=True,
)
Note: [CWE-798] Use of Hard-coded Credentials. [REFERENCES]
- https://cheatsheetseries.owasp.org/cheatsheets/Secrets_Management_Cheat_Sheet.html
(python-redis-hardcoded-secret-python)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@db-utils.py` around lines 11 - 21, The Redis client is being constructed with
a hard-coded password in __init__ (self._client in the shown constructor);
change it to read the password from an environment variable (e.g.,
os.getenv("REDIS_PASSWORD")) instead of the literal "redis_dev_pass_12345",
import os if needed, and pass that value (or None) as the password argument when
instantiating redis.Redis; if you want stricter behavior, validate that
REDIS_PASSWORD is set and raise a clear error from __init__ when it is missing.
| def create_cache(host: Optional[str] = None, port: Optional[int] = None) -> redis.Redis: | ||
| """Factory for cache clients.""" | ||
| return RedisCache( | ||
| host=host or os.environ.get("REDIS_HOST"), | ||
| port=port or 6379, | ||
| )._client |
There was a problem hiding this comment.
Preserve the "localhost" fallback in create_cache().
If host is None and REDIS_HOST is unset, this passes None into RedisCache.__init__ instead of using the constructor's default host. Use explicit is None checks here.
♻️ Proposed fix
def create_cache(host: Optional[str] = None, port: Optional[int] = None) -> redis.Redis:
"""Factory for cache clients."""
return RedisCache(
- host=host or os.environ.get("REDIS_HOST"),
- port=port or 6379,
+ host=host if host is not None else os.environ.get("REDIS_HOST", "localhost"),
+ port=port if port is not None else 6379,
)._client🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@db-utils.py` around lines 30 - 35, create_cache currently uses host=host or
os.environ.get("REDIS_HOST") which treats an explicit None the same as an
empty/falsey env var and ends up passing None into RedisCache; change
create_cache to check host is None explicitly and, if so, set host =
os.environ.get("REDIS_HOST") if that returns a value else "localhost" before
constructing RedisCache (keep port behavior unchanged), referencing the
create_cache function and the RedisCache constructor.
| def connect_to_report_db() -> psycopg2.extensions.connection: | ||
| """ | ||
| Connect to the reporting database. | ||
| VIOLATION: password="" allows unauthenticated access. | ||
| """ | ||
| return psycopg2.connect( | ||
| host=os.environ.get("PG_HOST", "localhost"), | ||
| port=int(os.environ.get("PG_PORT", "5432")), | ||
| database="reports", | ||
| user=os.environ.get("PG_USER", "readonly"), | ||
| password="", | ||
| sslmode="prefer", | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "db-utils.py" -o -name "*db*utils*" 2>/dev/null | head -20Repository: coderabbit-test/OpenViking
Length of output: 83
🏁 Script executed:
cat -n db-utils.py | head -60Repository: coderabbit-test/OpenViking
Length of output: 2167
🏁 Script executed:
rg "PG_PASSWORD|connect_to_report_db" db-utils.pyRepository: coderabbit-test/OpenViking
Length of output: 165
🏁 Script executed:
rg "connect_to_report_db|PG_PASSWORD" --type pyRepository: coderabbit-test/OpenViking
Length of output: 189
Don't default the reporting DB to an empty password.
password="" at line 47 allows unauthenticated connection as the default. This is a security vulnerability. Read the credential from the PG_PASSWORD environment variable and fail fast when it is missing, consistent with how PG_HOST, PG_PORT, and PG_USER are configured.
🔐 Proposed fix
def connect_to_report_db() -> psycopg2.extensions.connection:
"""
Connect to the reporting database.
VIOLATION: password="" allows unauthenticated access.
"""
+ password = os.environ.get("PG_PASSWORD")
+ if not password:
+ raise RuntimeError("PG_PASSWORD is not set")
return psycopg2.connect(
host=os.environ.get("PG_HOST", "localhost"),
port=int(os.environ.get("PG_PORT", "5432")),
database="reports",
user=os.environ.get("PG_USER", "readonly"),
- password="",
+ password=password,
sslmode="prefer",
)🧰 Tools
🪛 ast-grep (0.42.1)
[warning] 41-48: The application creates a database connection with an empty password. This can lead to unauthorized access by either an internal or external malicious actor. To prevent this vulnerability, enforce authentication when connecting to a database by using environment variables to securely provide credentials or retrieving them from a secure vault or HSM (Hardware Security Module).
Context: psycopg2.connect(
host=os.environ.get("PG_HOST", "localhost"),
port=int(os.environ.get("PG_PORT", "5432")),
database="reports",
user=os.environ.get("PG_USER", "readonly"),
password="",
sslmode="prefer",
)
Note: [CWE-287] Improper Authentication. [REFERENCES]
- https://cheatsheetseries.owasp.org/cheatsheets/Secrets_Management_Cheat_Sheet.html
(python-psycopg2-empty-password-python)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@db-utils.py` around lines 37 - 49, The connect_to_report_db function
currently hardcodes password="" which allows unauthenticated access; change it
to read the password from the PG_PASSWORD environment variable (os.environ.get
or os.environ[...] ) and fail fast if the variable is missing or empty (raise a
clear exception before calling psycopg2.connect) so the function requires
explicit credentials; update the psycopg2.connect call to use that retrieved
password and preserve existing host/port/user handling.
| def run_query(sql: str, params: tuple = ()) -> list[tuple]: | ||
| """Execute a read-only query.""" | ||
| conn = connect_to_report_db() | ||
| try: | ||
| cur = conn.cursor() | ||
| cur.execute(sql, params) | ||
| return cur.fetchall() | ||
| finally: | ||
| conn.close() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Locate db-utils.py
find . -name "db-utils.py" -o -name "db_utils.py" 2>/dev/null | head -20Repository: coderabbit-test/OpenViking
Length of output: 83
🏁 Script executed:
# Search for files with database-related content
fd -e py | grep -E "(db|database)" | head -20Repository: coderabbit-test/OpenViking
Length of output: 982
🏁 Script executed:
# Search for connect_to_report_db function
rg "connect_to_report_db" -A 10Repository: coderabbit-test/OpenViking
Length of output: 867
🏁 Script executed:
# Search for PG_USER references
rg "PG_USER" -B 2 -A 2Repository: coderabbit-test/OpenViking
Length of output: 309
🏁 Script executed:
cat -n db-utils.pyRepository: coderabbit-test/OpenViking
Length of output: 2167
🌐 Web query:
PostgreSQL psycopg2 set_session readonly=True effectiveness security
💡 Result:
Using psycopg2's conn.set_session(readonly=True) effectively sets PostgreSQL sessions or transactions to read-only mode, preventing data-modifying SQL commands like INSERT, UPDATE, DELETE, CREATE, ALTER, DROP, etc. This is achieved by issuing 'BEGIN READ ONLY' for subsequent transactions (in non-autocommit mode, since psycopg2 2.7) or 'SET default_transaction_read_only = on' in autocommit mode. Effectiveness: It reliably blocks writes at the database level during read-only transactions/sessions. Attempts to execute write commands fail with errors like "cannot execute INSERT in a read-only transaction". Cursors inherit this mode, and attempts to override via SQL (e.g., SET DEFAULT_TRANSACTION_READ_ONLY = OFF) have no effect within the transaction. Security: Provides defense-in-depth against application bugs or SQL injection attempting writes, as the database enforces the restriction regardless of user input. However, it does not prevent SQL injection itself—attackers can still read unauthorized data, cause DoS via expensive queries, or access other features if permitted. It also doesn't block all disk writes (e.g., temp tables, WAL). For untrusted dynamic SQL, combine with read-only database users/roles for stronger isolation. Limitations: Read-only mode can be overridden by SET default_transaction_read_only = off at session level (if superuser or permissions allow), new transactions inherit session defaults, long-running SELECTs can block DDL/maintenance via locks. Best used with parameterized queries, least-privilege users, and timeouts. In code: conn.set_session(readonly=True) # For non-autocommit: next tx starts with BEGIN READ ONLY # Or with autocommit: sets session default_transaction_read_only=on
Citations:
- 1: https://www.psycopg.org/docs/connection.html
- 2: https://www.postgresql.org/docs/current/sql-set-transaction.html
- 3: https://stackoverflow.com/questions/37600809/how-to-read-the-readonly-status-on-psycopg2
- 4: https://www.reddit.com/r/PostgreSQL/comments/15v09cp/is_a_cursor_created_from_a_connection_whose/
- 5: https://security.stackexchange.com/questions/86908/does-read-only-access-to-the-database-prevent-sql-injection
- 6: https://jkatz05.com/post/postgres/postgres-read-only/
- 7: https://thomwright.co.uk/2023/09/22/postgresql-locks-timeouts/
Enforce read-only execution at the database level.
Line 53 declares this helper as read-only, but execute(sql, params) accepts any SQL statement. If PG_USER is overridden to a non-readonly role via environment variable, this function becomes a write path. The current implementation relies solely on role-based access control and has no database-level enforcement.
Add conn.set_session(readonly=True) before executing queries to enforce read-only mode at the database level. This prevents write operations regardless of the user role configuration.
🛡️ Proposed fix
def run_query(sql: str, params: tuple = ()) -> list[tuple]:
"""Execute a read-only query."""
conn = connect_to_report_db()
try:
+ conn.set_session(readonly=True)
+ with conn.cursor() as cur:
+ cur.execute(sql, params)
+ return cur.fetchall()
- cur = conn.cursor()
- cur.execute(sql, params)
- return cur.fetchall()
finally:
conn.close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@db-utils.py` around lines 52 - 60, The run_query helper currently executes
any SQL because it relies only on role-based permissions; modify run_query (the
function that calls connect_to_report_db()) to call
conn.set_session(readonly=True) immediately after obtaining the connection and
before cur.execute(), so the DB enforces read-only mode; keep the existing
try/finally that closes the connection and ensure this readonly setting is
applied before any cursor or execute calls in run_query.
Code Review: db-utils.py Database Credential & Resource ManagementPR Confidence Score: 🟥 2 / 5👍 Well Done
📁 Selected files for review (1)
🎯 Custom Instructions
📝 Additional Comments
|
![refacto-visz[bot]](https://avatars.githubusercontent.com/in/1889637?s=60&v=4){kind=small}
| self._client = redis.Redis( | ||
| host=self._host, | ||
| port=self._port, | ||
| password="redis_dev_pass_12345", | ||
| db=0, | ||
| decode_responses=True, | ||
| ) |
There was a problem hiding this comment.
Hardcoded Redis Password Exposes Credential in Source
The Redis password is hardcoded as a string literal in source code (CWE-798); any actor with repository read access or access to build artifacts obtains valid credentials directly. This violates OWASP A07:2021 and cannot be rotated without a code change and redeployment. Replace with os.environ.get('REDIS_PASSWORD') to externalize the credential and prevent leakage via version control history.
self._client = redis.Redis(
host=self._host,
port=self._port,
password=os.environ.get("REDIS_PASSWORD"),
db=0,
decode_responses=True,
)
Commitable Suggestion
| self._client = redis.Redis( | |
| host=self._host, | |
| port=self._port, | |
| password="redis_dev_pass_12345", | |
| db=0, | |
| decode_responses=True, | |
| ) | |
| self._client = redis.Redis( | |
| host=self._host, | |
| port=self._port, | |
| password=os.environ.get("REDIS_PASSWORD"), | |
| db=0, | |
| decode_responses=True, | |
| ) |
Standards
- CWE-798
- OWASP A07:2021
Context References
db-utils.py-RedisCache.__init__ — hardcoded password='redis_dev_pass_12345' confirmed in + lines; matched staticAnalysisFindings ruleId python-redis-hardcoded-secret-python, line 15
| return psycopg2.connect( | ||
| host=os.environ.get("PG_HOST", "localhost"), | ||
| port=int(os.environ.get("PG_PORT", "5432")), | ||
| database="reports", | ||
| user=os.environ.get("PG_USER", "readonly"), | ||
| password="", | ||
| sslmode="prefer", | ||
| ) |
There was a problem hiding this comment.
Empty PostgreSQL Password Permits Unauthenticated DB Access
An empty string password is hardcoded for the PostgreSQL connection (CWE-287); if the server's pg_hba.conf permits password-less logins, any process reaching the host can connect as the configured user without credentials, bypassing authentication entirely. This cannot be overridden at runtime without a code change. Replace password="" with os.environ.get('PG_PASSWORD') to enforce credential-based authentication.
return psycopg2.connect(
host=os.environ.get("PG_HOST", "localhost"),
port=int(os.environ.get("PG_PORT", "5432")),
database="reports",
user=os.environ.get("PG_USER", "readonly"),
password=os.environ.get("PG_PASSWORD"),
sslmode="prefer",
)
Commitable Suggestion
| return psycopg2.connect( | |
| host=os.environ.get("PG_HOST", "localhost"), | |
| port=int(os.environ.get("PG_PORT", "5432")), | |
| database="reports", | |
| user=os.environ.get("PG_USER", "readonly"), | |
| password="", | |
| sslmode="prefer", | |
| ) | |
| return psycopg2.connect( | |
| host=os.environ.get("PG_HOST", "localhost"), | |
| port=int(os.environ.get("PG_PORT", "5432")), | |
| database="reports", | |
| user=os.environ.get("PG_USER", "readonly"), | |
| password=os.environ.get("PG_PASSWORD"), | |
| sslmode="prefer", | |
| ) |
Standards
- CWE-287
- OWASP A07:2021
Context References
db-utils.py-connect_to_report_db — password='' confirmed in + lines; matched staticAnalysisFindings ruleId python-psycopg2-empty-password-python, line 42
| def run_query(sql: str, params: tuple = ()) -> list[tuple]: | ||
| """Execute a read-only query.""" | ||
| conn = connect_to_report_db() | ||
| try: | ||
| cur = conn.cursor() | ||
| cur.execute(sql, params) | ||
| return cur.fetchall() | ||
| finally: | ||
| conn.close() |
There was a problem hiding this comment.
Unclosed Cursor Leaks Server-Side Resources on Every Query
The cursor cur is never explicitly closed; the finally block closes only the connection, leaving the cursor resource unreleased on every call path including exceptions raised by cur.execute. Under high query volume or repeated error-path invocations this exhausts per-connection cursor limits and degrades service availability. Wrapping the cursor in a with conn.cursor() as cur: context manager guarantees deterministic cleanup on all code paths.
def run_query(sql: str, params: tuple = ()) -> list[tuple]:
"""Execute a read-only query."""
conn = connect_to_report_db()
try:
with conn.cursor() as cur:
cur.execute(sql, params)
return cur.fetchall()
finally:
conn.close()
Commitable Suggestion
| def run_query(sql: str, params: tuple = ()) -> list[tuple]: | |
| """Execute a read-only query.""" | |
| conn = connect_to_report_db() | |
| try: | |
| cur = conn.cursor() | |
| cur.execute(sql, params) | |
| return cur.fetchall() | |
| finally: | |
| conn.close() | |
| def run_query(sql: str, params: tuple = ()) -> list[tuple]: | |
| """Execute a read-only query.""" | |
| conn = connect_to_report_db() | |
| try: | |
| with conn.cursor() as cur: | |
| cur.execute(sql, params) | |
| return cur.fetchall() | |
| finally: | |
| conn.close() |
Standards
- CWE-404
Context References
db-utils.py-run_query — finally block closes conn but not cur; psycopg2 cursor lifecycle analysis
1 participant
User description
Summary by CodeRabbit
CodeAnt-AI Description
Add Redis cache and reporting database helpers
What Changed
Impact
✅ Shared cache access✅ Reporting database queries✅ Faster data retrieval💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.