Merge pull request #5661 from pauline2k/boac-6760

BOAC-6760 Additional validation/parameterization for SQL and LDAP queries

Author: lyttam

boac/api/peer_advising_notes_controller.py

@@ -250,8 +250,11 @@ def get_notes_authored_by():
     # The 'timeframe' param (optional) has two properties: year and month.
     timeframe = params.get('timeframe') or None
     if timeframe:
-        month = timeframe['month']
-        year = timeframe['year']
+        try:
+            month = int(timeframe['month'])
+            year = int(timeframe['year'])
+        except (TypeError, ValueError):
+            raise BadRequestError('Invalid timeframe')
         timeframe = f"{year}-{f'0{month}' if month < 10 else month}"
     notes = []
     rows = Note.get_peer_advising_notes_authored_by(

boac/api/search_controller.py

@@ -232,7 +232,7 @@ def find_advisors_by_name():
     query = request.args.get('q')
     if not query:
         raise BadRequestError('Search query must be supplied')
-    limit = request.args.get('limit')
+    limit = request.args.get('limit', type=int)
     query_fragments = list(filter(None, set(query.upper().split(' '))))
     advisors = _advisors_by_name(query_fragments, limit=limit)
     legacy_note_authors = match_advising_note_authors_by_name(query_fragments, limit=limit)
@@ -261,7 +261,8 @@ def _advisors_by_name(tokens, limit=None):
         {' '.join(token_conditions)}
         ORDER BY a.advisor_name"""
     if limit:
-        sql += f' LIMIT {limit}'
+        sql += ' LIMIT :limit'
+        params['limit'] = limit
     benchmark('execute query')
     results = db.session.execute(text(sql), params)
     benchmark('end')
@@ -402,7 +403,7 @@ def _student_search(search_phrase, params, order_by):
     student_results = search_for_students(
         search_phrase=search_phrase.replace(',', ' '),
         order_by=order_by,
-        offset=util.get(params, 'offset', 0),
+        offset=int(util.get(params, 'offset', 0)),
         limit=util.get(params, 'limit', 50),
     )
     students = student_results['students']

boac/externals/calnet.py

@@ -70,6 +70,7 @@ def connect(self):
         return ldap3.Connection(self.server, user=self.bind, password=self.password, auto_bind=ldap3.AUTO_BIND_TLS_BEFORE_BIND)
 
     def search_csids(self, csids, search_expired=False):
+        csids = [csid for csid in csids if str(csid).isdigit()]
         all_out = []
         for i in range(0, len(csids), BATCH_QUERY_MAXIMUM):
             csids_batch = csids[i:i + BATCH_QUERY_MAXIMUM]
@@ -80,6 +81,7 @@ def search_csids(self, csids, search_expired=False):
         return all_out
 
     def search_uids(self, uids, search_expired=False):
+        uids = [uid for uid in uids if str(uid).isdigit()]
         all_out = []
         for i in range(0, len(uids), BATCH_QUERY_MAXIMUM):
             uids_batch = uids[i:i + BATCH_QUERY_MAXIMUM]
@@ -91,7 +93,8 @@ def search_uids(self, uids, search_expired=False):
 
     @classmethod
     def _ldap_search_filter(cls, ids, id_type, search_expired=False):
-        ids_filter = ''.join(f'({id_type}={_id})' for _id in ids)
+        numeric_ids = [_id for _id in ids if str(_id).isdigit()]
+        ids_filter = ''.join(f'({id_type}={_id})' for _id in numeric_ids)
         ou_scope = '(ou=expired people)' if search_expired else '(ou=people) (ou=advcon people)'
         return f"""(&
             (objectclass=person)

boac/externals/data_loch.py

@@ -501,7 +501,8 @@ def match_appointment_advisors_by_name(prefixes, limit=None):
         {' '.join(prefix_conditions)}
         ORDER BY a.first_name, a.last_name"""
     if limit:
-        sql += f' LIMIT {limit}'
+        sql += ' LIMIT %(limit)s'
+        prefix_kwargs['limit'] = limit
     return safe_execute_rds(sql, **prefix_kwargs)
 
 
@@ -520,7 +521,8 @@ def match_advising_note_authors_by_name(prefixes, limit=None):
         {' '.join(prefix_conditions)}
         ORDER BY a.first_name, a.last_name"""
     if limit:
-        sql += f' LIMIT {limit}'
+        sql += ' LIMIT %(limit)s'
+        prefix_kwargs['limit'] = limit
     return safe_execute_rds(sql, **prefix_kwargs)
 
 
@@ -692,7 +694,7 @@ def get_admitted_student_by_sid(sid):
 
 
 def get_admitted_students_by_sids(offset, sids, limit=None, order_by='last_name'):
-    limit_clause = f'LIMIT {limit}' if limit else ''
+    limit_clause = 'LIMIT %(limit)s' if limit else ''
     sql = f"""
         SELECT a.applyuc_cpid, a.cs_empl_id AS sid, a.uid, s.uid AS student_uid,
         a.residency_category, a.freshman_or_transfer, a.admit_term, a.admit_status, a.current_sir, college, a.first_name, a.middle_name,
@@ -1495,7 +1497,7 @@ def get_students_query(  # noqa: C901, PLR0912, PLR0913, PLR0915
         query_bindings.update({'previous_term_id': previous_term_id(current_term_id)})
     query_filter += _number_ranges_to_sql('spi.units', unit_ranges) if unit_ranges else ''
     if last_name_ranges:
-        query_filter += _last_name_ranges_to_sql(last_name_ranges)
+        query_filter += _last_name_ranges_to_sql(last_name_ranges, query_bindings)
     if degree_terms:
         query_filter += ' AND sd.term_id = ANY(%(degree_terms)s)'
         query_bindings.update({'degree_terms': degree_terms})
@@ -1713,10 +1715,11 @@ def get_students_ordering(term_id, order_by=None, group_codes=None, majors=None,
         o = 'set.enrolled_units'
     elif order_by and order_by.startswith('term_gpa_'):
         gpa_term_id = order_by.replace('term_gpa_', '')
-        supplemental_query_tables = f"""
-            LEFT JOIN {student_schema()}.student_enrollment_terms set
-            ON set.sid = spi.sid AND set.term_id = '{gpa_term_id}'"""
-        o = 'set.term_gpa'
+        if gpa_term_id.isdigit():
+            supplemental_query_tables = f"""
+                LEFT JOIN {student_schema()}.student_enrollment_terms set
+                ON set.sid = spi.sid AND set.term_id = '{gpa_term_id}'"""
+            o = 'set.term_gpa'
     o_secondary = by_first_name if order_by == 'last_name' else by_last_name
     diff = {by_first_name, by_last_name} - {o, o_secondary}
     o_tertiary = diff.pop() if diff else 'spi.sid'
@@ -1840,9 +1843,9 @@ def _match_students_by_sid(sid, limit=None):
         SELECT spi.first_name, spi.last_name, spi.email_address, spi.sid, spi.uid
         FROM {student_schema()}.student_profile_index spi
         WHERE spi.sid LIKE %(starts_with)s
-        {f' LIMIT {limit}' if limit else ''}
+        {'LIMIT %(limit)s' if limit else ''}
     """
-    return safe_execute_rds(sql, **{'starts_with': f'{sid}%'})
+    return safe_execute_rds(sql, **{'starts_with': f'{sid}%', 'limit': limit})
 
 
 def _match_students_by_name_or_email(phrase, limit=None, prefix_only=False):
@@ -1864,12 +1867,13 @@ def _match_students_by_name_or_email(phrase, limit=None, prefix_only=False):
                 ELSE 2
                 END
             ), s.first_name, s.last_name
-            {f'LIMIT {limit}' if limit else ''}
+            {'LIMIT %(limit)s' if limit else ''}
         ) AS s
     """
     return safe_execute_rds(sql, **{
         'contains': f'%{phrase}%',
         'starts_with': f'{phrase}%',
+        'limit': limit,
     })
 
 
@@ -1918,8 +1922,9 @@ def _search_for_students(phrases, limit=None, prefix_only=False):
             ELSE 2
             END
         ), s.first_name, s.last_name
-        {f' LIMIT {limit}' if limit else ''}
+        {'LIMIT %(limit)s' if limit else ''}
     """
+    sql_params['limit'] = limit
     return safe_execute_rds(sql, **sql_params)
 
 
@@ -1965,21 +1970,28 @@ def _number_ranges_to_sql(column, number_ranges):
         return ''
 
 
-def _last_name_ranges_to_sql(last_name_ranges):
+def _last_name_ranges_to_sql(last_name_ranges, query_bindings):
     query_filter = ''
     count = len(last_name_ranges)
     if count:
         query_filter += ' AND ('
         for idx, last_name_range in enumerate(last_name_ranges):
             range_min = last_name_range['min'].upper()
             range_max = last_name_range['max'].upper()
+            min_key = f'last_name_min_{idx}'
+            max_key = f'last_name_max_{idx}'
             if range_max == range_min:
-                query_filter += f'(spi.last_name ILIKE \'{range_min}%%\')'
+                query_filter += f'(spi.last_name ILIKE %({min_key})s)'
+                query_bindings[min_key] = f'{range_min}%'
             else:
-                query_filter += f'(UPPER(SUBSTRING(spi.last_name, 0, {len(range_min) + 1})) >= \'{range_min}\''
+                query_filter += f'(UPPER(SUBSTRING(spi.last_name, 0, %({min_key}_len)s)) >= %({min_key})s'
+                query_bindings[min_key] = range_min
+                query_bindings[f'{min_key}_len'] = len(range_min) + 1
                 if range_max < 'ZZ':
                     # If 'stop' were 'ZZ' then upper bound would not be necessary
-                    query_filter += f' AND UPPER(SUBSTRING(spi.last_name, 0, {len(range_max) + 1})) <= \'{range_max}\''
+                    query_filter += f' AND UPPER(SUBSTRING(spi.last_name, 0, %({max_key}_len)s)) <= %({max_key})s'
+                    query_bindings[max_key] = range_max
+                    query_bindings[f'{max_key}_len'] = len(range_max) + 1
                 query_filter += ')'
             if idx < count - 1:
                 query_filter += ' OR '

boac/merged/student.py

@@ -522,7 +522,8 @@ def search_for_students(
         ORDER BY MIN({o}) {o_direction} NULLS FIRST, MIN({o_secondary}) NULLS FIRST, MIN({o_tertiary}) NULLS FIRST"""
     if o_tertiary != 'spi.sid':
         sql += ', spi.sid'
-    sql += f' OFFSET {offset}'
+    sql += ' OFFSET %(offset)s'
+    query_bindings['offset'] = offset
     if limit and limit < 100:  # Sanity check large limits
         sql += ' LIMIT %(limit)s'
         query_bindings['limit'] = limit

boac/models/note.py

@@ -154,9 +154,10 @@ def get_draft_note_count(cls, author_uid=None):
               count(*) FROM notes
             WHERE
               deleted_at IS NULL AND is_draft IS TRUE
-              { f"AND author_uid = '{author_uid}'" if author_uid else ''}
+              {'AND author_uid = :author_uid' if author_uid else ''}
         """
-        return db.session.execute(text(sql)).mappings().first()['count']
+        params = {'author_uid': author_uid} if author_uid else {}
+        return db.session.execute(text(sql), params).mappings().first()['count']
 
     @classmethod
     def get_draft_notes(cls, author_uid=None):
@@ -168,11 +169,12 @@ def get_draft_notes(cls, author_uid=None):
             WHERE
                 n.is_draft IS TRUE
                 AND n.deleted_at IS NULL
-                {f"AND author_uid = '{author_uid}'" if author_uid else ''}
+                {'AND author_uid = :author_uid' if author_uid else ''}
             GROUP BY n.id
             ORDER BY n.updated_at DESC
         """
-        for row in db.session.execute(text(sql)).mappings():
+        params = {'author_uid': author_uid} if author_uid else {}
+        for row in db.session.execute(text(sql), params).mappings():
             draft_notes.append({
                 'id': row['id'],
                 'attachmentCount': row['attachment_count'],
@@ -250,11 +252,13 @@ def get_peer_advising_notes_authored_by(
               AND n.is_private IS FALSE
               AND n.parent_note_id IS NULL
               AND n.peer_advising_department_id = :peer_advising_department_id
-              {f" AND to_char(n.created_at, 'YYYY-MM') = '{timeframe_month}'" if timeframe_month else ''}
+              {"AND to_char(n.created_at, 'YYYY-MM') = :timeframe_month" if timeframe_month else ''}
             GROUP BY n.id, a.id, t.topic
             ORDER BY n.updated_at DESC
         """
         params = {'author_uid': author_uid, 'peer_advising_department_id': peer_advising_department_id}
+        if timeframe_month:
+            params['timeframe_month'] = timeframe_month
         return db.session.execute(text(sql), params).mappings().all()
 
     @classmethod
@@ -596,10 +600,11 @@ def _fetch_result(is_count_query=False):
                 {where_clause}
             """
             if not is_count_query:
-                sql += f"""
+                sql += """
                     ORDER BY notes.updated_at DESC, fts.rank DESC, notes.id DESC
-                    OFFSET {offset} LIMIT {limit}
+                    OFFSET :offset LIMIT :limit
                 """
+                params.update({'offset': offset, 'limit': limit})
             return db.session.execute(text(sql), params).mappings()
         return {
             'results': [row for row in _fetch_result().all()],