fix(codeql): Resolve static analysis security alerts#100
Merged
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR systematically addresses CodeQL static analysis security alerts across ERROR, WARNING, and NOTE severity levels. The changes focus on fixing uninitialized variables, removing dead code, eliminating unused imports/variables, and standardizing logging patterns to use lazy formatting.
Key Changes
- Fixed critical uninitialized variable issues in test files by adding explicit initialization
- Removed unreachable dead code blocks in visualization and evaluation modules
- Standardized logging across the codebase to use
%splaceholder formatting instead of f-strings for better performance - Applied proper context manager patterns and cleaned up unused variable assignments in tests
- Updated dependencies (urllib3 for security, kubernetes downgrade)
Reviewed changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Dependency version updates for urllib3 (2.5.0) and kubernetes (33.1.0) |
| pyproject.toml | Added explicit urllib3>=2.5.0 requirement for CVE fixes |
| tests/e2e/test_docker_health.py | Initialize dict variables before try blocks to satisfy static analysis |
| tests/unit/cli/test_init.py | Removed unused return value assignment from procedure callback |
| tests/unit/cli/test_similarity_commands.py | Fixed import-and-import-from pattern by using module imports |
| tests/unit/retrieval/test_api_helpers.py | Removed unused result variable, improved comment clarity |
| tests/unit/data_processing/test_hpo_database.py | Applied context manager pattern for database fixtures |
| tests/unit/retrieval/test_dense_retriever_real.py | Added descriptive alias for chromadb import used in mocking |
| tests/e2e/test_docker_security.py | Simplified PIDs limit assertion logic |
| tests/unit/cli/test_index_commands.py | Removed unused mock variables |
| tests/unit/cli/test_data_commands.py | Removed unused mock variables |
| tests/unit/cli/test_cli_performance.py | Made string concatenation explicit with parentheses |
| tests/unit/api/test_dependencies_model_loading.py | Properly captured reload module result |
| api/main.py | Used imported CORS and logging config values instead of hardcoded literals |
| api/dependencies.py | Converted logging to use lazy %s formatting |
| api/routers/*.py | Converted logging to use lazy %s formatting throughout |
| phentrieve/cli/text_commands.py | Removed redundant logging import inside function |
| phentrieve/visualization/plot_utils.py | Removed unreachable condition (num_models==1 inside num_models>1 block) and converted logging to %s format |
| phentrieve/evaluation/semantic_metrics.py | Removed dead code block with if False and ... condition, converted logging to %s format |
| phentrieve/evaluation/metrics.py | Converted logging to use lazy %s formatting |
| phentrieve/retrieval/*.py | Converted logging to use lazy %s formatting |
| phentrieve/text_processing/*.py | Converted logging to use lazy %s formatting |
| frontend/vite-icon-optimizer.js | Removed redundant variable assignment |
| frontend/src/services/logService.js | Changed to proper re-export pattern for useLogStore |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # (dense_retriever imports it locally inside functions) | ||
| import chromadb # noqa: F401 | ||
| # The import is used for mocking PersistentClient in tests below | ||
| import chromadb as _chromadb_for_mocking # noqa: F401 |
There was a problem hiding this comment.
Import of '_chromadb_for_mocking' is not used.
## Log Injection Fixes (py/log-injection) - Convert all f-string logging to %-style formatting across 15+ files - Fixed in: api/dependencies.py, api/routers/*.py, phentrieve/retrieval/*.py, phentrieve/text_processing/*.py, phentrieve/evaluation/*.py ## Code Quality Fixes - Fix py/redundant-comparison in plot_utils.py (4) and test_docker_security.py (1) - Fix py/implicit-string-concatenation-in-list in test_cli_performance.py - Fix py/unused-local-variable in test files (removed unused mock assignments) - Fix py/unused-import in test_dense_retriever_real.py (clarify chromadb import) - Fix py/uninitialized-local-variable in test_docker_health.py ## JavaScript Fixes - Fix js/useless-assignment-to-local in vite-icon-optimizer.js - Fix js/unused-local-variable in logService.js (convert to re-export) ## Security Updates - Update urllib3>=2.5.0 for CVE fixes (GHSA-48p4-8xcf-vxj5, GHSA-pq67-6m6q-mj2v) All tests pass (580 passed), 0 lint errors, 0 type errors.
berntpopp
force-pushed
the
fix/codeql-security-alerts
branch
from
b65f415 to
6f9814e
Compare
Implement OWASP-compliant log sanitization across all modules to address CodeQL py/log-injection alerts. This prevents CWE-117 log forging attacks. Changes: - Add sanitize_log_value() utility in phentrieve/utils.py that: - Removes CRLF characters (\r\n, \r, \n) - Replaces tabs with spaces for readability - Removes non-printable control characters (< ASCII 32) - Truncates to max 500 chars to prevent log flooding - Apply sanitization to user-provided values in log statements across: - api/dependencies.py (29 statements) - api/routers/*.py (34 statements) - phentrieve/retrieval/*.py (5 files) - phentrieve/text_processing/*.py (4 files) - phentrieve/evaluation/*.py (2 files) References: - https://owasp.org/www-community/attacks/Log_Injection - https://codeql.github.com/codeql-query-help/python/py-log-injection/
- test_hpo_database.py: Use context managers for HPODatabase (py/should-use-with) - test_docker_health.py: Add explicit raise after pytest.fail (py/uninitialized-local-variable) - test_similarity_commands.py: Use consistent import style (py/import-and-import-from) These fixes address CodeQL alerts related to: - CWE-404: Resource leak prevention with context managers - Static analysis flow control clarity - Import hygiene
- similarity_router.py: Sanitize error_detail containing user HPO terms - text_processing_router.py: Sanitize numeric config values in debug logs - test_dense_retriever_real.py: Add explicit chromadb reference for static analysis This fixes the 6 NEW alerts identified by CodeQL in this PR: - 5 high-severity log injection vulnerabilities - 1 unused import warning
Configure ALLOWED_ORIGINS environment variable for phentrieve_api in docker-compose.dev.yml to enable frontend-API communication during local Docker development (ports 8080, 8001, 5734, 8734).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes Made
ERROR Fixes (Critical)
tests/e2e/test_docker_health.py: Initializedata: dict = {}before try blocks to prevent uninitialized variable accessWARNING Fixes
phentrieve/evaluation/semantic_metrics.py: Remove dead code block (if False and ...)phentrieve/visualization/plot_utils.py: Remove impossible condition (if num_models==1insideif num_models>1block)tests/unit/cli/test_init.py: Don't assign procedure return valueNOTE Fixes (Code Quality)
api/main.py: Use imported config values (CORS settings, LOG_LEVEL)phentrieve/cli/text_commands.py: Remove redundant logging importtests/unit/cli/test_similarity_commands.py: Fix import-and-import-from issuestests/unit/api/test_dependencies_model_loading.py: Properly assign reload resulttests/unit/data_processing/test_hpo_database.py: Use context managers for DB fixturestests/unit/retrieval/test_dense_retriever_real.py: Add descriptive alias for mocking importNot Fixed (Informational)
Test plan
make check- Format and lint checks passmake typecheck-fast- Type checking passes (0 errors)