From b734b9cb9fd4690c974f47370e9e9d6fe65deb52 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 19 Jun 2025 11:03:04 -0700 Subject: [PATCH 1/7] fix input handling --- app/job/rest.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/app/job/rest.py b/app/job/rest.py index 0f80fab2d..457efb812 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -1,7 +1,8 @@ +import re from zoneinfo import ZoneInfo import dateutil -from flask import Blueprint, current_app, jsonify, request +from flask import Blueprint, abort, current_app, jsonify, request from app import db from app.aws.s3 import ( @@ -44,6 +45,15 @@ job_blueprint = Blueprint("job", __name__, url_prefix="/service/", methods=["GET"]) def get_job_by_service_and_job_id(service_id, job_id): job = dao_get_job_by_service_id_and_job_id(service_id, job_id) @@ -194,6 +204,10 @@ def get_recent_notifications_for_service_job(service_id, job_id): @job_blueprint.route("//notification_count", methods=["GET"]) def get_notification_count_for_job_id(service_id, job_id): + if is_suspicious_input(service_id) or is_suspicious_input(job_id): + abort(403) + if not is_valid_id(service_id) or not is_valid_id(job_id): + abort(403) dao_get_job_by_service_id_and_job_id(service_id, job_id) count = dao_get_notification_count_for_job_id(job_id=job_id) return jsonify(count=count), 200 From 69e7244485f4fe004be9b789fb18e40a0c85394a Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 19 Jun 2025 11:12:42 -0700 Subject: [PATCH 2/7] fix input handling --- app/job/rest.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/job/rest.py b/app/job/rest.py index 457efb812..32e845b6a 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -45,12 +45,16 @@ job_blueprint = Blueprint("job", __name__, url_prefix="/service/ Date: Thu, 19 Jun 2025 11:31:37 -0700 Subject: [PATCH 3/7] fix input handling --- app/job/rest.py | 10 ++++++++-- tests/app/job/test_rest.py | 17 +++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/app/job/rest.py b/app/job/rest.py index 32e845b6a..9a7de309c 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -47,7 +47,7 @@ register_errors(job_blueprint) def is_suspicious_input(input_str): if not isinstance(input_str, str): - return True + return False pattern = r"(?i)\b(OR|AND|UNION|SELECT|DROP|INSERT|UPDATE|DELETE|EXEC|TRUNCATE|CREATE|ALTER|--|/\*|\bpg_sleep\b|\bsleep\b)|[';]{2,}" # noqa return bool(re.search(pattern, input_str)) @@ -55,7 +55,7 @@ def is_suspicious_input(input_str): def is_valid_id(id): if not isinstance(id, str): return True - return bool(re.match(r"^[a-zA-Z0-9_-]{1,32}$", id)) + return bool(re.match(r"^[a-zA-Z0-9_-]{1,50}$", id)) @job_blueprint.route("/", methods=["GET"]) @@ -209,8 +209,14 @@ def get_recent_notifications_for_service_job(service_id, job_id): @job_blueprint.route("//notification_count", methods=["GET"]) def get_notification_count_for_job_id(service_id, job_id): if is_suspicious_input(service_id) or is_suspicious_input(job_id): + current_app.logger.error( + f"Service Id {service_id} is suspicious input or job id {job_id} is." + ) abort(403) if not is_valid_id(service_id) or not is_valid_id(job_id): + current_app.logger.error( + f"service id {service_id} or job_id {job_id} is not a valid id" + ) abort(403) dao_get_job_by_service_id_and_job_id(service_id, job_id) count = dao_get_notification_count_for_job_id(job_id=job_id) diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index f65ce2458..e39b24b54 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -16,6 +16,7 @@ from app.enums import ( NotificationType, TemplateType, ) +from app.job.rest import is_suspicious_input, is_valid_id from app.utils import utc_now from tests import create_admin_authorization_header from tests.app.db import ( @@ -586,6 +587,22 @@ def test_get_all_notifications_for_job_returns_correct_format( assert resp["notifications"][0]["status"] == sample_notification_with_job.status +def test_is_valid_id(sample_job): + returnVal = is_valid_id(sample_job.service_id) + assert returnVal is True + + returnVal = is_valid_id("abc pgsleep(1)") + assert returnVal is False + + +def test_is_suspicious_input(sample_job): + returnVal = is_suspicious_input(sample_job.id) + assert returnVal is False + + returnVal = is_suspicious_input("1 OR pg_sleep(1)") + assert returnVal is True + + def test_get_notification_count_for_job_id(admin_request, mocker, sample_job): mock_dao = mocker.patch( "app.job.rest.dao_get_notification_count_for_job_id", return_value=3 From 9bbe504fa3da1e9b6c8d7a57fb31305df7bc123d Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 19 Jun 2025 11:46:40 -0700 Subject: [PATCH 4/7] fix input handling --- app/job/rest.py | 38 ++++++++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/app/job/rest.py b/app/job/rest.py index 9a7de309c..938c19450 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -58,8 +58,17 @@ def is_valid_id(id): return bool(re.match(r"^[a-zA-Z0-9_-]{1,50}$", id)) +def check_suspicious_id(id): + if not is_valid_id(id): + abort(403) + if is_suspicious_input(id): + abort(403) + + @job_blueprint.route("/", methods=["GET"]) def get_job_by_service_and_job_id(service_id, job_id): + check_suspicious_id(service_id) + check_suspicious_id(job_id) job = dao_get_job_by_service_id_and_job_id(service_id, job_id) statistics = dao_get_notification_outcomes_for_job(service_id, job_id) data = JobSchema(session=db.session).dump(job) @@ -73,6 +82,9 @@ def get_job_by_service_and_job_id(service_id, job_id): @job_blueprint.route("//cancel", methods=["POST"]) def cancel_job(service_id, job_id): + check_suspicious_id(service_id) + check_suspicious_id(job_id) + job = dao_get_future_scheduled_job_by_id_and_service_id(job_id, service_id) job.job_status = JobStatus.CANCELLED dao_update_job(job) @@ -82,6 +94,9 @@ def cancel_job(service_id, job_id): @job_blueprint.route("//notifications", methods=["GET"]) def get_all_notifications_for_service_job(service_id, job_id): + check_suspicious_id(service_id) + check_suspicious_id(job_id) + data = notifications_filter_schema.load(request.args) page = data["page"] if "page" in data else 1 page_size = ( @@ -143,6 +158,9 @@ def get_all_notifications_for_service_job(service_id, job_id): @job_blueprint.route("//recent_notifications", methods=["GET"]) def get_recent_notifications_for_service_job(service_id, job_id): + check_suspicious_id(service_id) + check_suspicious_id(job_id) + data = notifications_filter_schema.load(request.args) page = data["page"] if "page" in data else 1 page_size = ( @@ -208,16 +226,9 @@ def get_recent_notifications_for_service_job(service_id, job_id): @job_blueprint.route("//notification_count", methods=["GET"]) def get_notification_count_for_job_id(service_id, job_id): - if is_suspicious_input(service_id) or is_suspicious_input(job_id): - current_app.logger.error( - f"Service Id {service_id} is suspicious input or job id {job_id} is." - ) - abort(403) - if not is_valid_id(service_id) or not is_valid_id(job_id): - current_app.logger.error( - f"service id {service_id} or job_id {job_id} is not a valid id" - ) - abort(403) + check_suspicious_id(service_id) + check_suspicious_id(job_id) + dao_get_job_by_service_id_and_job_id(service_id, job_id) count = dao_get_notification_count_for_job_id(job_id=job_id) return jsonify(count=count), 200 @@ -225,6 +236,7 @@ def get_notification_count_for_job_id(service_id, job_id): @job_blueprint.route("", methods=["GET"]) def get_jobs_by_service(service_id): + check_suspicious_id(service_id) if request.args.get("limit_days"): try: limit_days = int(request.args["limit_days"]) @@ -258,6 +270,8 @@ def get_jobs_by_service(service_id): @job_blueprint.route("", methods=["POST"]) def create_job(service_id): + check_suspicious_id(service_id) + """Entry point from UI for one-off messages as well as CSV uploads.""" service = dao_fetch_service_by_id(service_id) if not service.active: @@ -277,6 +291,7 @@ def create_job(service_id): ) data["template"] = data.pop("template_id") + check_suspicious_id(data["template"]) template = dao_get_template_by_id(data["template"]) if data.get("valid") != "True": @@ -301,6 +316,7 @@ def create_job(service_id): dao_create_job(job) sender_id = data.get("sender_id") + check_suspicious_id(sender_id) # Kick off job in tasks.py if job.job_status == JobStatus.PENDING: process_job.apply_async( @@ -315,6 +331,8 @@ def create_job(service_id): @job_blueprint.route("/scheduled-job-stats", methods=["GET"]) def get_scheduled_job_stats(service_id): + check_suspicious_id(service_id) + count, soonest_scheduled_for = dao_get_scheduled_job_stats(service_id) return ( jsonify( From 77b3c1afa673a17b3cb8a3b4689ee8d49e848c4f Mon Sep 17 00:00:00 2001 From: Kenneth Kehl Date: Wed, 25 Jun 2025 08:44:39 -0700 Subject: [PATCH 5/7] Update app/job/rest.py Co-authored-by: ccostino --- app/job/rest.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/app/job/rest.py b/app/job/rest.py index 938c19450..3e06aeb53 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -58,11 +58,12 @@ def is_valid_id(id): return bool(re.match(r"^[a-zA-Z0-9_-]{1,50}$", id)) -def check_suspicious_id(id): - if not is_valid_id(id): - abort(403) - if is_suspicious_input(id): - abort(403) +def check_suspicious_id(*args): + for id in args: + if not is_valid_id(id): + abort(403) + if is_suspicious_input(id): + abort(403) @job_blueprint.route("/", methods=["GET"]) From 5b2d8e064855c6bb0f9f7b9cb14fcdcf41eb3da4 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 25 Jun 2025 08:54:44 -0700 Subject: [PATCH 6/7] code review feedback --- app/job/rest.py | 15 +++++---------- tests/app/job/test_rest.py | 12 +++++++++++- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/app/job/rest.py b/app/job/rest.py index 3e06aeb53..5189c6917 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -68,8 +68,7 @@ def check_suspicious_id(*args): @job_blueprint.route("/", methods=["GET"]) def get_job_by_service_and_job_id(service_id, job_id): - check_suspicious_id(service_id) - check_suspicious_id(job_id) + check_suspicious_id(service_id, job_id) job = dao_get_job_by_service_id_and_job_id(service_id, job_id) statistics = dao_get_notification_outcomes_for_job(service_id, job_id) data = JobSchema(session=db.session).dump(job) @@ -83,8 +82,7 @@ def get_job_by_service_and_job_id(service_id, job_id): @job_blueprint.route("//cancel", methods=["POST"]) def cancel_job(service_id, job_id): - check_suspicious_id(service_id) - check_suspicious_id(job_id) + check_suspicious_id(service_id, job_id) job = dao_get_future_scheduled_job_by_id_and_service_id(job_id, service_id) job.job_status = JobStatus.CANCELLED @@ -95,8 +93,7 @@ def cancel_job(service_id, job_id): @job_blueprint.route("//notifications", methods=["GET"]) def get_all_notifications_for_service_job(service_id, job_id): - check_suspicious_id(service_id) - check_suspicious_id(job_id) + check_suspicious_id(service_id, job_id) data = notifications_filter_schema.load(request.args) page = data["page"] if "page" in data else 1 @@ -159,8 +156,7 @@ def get_all_notifications_for_service_job(service_id, job_id): @job_blueprint.route("//recent_notifications", methods=["GET"]) def get_recent_notifications_for_service_job(service_id, job_id): - check_suspicious_id(service_id) - check_suspicious_id(job_id) + check_suspicious_id(service_id, job_id) data = notifications_filter_schema.load(request.args) page = data["page"] if "page" in data else 1 @@ -227,8 +223,7 @@ def get_recent_notifications_for_service_job(service_id, job_id): @job_blueprint.route("//notification_count", methods=["GET"]) def get_notification_count_for_job_id(service_id, job_id): - check_suspicious_id(service_id) - check_suspicious_id(job_id) + check_suspicious_id(service_id, job_id) dao_get_job_by_service_id_and_job_id(service_id, job_id) count = dao_get_notification_count_for_job_id(job_id=job_id) diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index e39b24b54..dbba6d729 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -5,6 +5,7 @@ from unittest.mock import ANY from zoneinfo import ZoneInfo import pytest +import werkzeug from freezegun import freeze_time import app.celery.tasks @@ -16,7 +17,7 @@ from app.enums import ( NotificationType, TemplateType, ) -from app.job.rest import is_suspicious_input, is_valid_id +from app.job.rest import check_suspicious_id, is_suspicious_input, is_valid_id from app.utils import utc_now from tests import create_admin_authorization_header from tests.app.db import ( @@ -595,6 +596,15 @@ def test_is_valid_id(sample_job): assert returnVal is False +def test_check_suspicious_id(sample_job): + # This should be good + check_suspicious_id(sample_job.id, sample_job.service_id) + + # This should be bad + with pytest.raises(werkzeug.exceptions.Forbidden): + check_suspicious_id(sample_job.id, "what is this???") + + def test_is_suspicious_input(sample_job): returnVal = is_suspicious_input(sample_job.id) assert returnVal is False From b52518ca0849b42f316aeac4104a4e2f0b1d818e Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 25 Jun 2025 10:37:07 -0700 Subject: [PATCH 7/7] make regex verbose --- app/job/rest.py | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/app/job/rest.py b/app/job/rest.py index 5189c6917..831c3f2f4 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -48,7 +48,35 @@ register_errors(job_blueprint) def is_suspicious_input(input_str): if not isinstance(input_str, str): return False - pattern = r"(?i)\b(OR|AND|UNION|SELECT|DROP|INSERT|UPDATE|DELETE|EXEC|TRUNCATE|CREATE|ALTER|--|/\*|\bpg_sleep\b|\bsleep\b)|[';]{2,}" # noqa + + pattern = re.compile( + r""" + (?i) # case insensite + \b # word boundary + ( # start of group for SQL keywords + OR # match SQL keyword OR + |AND + |UNION + |SELECT + |DROP + |INSERT + |UPDATE + |DELETE + |EXEC + |TRUNCATE + |CREATE + |ALTER + |-- # match SQL single-line comment + |/\* # match SQL multi-line comment + |\bpg_sleep\b # Match PostgreSQL 'pg_sleep' function + + |\bsleep\b # Match SQL Server 'sleep' function + ) # End SQL keywords and function group + | # OR operator to include an alternate pattern + [';]{2,} # Match two or more consecutive single quotes or semi-colons + """, + re.VERBOSE, + ) return bool(re.search(pattern, input_str))