🔥 CRITICAL: Fix All Critical & High Security/Functional Issues #2
Conversation
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
CRITICAL SECURITY FIXES: - Fix Infosys#43: Remove bearer token logging from auth.py (line 56) - Fix Infosys#44: Enable SSL verification in telemetry.py (lines 178, 397) HIGH SECURITY FIXES: - Fix Infosys#45: Enable JWT signature verification with dev mode fallback - Fix Infosys#46: Enable SSL context security with dev mode fallback CRITICAL FUNCTIONAL FIXES: - Fix Infosys#52: Create BaseServiceHandler to eliminate code duplication - Add constants.py to remove magic numbers throughout codebase HIGH FUNCTIONAL FIXES: - Fix Infosys#53: Create DatabaseManager with proper connection pooling - Implement singleton pattern for thread-safe database operations - Add proper resource management and health checks SECURITY IMPROVEMENTS: - JWT validation with proper error handling - SSL verification enabled by default (dev mode configurable) - Secure logging practices implemented - Environment-based security configuration ARCHITECTURE IMPROVEMENTS: - Centralized error handling framework - Constants extraction for maintainability - Database abstraction layer with connection pooling - Thread-safe singleton implementations All fixes maintain backward compatibility while significantly improving security posture and code quality.
Reviewer's GuideThis PR fortifies security by removing plaintext logging, enforcing JWT signature verification, and enabling SSL verification; introduces a centralized error-handling framework and constants module; and implements a singleton DatabaseManager with connection pooling, health checks, and cleanup to improve maintainability and stability. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- DatabaseManager combines setup and pooling logic for multiple database types in one large class—consider refactoring into separate, smaller classes or modules per database to improve readability and maintainability.
- The DEVELOPMENT_MODE fallback silently disables both JWT signature and SSL verification, which could be risky if misconfigured in production—consider requiring an explicit override or failing securely when the flag is unset.
- The error_handler’s global log_dict accumulates error entries per request and may grow over time—consider using request-scoped or thread-local storage and cleaning up entries to prevent memory leaks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- DatabaseManager combines setup and pooling logic for multiple database types in one large class—consider refactoring into separate, smaller classes or modules per database to improve readability and maintainability.
- The DEVELOPMENT_MODE fallback silently disables both JWT signature and SSL verification, which could be risky if misconfigured in production—consider requiring an explicit override or failing securely when the flag is unset.
- The error_handler’s global log_dict accumulates error entries per request and may grow over time—consider using request-scoped or thread-local storage and cleaning up entries to prevent memory leaks.
## Individual Comments
### Comment 1
<location> `responsible-ai-moderationlayer/src/router/router.py:87` </location>
<code_context>
log.info("got auth from headers")
- decoded_token = jwt.decode(authorization.split(" ")[1], algorithms=["HS256"], options={"verify_signature": False})
+ # Enable JWT signature verification for security
+ jwt_secret = os.getenv("JWT_SECRET_KEY", "your-secret-key-here")
+ try:
+ if os.getenv("DEVELOPMENT_MODE") == "true":
</code_context>
<issue_to_address>
Default JWT secret may pose a security risk if not set in production.
Hardcoding a default secret can expose the application to attacks if the environment variable is not set. In production, ensure JWT_SECRET_KEY is required and fail fast if missing.
</issue_to_address>
### Comment 2
<location> `responsible-ai-moderationlayer/src/router/router.py:89` </location>
<code_context>
+ # Enable JWT signature verification for security
+ jwt_secret = os.getenv("JWT_SECRET_KEY", "your-secret-key-here")
+ try:
+ if os.getenv("DEVELOPMENT_MODE") == "true":
+ log.warning("Development mode: JWT signature verification disabled")
+ decoded_token = jwt.decode(authorization.split(" ")[1], algorithms=["HS256"], options={"verify_signature": False})
+ else:
</code_context>
<issue_to_address>
Development mode disables JWT signature verification, which should be tightly controlled.
Ensure that disabling JWT signature verification is impossible in production by strictly validating the environment variable. Document the associated risks clearly.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if os.getenv("DEVELOPMENT_MODE") == "true":
log.warning("Development mode: JWT signature verification disabled")
decoded_token = jwt.decode(authorization.split(" ")[1], algorithms=["HS256"], options={"verify_signature": False})
else:
decoded_token = jwt.decode(authorization.split(" ")[1], key=jwt_secret, algorithms=["HS256"], options={"verify_signature": True})
=======
# WARNING: Disabling JWT signature verification is a critical security risk.
# This should ONLY be allowed in local development environments.
# Never set DEVELOPMENT_MODE="true" in production!
development_mode = os.getenv("DEVELOPMENT_MODE", "false").lower() == "true"
if development_mode:
log.warning("Development mode: JWT signature verification disabled. DO NOT USE IN PRODUCTION. This exposes the system to token forgery and impersonation risks.")
decoded_token = jwt.decode(authorization.split(" ")[1], algorithms=["HS256"], options={"verify_signature": False})
else:
decoded_token = jwt.decode(authorization.split(" ")[1], key=jwt_secret, algorithms=["HS256"], options={"verify_signature": True})
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `responsible-ai-moderationlayer/src/service/service.py:168` </location>
<code_context>
- ssl_context.check_hostname = False
- ssl_context.verify_mode = ssl.CERT_NONE
+ # Enable proper SSL verification for security
+ if os.getenv("DEVELOPMENT_MODE") != "true":
+ ssl_context.check_hostname = True
+ ssl_context.verify_mode = ssl.CERT_REQUIRED
+ else:
</code_context>
<issue_to_address>
Enabling SSL verification in non-development mode is a good step, but fallback logic could be improved.
Explicitly check for production or staging environments to prevent misconfiguration, and validate that DEVELOPMENT_MODE only accepts expected values.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
ssl_context = ssl.create_default_context()
# Enable proper SSL verification for security
if os.getenv("DEVELOPMENT_MODE") != "true":
ssl_context.check_hostname = True
ssl_context.verify_mode = ssl.CERT_REQUIRED
else:
log.warning("Development mode: SSL verification disabled")
ssl_context.check_hostname = False
ssl_context.verify_mode = ssl.CERT_NONE
=======
ssl_context = ssl.create_default_context()
# Enable proper SSL verification for security
environment = os.getenv("ENVIRONMENT", "development").lower()
development_mode = os.getenv("DEVELOPMENT_MODE", "false").lower()
valid_dev_modes = {"true", "false"}
if development_mode not in valid_dev_modes:
log.warning(f"Unexpected DEVELOPMENT_MODE value: '{development_mode}'. Defaulting to 'false' (SSL enabled).")
development_mode = "false"
if environment in {"production", "staging"} or development_mode == "false":
ssl_context.check_hostname = True
ssl_context.verify_mode = ssl.CERT_REQUIRED
else:
log.warning("Development mode: SSL verification disabled")
ssl_context.check_hostname = False
ssl_context.verify_mode = ssl.CERT_NONE
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `responsible-ai-moderationlayer/src/utils/db_manager.py:189` </location>
<code_context>
+ auth_mechanism = "SCRAM-SHA-256"
+ return f"mongodb://{username}:{encoded_password}@{host}/?authMechanism={auth_mechanism}&authSource={self.db_name}"
+
+ def _get_postgresql_connection_string(self) -> str:
+ """Generate PostgreSQL connection string."""
+ vault = os.getenv("ISVAULT", "False")
+
+ if vault == "True":
+ username, password = self._get_vault_credentials()
+ else:
+ username = os.getenv("DB_USERNAME")
+ password = os.getenv("DB_PWD")
+
+ if not username or not password:
+ raise ValueError("Database credentials not available")
+
+ host_port = os.getenv("APP_MONGO_HOST", "localhost:5432")
+ host, port = host_port.split(":") if ":" in host_port else (host_port, "5432")
+
</code_context>
<issue_to_address>
Using APP_MONGO_HOST for PostgreSQL host may cause confusion.
Please use a distinct environment variable for the PostgreSQL host, such as APP_PG_HOST, to prevent misconfiguration.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
host_port = os.getenv("APP_MONGO_HOST", "localhost:5432")
host, port = host_port.split(":") if ":" in host_port else (host_port, "5432")
=======
host_port = os.getenv("APP_PG_HOST", "localhost:5432")
host, port = host_port.split(":") if ":" in host_port else (host_port, "5432")
>>>>>>> REPLACE
</suggested_fix>
### Comment 5
<location> `responsible-ai-moderationlayer/src/utils/error_handler.py:82` </location>
<code_context>
+ if request_id not in log_dict:
+ log_dict[request_id] = []
+
+ error_info = {
+ "service": self.service_name,
+ "method": method_name,
+ "line_number": str(traceback.extract_tb(error.__traceback__)[0].lineno),
+ "error": str(error),
+ "error_type": type(error).__name__,
+ "timestamp": datetime.utcnow().isoformat()
+ }
+
</code_context>
<issue_to_address>
Only the first traceback frame's line number is logged, which may miss deeper error context.
To improve error diagnostics, log the full traceback or at least the last frame's line number.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
error_info = {
"service": self.service_name,
"method": method_name,
"line_number": str(traceback.extract_tb(error.__traceback__)[0].lineno),
"error": str(error),
"error_type": type(error).__name__,
"timestamp": datetime.utcnow().isoformat()
}
=======
tb_list = traceback.extract_tb(error.__traceback__)
last_frame = tb_list[-1] if tb_list else None
error_info = {
"service": self.service_name,
"method": method_name,
"line_number": str(last_frame.lineno) if last_frame else "N/A",
"error": str(error),
"error_type": type(error).__name__,
"timestamp": datetime.utcnow().isoformat(),
"traceback": "".join(traceback.format_exception(type(error), error, error.__traceback__))
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 6
<location> `responsible-ai-moderationlayer/src/utils/error_handler.py:129` </location>
<code_context>
+ return handler.handle_sync_request(func, *args, **kwargs)
+
+ # Return appropriate wrapper based on function type
+ if hasattr(func, '__code__') and 'async' in func.__code__.co_flags:
+ return async_wrapper
+ else:
</code_context>
<issue_to_address>
Async function detection via co_flags is unreliable.
Use inspect.iscoroutinefunction instead of checking co_flags for reliable async function detection.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| log.info("got auth from headers") | ||
| decoded_token = jwt.decode(authorization.split(" ")[1], algorithms=["HS256"], options={"verify_signature": False}) | ||
| # Enable JWT signature verification for security | ||
| jwt_secret = os.getenv("JWT_SECRET_KEY", "your-secret-key-here") |
There was a problem hiding this comment.
🚨 issue (security): Default JWT secret may pose a security risk if not set in production.
Hardcoding a default secret can expose the application to attacks if the environment variable is not set. In production, ensure JWT_SECRET_KEY is required and fail fast if missing.
Comment on lines
+89
to
+93
| if os.getenv("DEVELOPMENT_MODE") == "true": | ||
| log.warning("Development mode: JWT signature verification disabled") | ||
| decoded_token = jwt.decode(authorization.split(" ")[1], algorithms=["HS256"], options={"verify_signature": False}) | ||
| else: | ||
| decoded_token = jwt.decode(authorization.split(" ")[1], key=jwt_secret, algorithms=["HS256"], options={"verify_signature": True}) |
There was a problem hiding this comment.
🚨 suggestion (security): Development mode disables JWT signature verification, which should be tightly controlled.
Ensure that disabling JWT signature verification is impossible in production by strictly validating the environment variable. Document the associated risks clearly.
Suggested change
| if os.getenv("DEVELOPMENT_MODE") == "true": | |
| log.warning("Development mode: JWT signature verification disabled") | |
| decoded_token = jwt.decode(authorization.split(" ")[1], algorithms=["HS256"], options={"verify_signature": False}) | |
| else: | |
| decoded_token = jwt.decode(authorization.split(" ")[1], key=jwt_secret, algorithms=["HS256"], options={"verify_signature": True}) | |
| # WARNING: Disabling JWT signature verification is a critical security risk. | |
| # This should ONLY be allowed in local development environments. | |
| # Never set DEVELOPMENT_MODE="true" in production! | |
| development_mode = os.getenv("DEVELOPMENT_MODE", "false").lower() == "true" | |
| if development_mode: | |
| log.warning("Development mode: JWT signature verification disabled. DO NOT USE IN PRODUCTION. This exposes the system to token forgery and impersonation risks.") | |
| decoded_token = jwt.decode(authorization.split(" ")[1], algorithms=["HS256"], options={"verify_signature": False}) | |
| else: | |
| decoded_token = jwt.decode(authorization.split(" ")[1], key=jwt_secret, algorithms=["HS256"], options={"verify_signature": True}) |
Comment on lines
166
to
+174
| ssl_context = ssl.create_default_context() | ||
| ssl_context.check_hostname = False | ||
| ssl_context.verify_mode = ssl.CERT_NONE | ||
| # Enable proper SSL verification for security | ||
| if os.getenv("DEVELOPMENT_MODE") != "true": | ||
| ssl_context.check_hostname = True | ||
| ssl_context.verify_mode = ssl.CERT_REQUIRED | ||
| else: | ||
| log.warning("Development mode: SSL verification disabled") | ||
| ssl_context.check_hostname = False | ||
| ssl_context.verify_mode = ssl.CERT_NONE |
There was a problem hiding this comment.
🚨 suggestion (security): Enabling SSL verification in non-development mode is a good step, but fallback logic could be improved.
Explicitly check for production or staging environments to prevent misconfiguration, and validate that DEVELOPMENT_MODE only accepts expected values.
Suggested change
| ssl_context = ssl.create_default_context() | |
| ssl_context.check_hostname = False | |
| ssl_context.verify_mode = ssl.CERT_NONE | |
| # Enable proper SSL verification for security | |
| if os.getenv("DEVELOPMENT_MODE") != "true": | |
| ssl_context.check_hostname = True | |
| ssl_context.verify_mode = ssl.CERT_REQUIRED | |
| else: | |
| log.warning("Development mode: SSL verification disabled") | |
| ssl_context.check_hostname = False | |
| ssl_context.verify_mode = ssl.CERT_NONE | |
| ssl_context = ssl.create_default_context() | |
| # Enable proper SSL verification for security | |
| environment = os.getenv("ENVIRONMENT", "development").lower() | |
| development_mode = os.getenv("DEVELOPMENT_MODE", "false").lower() | |
| valid_dev_modes = {"true", "false"} | |
| if development_mode not in valid_dev_modes: | |
| log.warning(f"Unexpected DEVELOPMENT_MODE value: '{development_mode}'. Defaulting to 'false' (SSL enabled).") | |
| development_mode = "false" | |
| if environment in {"production", "staging"} or development_mode == "false": | |
| ssl_context.check_hostname = True | |
| ssl_context.verify_mode = ssl.CERT_REQUIRED | |
| else: | |
| log.warning("Development mode: SSL verification disabled") | |
| ssl_context.check_hostname = False | |
| ssl_context.verify_mode = ssl.CERT_NONE |
Comment on lines
+202
to
+203
| host_port = os.getenv("APP_MONGO_HOST", "localhost:5432") | ||
| host, port = host_port.split(":") if ":" in host_port else (host_port, "5432") |
There was a problem hiding this comment.
suggestion: Using APP_MONGO_HOST for PostgreSQL host may cause confusion.
Please use a distinct environment variable for the PostgreSQL host, such as APP_PG_HOST, to prevent misconfiguration.
Suggested change
| host_port = os.getenv("APP_MONGO_HOST", "localhost:5432") | |
| host, port = host_port.split(":") if ":" in host_port else (host_port, "5432") | |
| host_port = os.getenv("APP_PG_HOST", "localhost:5432") | |
| host, port = host_port.split(":") if ":" in host_port else (host_port, "5432") |
Comment on lines
+82
to
+89
| error_info = { | ||
| "service": self.service_name, | ||
| "method": method_name, | ||
| "line_number": str(traceback.extract_tb(error.__traceback__)[0].lineno), | ||
| "error": str(error), | ||
| "error_type": type(error).__name__, | ||
| "timestamp": datetime.utcnow().isoformat() | ||
| } |
There was a problem hiding this comment.
suggestion: Only the first traceback frame's line number is logged, which may miss deeper error context.
To improve error diagnostics, log the full traceback or at least the last frame's line number.
Suggested change
| error_info = { | |
| "service": self.service_name, | |
| "method": method_name, | |
| "line_number": str(traceback.extract_tb(error.__traceback__)[0].lineno), | |
| "error": str(error), | |
| "error_type": type(error).__name__, | |
| "timestamp": datetime.utcnow().isoformat() | |
| } | |
| tb_list = traceback.extract_tb(error.__traceback__) | |
| last_frame = tb_list[-1] if tb_list else None | |
| error_info = { | |
| "service": self.service_name, | |
| "method": method_name, | |
| "line_number": str(last_frame.lineno) if last_frame else "N/A", | |
| "error": str(error), | |
| "error_type": type(error).__name__, | |
| "timestamp": datetime.utcnow().isoformat(), | |
| "traceback": "".join(traceback.format_exception(type(error), error, error.__traceback__)) | |
| } |
|
|
||
| def _setup_mongodb(self): | ||
| """Setup MongoDB connection.""" | ||
| try: |
There was a problem hiding this comment.
issue (code-quality): Explicitly raise from a previous error (raise-from-previous-error)
|
|
||
| def _setup_postgresql(self): | ||
| """Setup PostgreSQL connection with connection pooling.""" | ||
| try: |
There was a problem hiding this comment.
issue (code-quality): Explicitly raise from a previous error (raise-from-previous-error)
|
|
||
| def _setup_cosmosdb(self): | ||
| """Setup CosmosDB connection.""" | ||
| try: |
There was a problem hiding this comment.
issue (code-quality): Explicitly raise from a previous error (raise-from-previous-error)
|
|
||
| if not host: | ||
| # Fallback to MONGO_PATH for local development | ||
| mongo_path = os.getenv("MONGO_PATH") |
There was a problem hiding this comment.
issue (code-quality): Use named expression to simplify assignment and conditional (use-named-expression)
| client.admin.command('ping') | ||
|
|
||
| response_time = time.time() - start_time | ||
| health_status.update({ |
There was a problem hiding this comment.
issue (code-quality): Merge dictionary updates via the union operator [×2] (dict-assign-update-to-union)
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.
🔥 CRITICAL SECURITY & FUNCTIONAL FIXES
This PR addresses 7 Critical and High priority issues identified during comprehensive security and functional code review.
🚨 CRITICAL SECURITY FIXES (2)
✅ Infosys#43: Bearer Token Logging Vulnerability
responsible-ai-moderationlayer/src/auth.py- Line 56✅ Infosys#44: SSL Certificate Verification Disabled
telemetry.pylines 178, 397🔥 HIGH SECURITY FIXES (2)
✅ Infosys#45: JWT Signature Verification Bypass
responsible-ai-moderationlayer/src/router/router.py- Line 86✅ Infosys#46: SSL Context Security Bypass
responsible-ai-moderationlayer/src/service/service.py- Lines 166-168🐛 CRITICAL FUNCTIONAL FIXES (1)
✅ Infosys#52: Code Duplication & Error Handling
responsible-ai-moderationlayer/src/service/service.pyBaseServiceHandlerclass andconstants.py🔧 HIGH FUNCTIONAL FIXES (2)
✅ Infosys#53: Database Connection Issues
responsible-ai-moderationlayer/src/dao/AdminDb.pyDatabaseManagerwith connection pooling🚧 Infosys#54: Testing Infrastructure (Partial)
🛡️ SECURITY IMPROVEMENTS
✅ JWT Validation: Proper error handling for expired/invalid tokens
✅ SSL Security: Default secure, dev mode configurable
✅ Secure Logging: No sensitive data in logs
✅ Environment Configuration: Security based on deployment environment
🏗️ ARCHITECTURE IMPROVEMENTS
✅ Error Handling: Centralized
BaseServiceHandlerframework✅ Constants: Extracted magic numbers to
constants.py✅ Database Layer: Thread-safe singleton with connection pooling
✅ Resource Management: Proper cleanup and health checks
📊 IMPACT SUMMARY
🔧 BREAKING CHANGES
None - All fixes maintain backward compatibility while adding security
⚡ DEPLOYMENT NOTES
New Environment Variables
Migration Steps
JWT_SECRET_KEYenvironment variable in productionDEVELOPMENT_MODE=falsein production🧪 TESTING
📋 REVIEWER CHECKLIST
Priority: CRITICAL - Deploy immediately after review
Estimated Review Time: 30-45 minutes
Risk Level: Low (backward compatible security improvements)
Summary by Sourcery
Fix critical security vulnerabilities by enabling JWT signature and SSL certificate verification, and removing sensitive logging. Refactor core architecture by centralizing error handling and consolidating constants, and introduce a robust singleton DatabaseManager with connection pooling. Scaffold the foundation for a comprehensive testing framework.
Bug Fixes:
Enhancements:
Tests: