Conversation
|
DeputyDev has started reviewing your pull request. |
![deputydev-agent[bot]](https://avatars.githubusercontent.com/u/182096911?s=60&v=4){kind=small}
| "verification": "PHARMACY" | ||
| }, | ||
| "AUTHORIZATION": { | ||
| "CORPORATE": "Basic VkN6SjkxZVQ3SkV6R2sxdmtMblZkb092M1dHUVR2RXZ2ekVUVlJBNHZsTTo=", |
There was a problem hiding this comment.
SECURITY: Hardcoded Base64 encoded authorization tokens found in the WhatsApp provider configuration. These appear to be Basic Authentication credentials which contain encoded username:password pairs.
Hardcoded credentials in source code present a significant security risk as they could be extracted by anyone with access to the codebase. These credentials could potentially be used to gain unauthorized access to the WhatsApp API. Base64 encoding is easily reversible and does not provide security. Credentials should be stored in environment variables or a secure credential management system rather than in code.
AUTHORIZATION = {
"CORPORATE": "${CORPORATE_AUTH_TOKEN}",
"DEFAULT": "${DEFAULT_AUTH_TOKEN}",
"DIAGNOSTICS": "${DIAGNOSTICS_AUTH_TOKEN}",
"PHARMACY": "${PHARMACY_AUTH_TOKEN}"
}
| request.ctx.user = user_result.get("email") | ||
|
|
||
| @classmethod | ||
| def validate_roles(cls, user_object, roles): |
There was a problem hiding this comment.
SECURITY: The role validation logic has a flaw where it first checks if the user has any app role but then doesn't verify the permissions within those specific apps.
The current implementation has a potential authorization bypass issue. It first checks if the user has roles for any of the apps, but then proceeds to check roles across all apps even if the user doesn't have roles for that specific app. This could allow a user with some app role but incorrect permissions to gain access. The validation should first confirm the user has roles for the specific apps and then check the permissions within those specific apps.
async def validate_roles(cls, user_object, roles):
if user_object is None:
raise UnAuthorizeException("user is not authorized")
# Check if user has any of the required app roles
app_roles_found = [app for app in [cls.APP_NAME, cls.LARA_APP] if app in user_object["roles"]]
if not app_roles_found:
raise UnAuthorizeException("user is not authorized")
# Check if user has any of the required roles
authenticated = False
for app in app_roles_found:
for user_role in user_object["roles"].get(app, []):
if user_role in roles:
authenticated = True
break
if authenticated:
break
if not authenticated:
raise ForbiddenException("user is not permitted")
| }, | ||
| "TEST_ENVIRONMENT": false, | ||
| "TEST_ALLOWED_EMAILS": [], | ||
| "TEST_ENVIRONMENT": true, |
There was a problem hiding this comment.
SECURITY: The configuration template sets TEST_ENVIRONMENT to true and contains a specific email in TEST_ALLOWED_EMAILS which could lead to security bypasses in production.
Setting TEST_ENVIRONMENT to true in the configuration template creates a risk that production environments could accidentally run with test settings enabled. This could bypass security controls designed for production use. Additionally, including specific test email addresses in the template could lead to unintended access in production environments if the template is not properly customized. Default configurations should always use the most secure settings, requiring explicit relaxation for test environments.
"TEST_ENVIRONMENT": false,
"TEST_ALLOWED_EMAILS": [],
|
DeputyDev has completed a review of your pull request for commit 3aed402. |
| from commonutils.utils import CustomEnum | ||
|
|
||
|
|
||
| class EventPriority(CustomEnum): |
| :return: None | ||
| """ | ||
| message = context.get("message") | ||
| if message not in ["Unclosed connector", "Unclosed client session"]: |
There was a problem hiding this comment.
Need to check if this for excluding these only.
| return request_body["source_identifier"] | ||
| def get_source_identifier_for_event(cls, event_name, request_body): | ||
| # TODO add more logic for other apps of needed | ||
| source_identifier = None |
| APNS = { | ||
| "name": "Apple Push Notification Service", | ||
| "code": "APNS", | ||
| "logo": "https://raw.githubusercontent.com/tata1mg/notifyone-core/refs/heads/notifyone/issues/14/media/logo/apns.png", |
| "CAPACITY": 0, | ||
| "AUTHORIZATION_TOKEN": "" | ||
| "APP_AUTHORIZATIONS": { | ||
| "corporate-service": "CORPORATE", |
There was a problem hiding this comment.
Should be in config and not part of code.
| @email_blueprint.route("/email/subtemplate/<id:int>", methods=["PUT"], name="update_email_subtemplate") | ||
| async def update_email_subtemplate(request: Request, id: int): | ||
| payload = request.custom_json() | ||
| user_email = "admin@ns.com" |
| @event_blueprint.route("/event/<event_id:int>", methods=["PUT"], name="update_event") | ||
| async def update_event(request: Request, event_id: int): | ||
| data = request.custom_json() | ||
| data["user_email"] = "temp@ns.com" |
|
|
||
| push_apis = Blueprint("PushAPIs") | ||
|
|
||
|
|
|
|
||
| sms_apis = Blueprint("SmsAPIs") | ||
|
|
||
|
|
| from redis_wrapper import RegisterRedis | ||
|
|
||
|
|
||
| from redis_wrapper import RegisterRedis |
2 participants
DeputyDev generated PR summary:
Size XL: This PR changes include 635 lines and should take approximately 3-6 hours to review
The pull request titled "Core updates" involves several significant changes to the codebase:
Dependency Updates:
Pipfile, changing version numbers and switching the Git protocol from HTTPS to SSH for accessing certain repositories.Constants and Configurations:
Code Enhancements:
Database and Model Changes:
typefor push notifications andcontent_lengthfor logs.Service and Client Integration:
Logging and Error Handling:
Utilities and Helpers:
Template and Styling Updates:
Overall, the pull request introduces significant enhancements and updates to dependencies, configurations, service clients, database models, utilities, and templates.
DeputyDev generated PR summary until 3aed402