Skip to content

Commit 1e83c23

Browse files
fmellharalde
authored andcommitted
Log exceptions instead of storing in database
1 parent 1da2e53 commit 1e83c23

5 files changed

Lines changed: 75 additions & 9 deletions

File tree

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
"""drop results.python_exception (PickleType)
2+
3+
Revision ID: 7c2e1f4b8a90
4+
Revises: 9b4e2c1a7d50
5+
Create Date: 2026-05-19 00:00:00.000000
6+
7+
Removes the ``python_exception`` column on ``results``, which used
8+
SQLAlchemy ``PickleType``. Loading that column unpickled arbitrary bytes
9+
from the database on every result read, which is an insecure-
10+
deserialization sink (RCE primitive given any DB write capability).
11+
12+
The column was not used by application logic, so the existing pickled blobs are discarded.
13+
Full exception details are logged to Application Insights via ``logger.exception`` instead
14+
of being persisted in the database.
15+
"""
16+
17+
from typing import Sequence, Union
18+
19+
import sqlalchemy as sa
20+
from alembic import op
21+
22+
23+
revision: str = "7c2e1f4b8a90"
24+
down_revision: Union[str, Sequence[str], None] = "9b4e2c1a7d50"
25+
branch_labels: Union[str, Sequence[str], None] = None
26+
depends_on: Union[str, Sequence[str], None] = None
27+
28+
29+
def upgrade() -> None:
30+
op.drop_column("results", "python_exception")
31+
32+
33+
def downgrade() -> None:
34+
# Restore the column shape for compatibility, but not the data:
35+
# re-creating PickleType data would require reintroducing the
36+
# deserialization sink that this migration exists to remove.
37+
op.add_column(
38+
"results",
39+
sa.Column("python_exception", sa.PickleType(), nullable=True),
40+
)

backend/src/acidwatch_api/database.py

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010
Engine,
1111
ForeignKey,
1212
DateTime,
13-
PickleType,
1413
Uuid,
1514
JSON,
1615
create_engine,
@@ -71,7 +70,6 @@ class ModelResult(Base):
7170
model_input_id: Mapped[UUID] = mapped_column(ForeignKey("model_inputs.id"))
7271
concentrations: Mapped[dict[str, float]] = mapped_column(JSON)
7372
panels: Mapped[list[Any]] = mapped_column(JSON)
74-
python_exception: Mapped[BaseException | None] = mapped_column(PickleType)
7573
error: Mapped[str | None] = mapped_column()
7674

7775
model_input: Mapped[ModelInput] = relationship("ModelInput")

backend/src/acidwatch_api/routes/models.py

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
from __future__ import annotations
22

3-
import sys
3+
import logging
44
from collections import defaultdict
55
from typing import Annotated
66
from uuid import UUID, uuid4
@@ -42,6 +42,8 @@
4242

4343
router = APIRouter()
4444

45+
logger = logging.getLogger(__name__)
46+
4547

4648
type AdapterSet = dict[str, type[BaseAdapter]]
4749

@@ -126,18 +128,23 @@ async def _run_adapter(
126128
model_input_id=model_input_id,
127129
concentrations=concs,
128130
panels=[p.model_dump(mode="json", by_alias=True) for p in panels],
129-
python_exception=None,
130131
error=None,
131132
)
132133

133134
return concs
134135
except BaseException as exc:
136+
# Full traceback goes to logs (App Insights); only a short message
137+
# is persisted for surfacing to the API caller.
138+
logger.exception(
139+
"Adapter %s failed for model_input %s",
140+
adapter.model_id,
141+
model_input_id,
142+
)
135143
result_obj = db.ModelResult(
136144
model_input_id=model_input_id,
137145
concentrations={},
138146
panels=[],
139-
python_exception=exc,
140-
error=str(exc),
147+
error=f"{type(exc).__name__}: {exc}",
141148
)
142149
return {}
143150

@@ -187,7 +194,7 @@ def get_result_for_simulation(
187194
continue
188195

189196
if result.error is not None:
190-
print(result.error, file=sys.stderr)
197+
logger.error("Simulation %s failed: %s", simulation_id, result.error)
191198
raise HTTPException(
192199
status_code=500,
193200
detail=f"Simulation encountered an error: {result.error}",

backend/tests/test_models_endpoints.py

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -474,7 +474,6 @@ def test_results_order(client, sql_session, swap):
474474
result=db.ModelResult(
475475
concentrations={"A": 1},
476476
panels=[],
477-
python_exception=None,
478477
error=None,
479478
),
480479
),
@@ -484,7 +483,6 @@ def test_results_order(client, sql_session, swap):
484483
result=db.ModelResult(
485484
concentrations={"B": 2},
486485
panels=[],
487-
python_exception=None,
488486
error=None,
489487
),
490488
),
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
from __future__ import annotations
2+
3+
from sqlalchemy import PickleType
4+
from acidwatch_api.database import Base
5+
6+
7+
def test_no_pickle_type_columns() -> None:
8+
"""``PickleType`` columns deserialize arbitrary Python objects on read.
9+
10+
Forbidding them at the schema level prevents the insecure-
11+
deserialization sink that ``results.python_exception`` previously
12+
introduced from being reintroduced by accident.
13+
"""
14+
offenders = [
15+
f"{table.name}.{column.name}"
16+
for table in Base.metadata.tables.values()
17+
for column in table.columns
18+
if isinstance(column.type, PickleType)
19+
]
20+
assert not offenders, (
21+
f"PickleType columns are forbidden; found: {offenders}. "
22+
"Log via logging.exception() or store text/JSON instead."
23+
)

0 commit comments

Comments
 (0)