From 7c6c332d821d9c09ee17a68b44acfcacbadb3993 Mon Sep 17 00:00:00 2001 From: Badry Date: Tue, 1 Sep 2026 17:00:01 +0300 Subject: [PATCH 1/7] fix(snapshot): validate cached data and typed previews Reject scheduled snapshot payloads whose cached values contradict resolved filter parameters, revalidate retained candidates before serving, and preserve older valid coverage. Coerce SQL preview values through the endpoint parameter schema, add inline preview type controls, clarify snapshot request mappings, and cover the behavior in backend/frontend tests and documentation. --- backend/app/schemas/endpoint.py | 29 +++++ backend/app/services/data.py | 51 ++++++++- backend/app/services/endpoint.py | 21 +++- backend/app/services/scheduler.py | 10 ++ backend/app/services/snapshot_filtering.py | 47 ++++++++ backend/tests/test_endpoints.py | 66 ++++++++++++ backend/tests/test_schedules.py | 87 ++++++++++++++- .../tests/test_snapshot_request_filtering.py | 95 +++++++++++++++- docs/architecture.md | 7 ++ docs/operations.md | 14 ++- docs/scheduler_parameter_bindings.md | 37 +++++-- docs/security_checklist.md | 2 +- .../endpoints/EndpointWizard.test.tsx | 32 ++++++ .../components/endpoints/EndpointWizard.tsx | 10 ++ .../endpoints/SnapshotFilterMappings.tsx | 20 +++- .../endpoints/wizard/ConfigStep.test.tsx | 3 + .../endpoints/wizard/SqlStep.test.tsx | 47 ++++++++ .../components/endpoints/wizard/SqlStep.tsx | 102 ++++++++++++++---- frontend/src/types/endpoint.ts | 1 + 19 files changed, 640 insertions(+), 41 deletions(-) diff --git a/backend/app/schemas/endpoint.py b/backend/app/schemas/endpoint.py index 8d839ce..79f1968 100644 --- a/backend/app/schemas/endpoint.py +++ b/backend/app/schemas/endpoint.py @@ -388,6 +388,13 @@ class SqlPreviewRequest(BaseModel): connection_id: uuid.UUID sql_text: str = Field(..., min_length=1) params: dict[str, str | int | float | bool | None] = Field(default_factory=dict) + param_schema: dict[str, ParamDescriptor] = Field( + default_factory=dict, + description=( + "Optional typed descriptors for preview bind parameters. When supplied, the schema " + "must match the SQL bind names and values are coerced before Oracle execution." + ), + ) max_rows: int = Field(10, ge=1, le=100) @field_validator("sql_text") @@ -398,6 +405,28 @@ def validate_sql(cls, v: str) -> str: raise ValueError("; ".join(errors)) return v + @model_validator(mode="after") + def typed_schema_matches_bind_params(self) -> Self: + # ``param_schema`` is additive for compatibility with existing admin + # API clients. New callers should always send it when SQL has binds so + # preview uses the same typed-value contract as published endpoints. + if not self.param_schema: + return self + + sql_params = set(extract_bind_params(self.sql_text)) + schema_params = set(self.param_schema) + undeclared = sql_params - schema_params + unused = schema_params - sql_params + if undeclared: + raise ValueError( + f"SQL references preview params not declared in schema: {sorted(undeclared)}" + ) + if unused: + raise ValueError( + f"Preview schema declares params not referenced in SQL: {sorted(unused)}" + ) + return self + class SqlPreviewResponse(BaseModel): """Response from SQL preview execution.""" diff --git a/backend/app/services/data.py b/backend/app/services/data.py index efd0dd0..60f8dd0 100644 --- a/backend/app/services/data.py +++ b/backend/app/services/data.py @@ -46,6 +46,7 @@ snapshot_covers_request, unavailable_snapshot_filter_columns, validate_snapshot_parameter_ranges, + validate_snapshot_rows_match_resolved_parameters, ) from app.sql.executor import SqlExecutionError, execute_query from app.sql.param_models import build_param_model @@ -345,6 +346,7 @@ async def _serve_snapshot( ) snapshot = None + integrity_failures: list[tuple[str, str]] = [] if not param_schema: snapshot = snapshots[0] else: @@ -361,15 +363,55 @@ async def _serve_snapshot( if candidate.job_run_id is None: continue job_run = job_runs_by_id.get(candidate.job_run_id) - if job_run is not None and snapshot_covers_request( + if job_run is None or not snapshot_covers_request( filters=filters, request_params=params, resolved_params=job_run.resolved_params_json or {}, ): + continue + + candidate_data: list[dict[str, object]] = ( + candidate.data if isinstance(candidate.data, list) else [] + ) + if unavailable_snapshot_filter_columns(rows=candidate_data, filters=filters): + # Preserve the existing configuration-error response below. An endpoint edit + # can invalidate mappings even when the stored snapshot itself was valid. snapshot = candidate break + try: + validate_snapshot_rows_match_resolved_parameters( + rows=candidate_data, + filters=filters, + resolved_params=job_run.resolved_params_json or {}, + ) + except ValueError as exc: + integrity_failures.append((str(candidate.id), str(exc))) + continue + snapshot = candidate + break if snapshot is None: + if integrity_failures: + self._log_snapshot_rejection( + request=request, + endpoint=endpoint, + path=path, + principal=principal, + started_at=started_at, + event="snapshot_integrity_failed", + response_status=status.HTTP_503_SERVICE_UNAVAILABLE, + details={ + "snapshot_ids": [item[0] for item in integrity_failures], + "integrity_errors": [item[1] for item in integrity_failures], + }, + ) + return JSONResponse( + status_code=status.HTTP_503_SERVICE_UNAVAILABLE, + content={ + "code": "snapshot_integrity_failed", + "detail": "No retained snapshot passed integrity validation.", + }, + ) self._log_snapshot_rejection( request=request, endpoint=endpoint, @@ -438,18 +480,21 @@ def _log_snapshot_rejection( principal: str | None, started_at: float, event: str, + response_status: int = status.HTTP_422_UNPROCESSABLE_ENTITY, + details: dict[str, object] | None = None, ) -> None: - """Emit the required request context before a snapshot HTTP 422 response.""" + """Emit required request context before a snapshot rejection response.""" log.warning( event, request_id=resolve_request_id(request), user=principal or "anonymous", endpoint=path, endpoint_id=str(endpoint.id), - status=status.HTTP_422_UNPROCESSABLE_ENTITY, + status=response_status, duration_ms=round((time.perf_counter() - started_at) * 1000, 2), method=request.method, client_ip=request.client.host if request.client else None, + **(details or {}), ) # ── Live mode ─────────────────────────────────────────────────────────── diff --git a/backend/app/services/endpoint.py b/backend/app/services/endpoint.py index a50036b..bdbc9a3 100644 --- a/backend/app/services/endpoint.py +++ b/backend/app/services/endpoint.py @@ -35,6 +35,7 @@ ) from app.services.schedule_bindings import ScheduleBindingError, resolve_schedule_parameters from app.sql.executor import SqlExecutionError, execute_query +from app.sql.param_models import build_param_model log = structlog.get_logger() @@ -284,12 +285,30 @@ async def preview_sql(self, payload: SqlPreviewRequest) -> SqlPreviewResponse: raise ValueError("Connection is not active.") bind_params = extract_bind_params(payload.sql_text) + params: dict[str, object] = dict(payload.params) + + if payload.param_schema: + try: + ParamModel = build_param_model( + { + name: descriptor.model_dump() + for name, descriptor in payload.param_schema.items() + }, + enforce_required=True, + ) + params = ParamModel.model_validate(payload.params).model_dump() + except ValidationError as exc: + first = exc.errors()[0] + field = ".".join(str(part) for part in first.get("loc", ())) or "?" + raise ValueError( + f"Invalid value for preview parameter '{field}': {first.get('msg')}" + ) from exc try: columns, rows, duration_ms = await execute_query( connection=conn, sql=payload.sql_text, - params=dict(payload.params), + params=params, max_rows=payload.max_rows, ) except SqlExecutionError as exc: diff --git a/backend/app/services/scheduler.py b/backend/app/services/scheduler.py index e3252e8..8a31c9d 100644 --- a/backend/app/services/scheduler.py +++ b/backend/app/services/scheduler.py @@ -32,6 +32,10 @@ from app.repositories.snapshot import SnapshotRepository from app.schemas.schedule import ScheduleWindow from app.services.schedule_bindings import resolve_schedule_parameters +from app.services.snapshot_filtering import ( + compile_snapshot_filters, + validate_snapshot_rows_match_resolved_parameters, +) from app.sql.executor import execute_query log = structlog.get_logger().bind( @@ -236,6 +240,12 @@ async def execute_scheduled_job( mapped_rows.append(new_row) rows = mapped_rows + validate_snapshot_rows_match_resolved_parameters( + rows=rows, + filters=compile_snapshot_filters(param_schema), + resolved_params=params, + ) + # Save snapshot snapshot = Snapshot( endpoint_id=eid, diff --git a/backend/app/services/snapshot_filtering.py b/backend/app/services/snapshot_filtering.py index 223eb04..e7e96ef 100644 --- a/backend/app/services/snapshot_filtering.py +++ b/backend/app/services/snapshot_filtering.py @@ -174,6 +174,9 @@ def unavailable_snapshot_filter_columns( """Return configured output columns absent from a non-empty snapshot.""" if not rows: return [] + # TODO: Resolve configured filter columns against snapshot keys case-insensitively before both + # availability validation and row filtering, while rejecting ambiguous keys that differ only + # by case. available = {column for row in rows for column in row} return sorted({item.column for item in filters} - available) @@ -219,3 +222,47 @@ def filter_snapshot_rows( if matches: filtered.append(row) return filtered + + +def validate_snapshot_rows_match_resolved_parameters( + *, + rows: list[dict[str, object]], + filters: tuple[CompiledSnapshotFilter, ...], + resolved_params: dict[str, object], +) -> None: + """Reject non-empty snapshot results that contradict their resolved schedule bounds.""" + if not rows: + return + + missing_columns = unavailable_snapshot_filter_columns(rows=rows, filters=filters) + if missing_columns: + raise ValueError( + "Snapshot integrity validation failed: configured filter columns are absent from " + f"the cached output: {', '.join(missing_columns)}." + ) + + normalized_params = dict(resolved_params) + for item in filters: + if item.parameter not in resolved_params or resolved_params[item.parameter] is None: + continue + try: + normalized_params[item.parameter] = item.coerce_value(resolved_params[item.parameter]) + except (ValidationError, ValueError, TypeError) as exc: + raise ValueError( + "Snapshot integrity validation failed: the schedule's resolved parameter " + f":{item.parameter} cannot be coerced to {item.param_type}." + ) from exc + + matching_rows = filter_snapshot_rows( + rows=rows, + filters=filters, + request_params=normalized_params, + ) + invalid_row_count = len(rows) - len(matching_rows) + if invalid_row_count: + columns = sorted({item.column for item in filters}) + raise ValueError( + "Snapshot integrity validation failed: " + f"{invalid_row_count} of {len(rows)} cached rows do not match the schedule's " + f"resolved filter parameters for columns: {', '.join(columns)}." + ) diff --git a/backend/tests/test_endpoints.py b/backend/tests/test_endpoints.py index 8ff21fe..16d7bd6 100644 --- a/backend/tests/test_endpoints.py +++ b/backend/tests/test_endpoints.py @@ -7,6 +7,9 @@ """ import uuid +from datetime import date +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock import pytest from app.schemas.endpoint import ( @@ -303,9 +306,72 @@ def test_sql_preview_request_valid() -> None: connection_id=uuid.uuid4(), sql_text="SELECT * FROM employees WHERE dept_id = :dept_id", params={"dept_id": 10}, + param_schema={"dept_id": {"type": "integer", "required": True}}, max_rows=5, ) assert payload.max_rows == 5 + assert payload.param_schema["dept_id"].type == "integer" + + +def test_sql_preview_request_rejects_typed_schema_mismatch() -> None: + with pytest.raises(ValueError, match="not declared in schema"): + SqlPreviewRequest( + connection_id=uuid.uuid4(), + sql_text="SELECT * FROM employees WHERE hired_on >= :start_date", + params={"start_date": "2026-08-30"}, + param_schema={"other_date": {"type": "date", "required": True}}, + ) + + +@pytest.mark.asyncio +async def test_sql_preview_coerces_date_before_oracle_execution( + monkeypatch: pytest.MonkeyPatch, +) -> None: + from app.services.endpoint import EndpointService + + connection = SimpleNamespace(is_active=True) + connection_repo = MagicMock() + connection_repo.get_by_id = AsyncMock(return_value=connection) + execute_query = AsyncMock(return_value=(["DT"], [], 1.0)) + monkeypatch.setattr("app.services.endpoint.execute_query", execute_query) + service = EndpointService(repo=MagicMock(), conn_repo=connection_repo) + + await service.preview_sql( + SqlPreviewRequest( + connection_id=uuid.uuid4(), + sql_text="SELECT DT FROM orders WHERE DT >= :start_date", + params={"start_date": "30-08-2026"}, + param_schema={"start_date": {"type": "date", "required": True}}, + ) + ) + + execute_query.assert_awaited_once() + assert execute_query.await_args.kwargs["params"] == {"start_date": date(2026, 8, 30)} + + +@pytest.mark.asyncio +async def test_sql_preview_rejects_invalid_typed_date_before_oracle( + monkeypatch: pytest.MonkeyPatch, +) -> None: + from app.services.endpoint import EndpointService + + connection_repo = MagicMock() + connection_repo.get_by_id = AsyncMock(return_value=SimpleNamespace(is_active=True)) + execute_query = AsyncMock() + monkeypatch.setattr("app.services.endpoint.execute_query", execute_query) + service = EndpointService(repo=MagicMock(), conn_repo=connection_repo) + + with pytest.raises(ValueError, match="Invalid value for preview parameter 'start_date'"): + await service.preview_sql( + SqlPreviewRequest( + connection_id=uuid.uuid4(), + sql_text="SELECT DT FROM orders WHERE DT >= :start_date", + params={"start_date": "not-a-date"}, + param_schema={"start_date": {"type": "date", "required": True}}, + ) + ) + + execute_query.assert_not_awaited() # ── API integration tests (require PostgreSQL) ────────────────────────────── diff --git a/backend/tests/test_schedules.py b/backend/tests/test_schedules.py index be0d4ff..1b04b90 100644 --- a/backend/tests/test_schedules.py +++ b/backend/tests/test_schedules.py @@ -494,14 +494,24 @@ async def test_preview_schedule_resolves_next_runs(async_client: object) -> None @pytest.mark.integration async def test_scheduled_execution_persists_logical_context_and_is_idempotent( - async_client: object, monkeypatch: pytest.MonkeyPatch + async_client: object, + db_session: object, + monkeypatch: pytest.MonkeyPatch, ) -> None: from unittest.mock import AsyncMock from app.services import scheduler as scheduler_service from httpx import AsyncClient + from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker client: AsyncClient = async_client # type: ignore[assignment] + db: AsyncSession = db_session # type: ignore[assignment] + assert db.bind is not None + monkeypatch.setattr( + scheduler_service, + "AsyncSessionLocal", + async_sessionmaker(db.bind, class_=AsyncSession, expire_on_commit=False), + ) endpoint_id = await _create_snapshot_endpoint_with_date_range(client) created = await client.post( "/api/v1/admin/schedules/", @@ -522,8 +532,8 @@ async def test_scheduled_execution_persists_logical_context_and_is_idempotent( execute_query = AsyncMock( return_value=( - ["ORDER_ID"], - [{"ORDER_ID": 42}], + ["business_date", "ORDER_ID"], + [{"business_date": "2026-08-25 00:00:00", "ORDER_ID": 42}], 12, ) ) @@ -567,6 +577,77 @@ async def test_scheduled_execution_persists_logical_context_and_is_idempotent( } +@pytest.mark.integration +async def test_scheduled_execution_rejects_rows_outside_resolved_snapshot_window( + async_client: object, + db_session: object, + monkeypatch: pytest.MonkeyPatch, +) -> None: + from unittest.mock import AsyncMock + + from app.models.snapshot import Snapshot + from app.services import scheduler as scheduler_service + from httpx import AsyncClient + from sqlalchemy import select + from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker + + client: AsyncClient = async_client # type: ignore[assignment] + db: AsyncSession = db_session # type: ignore[assignment] + assert db.bind is not None + monkeypatch.setattr( + scheduler_service, + "AsyncSessionLocal", + async_sessionmaker(db.bind, class_=AsyncSession, expire_on_commit=False), + ) + endpoint_id = await _create_snapshot_endpoint_with_date_range(client) + created = await client.post( + "/api/v1/admin/schedules/", + json={ + "endpoint_id": endpoint_id, + "schedule_type": "cron", + "cron_expression": "0 6 * * *", + "timezone": "Asia/Riyadh", + "parameter_bindings": { + "start_date": {"source": "window_start"}, + "end_date": {"source": "window_end"}, + }, + "window": {"preset": "last_n_complete_days", "days": 7}, + }, + ) + assert created.status_code == 201 + schedule_id = created.json()["id"] + + execute_query = AsyncMock( + return_value=( + ["business_date", "ORDER_ID"], + [{"business_date": "0026-08-25 00:00:00", "ORDER_ID": 42}], + 12, + ) + ) + monkeypatch.setattr(scheduler_service, "execute_query", execute_query) + + await scheduler_service.execute_scheduled_job( + schedule_id, + endpoint_id, + scheduled_for=datetime(2026, 8, 31, 3, tzinfo=UTC), + ) + + runs_response = await client.get( + "/api/v1/admin/schedules/jobs/", + params={"schedule_id": schedule_id}, + ) + assert runs_response.status_code == 200 + runs = runs_response.json() + assert len(runs) == 1 + assert runs[0]["status"] == "failed" + assert "1 of 1 cached rows do not match" in runs[0]["error_detail"] + + snapshot = await db.scalar( + select(Snapshot).where(Snapshot.endpoint_id == uuid.UUID(endpoint_id)) + ) + assert snapshot is None + + @pytest.mark.integration async def test_run_now_accepts_an_explicit_logical_date( async_client: object, monkeypatch: pytest.MonkeyPatch diff --git a/backend/tests/test_snapshot_request_filtering.py b/backend/tests/test_snapshot_request_filtering.py index 8708342..314d2f8 100644 --- a/backend/tests/test_snapshot_request_filtering.py +++ b/backend/tests/test_snapshot_request_filtering.py @@ -2,7 +2,7 @@ import uuid from collections.abc import Sequence -from datetime import UTC, datetime, timedelta +from datetime import UTC, date, datetime, timedelta import pytest from app.models.endpoint import ApiEndpoint, DataStrategy @@ -13,6 +13,7 @@ from app.services.snapshot_filtering import ( compile_snapshot_filters, validate_snapshot_parameter_ranges, + validate_snapshot_rows_match_resolved_parameters, ) from httpx import AsyncClient from sqlalchemy import select @@ -131,6 +132,64 @@ def test_snapshot_range_validation_retains_duplicate_directional_bounds() -> Non ) +def test_snapshot_integrity_rejects_cached_dates_outside_resolved_window() -> None: + filters = compile_snapshot_filters( + { + "start_date": { + "type": "date", + "snapshot_filter": {"column": "DT", "operator": "gte"}, + }, + "end_date": { + "type": "date", + "snapshot_filter": {"column": "DT", "operator": "lte"}, + }, + } + ) + + with pytest.raises( + ValueError, + match="1 of 1 cached rows do not match the schedule's resolved filter parameters", + ): + validate_snapshot_rows_match_resolved_parameters( + rows=[{"DT": "0026-08-24 00:00:00", "ORDER_COUNT": 0}], + filters=filters, + resolved_params={ + "start_date": date(2026, 8, 24), + "end_date": date(2026, 8, 31), + }, + ) + + +def test_snapshot_integrity_accepts_empty_and_in_window_results() -> None: + filters = compile_snapshot_filters( + { + "start_date": { + "type": "date", + "snapshot_filter": {"column": "DT", "operator": "gte"}, + }, + "end_date": { + "type": "date", + "snapshot_filter": {"column": "DT", "operator": "lte"}, + }, + } + ) + resolved_params = { + "start_date": date(2026, 8, 24), + "end_date": date(2026, 8, 31), + } + + validate_snapshot_rows_match_resolved_parameters( + rows=[], + filters=filters, + resolved_params=resolved_params, + ) + validate_snapshot_rows_match_resolved_parameters( + rows=[{"DT": "2026-08-24 17:30:00", "ORDER_COUNT": 4}], + filters=filters, + resolved_params=resolved_params, + ) + + async def _seed_snapshot_endpoint( client: AsyncClient, session: AsyncSession, @@ -372,6 +431,40 @@ async def test_snapshot_returns_empty_data_for_no_matches_inside_coverage( assert response.json()["meta"]["row_count"] == 0 +@pytest.mark.integration +async def test_snapshot_rejects_retained_rows_that_contradict_resolved_coverage( + async_client: object, + db_session: AsyncSession, +) -> None: + client: AsyncClient = async_client # type: ignore[assignment] + path = await _seed_snapshot_endpoint(client, db_session) + endpoint = ( + await db_session.execute(select(ApiEndpoint).where(ApiEndpoint.path == path)) + ).scalar_one() + snapshot = ( + await db_session.execute(select(Snapshot).where(Snapshot.endpoint_id == endpoint.id)) + ).scalar_one() + snapshot.data = [{"business_date": "0026-08-20 00:00:00", "store_id": 2, "amount": 20}] + snapshot.row_count = 1 + await db_session.flush() + + with capture_logs() as logs: + response = await client.get( + f"/api/v1/data/{path}", + params={"start_date": "2026-08-20", "end_date": "2026-08-20"}, + ) + + assert response.status_code == 503 + assert response.json() == { + "code": "snapshot_integrity_failed", + "detail": "No retained snapshot passed integrity validation.", + } + rejection = next(entry for entry in logs if entry.get("event") == "snapshot_integrity_failed") + assert rejection["status"] == 503 + assert rejection["snapshot_ids"] == [str(snapshot.id)] + assert "1 of 1 cached rows do not match" in rejection["integrity_errors"][0] + + @pytest.mark.integration async def test_snapshot_selects_newest_retained_snapshot_that_covers_request( async_client: object, diff --git a/docs/architecture.md b/docs/architecture.md index 89548a7..1710007 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -87,6 +87,13 @@ binding sources are fixed literal, explicit SQL `NULL` for an optional bind, log relative logical date, and inclusive window start/end. Supported windows are previous day, last N complete days, week to date, previous week, month to date, and previous month. +After final output-column mapping and before persistence, each non-empty scheduled result must +match its resolved schedule parameters under the endpoint's declared `eq`, `gte`, and `lte` +snapshot filters. An inconsistent or unparseable result fails the job without replacing retained +valid snapshots; the scheduler never guesses or rewrites source values. The data plane revalidates +retained candidates, falls back to an older valid covering snapshot when possible, and returns an +explicit HTTP 503 integrity error when none is usable. + Each parameterized snapshot endpoint also declares an explicit final cached-output-column mapping and one whitelisted operator (`eq`, `gte`, or `lte`). The data plane validates required fields and types, rejects reversed ranges, selects the newest retained job snapshot whose persisted resolved diff --git a/docs/operations.md b/docs/operations.md index 1c0dfe8..01ae59e 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -252,12 +252,22 @@ the next resolved logical dates and typed SQL values before saving a schedule. schedule bindings, window, timezone, and snapshot retention. - `snapshot_filter_column_unavailable` means a configured mapped column is absent from the non-empty cached payload. + - `snapshot_integrity_failed` means every covering retained snapshot contains mapped values + that contradict its persisted schedule parameters. Inspect the structured log's + `snapshot_ids` and `integrity_errors` fields. - A covered request with no matching business rows is successful and returns `data: []`. - No retained snapshot returns HTTP 503 rather than a coverage error. 5. **Check snapshot staleness**: For snapshot endpoints, compare `snapshot_created_at`, `snapshot_row_count`, and the filtered `row_count` response metadata. -6. **Check SQL syntax**: Use the SQL preview feature with temporary sample values. Ensure bind - placeholders are not enclosed in single quotes. +6. **Check refresh integrity failures**: A scheduled result whose mapped values are malformed or + outside the resolved schedule bounds is marked failed and is not stored. Inspect the job's + `error_detail`, correct SQL that reconverts already typed binds (for example, unnecessary + `TO_DATE(:date_param, ...)` calls), and rerun the schedule. Previously retained valid snapshots + remain available. +7. **Check SQL preview types**: Select the correct type beside every temporary sample value. The + backend converts preview samples through the endpoint parameter model before Oracle execution, + so a date bind reaches Oracle as a native date. Keep bind placeholders outside single quotes + and do not reconvert native date binds with `TO_DATE`. See [Endpoint, scheduler, and snapshot parameter contracts](scheduler_parameter_bindings.md) for the complete selection and coverage rules. diff --git a/docs/scheduler_parameter_bindings.md b/docs/scheduler_parameter_bindings.md index 45f8f79..b5460b5 100644 --- a/docs/scheduler_parameter_bindings.md +++ b/docs/scheduler_parameter_bindings.md @@ -21,8 +21,12 @@ QueryGateway treats the same SQL bind name differently depending on where it is `NULL`. - Date requests accept `YYYY-MM-DD` and `DD-MM-YYYY` and normalize to a Python `date` before binding. Boolean requests accept `true`, `false`, `1`, `0`, `yes`, or `no`. -- Preview sample values are request-local and never become endpoint defaults or schedule - bindings. +- In SQL preview, choose each bind's declared type before running the query. Preview sends the + temporary values and typed schema together; the backend validates and converts them through the + same parameter model used by published endpoints before Oracle execution. +- Preview sample values are request-local and never become endpoint defaults or schedule bindings. + Keep native date binds such as `:start_date` in SQL; do not wrap an already typed date bind in + `TO_DATE(...)`. For example, a required date range and optional store filter are supplied as ordinary query parameters: @@ -100,10 +104,12 @@ window boundaries, and resolved typed parameters. ## Snapshot request filters and coverage -Schedule bindings decide which rows Oracle loads into a snapshot. Authenticated data-request -parameters decide which rows are returned from that cache. Every parameterized snapshot endpoint -must explicitly map each request parameter to a cached output column after `column_map` renaming -and one whitelisted comparison: +Schedule bindings decide which rows Oracle loads into a snapshot. Snapshot request filters do a +different job: they decide which of those cached rows an authenticated API request receives. Every +parameterized snapshot endpoint must map each request parameter to the final cached output column +after `column_map` renaming and one whitelisted comparison. For example, `:start_date` and +`:end_date` can both map to `DT` with `gte` and `lte`, while `:store_id` maps to `STR_NO` +with `eq`: | Operator | Row selection | Coverage requirement | |---|---|---| @@ -153,6 +159,16 @@ persisted job-run parameters cover the complete request. It then applies every c to the cached rows using the parameter's declared type. Date columns containing Oracle DATE/TIMESTAMP ISO strings are normalized to dates before comparison. Behavior is explicit: +- Before persistence, every non-empty scheduled result is validated against the schedule's + resolved parameters using the endpoint's final cached-column mappings. A missing mapped column, + an unparseable mapped value, or a row outside an `eq`, `gte`, or `lte` bound fails the job and no + new snapshot is stored. Previously retained valid snapshots remain available. The scheduler does + not guess or rewrite malformed values such as a cached year `0026`; correct the SQL bind handling + and rerun the schedule. +- Retained snapshots are revalidated when selected. An inconsistent candidate is skipped when an + older valid snapshot covers the request. If every covering candidate fails integrity validation, + the request returns HTTP 503 with `code=snapshot_integrity_failed`; structured logs include the + rejected snapshot IDs and integrity errors. - Missing or invalid required parameters return HTTP 422 with the field in `detail`. - Lower and upper mappings for the same cached column must declare the same parameter type. - When multiple parameters provide bounds for the same cached column, validation retains every @@ -164,6 +180,8 @@ DATE/TIMESTAMP ISO strings are normalized to dates before comparison. Behavior i - A request inside coverage with no matching business rows returns HTTP 200 with `data: []`. - An endpoint created before this contract without complete mappings returns HTTP 422 with `code=snapshot_filter_not_configured`; add mappings through the endpoint edit dialog or admin update API. - A mapping that does not exist in a non-empty cached row returns HTTP 422 with `code=snapshot_filter_column_unavailable`. +- If no covering retained snapshot passes row-integrity validation, the request returns HTTP 503 + with `code=snapshot_integrity_failed`. - No retained snapshot still returns HTTP 503. Representative stable error bodies are: @@ -182,6 +200,13 @@ Representative stable error bodies are: } ``` +```json +{ + "code": "snapshot_integrity_failed", + "detail": "No retained snapshot passed integrity validation." +} +``` + The mapping is stored inside the endpoint's existing JSON parameter schema, so it requires no relational database migration. Schedule-owned bindings, timezone, logical-run fields, resolved parameter audit data, and the `(schedule_id, scheduled_for)` idempotency constraint were added by diff --git a/docs/security_checklist.md b/docs/security_checklist.md index 5c4f179..157737f 100644 --- a/docs/security_checklist.md +++ b/docs/security_checklist.md @@ -40,7 +40,7 @@ Comprehensive security validation for QueryGateway production deployments. All i | 22 | Template interpolation rejected (`${`, `{var}`) | Verified | Pattern included in safety validation | | 23 | SQL validation runs on both create and update | Verified | `EndpointCreate` and `EndpointUpdate` share the validator | | 24 | SQL preview also validates before execution | Verified | `SqlPreviewRequest` includes the same validator | -| 25 | Parameters coerced through typed schemas before SQL execution | Verified | `build_param_model()` creates the Pydantic request model; `DataService` enforces required fields before binding | +| 25 | Parameters coerced through typed schemas before SQL execution | Verified | `build_param_model()` creates the Pydantic request model; `DataService` and typed admin SQL preview validate values before binding | | 26 | SQLAlchemy `text()` with bind dict used for execution | Verified | `sql/executor.py` uses parameterized execution | ## Input Validation diff --git a/frontend/src/components/endpoints/EndpointWizard.test.tsx b/frontend/src/components/endpoints/EndpointWizard.test.tsx index 1412225..b9f4ce6 100644 --- a/frontend/src/components/endpoints/EndpointWizard.test.tsx +++ b/frontend/src/components/endpoints/EndpointWizard.test.tsx @@ -88,6 +88,13 @@ describe("EndpointWizard preview coordination", () => { await waitFor(() => expect(previewMock).toHaveBeenCalledOnce()); expect(previewMock.mock.calls[0][0]).toMatchObject({ params: { customer_id: null }, + param_schema: { + customer_id: expect.objectContaining({ + type: "string", + required: false, + default_is_null: true, + }), + }, }); }); @@ -104,6 +111,31 @@ describe("EndpointWizard preview coordination", () => { await waitFor(() => expect(previewMock).toHaveBeenCalledOnce()); expect(previewMock.mock.calls[0][0].params.customer_id).toMatch(/^\d{4}-\d{2}-\d{2}$/); + expect(previewMock.mock.calls[0][0].param_schema.customer_id.type).toBe("date"); + }); + + it("sends an inline preview type with the sample value", async () => { + renderWizard(); + fireEvent.click(await screen.findByRole("button", { name: /Test Oracle/ })); + fireEvent.click(screen.getByRole("button", { name: "Next" })); + fireEvent.change(screen.getByLabelText("SQL query"), { + target: { value: "SELECT * FROM orders WHERE business_date >= :start_date" }, + }); + fireEvent.change(screen.getByLabelText("Preview type for start_date"), { + target: { value: "date" }, + }); + fireEvent.change(screen.getByLabelText(":start_date"), { + target: { value: "2026-08-30" }, + }); + fireEvent.click(screen.getByRole("button", { name: "Preview Query" })); + + await waitFor(() => expect(previewMock).toHaveBeenCalledOnce()); + expect(previewMock.mock.calls[0][0]).toMatchObject({ + params: { start_date: "2026-08-30" }, + param_schema: { + start_date: expect.objectContaining({ type: "date", required: true }), + }, + }); }); it("requires snapshot row mappings before review", async () => { diff --git a/frontend/src/components/endpoints/EndpointWizard.tsx b/frontend/src/components/endpoints/EndpointWizard.tsx index 15ab32c..59c952c 100644 --- a/frontend/src/components/endpoints/EndpointWizard.tsx +++ b/frontend/src/components/endpoints/EndpointWizard.tsx @@ -73,6 +73,7 @@ export function EndpointWizard({ onSuccess, onCancel }: EndpointWizardProps) { return [name, value]; }), ), + param_schema: state.param_schema, max_rows: 10, }), onSuccess: (data) => { @@ -137,6 +138,14 @@ export function EndpointWizard({ onSuccess, onCancel }: EndpointWizardProps) { param_schema: { ...s.param_schema, [name]: descriptor }, }; }); + if (field === "type") { + setPreviewParams((current) => { + const next = { ...current }; + delete next[name]; + return next; + }); + } + setPreview(null); }, []); const canNext = (): boolean => { @@ -216,6 +225,7 @@ export function EndpointWizard({ onSuccess, onCancel }: EndpointWizardProps) { previewParams={previewParams} isPreviewing={previewMutation.isPending} onPreview={() => previewMutation.mutate()} + onUpdateParam={updateParam} onUpdatePreviewParam={(name, value) => setPreviewParams((current) => ({ ...current, [name]: value })) } diff --git a/frontend/src/components/endpoints/SnapshotFilterMappings.tsx b/frontend/src/components/endpoints/SnapshotFilterMappings.tsx index 5214a99..1a14a25 100644 --- a/frontend/src/components/endpoints/SnapshotFilterMappings.tsx +++ b/frontend/src/components/endpoints/SnapshotFilterMappings.tsx @@ -40,9 +40,25 @@ export function SnapshotFilterMappings({

Snapshot request filters

- Map every request parameter to a cached output column. These mappings select rows; they do - not grant tenant access. + Define how each API request parameter filters rows already stored in a snapshot. For each + parameter, choose the matching cached output column and comparison. Use the final column + name returned by the query after any output-column renaming.

+
+

+ Example: map :start_date to DT with From / minimum,{" "} + :end_date to DT with To / maximum, and :store_id{" "} + to STR_NO with Equals. +

+

+ From / minimum keeps rows on or after the request value; To / maximum keeps rows on or + before it. Equals keeps exact matches. +

+

+ These mappings filter cached rows only. They do not change what the schedule loads and + do not grant tenant access. +

+
{Object.entries(paramSchema).map(([name, descriptor]) => { const mapping = descriptor.snapshot_filter; diff --git a/frontend/src/components/endpoints/wizard/ConfigStep.test.tsx b/frontend/src/components/endpoints/wizard/ConfigStep.test.tsx index a2a8d4b..14e48e9 100644 --- a/frontend/src/components/endpoints/wizard/ConfigStep.test.tsx +++ b/frontend/src/components/endpoints/wizard/ConfigStep.test.tsx @@ -106,6 +106,9 @@ describe("ConfigStep snapshot scheduling guidance", () => { ); expect(screen.getByText(/Snapshot request filters/i)).toBeInTheDocument(); + expect(screen.getByText(/filters rows already stored in a snapshot/i)).toBeInTheDocument(); + expect(screen.getByText(/From \/ minimum keeps rows on or after/i)).toBeInTheDocument(); + expect(screen.getByText(/do not change what the schedule loads/i)).toBeInTheDocument(); fireEvent.change(screen.getByLabelText("Snapshot column for start_date"), { target: { value: "business_date" }, }); diff --git a/frontend/src/components/endpoints/wizard/SqlStep.test.tsx b/frontend/src/components/endpoints/wizard/SqlStep.test.tsx index 5e6dd03..95218e9 100644 --- a/frontend/src/components/endpoints/wizard/SqlStep.test.tsx +++ b/frontend/src/components/endpoints/wizard/SqlStep.test.tsx @@ -37,6 +37,7 @@ describe("SqlStep preview parameters", () => { previewParams={{}} isPreviewing={false} onPreview={vi.fn()} + onUpdateParam={vi.fn()} onUpdatePreviewParam={vi.fn()} />, ); @@ -56,6 +57,7 @@ describe("SqlStep preview parameters", () => { previewParams={{}} isPreviewing={false} onPreview={onPreview} + onUpdateParam={vi.fn()} onUpdatePreviewParam={onUpdatePreviewParam} />, ); @@ -71,6 +73,7 @@ describe("SqlStep preview parameters", () => { previewParams={{ customer_id: "42" }} isPreviewing={false} onPreview={onPreview} + onUpdateParam={vi.fn()} onUpdatePreviewParam={onUpdatePreviewParam} />, ); @@ -95,6 +98,7 @@ describe("SqlStep preview parameters", () => { previewParams={{}} isPreviewing={false} onPreview={vi.fn()} + onUpdateParam={vi.fn()} onUpdatePreviewParam={vi.fn()} />, ); @@ -119,10 +123,53 @@ describe("SqlStep preview parameters", () => { previewParams={{}} isPreviewing={false} onPreview={vi.fn()} + onUpdateParam={vi.fn()} onUpdatePreviewParam={vi.fn()} />, ); expect(screen.getByRole("button", { name: "Preview Query" })).toBeEnabled(); }); + + it("lets the author select a date type before the first preview", () => { + const onUpdateParam = vi.fn(); + const state = makeState(); + const { rerender } = render( + , + ); + + fireEvent.change(screen.getByLabelText("Preview type for customer_id"), { + target: { value: "date" }, + }); + expect(onUpdateParam).toHaveBeenCalledWith("customer_id", "type", "date"); + + state.param_schema.customer_id = { + type: "date", + required: true, + default: null, + }; + rerender( + , + ); + + expect(screen.getByLabelText(":customer_id")).toHaveAttribute("type", "date"); + }); }); diff --git a/frontend/src/components/endpoints/wizard/SqlStep.tsx b/frontend/src/components/endpoints/wizard/SqlStep.tsx index f55f91c..59a3c71 100644 --- a/frontend/src/components/endpoints/wizard/SqlStep.tsx +++ b/frontend/src/components/endpoints/wizard/SqlStep.tsx @@ -2,9 +2,10 @@ import { Button } from "@/components/ui/button"; import { SqlEditor } from "@/components/endpoints/SqlEditor"; import { Input } from "@/components/ui/input"; import { Label } from "@/components/ui/label"; -import type { SqlPreviewResponse } from "@/types/endpoint"; +import { Select } from "@/components/ui/select"; +import type { ParamDescriptor, SqlPreviewResponse } from "@/types/endpoint"; -import { hasParameterDefault } from "./parameterDefaults"; +import { hasParameterDefault, resolvePreviewParameterDefault } from "./parameterDefaults"; import type { WizardState, WizardUpdate } from "./types"; interface SqlStepProps { @@ -14,6 +15,7 @@ interface SqlStepProps { previewParams: Record; isPreviewing: boolean; onPreview: () => void; + onUpdateParam: (name: string, field: keyof ParamDescriptor, value: unknown) => void; onUpdatePreviewParam: (name: string, value: string) => void; } @@ -24,6 +26,7 @@ export function SqlStep({ previewParams, isPreviewing, onPreview, + onUpdateParam, onUpdatePreviewParam, }: SqlStepProps) { const bindParams = Object.entries(state.param_schema); @@ -45,28 +48,83 @@ export function SqlStep({

Preview parameters

- Enter a sample value for each bind parameter. These values are used only for this - preview and are not saved as endpoint defaults. + Choose the Oracle bind type and enter a sample value for each parameter. Values are + validated and typed before execution, used only for this preview, and not saved as + endpoint defaults.

-
- {bindParams.map(([name, descriptor]) => ( -
- - onUpdatePreviewParam(name, event.target.value)} - placeholder="Sample value" - /> -
- ))} +
+ {bindParams.map(([name, descriptor]) => { + const typeInputId = `preview-param-type-${name}`; + const valueInputId = `preview-param-${name}`; + const resolvedDefault = resolvePreviewParameterDefault(descriptor); + const value = + previewParams[name] ?? + (resolvedDefault !== null && resolvedDefault !== undefined + ? String(resolvedDefault) + : ""); + + return ( +
+

:{name}

+
+
+ + +
+
+ + {descriptor.type === "boolean" ? ( + + ) : ( + onUpdatePreviewParam(name, event.target.value)} + placeholder="Sample value" + /> + )} +
+
+
+ ); + })}
)} diff --git a/frontend/src/types/endpoint.ts b/frontend/src/types/endpoint.ts index 544dbf5..a4bb798 100644 --- a/frontend/src/types/endpoint.ts +++ b/frontend/src/types/endpoint.ts @@ -75,6 +75,7 @@ export interface SqlPreviewRequest { connection_id: string; sql_text: string; params?: Record; + param_schema?: Record; max_rows?: number; } From b03ef5f9b81954b92d7a0d44b052d056f4cf8031 Mon Sep 17 00:00:00 2001 From: Badry Date: Tue, 1 Sep 2026 17:06:44 +0300 Subject: [PATCH 2/7] fix(ci): satisfy strict snapshot typing --- backend/app/services/snapshot_filtering.py | 7 ++++--- backend/tests/test_endpoints.py | 4 +++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/backend/app/services/snapshot_filtering.py b/backend/app/services/snapshot_filtering.py index e7e96ef..8953857 100644 --- a/backend/app/services/snapshot_filtering.py +++ b/backend/app/services/snapshot_filtering.py @@ -1,6 +1,7 @@ """Typed request filtering for persisted snapshot rows.""" import json +from collections.abc import Mapping from dataclasses import dataclass from datetime import datetime from functools import lru_cache @@ -96,8 +97,8 @@ def _coerce_cached_row_value(item: CompiledSnapshotFilter, value: object) -> Any def snapshot_covers_request( *, filters: tuple[CompiledSnapshotFilter, ...], - request_params: dict[str, object], - resolved_params: dict[str, object], + request_params: Mapping[str, object], + resolved_params: Mapping[str, object], ) -> bool: """Return whether one snapshot job run contains the requested selection.""" for item in filters: @@ -228,7 +229,7 @@ def validate_snapshot_rows_match_resolved_parameters( *, rows: list[dict[str, object]], filters: tuple[CompiledSnapshotFilter, ...], - resolved_params: dict[str, object], + resolved_params: Mapping[str, object], ) -> None: """Reject non-empty snapshot results that contradict their resolved schedule bounds.""" if not rows: diff --git a/backend/tests/test_endpoints.py b/backend/tests/test_endpoints.py index 16d7bd6..bb1d114 100644 --- a/backend/tests/test_endpoints.py +++ b/backend/tests/test_endpoints.py @@ -346,7 +346,9 @@ async def test_sql_preview_coerces_date_before_oracle_execution( ) execute_query.assert_awaited_once() - assert execute_query.await_args.kwargs["params"] == {"start_date": date(2026, 8, 30)} + execute_call = execute_query.await_args + assert execute_call is not None + assert execute_call.kwargs["params"] == {"start_date": date(2026, 8, 30)} @pytest.mark.asyncio From e9e3e2a767244557e62f93ee9cc7d8b77f178278 Mon Sep 17 00:00:00 2001 From: Badry Date: Tue, 1 Sep 2026 17:24:42 +0300 Subject: [PATCH 3/7] fix(review): enforce typed previews and snapshot payloads --- backend/app/schemas/endpoint.py | 14 ++++---- backend/app/services/data.py | 9 +++-- backend/tests/test_endpoints.py | 17 ++++++++++ .../tests/test_snapshot_request_filtering.py | 33 ++++++++++++++++++- docs/scheduler_parameter_bindings.md | 7 ++-- frontend/src/types/endpoint.ts | 2 +- 6 files changed, 68 insertions(+), 14 deletions(-) diff --git a/backend/app/schemas/endpoint.py b/backend/app/schemas/endpoint.py index 79f1968..aa29973 100644 --- a/backend/app/schemas/endpoint.py +++ b/backend/app/schemas/endpoint.py @@ -391,8 +391,8 @@ class SqlPreviewRequest(BaseModel): param_schema: dict[str, ParamDescriptor] = Field( default_factory=dict, description=( - "Optional typed descriptors for preview bind parameters. When supplied, the schema " - "must match the SQL bind names and values are coerced before Oracle execution." + "Typed descriptors for preview bind parameters. Bind-bearing SQL requires an exact " + "schema match so values can be coerced before Oracle execution." ), ) max_rows: int = Field(10, ge=1, le=100) @@ -407,13 +407,15 @@ def validate_sql(cls, v: str) -> str: @model_validator(mode="after") def typed_schema_matches_bind_params(self) -> Self: - # ``param_schema`` is additive for compatibility with existing admin - # API clients. New callers should always send it when SQL has binds so - # preview uses the same typed-value contract as published endpoints. + sql_params = set(extract_bind_params(self.sql_text)) if not self.param_schema: + if sql_params: + raise ValueError( + "SQL preview bind parameters require typed schema descriptors for: " + f"{sorted(sql_params)}" + ) return self - sql_params = set(extract_bind_params(self.sql_text)) schema_params = set(self.param_schema) undeclared = sql_params - schema_params unused = schema_params - sql_params diff --git a/backend/app/services/data.py b/backend/app/services/data.py index 60f8dd0..04e032f 100644 --- a/backend/app/services/data.py +++ b/backend/app/services/data.py @@ -370,9 +370,12 @@ async def _serve_snapshot( ): continue - candidate_data: list[dict[str, object]] = ( - candidate.data if isinstance(candidate.data, list) else [] - ) + if not isinstance(candidate.data, list): + integrity_failures.append( + (str(candidate.id), "Snapshot payload is not a row array.") + ) + continue + candidate_data = candidate.data if unavailable_snapshot_filter_columns(rows=candidate_data, filters=filters): # Preserve the existing configuration-error response below. An endpoint edit # can invalidate mappings even when the stored snapshot itself was valid. diff --git a/backend/tests/test_endpoints.py b/backend/tests/test_endpoints.py index bb1d114..03b4b17 100644 --- a/backend/tests/test_endpoints.py +++ b/backend/tests/test_endpoints.py @@ -323,6 +323,23 @@ def test_sql_preview_request_rejects_typed_schema_mismatch() -> None: ) +def test_sql_preview_request_requires_typed_schema_for_binds() -> None: + with pytest.raises(ValueError, match="require typed schema descriptors"): + SqlPreviewRequest( + connection_id=uuid.uuid4(), + sql_text="SELECT * FROM employees WHERE hired_on >= :start_date", + params={"start_date": "2026-08-30"}, + ) + + +def test_sql_preview_request_allows_no_schema_without_binds() -> None: + payload = SqlPreviewRequest( + connection_id=uuid.uuid4(), + sql_text="SELECT 1 FROM dual", + ) + assert payload.param_schema == {} + + @pytest.mark.asyncio async def test_sql_preview_coerces_date_before_oracle_execution( monkeypatch: pytest.MonkeyPatch, diff --git a/backend/tests/test_snapshot_request_filtering.py b/backend/tests/test_snapshot_request_filtering.py index 314d2f8..23508b6 100644 --- a/backend/tests/test_snapshot_request_filtering.py +++ b/backend/tests/test_snapshot_request_filtering.py @@ -16,7 +16,7 @@ validate_snapshot_rows_match_resolved_parameters, ) from httpx import AsyncClient -from sqlalchemy import select +from sqlalchemy import select, update from sqlalchemy.ext.asyncio import AsyncSession from structlog.testing import capture_logs @@ -465,6 +465,37 @@ async def test_snapshot_rejects_retained_rows_that_contradict_resolved_coverage( assert "1 of 1 cached rows do not match" in rejection["integrity_errors"][0] +@pytest.mark.integration +async def test_snapshot_rejects_retained_non_array_payload( + async_client: object, + db_session: AsyncSession, +) -> None: + client: AsyncClient = async_client # type: ignore[assignment] + path = await _seed_snapshot_endpoint(client, db_session) + endpoint = ( + await db_session.execute(select(ApiEndpoint).where(ApiEndpoint.path == path)) + ).scalar_one() + snapshot = ( + await db_session.execute(select(Snapshot).where(Snapshot.endpoint_id == endpoint.id)) + ).scalar_one() + await db_session.execute( + update(Snapshot).where(Snapshot.id == snapshot.id).values(data={"unexpected": "object"}) + ) + await db_session.flush() + + with capture_logs() as logs: + response = await client.get( + f"/api/v1/data/{path}", + params={"start_date": "2026-08-20", "end_date": "2026-08-20"}, + ) + + assert response.status_code == 503 + assert response.json()["code"] == "snapshot_integrity_failed" + rejection = next(entry for entry in logs if entry.get("event") == "snapshot_integrity_failed") + assert rejection["snapshot_ids"] == [str(snapshot.id)] + assert rejection["integrity_errors"] == ["Snapshot payload is not a row array."] + + @pytest.mark.integration async def test_snapshot_selects_newest_retained_snapshot_that_covers_request( async_client: object, diff --git a/docs/scheduler_parameter_bindings.md b/docs/scheduler_parameter_bindings.md index b5460b5..308e792 100644 --- a/docs/scheduler_parameter_bindings.md +++ b/docs/scheduler_parameter_bindings.md @@ -21,9 +21,10 @@ QueryGateway treats the same SQL bind name differently depending on where it is `NULL`. - Date requests accept `YYYY-MM-DD` and `DD-MM-YYYY` and normalize to a Python `date` before binding. Boolean requests accept `true`, `false`, `1`, `0`, `yes`, or `no`. -- In SQL preview, choose each bind's declared type before running the query. Preview sends the - temporary values and typed schema together; the backend validates and converts them through the - same parameter model used by published endpoints before Oracle execution. +- In SQL preview, choose each bind's declared type before running the query. A preview query that + contains binds is rejected unless its typed schema describes every detected bind exactly. The + backend then validates and converts the temporary values through the same parameter model used + by published endpoints before Oracle execution. - Preview sample values are request-local and never become endpoint defaults or schedule bindings. Keep native date binds such as `:start_date` in SQL; do not wrap an already typed date bind in `TO_DATE(...)`. diff --git a/frontend/src/types/endpoint.ts b/frontend/src/types/endpoint.ts index a4bb798..682be11 100644 --- a/frontend/src/types/endpoint.ts +++ b/frontend/src/types/endpoint.ts @@ -75,7 +75,7 @@ export interface SqlPreviewRequest { connection_id: string; sql_text: string; params?: Record; - param_schema?: Record; + param_schema: Record; max_rows?: number; } From 0bc76591c5baf49d15485304d2d6ea2ed2f173d1 Mon Sep 17 00:00:00 2001 From: Badry Date: Tue, 1 Sep 2026 17:42:53 +0300 Subject: [PATCH 4/7] fix(review): ignore SQL comments in bind discovery --- backend/app/schemas/endpoint.py | 9 ++++++--- backend/tests/test_endpoints.py | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/backend/app/schemas/endpoint.py b/backend/app/schemas/endpoint.py index aa29973..afc4664 100644 --- a/backend/app/schemas/endpoint.py +++ b/backend/app/schemas/endpoint.py @@ -18,6 +18,10 @@ # Regex to find named bind parameters in Oracle SQL (:param_name). _BIND_PARAM_RE = re.compile(r":([A-Za-z_]\w*)") +_SQL_NON_CODE_RE = re.compile( + r"'(?:''|[^'])*'|\"(?:\"\"|[^\"])*\"|--[^\r\n]*|/\*.*?\*/", + re.DOTALL, +) # Reject obvious string-interpolation patterns that bypass bind variables. _UNSAFE_PATTERNS = [ @@ -51,9 +55,8 @@ class SnapshotConfigurationError(ValueError): def extract_bind_params(sql: str) -> list[str]: - """Return deduplicated bind parameter names from SQL text.""" - # Exclude matches inside single-quoted string literals. - cleaned = re.sub(r"'[^']*'", "", sql) + """Return deduplicated binds outside SQL literals, identifiers, and comments.""" + cleaned = _SQL_NON_CODE_RE.sub(" ", sql) return list(dict.fromkeys(_BIND_PARAM_RE.findall(cleaned))) diff --git a/backend/tests/test_endpoints.py b/backend/tests/test_endpoints.py index 03b4b17..5e65d14 100644 --- a/backend/tests/test_endpoints.py +++ b/backend/tests/test_endpoints.py @@ -43,6 +43,16 @@ def test_extract_bind_params_ignores_strings() -> None: assert params == ["name"] +def test_extract_bind_params_ignores_line_comments() -> None: + sql = "SELECT * FROM t WHERE id = :id -- ignore :debug\nAND status = :status" + assert extract_bind_params(sql) == ["id", "status"] + + +def test_extract_bind_params_ignores_block_comments() -> None: + sql = "SELECT /* ignore :debug */ * FROM t WHERE id = :id /* ignore :trace */" + assert extract_bind_params(sql) == ["id"] + + def test_extract_bind_params_empty() -> None: sql = "SELECT 1 FROM dual" params = extract_bind_params(sql) @@ -340,6 +350,14 @@ def test_sql_preview_request_allows_no_schema_without_binds() -> None: assert payload.param_schema == {} +def test_sql_preview_request_ignores_commented_bind_tokens() -> None: + payload = SqlPreviewRequest( + connection_id=uuid.uuid4(), + sql_text="SELECT 1 FROM dual -- :debug\n/* :trace */", + ) + assert payload.param_schema == {} + + @pytest.mark.asyncio async def test_sql_preview_coerces_date_before_oracle_execution( monkeypatch: pytest.MonkeyPatch, From 3b493ebe35238f359f97b90be77b41ba66f69d46 Mon Sep 17 00:00:00 2001 From: Badry Date: Tue, 1 Sep 2026 18:08:06 +0300 Subject: [PATCH 5/7] fix(review): support Oracle alternative quoting --- backend/app/schemas/endpoint.py | 10 +++++++++- backend/tests/test_endpoints.py | 25 +++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/backend/app/schemas/endpoint.py b/backend/app/schemas/endpoint.py index afc4664..b581cdb 100644 --- a/backend/app/schemas/endpoint.py +++ b/backend/app/schemas/endpoint.py @@ -18,8 +18,16 @@ # Regex to find named bind parameters in Oracle SQL (:param_name). _BIND_PARAM_RE = re.compile(r":([A-Za-z_]\w*)") +_ORACLE_ALT_QUOTED_LITERAL_PATTERN = ( + r"(?'|" + r"'(?P[^\s\[\{\(<'])(?:(?!(?P=q_delimiter)').)*" + r"(?P=q_delimiter)'" + r")" +) _SQL_NON_CODE_RE = re.compile( - r"'(?:''|[^'])*'|\"(?:\"\"|[^\"])*\"|--[^\r\n]*|/\*.*?\*/", + _ORACLE_ALT_QUOTED_LITERAL_PATTERN + + r"|'(?:''|[^'])*'|\"(?:\"\"|[^\"])*\"|--[^\r\n]*|/\*.*?\*/", re.DOTALL, ) diff --git a/backend/tests/test_endpoints.py b/backend/tests/test_endpoints.py index 5e65d14..521ac0d 100644 --- a/backend/tests/test_endpoints.py +++ b/backend/tests/test_endpoints.py @@ -53,6 +53,23 @@ def test_extract_bind_params_ignores_block_comments() -> None: assert extract_bind_params(sql) == ["id"] +@pytest.mark.parametrize( + "literal", + [ + "q'[John's :debug]'", + "q'{ignore :debug}'", + "q'(ignore :debug)'", + "Q''", + "q'!ignore :debug!'", + "nq'[national :debug]'", + "q''ignore :debug''", + ], +) +def test_extract_bind_params_ignores_oracle_alternative_quoted_literals(literal: str) -> None: + sql = f"SELECT {literal} FROM dual WHERE id = :id" + assert extract_bind_params(sql) == ["id"] + + def test_extract_bind_params_empty() -> None: sql = "SELECT 1 FROM dual" params = extract_bind_params(sql) @@ -358,6 +375,14 @@ def test_sql_preview_request_ignores_commented_bind_tokens() -> None: assert payload.param_schema == {} +def test_sql_preview_request_ignores_bind_tokens_in_oracle_alternative_quotes() -> None: + payload = SqlPreviewRequest( + connection_id=uuid.uuid4(), + sql_text="SELECT q'[John's :debug]' FROM dual", + ) + assert payload.param_schema == {} + + @pytest.mark.asyncio async def test_sql_preview_coerces_date_before_oracle_execution( monkeypatch: pytest.MonkeyPatch, From caf01bb2097538d08c90d28536f86a0ea0266831 Mon Sep 17 00:00:00 2001 From: Badry Date: Tue, 1 Sep 2026 22:09:45 +0300 Subject: [PATCH 6/7] fix(security): make SQL bind discovery linear --- backend/app/schemas/endpoint.py | 93 ++++++++++++++++++++++++++++----- backend/tests/test_endpoints.py | 27 ++++++++++ docs/security_checklist.md | 5 +- 3 files changed, 110 insertions(+), 15 deletions(-) diff --git a/backend/app/schemas/endpoint.py b/backend/app/schemas/endpoint.py index b581cdb..982ee59 100644 --- a/backend/app/schemas/endpoint.py +++ b/backend/app/schemas/endpoint.py @@ -18,18 +18,7 @@ # Regex to find named bind parameters in Oracle SQL (:param_name). _BIND_PARAM_RE = re.compile(r":([A-Za-z_]\w*)") -_ORACLE_ALT_QUOTED_LITERAL_PATTERN = ( - r"(?'|" - r"'(?P[^\s\[\{\(<'])(?:(?!(?P=q_delimiter)').)*" - r"(?P=q_delimiter)'" - r")" -) -_SQL_NON_CODE_RE = re.compile( - _ORACLE_ALT_QUOTED_LITERAL_PATTERN - + r"|'(?:''|[^'])*'|\"(?:\"\"|[^\"])*\"|--[^\r\n]*|/\*.*?\*/", - re.DOTALL, -) +_ORACLE_ALT_QUOTE_PAIRS = {"[": "]", "{": "}", "(": ")", "<": ">"} # Reject obvious string-interpolation patterns that bypass bind variables. _UNSAFE_PATTERNS = [ @@ -62,9 +51,87 @@ class SnapshotConfigurationError(ValueError): """Raised when a snapshot endpoint cannot execute without request inputs.""" +def _is_oracle_identifier_char(value: str) -> bool: + """Match the identifier boundary used by Oracle alternative-quote prefixes.""" + return (value.isascii() and value.isalnum()) or value in "_$#" + + +def _quoted_region_end(sql: str, start: int, quote: str) -> int: + """Return the end of a standard SQL literal or quoted identifier.""" + index = start + 1 + while index < len(sql): + if sql[index] != quote: + index += 1 + continue + if index + 1 < len(sql) and sql[index + 1] == quote: + index += 2 + continue + return index + 1 + return len(sql) + + +def _oracle_alt_quote_end(sql: str, start: int) -> int | None: + """Return the end of a Q/NQ literal, or ``None`` when no prefix starts here.""" + if start > 0 and _is_oracle_identifier_char(sql[start - 1]): + return None + + quote_index: int + if sql[start] in "qQ": + quote_index = start + 1 + elif sql[start] in "nN" and start + 1 < len(sql) and sql[start + 1] in "qQ": + quote_index = start + 2 + else: + return None + + delimiter_index = quote_index + 1 + if ( + quote_index >= len(sql) + or sql[quote_index] != "'" + or delimiter_index >= len(sql) + or sql[delimiter_index].isspace() + ): + return None + + closing_delimiter = _ORACLE_ALT_QUOTE_PAIRS.get(sql[delimiter_index], sql[delimiter_index]) + closing_index = sql.find(closing_delimiter + "'", delimiter_index + 1) + return len(sql) if closing_index < 0 else closing_index + 2 + + +def _mask_sql_non_code(sql: str) -> str: + """Mask quoted and commented regions in one monotonic pass.""" + segments: list[str] = [] + code_start = 0 + index = 0 + + while index < len(sql): + region_end: int | None = None + if sql.startswith("--", index): + newline_index = sql.find("\n", index + 2) + region_end = len(sql) if newline_index < 0 else newline_index + elif sql.startswith("/*", index): + comment_end = sql.find("*/", index + 2) + region_end = len(sql) if comment_end < 0 else comment_end + 2 + elif sql[index] in "qQnN": + region_end = _oracle_alt_quote_end(sql, index) + if region_end is None and sql[index] in {"'", '"'}: + region_end = _quoted_region_end(sql, index, sql[index]) + + if region_end is None: + index += 1 + continue + + segments.append(sql[code_start:index]) + segments.append(" " * (region_end - index)) + index = region_end + code_start = region_end + + segments.append(sql[code_start:]) + return "".join(segments) + + def extract_bind_params(sql: str) -> list[str]: """Return deduplicated binds outside SQL literals, identifiers, and comments.""" - cleaned = _SQL_NON_CODE_RE.sub(" ", sql) + cleaned = _mask_sql_non_code(sql) return list(dict.fromkeys(_BIND_PARAM_RE.findall(cleaned))) diff --git a/backend/tests/test_endpoints.py b/backend/tests/test_endpoints.py index 521ac0d..2d13ef4 100644 --- a/backend/tests/test_endpoints.py +++ b/backend/tests/test_endpoints.py @@ -8,6 +8,7 @@ import uuid from datetime import date +from time import perf_counter from types import SimpleNamespace from unittest.mock import AsyncMock, MagicMock @@ -43,6 +44,11 @@ def test_extract_bind_params_ignores_strings() -> None: assert params == ["name"] +def test_extract_bind_params_ignores_escaped_quotes_and_quoted_identifiers() -> None: + sql = "SELECT 'it''s :not_a_param', \"COLUMN:ALSO_NOT\" FROM t WHERE id = :id" + assert extract_bind_params(sql) == ["id"] + + def test_extract_bind_params_ignores_line_comments() -> None: sql = "SELECT * FROM t WHERE id = :id -- ignore :debug\nAND status = :status" assert extract_bind_params(sql) == ["id", "status"] @@ -70,6 +76,27 @@ def test_extract_bind_params_ignores_oracle_alternative_quoted_literals(literal: assert extract_bind_params(sql) == ["id"] +@pytest.mark.parametrize( + "sql", + [ + "SELECT ':ignored", + 'SELECT "column:ignored', + "SELECT /* :ignored", + "SELECT q'[unterminated :ignored", + ], +) +def test_extract_bind_params_masks_unterminated_non_code_regions(sql: str) -> None: + assert extract_bind_params(sql) == [] + + +def test_extract_bind_params_handles_adversarial_alt_quotes_in_linear_time() -> None: + sql = ("q'[" * 32_768) + "unterminated" + started_at = perf_counter() + + assert extract_bind_params(sql) == [] + assert perf_counter() - started_at < 0.5 + + def test_extract_bind_params_empty() -> None: sql = "SELECT 1 FROM dual" params = extract_bind_params(sql) diff --git a/docs/security_checklist.md b/docs/security_checklist.md index 157737f..2d62307 100644 --- a/docs/security_checklist.md +++ b/docs/security_checklist.md @@ -126,11 +126,12 @@ Comprehensive security validation for QueryGateway production deployments. All i | 68 | Cached data is served only after retained-run coverage and typed row filtering | Verified | `DataService._serve_snapshot()` selects the newest covering job run, validates mapped columns, and calls `filter_snapshot_rows()`; no unfiltered fallback exists | | 69 | Range and SQL `NULL` coverage semantics fail closed | Verified | Reversed bounds return 422; range types must match; `null_means_all` is limited to optional equality mappings | | 70 | Snapshot filter mappings cannot substitute for authorization | Verified | Data authentication runs before snapshot selection; `store_id` and other mapped values are ordinary row filters only | +| 71 | SQL bind discovery has linear processing cost | Verified | A monotonic scanner masks quoted/commented regions before bind extraction; adversarial unterminated Q-quote input is regression-tested | ## Summary -- **Verified items**: 64/70 -- **Action required**: 6/70 +- **Verified items**: 65/71 +- **Action required**: 6/71 - **High-severity unresolved findings**: 0 - **All code-level security controls validated through automated tests** From 63b5be41a73e440cb520ef2ab976d72e77a423c6 Mon Sep 17 00:00:00 2001 From: Badry Date: Tue, 1 Sep 2026 22:15:31 +0300 Subject: [PATCH 7/7] chore(deps): remediate frontend audit findings --- frontend/package-lock.json | 63 ++++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 30 deletions(-) diff --git a/frontend/package-lock.json b/frontend/package-lock.json index a32a298..4201c75 100644 --- a/frontend/package-lock.json +++ b/frontend/package-lock.json @@ -2744,9 +2744,9 @@ "license": "MIT" }, "node_modules/baseline-browser-mapping": { - "version": "2.10.0", - "resolved": "https://registry.npmjs.org/baseline-browser-mapping/-/baseline-browser-mapping-2.10.0.tgz", - "integrity": "sha512-lIyg0szRfYbiy67j9KN8IyeD7q7hcmqnJ1ddWmNt19ItGpNN64mnllmxUNFIOdOm6by97jlL6wfpTTJrmnjWAA==", + "version": "2.11.20", + "resolved": "https://registry.npmjs.org/baseline-browser-mapping/-/baseline-browser-mapping-2.11.20.tgz", + "integrity": "sha512-H0ulySigv6icDJ1F7SjtdCD6PrhTpdYCmP0CactWy1+ekh0AFd0o1Wn5T8b+hnTmdBx19u9yhL6wvCylXMY7zw==", "dev": true, "license": "Apache-2.0", "bin": { @@ -2794,9 +2794,9 @@ } }, "node_modules/browserslist": { - "version": "4.28.1", - "resolved": "https://registry.npmjs.org/browserslist/-/browserslist-4.28.1.tgz", - "integrity": "sha512-ZC5Bd0LgJXgwGqUknZY/vkUQ04r8NXnJZ3yYi4vDmSiZmC/pdSN0NbNRPxZpbtO4uAfDUAFffO8IZoM3Gj8IkA==", + "version": "4.28.8", + "resolved": "https://registry.npmjs.org/browserslist/-/browserslist-4.28.8.tgz", + "integrity": "sha512-V2NpofLblG64mfOtSgDhOJESZEGogzDMBv/q+W6oc4LXWP/q75eOXoOaaOu1EOadB9U4Bwx/e0yzbvwKH8zalA==", "dev": true, "funding": [ { @@ -2815,11 +2815,11 @@ "license": "MIT", "peer": true, "dependencies": { - "baseline-browser-mapping": "^2.9.0", - "caniuse-lite": "^1.0.30001759", - "electron-to-chromium": "^1.5.263", - "node-releases": "^2.0.27", - "update-browserslist-db": "^1.2.0" + "baseline-browser-mapping": "^2.11.12", + "caniuse-lite": "^1.0.30001809", + "electron-to-chromium": "^1.5.402", + "node-releases": "^2.0.53", + "update-browserslist-db": "^1.3.0" }, "bin": { "browserslist": "cli.js" @@ -2862,9 +2862,9 @@ } }, "node_modules/caniuse-lite": { - "version": "1.0.30001776", - "resolved": "https://registry.npmjs.org/caniuse-lite/-/caniuse-lite-1.0.30001776.tgz", - "integrity": "sha512-sg01JDPzZ9jGshqKSckOQthXnYwOEP50jeVFhaSFbZcOy05TiuuaffDOfcwtCisJ9kNQuLBFibYywv2Bgm9osw==", + "version": "1.0.30001810", + "resolved": "https://registry.npmjs.org/caniuse-lite/-/caniuse-lite-1.0.30001810.tgz", + "integrity": "sha512-TITQPUkaz+aVk5GL6NhOdwk1aEaNTSDPsGFWrTuhKGtjTF70jL/Oht2W4c6rXUe5fu7Ie19VIahAXHIIiWWNeg==", "dev": true, "funding": [ { @@ -3221,9 +3221,9 @@ } }, "node_modules/electron-to-chromium": { - "version": "1.5.307", - "resolved": "https://registry.npmjs.org/electron-to-chromium/-/electron-to-chromium-1.5.307.tgz", - "integrity": "sha512-5z3uFKBWjiNR44nFcYdkcXjKMbg5KXNdciu7mhTPo9tB7NbqSNP2sSnGR+fqknZSCwKkBN+oxiiajWs4dT6ORg==", + "version": "1.5.419", + "resolved": "https://registry.npmjs.org/electron-to-chromium/-/electron-to-chromium-1.5.419.tgz", + "integrity": "sha512-nHMPn8x4yCxCI0iSnL+LlHL5sUoUfjLXkcRIagZ4GBdrfFLFaiLNvzJWbJqZhFT9IAhw5tUSNlhggWN+otvp/A==", "dev": true, "license": "ISC" }, @@ -4458,11 +4458,14 @@ "license": "MIT" }, "node_modules/node-releases": { - "version": "2.0.36", - "resolved": "https://registry.npmjs.org/node-releases/-/node-releases-2.0.36.tgz", - "integrity": "sha512-TdC8FSgHz8Mwtw9g5L4gR/Sh9XhSP/0DEkQxfEFXOpiul5IiHgHan2VhYYb6agDSfp4KuvltmGApc8HMgUrIkA==", + "version": "2.0.54", + "resolved": "https://registry.npmjs.org/node-releases/-/node-releases-2.0.54.tgz", + "integrity": "sha512-YHs7BmmcsdAI5Ozuf8JZo6PT0mv2GIWC9vMfvUC3dp65M8hn7Ux8CPL+2oBI7juNuj9d0ndhTcznq2ODBps9cQ==", "dev": true, - "license": "MIT" + "license": "MIT", + "engines": { + "node": ">=18" + } }, "node_modules/normalize-path": { "version": "3.0.0", @@ -4809,9 +4812,9 @@ } }, "node_modules/postcss-nested/node_modules/postcss-selector-parser": { - "version": "6.1.2", - "resolved": "https://registry.npmjs.org/postcss-selector-parser/-/postcss-selector-parser-6.1.2.tgz", - "integrity": "sha512-Q8qQfPiZ+THO/3ZrOrO0cJJKfpYCagtMUkXbnEfmgUjwXg6z/WBeOyS9APBBPCTSiDV+s4SwQGu8yFsiMRIudg==", + "version": "6.1.4", + "resolved": "https://registry.npmjs.org/postcss-selector-parser/-/postcss-selector-parser-6.1.4.tgz", + "integrity": "sha512-bIoJLOmjCO1S9XdY/DcnR5hJxvrDir1PbGChrzXG3vw0/FOliy/fA3dmdhQ441kah4gKv+TwckGzex6wNS5cnQ==", "dev": true, "license": "MIT", "dependencies": { @@ -5499,9 +5502,9 @@ } }, "node_modules/tailwindcss/node_modules/postcss-selector-parser": { - "version": "6.1.2", - "resolved": "https://registry.npmjs.org/postcss-selector-parser/-/postcss-selector-parser-6.1.2.tgz", - "integrity": "sha512-Q8qQfPiZ+THO/3ZrOrO0cJJKfpYCagtMUkXbnEfmgUjwXg6z/WBeOyS9APBBPCTSiDV+s4SwQGu8yFsiMRIudg==", + "version": "6.1.4", + "resolved": "https://registry.npmjs.org/postcss-selector-parser/-/postcss-selector-parser-6.1.4.tgz", + "integrity": "sha512-bIoJLOmjCO1S9XdY/DcnR5hJxvrDir1PbGChrzXG3vw0/FOliy/fA3dmdhQ441kah4gKv+TwckGzex6wNS5cnQ==", "dev": true, "license": "MIT", "dependencies": { @@ -5750,9 +5753,9 @@ "license": "MIT" }, "node_modules/update-browserslist-db": { - "version": "1.2.3", - "resolved": "https://registry.npmjs.org/update-browserslist-db/-/update-browserslist-db-1.2.3.tgz", - "integrity": "sha512-Js0m9cx+qOgDxo0eMiFGEueWztz+d4+M3rGlmKPT+T4IS/jP4ylw3Nwpu6cpTTP8R1MAC1kF4VbdLt3ARf209w==", + "version": "1.3.2", + "resolved": "https://registry.npmjs.org/update-browserslist-db/-/update-browserslist-db-1.3.2.tgz", + "integrity": "sha512-UQ+MSxlhRm1bzjhU+DcuXfjFO1FzNtqhK5+9Yvlp90ItDLk5vT932A0rFu619nf7RVS+Y/VeaUW1jaRDqZ8VJw==", "dev": true, "funding": [ {