Address PR feedback

Author: LemmaLegal

README.md

@@ -81,11 +81,11 @@ See [discussion here](https://github.com/SuffolkLITLab/docassemble-AssemblyLine/
 Answer set JSON imports are intentionally restricted to reduce risk from malformed and malicious payloads.
 
 Default behavior:
-- Plain JSON values are imported by default, and object reconstruction is allowed only for allowlisted DAObject classes.
-- Top-level variable names must match `^[A-Za-z][A-Za-z0-9_]*$`.
-- Internal/protected variable names are blocked.
-- If `answer set import allowed variables` is not set, imports use a denylist-only policy for backwards compatibility.
-- Object payloads can be imported when classes are allowlisted; by default, known `docassemble.base` and `docassemble.AssemblyLine` DAObject descendants are allowed.
+- Plain JSON values are imported by default, and object reconstruction is allowed only for allowlisted DAObject classes.
+- Top-level variable names must match `^[A-Za-z][A-Za-z0-9_]*$`.
+- Internal/protected variable names are blocked.
+- If `answer set import allowed variables` is not set, imports allow safe variable names by default, still block protected/internal names, and intersect with the target interview's known variables when AssemblyLine can detect them.
+- Object payloads can be imported when classes are allowlisted; by default, known `docassemble.base` and `docassemble.AssemblyLine` DAObject descendants are allowed.
 
 Default import limits (`assembly line: answer set import limits`):
 - `max bytes`: `1048576` (1 MB)
@@ -96,7 +96,7 @@ Default import limits (`assembly line: answer set import limits`):
 - `max number abs`: `1000000000000000` (`10**15`)
 
 Final allowlist/config policy:
-- Default allowlist: unset (`answer set import allowed variables` omitted), to avoid breaking existing interviews unexpectedly.
+- Default allowlist: unset (`answer set import allowed variables` omitted), which falls back to safe-name/protected-name checks plus target-interview variable detection when available.
 - Recommended production policy: set an explicit allowlist to only shared/reusable variables in your jurisdiction.
 - `answer set import allow objects` defaults to `true`; set it to `false` if you want strict plain-JSON-only imports.
 - `answer set import allowed object classes` can extend the default DAObject class allowlist with explicit additional class paths.

docassemble/AssemblyLine/al_courts.py

@@ -454,7 +454,8 @@ def convert_zip(z: Any) -> str:
             "address_zip": convert_zip
         }
         if hasattr(self, "converters") and self.converters:
-            assert isinstance(self.converters, dict)
+            if not isinstance(self.converters, dict):
+                raise TypeError("converters must be a dict")
             merged_converters.update(self.converters)
         to_load = path_and_mimetype(load_path)[0]
         if self.filename.lower().endswith(".xlsx"):

docassemble/AssemblyLine/al_document.py

@@ -34,7 +34,7 @@
 from docassemble.base.pdfa import pdf_to_pdfa
 from textwrap import wrap
 from math import floor
-import subprocess
+import subprocess  # nosec B404
 import pikepdf
 from typing import Tuple
 import secrets
@@ -1104,7 +1104,7 @@ def as_pdf(
             try:
                 main_doc.set_attributes(filename=filename)
                 main_doc.set_mimetype("application/pdf")
-            except:
+            except Exception:  # nosec B110
                 pass
 
         if self.need_addendum():
@@ -1976,7 +1976,7 @@ def get_cacheable_documents(
                     )
                 )
                 result["download_filename"] = filename_root + ext
-            except:
+            except Exception:  # nosec B110
                 pass
             results.append(result)
 
@@ -2855,7 +2855,7 @@ def ocrmypdf_task(
 
     completed_ocr = None
     try:
-        completed_ocr = subprocess.run(
+        completed_ocr = subprocess.run(  # nosec B603
             ocr_params, timeout=60 * 60, check=False, capture_output=True
         )
         to_pdf.commit()

docassemble/AssemblyLine/al_general.py

@@ -642,7 +642,7 @@ def normalized_address(self) -> Union[Address, "ALAddress"]:
         """
         try:
             self.geocode()
-        except:
+        except Exception:  # nosec B110
             pass
         if self.was_geocoded_successfully() and hasattr(self, "norm_long"):
             return self.norm_long
@@ -672,7 +672,7 @@ def state_name(self, country_code: Optional[str] = None) -> str:
         if hasattr(self, "country") and self.country and len(self.country) == 2:
             try:
                 return state_name(self.state, country_code=self.country)
-            except:
+            except Exception:  # nosec B110
                 pass
         try:
             return state_name(
@@ -1037,7 +1037,7 @@ def phone_numbers(
         elif len(nums):
             return list(nums[0].keys())[0]
 
-        assert False  # We should never get here, no default return is necessary
+        raise AssertionError("Unreachable: no default return is necessary")
 
     def contact_methods(self) -> str:
         """Generates a formatted string of all provided contact methods.
@@ -2470,7 +2470,7 @@ def is_phone_or_email(text: str) -> bool:
             validation_error("Enter a valid phone number or email address")
         else:
             validation_error("Enter a valid email address")
-        assert False, "unreachable"
+        raise AssertionError("unreachable")
 
 
 def github_modified_date(

docassemble/AssemblyLine/data/questions/al_saved_sessions.yml

@@ -247,9 +247,12 @@ content: |
 template: al_sessions_import_rejected_template
 subject: Variables skipped during import
 content: |
-  % for item in al_sessions_last_import_report.get('rejected', []):
+  % for item in al_sessions_last_import_report.get('rejected', [])[:50]:
   * `${ item.get('path', '?') }`: ${ item.get('reason', 'unknown reason') }
   % endfor
+  % if len(al_sessions_last_import_report.get('rejected', [])) > 50:
+  * ${ len(al_sessions_last_import_report.get('rejected', [])) - 50 } more variables were skipped and are not shown here.
+  % endif
 ---
 question: |
   Upload a JSON file
@@ -266,4 +269,4 @@ validation code: |
 code: |
   al_sessions_snapshot_results = load_interview_json(al_sessions_json_file.slurp())
   al_sessions_last_import_report = get_last_import_report()
-  al_sessions_import_json = True
+  al_sessions_import_json = True

docassemble/AssemblyLine/sessions.py

@@ -33,7 +33,12 @@
 )
 from docassemble.webapp.db_object import init_sqlalchemy
 from sqlalchemy.sql import text
-from docassemble.base.functions import server, safe_json, serializable_dict
+from docassemble.base.functions import (
+    server,
+    safe_json,
+    serializable_dict,
+    this_thread,
+)
 from .al_document import (
     ALDocument,
     ALDocumentBundle,
@@ -270,18 +275,22 @@
     "DASet": "docassemble.base.util.DASet",
 }
 
-_last_import_report: Dict[str, Any] = {
-    "accepted": [],
-    "rejected": [],
-    "warnings": [],
-    "limits": {},
-    "contains_objects": False,
-    "remapped_classes": [],
-}
+LAST_IMPORT_REPORT_ATTR = "_assemblyline_last_import_report"
+
+
+def _default_import_report() -> Dict[str, Any]:
+    return {
+        "accepted": [],
+        "rejected": [],
+        "warnings": [],
+        "limits": {},
+        "contains_objects": False,
+        "remapped_classes": [],
+    }
 
 
 def get_last_import_report() -> Dict[str, Any]:
-    """Return a copy of the most recent JSON import report.
+    """Return a copy of the most recent JSON import report for this thread.
 
     The report dictionary contains the following keys:
     - ``accepted``: list of variable names that were imported successfully.
@@ -294,12 +303,18 @@ def get_last_import_report() -> Dict[str, Any]:
     Returns:
         Dict[str, Any]: sanitized copy of the last import report.
     """
-    return safe_json(_last_import_report)
+    misc = getattr(this_thread, "misc", None)
+    if not isinstance(misc, dict):
+        return safe_json(_default_import_report())
+    return safe_json(misc.get(LAST_IMPORT_REPORT_ATTR, _default_import_report()))
 
 
 def _set_last_import_report(report: Dict[str, Any]) -> None:
-    global _last_import_report
-    _last_import_report = report
+    misc = getattr(this_thread, "misc", None)
+    if not isinstance(misc, dict):
+        misc = {}
+        setattr(this_thread, "misc", misc)
+    misc[LAST_IMPORT_REPORT_ATTR] = safe_json(report)
 
 
 def _import_limits() -> Dict[str, int]:
@@ -382,10 +397,16 @@ def _safe_variable_name(name: str) -> bool:
 
 def _allowed_import_variables() -> Optional[Set[str]]:
     """Optional strict allowlist from config; when empty/undefined, default policy applies."""
-    allowed = get_config("assembly line", {}).get("answer set import allowed variables")
-    if not allowed:
+    raw_allowed = get_config("assembly line", {}).get("answer set import allowed variables")
+    if raw_allowed is None:
+        return None
+    if isinstance(raw_allowed, str):
+        iterable = [raw_allowed]
+    elif isinstance(raw_allowed, (list, tuple, set)):
+        iterable = raw_allowed
+    else:
         return None
-    cleaned = {str(x).strip() for x in allowed if str(x).strip()}
+    cleaned = {str(x).strip() for x in iterable if str(x).strip()}
     return cleaned if cleaned else None
 
 
@@ -424,7 +445,13 @@ def _allowed_object_classes() -> Set[str]:
     configured = get_config("assembly line", {}).get(
         "answer set import allowed object classes", []
     )
-    for class_name in configured:
+    if isinstance(configured, str):
+        iterable = [configured]
+    elif isinstance(configured, (list, tuple, set)):
+        iterable = configured
+    else:
+        iterable = []
+    for class_name in iterable:
         name = str(class_name).strip()
         if name:
             allowed.add(name)
@@ -1108,6 +1135,8 @@ def find_matching_sessions(
         metadata_search_conditions = "TRUE"
 
     # Add metadata filters
+    _ALLOWED_OPERATORS = {"=", "!=", "<", "<=", ">", ">=", "LIKE", "ILIKE"}
+    _ALLOWED_CAST_TYPES = {"int", "float"}
     if metadata_filters:
         metadata_filter_conditions = []
         for column, val_tuple in metadata_filters.items():
@@ -1116,6 +1145,14 @@ def find_matching_sessions(
                 cast_type = None
             else:
                 value, operator, cast_type = val_tuple
+
+            if operator.upper() not in _ALLOWED_OPERATORS:
+                raise ValueError(f"Disallowed SQL operator: {operator}")
+            if cast_type and cast_type.lower() not in _ALLOWED_CAST_TYPES:
+                raise ValueError(f"Disallowed SQL cast type: {cast_type}")
+            if not column.isidentifier():
+                raise ValueError(f"Invalid metadata column name: {column}")
+
             if cast_type:
                 column_expr = f"CAST(jsonstorage.data->>{repr(column)} AS {cast_type})"
             else:
@@ -1157,7 +1194,7 @@ def find_matching_sessions(
                     userdict.key as key,
                     {', '.join(f"jsonstorage.data->>{repr(column)} as {column}" for column in metadata_column_names)},
                     jsonstorage.data as data
-            FROM userdict 
+            FROM userdict
             NATURAL JOIN (
                 SELECT key, MAX(modtime) AS modtime, COUNT(key) AS num_keys
                 FROM userdict
@@ -1174,7 +1211,7 @@ def find_matching_sessions(
         ) AS unique_sessions
         ORDER BY modtime DESC
         LIMIT :limit OFFSET :offset;
-        """)
+        """)  # nosec B608
 
     if offset < 0:
         offset = 0
@@ -2322,7 +2359,9 @@ def update_session_metadata(
 
     # 2) Derive two signed 32‑bit ints from MD5(session_id|filename|tags)
     key_string = f"{session_id}|{filename}|{metadata_key_name}"
-    digest = hashlib.md5(key_string.encode("utf-8")).digest()
+    digest = hashlib.md5(
+        key_string.encode("utf-8"), usedforsecurity=False
+    ).digest()  # nosec B324
     high_u32, low_u32 = struct.unpack(">II", digest[:8])
 
     def to_signed_32(x: int) -> int: