Skip to content

Navigation Menu

Sign in
Sign up

UN-3393 [FEAT] Add SanitizedSerializerMixin foundation for input validation - #1965

Merged
chandrasekharan-zipstack merged 4 commits into
main from
feat/UN-3393-input-validation-foundation
Sep 8, 2026
Merged

UN-3393 [FEAT] Add SanitizedSerializerMixin foundation for input validation #1965
chandrasekharan-zipstack merged 4 commits into
main from
feat/UN-3393-input-validation-foundation

Conversation

@chandrasekharan-zipstack

@chandrasekharan-zipstack chandrasekharan-zipstack commented May 14, 2026

Copy link
Copy Markdown
Contributor

What

  • Foundation PR for the input-validation hardening plan (Jira UN-3393, CWE-20 finding from external pentest L2 report, pp. 11–14).
  • Adds SanitizedSerializerMixin plus pre-mixed ModelSerializer / Serializer / HyperlinkedModelSerializer under utils.serializer.sanitization, re-exported from utils.serializer. Mixin auto-attaches validate_no_html_tags to every writable CharField; opt out per-serializer via Meta.html_safe_fields = (...) for fields that legitimately accept HTML-like content (e.g. prompt text, regex literals). Read-only fields are naturally exempt (DRF skips validators on read_only=True).
  • Adds structured logger.warning("input_validation_rejected", extra={"field", "reason"}) in each rejection branch of validate_no_html_tags (html_tag, js_protocol, event_handler). Searchable in logs; no metric or alert wired.
  • Adds cleanup_html_payloads Django management command (under backend/commands/) — dry-run by default, --apply to redact stored pentest payloads in custom_tool.tool_name, custom_tool.description, workflow.workflow_name, workflow.description, api_deployment.display_name, api_deployment.description, adapter_instance.adapter_name.
  • 14 new mixin unit tests; 36 existing test_input_sanitizer tests still pass (50 total).

Why

  • External pentest (L2 report) flagged CWE-20: HTML/JS tags accepted in Prompt Studio project name, author/org, description (and equivalents on Workflow, API deployment, Adapter, Connector, Notification, Organization).
  • Threat class is stored XSS. Output encoding (React JSX, Django auto-escape) is already in place as the primary defence; this PR adds a serializer-level boundary as defence-in-depth and to close the auditor's reproducer.
  • Original Apr-22 proposal recommended Cloud Armor (edge WAF) as primary. Reversed to code-first after team feedback ("spawn agents to scour the codebase") and a prod scan that showed 19 stored payloads on US prod — all pentest leftovers, zero legitimate <...> content. Migration risk for forward-validation is effectively zero.
  • Full decision rationale in the KB under Obsidian Vault/zipstuff/UN-3393-input-validation/ (ADRs 0001–0007).

How

  • backend/utils/serializer/sanitization.py — new module. SanitizedSerializerMixin.__init__ walks self.fields, reads Meta.html_safe_fields (default ()), and appends validate_no_html_tags (bound to the field name via functools.partial) to every writable CharField's validators list. Pre-mixed ModelSerializer / Serializer / HyperlinkedModelSerializer are defined here.
  • backend/utils/serializer/__init__.py — re-exports the new classes alongside the existing IntegrityErrorMixin. Consumer convention: from utils.serializer import ModelSerializer.
  • backend/utils/input_sanitizer.py — adds a private _reject(field_name, reason, message) helper that logs and raises; the three rejection branches now go through it.
  • backend/commands/management/commands/cleanup_html_payloads.py — uses django.apps.get_model to resolve targets at runtime (so the command doesn't fail if a pluggable app is missing). Filters by *__isnull=False and non-empty, iterates with .only(...) and .iterator(), prints id + truncated value per match, applies inside transaction.atomic() when --apply is passed.
  • backend/utils/tests/test_sanitized_serializer_mixin.py — covers happy path, HTML/JS-protocol/event-handler rejection, html_safe_fields opt-out, missing Meta, read-only exemption, pre-mixed-class inheritance, raise_exception=True path.

Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)

  • No. Nothing in backend/ imports from utils.serializer.sanitization or the new pre-mixed ModelSerializer / Serializer / HyperlinkedModelSerializer re-exports yet. Existing serializers continue to import from rest_framework import serializers and inherit serializers.ModelSerializer directly — they are unchanged.
  • The structured log line added to validate_no_html_tags is emitted only when a rejection already happens (no new rejection paths) — same writes that previously got 400 still get 400, plus a single WARNING log line per rejection.
  • The cleanup_html_payloads management command is opt-in; it runs only when an operator explicitly invokes it. Default mode is --dry-run (read-only).

Database Migrations

  • None. No schema changes.

Env Config

  • None.

Relevant Docs

  • KB: Obsidian Vault/zipstuff/UN-3393-input-validation/ — INDEX, threat model, target architecture, rollout plan, prod baseline, ADRs 0001–0007.

Related Issues or PRs

  • Jira: UN-3393
  • Follow-up sweep PR (PR2): rewrites from rest_framework import serializers to from utils.serializer import ... across backend/ + backend/pluggable_apps/, deletes the redundant manual validate_<field> methods in the 8 serializers already covered, adds Meta.html_safe_fields opt-outs as needed.

Dependencies Versions

  • None.

Notes on Testing

  • New + existing tests:
    cd backend && uv run pytest utils/tests/test_sanitized_serializer_mixin.py utils/tests/test_input_sanitizer.py -q
    → 50 passed.
  • Manual: import sanity check — python -c \"from utils.serializer import ModelSerializer, Serializer, HyperlinkedModelSerializer, SanitizedSerializerMixin; print('ok')\".
  • Manual: dry-run the cleanup command on a populated dev DB — python manage.py cleanup_html_payloads — should print candidate rows without making changes.

Screenshots

n/a (no UI change)

Checklist

I have read and understood the Contribution Guidelines.

greptile-apps[bot] reacted with thumbs up emoji

coderabbitai Bot commented May 14, 2026
edited
Loading

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

i️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b2531d03-1d46-47f6-b4ac-15915f12917e

📥 Commits

Reviewing files that changed from the base of the PR and between d13a512 and c80c8d1.

📒 Files selected for processing (1)
  • backend/utils/input_sanitizer.py

Summary by CodeRabbit

  • New Features

    • Input validation has been enhanced to automatically detect and reject unsafe HTML tags, scripts, and dangerous protocols in user-submitted text fields.
  • Tests

    • Added comprehensive test coverage for input validation functionality.

Walkthrough

This PR introduces automatic HTML sanitization for Django REST Framework serializer fields. The input sanitizer is refactored with structured logging; a new SanitizedSerializerMixin automatically appends HTML validation to writable CharField fields with optional per-serializer exemptions, and convenience serializer base classes eliminate direct DRF imports. Tests verify sanitization, opt-out behavior, read-only exemption, and error handling.

Changes

HTML Sanitization for Serializer Fields

Layer / File(s) Summary
Input sanitizer refactoring with structured logging
backend/utils/input_sanitizer.py
_reject() helper centralizes validation failure handling by logging a warning with structured field and reason context before raising ValidationError. All three validation triggers in validate_no_html_tags() now route through _reject() instead of raising directly.
Sanitized serializer mixin and DRF integration classes
backend/utils/serializer/sanitization.py
SanitizedSerializerMixin attaches validate_no_html_tags to writable CharField fields during initialization, with Meta.html_safe_fields opt-out and automatic read-only field skipping. Convenience classes ModelSerializer, Serializer, and HyperlinkedModelSerializer combine the mixin with DRF base classes.
Package exports and public API
backend/utils/serializer/__init__.py
Re-exports SanitizedSerializerMixin, serializer subclasses, and IntegrityErrorMixin via __all__ for direct utils.serializer imports.
Comprehensive test suite for sanitization mixin
backend/utils/tests/test_sanitized_serializer_mixin.py
Fixture serializers exercise sanitization, opt-out, and read-only behavior. Tests verify HTML/script tag rejection, javascript/data URI protocol blocking, event-handler attribute rejection, per-field error attribution, Meta.html_safe_fields exemption, read-only field exemption, robustness with missing Meta, and ValidationError propagation.

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 15.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main feature addition (SanitizedSerializerMixin foundation for input validation) and is specific, concise, and directly related to the changeset.
Description check ✅ Passed The description comprehensively covers all required template sections: What (feature and design), Why (security context), How (implementation details), impact assessment, testing notes, and related Jira ticket. All critical sections are filled with substantive detail.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/UN-3393-input-validation-foundation

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

greptile-apps Bot commented May 14, 2026
edited
Loading

Copy link
Copy Markdown
Contributor

Greptile Summary

Adds a serializer-level input-validation foundation for writable DRF character fields.

  • Introduces sanitized base classes for standard, model, and hyperlinked model serializers.
  • Supports explicit field exemptions through Meta.html_safe_fields.
  • Narrows sanitization to script-capable tags, dangerous URI protocols, and DOM event handlers.
  • Adds structured rejection logging and unit coverage for validation behavior and exemptions.

Confidence Score: 5/5

The reviewed changes appear safe to merge, with no outstanding findings.

All previous findings were resolved, and no actionable regression or repository-rule violation remains in the current PR changes.

Important Files Changed

Filename Overview
backend/utils/input_sanitizer.py Refines dangerous-markup detection and centralizes logged validation rejection.
backend/utils/serializer/sanitization.py Adds sanitized DRF serializer base classes with writable-character-field validation and explicit exemptions.
backend/utils/serializer/init.py Exposes the new serializer classes and existing integrity-error mixin through one package interface.
backend/utils/tests/test_input_sanitizer.py Covers dangerous tags and protocols while preserving intended free-form text and inert markup.
backend/utils/tests/test_sanitized_serializer_mixin.py Verifies automatic validator attachment, exemptions, inheritance, field isolation, and DRF error propagation.

Reviews (7): Last reviewed commit: "UN-3393 [FIX] Narrow the sanitizer to sc..." | Re-trigger Greptile

Comment thread backend/utils/input_sanitizer.py Outdated
Comment thread backend/commands/management/commands/cleanup_html_payloads.py Outdated
Comment thread backend/commands/management/commands/cleanup_html_payloads.py Outdated
chandrasekharan-zipstack added a commit that referenced this pull request May 15, 2026
Greptile P2 review comment on PR #1965. The `_reject` helper always
raises `ValidationError`; annotating it `-> None` misleads mypy/pyright
into thinking execution can continue past the call site, which can
suppress dead-code / unreachable-branch warnings. `NoReturn` is the
correct signal.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copy link
Copy Markdown

chandrasekharan-zipstack added a commit that referenced this pull request Aug 28, 2026
Greptile P2 review comment on PR #1965. The `_reject` helper always
raises `ValidationError`; annotating it `-> None` misleads mypy/pyright
into thinking execution can continue past the call site, which can
suppress dead-code / unreachable-branch warnings. `NoReturn` is the
correct signal.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chandrasekharan-zipstack added a commit that referenced this pull request Sep 2, 2026
Greptile P2 review comment on PR #1965. The `_reject` helper always
raises `ValidationError`; annotating it `-> None` misleads mypy/pyright
into thinking execution can continue past the call site, which can
suppress dead-code / unreachable-branch warnings. `NoReturn` is the
correct signal.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
...dation
Foundation PR for the input-validation hardening plan (CWE-20). No
behaviour change yet — nothing imports the new pre-mixed classes; the
sweep that rewrites serializer imports lands in a follow-up PR.
- New `utils.serializer.sanitization` exposes `SanitizedSerializerMixin`
 plus pre-mixed `ModelSerializer`, `Serializer`, and
 `HyperlinkedModelSerializer`. The mixin walks `self.fields` in
 `__init__` and attaches `validate_no_html_tags` to every writable
 `CharField`. Opt out per-serializer via `Meta.html_safe_fields = (...)`
 for fields that legitimately accept HTML-like content (e.g. prompt
 text, regex literals). Read-only fields are naturally exempt — DRF
 skips validators on `read_only=True`.
- `utils.serializer.__init__` re-exports the pre-mixed classes alongside
 the existing `IntegrityErrorMixin`, so consumers can do
 `from utils.serializer import ModelSerializer`.
- `input_sanitizer.validate_no_html_tags` now emits a structured
 `logger.warning("input_validation_rejected", extra={"field", "reason"})`
 in each rejection branch (`html_tag`, `js_protocol`,
 `event_handler`). Searchable in logs; no metric / alert wired.
- New `cleanup_html_payloads` management command (under
 `backend/commands/...`) replaces stored pentest payloads in the
 columns flagged by the May 2026 prod scan: `custom_tool.tool_name`,
 `custom_tool.description`, `workflow.workflow_name`,
 `workflow.description`, `api_deployment.display_name`,
 `api_deployment.description`, `adapter_instance.adapter_name`. Default
 mode is dry-run; `--apply` performs the replacement inside a
 transaction.
- 14 new mixin unit tests; existing 36 `test_input_sanitizer` tests
 unchanged and still pass (50 total).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Removing the one-shot data-cleanup command added in the previous commit.
Decision: overkill for the actual risk.
- The 19 stored pentest payloads on US prod are inert under React /
 Django auto-escape (the only render path today). They don't fire.
- Forward-validation via the mixin (PR2) prevents any *new* payloads
 from landing.
- If a future non-React render path is added (PDF/email/CSV/server
 template), that path's owner can run a one-off SQL update at that
 time. Carrying a dedicated management command and the regex-import
 coupling it introduced is more code than the situation warrants.
The KB ADR 0007 (cleanup-via-management-command) is superseded
accordingly; see KB updates landing alongside.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Greptile P2 review comment on PR #1965. The `_reject` helper always
raises `ValidationError`; annotating it `-> None` misleads mypy/pyright
into thinking execution can continue past the call site, which can
suppress dead-code / unreachable-branch warnings. `NoReturn` is the
correct signal.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The denylist rejected any '<' followed by a letter, which made ordinary
free-form text fail validation: "qty <threshold", "extract the <invoice_no>
field" and "total is < 500 USD" were all rejected. With the mixin attaching
this validator to every writable CharField by default, that false-positive
rate is not survivable for prompt, chat and description fields.
Reject only tags that can execute script or load remote content, and only
the data: MIME types that can carry markup. Event-handler detection is
unchanged - it already required the handler name as a single word, so
"on error = retry" was never matched.
<img src=x onerror=...> is still rejected, now via the event-handler rule
rather than the tag rule.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MbK3GDtfcExzb6kceRAdhu 

sonarqubecloud Bot commented Sep 8, 2026

Copy link
Copy Markdown

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Unstract test results

Per-group results

Status Group Tier Passed Failed Errors Skipped Duration (s)
e2e-api-deployment e2e 3 0 0 0 18.8
e2e-coowners e2e 1 0 0 0 1.7
e2e-etl e2e 1 0 0 0 14.6
e2e-login e2e 2 0 0 0 1.3
e2e-prompt-studio e2e 1 0 0 0 10.0
e2e-smoke e2e 2 0 0 0 1.1
e2e-workflow e2e 1 0 0 0 22.3
frontend unit 0 1 0 0 0.0
integration-backend integration 310 0 0 26 37.0
integration-connectors integration 1 0 0 7 6.6
integration-workers integration 157 0 0 1 42.9
ui e2e 0 1 0 0 0.0
unit-backend unit 1146 0 0 1 42.7
unit-connectors unit 63 0 0 0 9.6
unit-core unit 33 0 0 0 1.3
unit-platform-service unit 15 0 0 0 2.6
unit-rig unit 120 0 0 0 4.5
unit-runner unit 5 0 0 0 2.9
unit-sdk1 unit 563 0 0 0 28.7
unit-workers unit 1386 0 0 1 124.2
TOTAL 3810 2 0 36 372.8

Critical paths

⚠️ Critical paths not yet covered

  • workflow-execution-fan-out — Multi-file workflow execution fans out to file-processing workers and rejoins. (declared coverage: no groups declared)
✅ Covered critical paths
  • auth-login — covered by e2e-login
  • adapter-register-llm — covered by integration-backend
  • workflow-author — covered by integration-backend
  • co-owner-manage — covered by integration-backend, e2e-coowners
  • workflow-create-execute — covered by e2e-workflow
  • api-deployment-provision — covered by integration-backend
  • api-deployment-auth — covered by integration-backend
  • api-deployment-run — covered by e2e-api-deployment
  • mcp-server-auth — covered by integration-backend
  • mcp-platform-auth — covered by integration-backend
  • prompt-studio-author — covered by integration-backend
  • prompt-studio-fetch-response — covered by e2e-prompt-studio
  • connector-register-test — covered by integration-backend
  • pipeline-etl-execute — covered by e2e-etl
  • usage-aggregate-read — covered by integration-backend
  • usage-token-tracking — covered by e2e-api-deployment
  • callback-result-delivery — covered by e2e-api-deployment

chandrasekharan-zipstack merged commit 607eed2 into main Sep 8, 2026
10 checks passed
chandrasekharan-zipstack deleted the feat/UN-3393-input-validation-foundation branch September 8, 2026 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments
@Deepak-Kesavan Deepak-Kesavan Deepak-Kesavan approved these changes
@vishnuszipstack vishnuszipstack Awaiting requested review from vishnuszipstack

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /