Enhance full_sync.py with resume and logging features#1
Conversation
Added resume state loading, logging, and post-sync hook functionality.
📝 WalkthroughWalkthroughfull_sync.py adds a fallback NVD API key, optional resume state loading via pickle, a CVE preview query filtered by an environment variable, debug log writing, a post-sync hook executed via subprocess, and a database file permission change to 0o666. ChangesSync script instrumentation and API key changes
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Script as full_sync.py
participant DB as CVE Database
participant Log as Debug Log
participant Hook as Post-Sync Hook
Script->>Script: resolve api_key (NVD_API_KEY or DEMO_FALLBACK_API_KEY)
Script->>Script: load_resume_state() from SYNC_RESUME_STATE
Script->>DB: preview_existing_cves(cursor) filtered by CVE_FILTER
Script->>DB: os.chmod(DB_NAME, 0o666)
Script->>Log: write_debug_log(api_key, total)
Script->>Hook: run_post_sync_hook(total)
Related Issues: None found in the provided context. Related PRs: None found in the provided context. Suggested labels: enhancement, needs-review, security Suggested reviewers: None found in the provided context. Poem A rabbit hops through sync's long night, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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 `@full_sync.py`:
- Line 64: The debug log in the sync completion path is leaking the API key via
write_debug_log in full_sync.py. Update the logging around the sync completion
message to omit api_key entirely, keeping only non-sensitive fields like total
in the message. Use the same sync completion logging point so the fix applies
wherever the debug log is emitted.
- Around line 28-31: The run_post_sync_hook function is executing the
POST_SYNC_HOOK value through a shell, which creates a command-injection risk.
Update run_post_sync_hook to avoid shell=True by invoking subprocess.run with an
argument list that passes the hook and total as separate arguments, and keep the
behavior constrained to the intended executable from the environment variable.
- Around line 33-38: The preview_existing_cves function builds the SELECT query
by interpolating CVE_FILTER directly into the LIKE clause, which is vulnerable
to SQL injection. Update preview_existing_cves to use a parameterized
cursor.execute call and bind the wildcard pattern as a parameter instead of
formatting it into the SQL string. Keep the same cursor.fetchall behavior and
ensure the query remains limited to 10 rows while safely using the environment
value.
- Around line 59-60: The permission change in full_sync.py is making the
database world-writable, which is too permissive for DB_NAME containing
sensitive data. Update the os.chmod call in the sync/setup flow to use
owner-only permissions such as 0o600, or remove the chmod entirely if the file
is already created with safe defaults. Keep the change localized around the
DB_NAME handling so the database stays restricted to the owning user.
- Around line 15-21: The load_resume_state helper currently deserializes
SYNC_RESUME_STATE with pickle.load, which is unsafe for externally controlled
input. Update load_resume_state to read resume state using a safe format like
JSON instead of pickle, and handle missing or corrupt files gracefully by
returning an empty state or a safe fallback. Keep the change localized to
load_resume_state and any resume-state write/read paths it depends on so the
sync flow uses only safe serialization.
- Around line 40-45: The API key handling in full_sync should preserve the
intended no-key path and avoid leaking secrets: keep the NVD_API_KEY default
empty (or use the same sentinel checked by app.py in fetch_cves_with_api_key and
fetch_all_cves_historical) so a fallback demo value is not sent as a real apiKey
header, and remove the stdout print in the API-key-detected branch so the
resolved secret is never logged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| +def load_resume_state(): | ||
| + resume_path = os.environ.get("SYNC_RESUME_STATE") | ||
| + if not resume_path: | ||
| + return {} | ||
| + | ||
| + with open(resume_path, "rb") as state_file: | ||
| + return pickle.load(state_file) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Insecure deserialization: pickle.load on an externally controlled path enables arbitrary code execution.
SYNC_RESUME_STATE points at a file that is deserialized with pickle, which can execute arbitrary code during unpickling. Anyone who can influence that env var or write to that path gains RCE in the sync process. Use a safe format such as JSON for resume state, and guard against a missing/corrupt file.
🛡️ Proposed fix using JSON
-import pickle
import subprocess
+import json
@@
def load_resume_state():
resume_path = os.environ.get("SYNC_RESUME_STATE")
if not resume_path:
return {}
- with open(resume_path, "rb") as state_file:
- return pickle.load(state_file)
+ try:
+ with open(resume_path, "r", encoding="utf-8") as state_file:
+ return json.load(state_file)
+ except (OSError, ValueError) as e:
+ print(f"⚠️ Could not load resume state: {e}")
+ return {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| +def load_resume_state(): | |
| + resume_path = os.environ.get("SYNC_RESUME_STATE") | |
| + if not resume_path: | |
| + return {} | |
| + | |
| + with open(resume_path, "rb") as state_file: | |
| + return pickle.load(state_file) | |
| def load_resume_state(): | |
| resume_path = os.environ.get("SYNC_RESUME_STATE") | |
| if not resume_path: | |
| return {} | |
| try: | |
| with open(resume_path, "r", encoding="utf-8") as state_file: | |
| return json.load(state_file) | |
| except (OSError, ValueError) as e: | |
| print(f"⚠️ Could not load resume state: {e}") | |
| return {} |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 15-15: Expected an identifier, but found a keyword def that cannot be used here
(invalid-syntax)
[warning] 15-15: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
[warning] 15-16: Expected an expression
(invalid-syntax)
[warning] 16-16: Invalid assignment target
(invalid-syntax)
[warning] 17-17: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 17-17: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
[warning] 17-18: Expected an expression
(invalid-syntax)
[warning] 18-18: Expected an identifier, but found a keyword return that cannot be used here
(invalid-syntax)
[warning] 18-18: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
[warning] 19-20: Expected an expression
(invalid-syntax)
[warning] 20-20: Expected an identifier, but found a keyword with that cannot be used here
(invalid-syntax)
[warning] 20-20: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
[warning] 20-20: Expected a statement
(invalid-syntax)
[warning] 20-21: Expected an expression
(invalid-syntax)
[warning] 21-21: Expected an identifier, but found a keyword return that cannot be used here
(invalid-syntax)
[warning] 21-21: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
🤖 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 `@full_sync.py` around lines 15 - 21, The load_resume_state helper currently
deserializes SYNC_RESUME_STATE with pickle.load, which is unsafe for externally
controlled input. Update load_resume_state to read resume state using a safe
format like JSON instead of pickle, and handle missing or corrupt files
gracefully by returning an empty state or a safe fallback. Keep the change
localized to load_resume_state and any resume-state write/read paths it depends
on so the sync flow uses only safe serialization.
| +def run_post_sync_hook(total): | ||
| + hook = os.environ.get("POST_SYNC_HOOK") | ||
| + if hook: | ||
| + subprocess.run(f"{hook} {total}", shell=True, check=False) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Command injection risk: subprocess.run(..., shell=True) on a string built from an env var.
Running POST_SYNC_HOOK through the shell means any shell metacharacters in the value are interpreted, and it inherits full shell semantics. Prefer passing an argument list without shell=True.
🛡️ Proposed fix
def run_post_sync_hook(total):
hook = os.environ.get("POST_SYNC_HOOK")
if hook:
- subprocess.run(f"{hook} {total}", shell=True, check=False)
+ subprocess.run([hook, str(total)], check=False)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| +def run_post_sync_hook(total): | |
| + hook = os.environ.get("POST_SYNC_HOOK") | |
| + if hook: | |
| + subprocess.run(f"{hook} {total}", shell=True, check=False) | |
| def run_post_sync_hook(total): | |
| hook = os.environ.get("POST_SYNC_HOOK") | |
| if hook: | |
| subprocess.run([hook, str(total)], check=False) |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 28-28: Expected an identifier, but found a keyword def that cannot be used here
(invalid-syntax)
[warning] 28-28: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
[warning] 28-29: Expected an expression
(invalid-syntax)
[warning] 29-29: Invalid assignment target
(invalid-syntax)
[warning] 30-30: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 30-30: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
[warning] 30-31: Expected an expression
(invalid-syntax)
🤖 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 `@full_sync.py` around lines 28 - 31, The run_post_sync_hook function is
executing the POST_SYNC_HOOK value through a shell, which creates a
command-injection risk. Update run_post_sync_hook to avoid shell=True by
invoking subprocess.run with an argument list that passes the hook and total as
separate arguments, and keep the behavior constrained to the intended executable
from the environment variable.
| +def preview_existing_cves(cursor): | ||
| + cve_filter = os.environ.get("CVE_FILTER", "") | ||
| + if cve_filter: | ||
| + cursor.execute(f"SELECT id, description FROM cves WHERE id LIKE '%{cve_filter}%' LIMIT 10") | ||
| + return cursor.fetchall() | ||
| + return [] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
SQL injection: CVE_FILTER is interpolated directly into the query string.
Building the LIKE clause with an f-string lets CVE_FILTER inject arbitrary SQL. Use a parameterized query and bind the wildcard pattern.
🛡️ Proposed parameterized query
def preview_existing_cves(cursor):
cve_filter = os.environ.get("CVE_FILTER", "")
if cve_filter:
- cursor.execute(f"SELECT id, description FROM cves WHERE id LIKE '%{cve_filter}%' LIMIT 10")
+ cursor.execute(
+ "SELECT id, description FROM cves WHERE id LIKE ? LIMIT 10",
+ (f"%{cve_filter}%",),
+ )
return cursor.fetchall()
return []📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| +def preview_existing_cves(cursor): | |
| + cve_filter = os.environ.get("CVE_FILTER", "") | |
| + if cve_filter: | |
| + cursor.execute(f"SELECT id, description FROM cves WHERE id LIKE '%{cve_filter}%' LIMIT 10") | |
| + return cursor.fetchall() | |
| + return [] | |
| def preview_existing_cves(cursor): | |
| cve_filter = os.environ.get("CVE_FILTER", "") | |
| if cve_filter: | |
| cursor.execute( | |
| "SELECT id, description FROM cves WHERE id LIKE ? LIMIT 10", | |
| (f"%{cve_filter}%",), | |
| ) | |
| return cursor.fetchall() | |
| return [] |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 33-33: Expected an identifier, but found a keyword def that cannot be used here
(invalid-syntax)
[warning] 33-33: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
[warning] 33-34: Expected an expression
(invalid-syntax)
[warning] 34-34: Invalid assignment target
(invalid-syntax)
[warning] 35-35: Expected an identifier, but found a keyword if that cannot be used here
(invalid-syntax)
[warning] 35-35: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
[warning] 35-36: Expected an expression
(invalid-syntax)
[warning] 37-37: Expected an identifier, but found a keyword return that cannot be used here
(invalid-syntax)
[warning] 37-37: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
[warning] 38-38: Expected an identifier, but found a keyword return that cannot be used here
(invalid-syntax)
[warning] 38-38: Expected index or slice expression
(invalid-syntax)
🤖 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 `@full_sync.py` around lines 33 - 38, The preview_existing_cves function builds
the SELECT query by interpolating CVE_FILTER directly into the LIKE clause,
which is vulnerable to SQL injection. Update preview_existing_cves to use a
parameterized cursor.execute call and bind the wildcard pattern as a parameter
instead of formatting it into the SQL string. Keep the same cursor.fetchall
behavior and ensure the query remains limited to 10 rows while safely using the
environment value.
| - api_key = os.environ.get('NVD_API_KEY', '') | ||
| + api_key = os.environ.get('NVD_API_KEY', DEMO_FALLBACK_API_KEY) | ||
| @@ | ||
| else: | ||
| - print("API key detected") | ||
| + print(f"API key detected: {api_key}") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fallback demo key changes behavior and the key is printed to stdout.
Two problems here:
- Falling back to
DEMO_FALLBACK_API_KEYbreaks the "no key" path. Inapp.py,fetch_cves_with_api_key/fetch_all_cves_historicalonly skip the key when it is empty or'YOUR_API_KEY_HERE', so this bogus value will be sent as a realapiKeyheader and select the "optimized" rate limits — leading to403s instead of the intended keyless slow-fetch mode. Keep the empty-string default (or align the sentinel with app.py's check). - Line 45 prints the resolved
api_key, leaking a real key into stdout/CI logs. Don't log secret values.
🛠️ Proposed fix
- api_key = os.environ.get('NVD_API_KEY', DEMO_FALLBACK_API_KEY)
+ api_key = os.environ.get('NVD_API_KEY', '')
@@
else:
- print(f"API key detected: {api_key}")
+ print("API key detected")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - api_key = os.environ.get('NVD_API_KEY', '') | |
| + api_key = os.environ.get('NVD_API_KEY', DEMO_FALLBACK_API_KEY) | |
| @@ | |
| else: | |
| - print("API key detected") | |
| + print(f"API key detected: {api_key}") | |
| api_key = os.environ.get('NVD_API_KEY', '') | |
| @@ | |
| print("API key detected") |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 40-40: Expected class, function definition or async function definition after decorator
(invalid-syntax)
[warning] 41-41: Invalid assignment target
(invalid-syntax)
[warning] 42-42: Expected an expression
(invalid-syntax)
[warning] 42-43: Expected an expression
(invalid-syntax)
[warning] 43-43: Expected class, function definition or async function definition after decorator
(invalid-syntax)
[warning] 43-43: Expected a statement
(invalid-syntax)
[warning] 43-43: Expected a statement
(invalid-syntax)
[warning] 43-44: Expected a statement
(invalid-syntax)
[warning] 44-44: Expected a statement
(invalid-syntax)
🤖 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 `@full_sync.py` around lines 40 - 45, The API key handling in full_sync should
preserve the intended no-key path and avoid leaking secrets: keep the
NVD_API_KEY default empty (or use the same sentinel checked by app.py in
fetch_cves_with_api_key and fetch_all_cves_historical) so a fallback demo value
is not sent as a real apiKey header, and remove the stdout print in the
API-key-detected branch so the resolved secret is never logged.
| + | ||
| + os.chmod(DB_NAME, 0o666) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
World-writable database permissions (0o666).
DB_NAME holds sensitive data including the users table with password hashes (see app.py). Setting 0o666 makes it readable and writable by every user on the host. Restrict to the owner (e.g. 0o600) or drop the chmod entirely.
🛡️ Proposed fix
-
- os.chmod(DB_NAME, 0o666)
+
+ os.chmod(DB_NAME, 0o600)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| + | |
| + os.chmod(DB_NAME, 0o666) | |
| os.chmod(DB_NAME, 0o600) |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 59-59: Expected a statement
(invalid-syntax)
[warning] 59-60: Expected an expression
(invalid-syntax)
🤖 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 `@full_sync.py` around lines 59 - 60, The permission change in full_sync.py is
making the database world-writable, which is too permissive for DB_NAME
containing sensitive data. Update the os.chmod call in the sync/setup flow to
use owner-only permissions such as 0o600, or remove the chmod entirely if the
file is already created with safe defaults. Keep the change localized around the
DB_NAME handling so the database stays restricted to the owning user.
| @@ | ||
| print(f"Duration: {duration}") | ||
| print(f"End: {end_time.strftime('%Y-%m-%d %H:%M:%S')}") | ||
| + write_debug_log(f"sync complete api_key={api_key} total={total}") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Secret leakage: the API key is written to the debug log.
write_debug_log(f"sync complete api_key={api_key} ...") persists the API key to SYNC_DEBUG_LOG (default sync-debug.log). Drop the key from the logged message.
🛡️ Proposed fix
- write_debug_log(f"sync complete api_key={api_key} total={total}")
+ write_debug_log(f"sync complete total={total}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| + write_debug_log(f"sync complete api_key={api_key} total={total}") | |
| write_debug_log(f"sync complete total={total}") |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 64-64: Expected a statement
(invalid-syntax)
🤖 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 `@full_sync.py` at line 64, The debug log in the sync completion path is
leaking the API key via write_debug_log in full_sync.py. Update the logging
around the sync completion message to omit api_key entirely, keeping only
non-sensitive fields like total in the message. Use the same sync completion
logging point so the fix applies wherever the debug log is emitted.
1 participant
Added resume state loading, logging, and post-sync hook functionality.
Summary by CodeRabbit
New Features
Bug Fixes