Skip to content

UN-3393 [FEAT] Wire AuditSerializer subclasses through SanitizedSerializerMixin - #1966

Open
chandrasekharan-zipstack wants to merge 9 commits into
feat/UN-3393-input-validation-foundationfrom
feat/UN-3393-input-validation-sweep
Open

UN-3393 [FEAT] Wire AuditSerializer subclasses through SanitizedSerializerMixin#1966
chandrasekharan-zipstack wants to merge 9 commits into
feat/UN-3393-input-validation-foundationfrom
feat/UN-3393-input-validation-sweep

Conversation

@chandrasekharan-zipstack

Copy link
Copy Markdown
Contributor

What

  • Builds on UN-3393 [FEAT] Add SanitizedSerializerMixin foundation for input validation #1965 (foundation PR) by routing every AuditSerializer subclass through SanitizedSerializerMixin. ~18 write-path serializers now auto-reject HTML/JS-shaped input on every writable CharField / TextField.
  • Affected (transitively): WorkflowSerializer, CustomToolSerializer, APIDeploymentSerializer, APIKeySerializer, ConnectorInstanceSerializer, BaseAdapterSerializer, PlatformKeySerializer, PlatformApiKey{Create,Update}Serializer, PipelineSerializer, ToolInstanceSerializer, ToolStudioPromptSerializer, PromptStudioOutputSerializer, ProfileManagerSerializer, IndexManagerSerializer, PromptStudioRegistry{,Info}Serializer, PromptStudioDocumentManagerSerializer.
  • Adds Meta.html_safe_fields opt-outs on the two serializers that legitimately carry LLM markup:
    • ToolStudioPromptSerializerprompt, assert_prompt, assertion_failure_prompt, output.
    • CustomToolSerializersummarize_prompt, preamble, postamble, output.
  • Removes the three redundant validate_description methods (in api_v2.APIDeploymentSerializer, workflow_v2.WorkflowSerializer, prompt_studio_core_v2.CustomToolSerializer) — the mixin now covers them.

Why

  • The foundation PR (UN-3393 [FEAT] Add SanitizedSerializerMixin foundation for input validation #1965) added SanitizedSerializerMixin and pre-mixed ModelSerializer / Serializer / HyperlinkedModelSerializer under utils.serializer but did not wire any production serializer through them. This PR is the smallest, highest-leverage wiring step: changing one parent class (AuditSerializer) closes the input-validation gap on the majority of write-path entities.
  • validate_<name> methods (which use validate_name_field) are kept because they also strip whitespace and reject empty values — behaviour the mixin doesn't duplicate. Only the validate_description methods (which just call validate_no_html_tags) are removed.
  • Free-form LLM prompt / output fields (prompt, output, summarize_prompt, etc.) legitimately contain <context> / <thinking> / arbitrary model output. They're explicitly opted out via Meta.html_safe_fields.

How

  • backend/backend/serializers.py: AuditSerializer now inherits utils.serializer.ModelSerializer instead of rest_framework.serializers.ModelSerializer. create / update semantics unchanged.
  • Per-serializer Meta.html_safe_fields declarations on the two LLM-bearing serializers (Prompt Studio).
  • Three validate_description removals + validate_no_html_tags import drop from the affected modules.

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)

  • Functional: The 8 serializers that were already hand-wired (in the in-flight fix) continue to behave identically. The 14 manual validate_<field> methods still exist — the mixin's validator is appended to the field's validators list, so the same rejection is enforced either way. Removing only the redundant validate_description methods avoids double-validation noise but doesn't change any acceptance outcome.
  • New rejections: AuditSerializer subclasses that were not previously sanitised now auto-reject HTML in their CharField / TextField (e.g. ConnectorInstance.connector_name, PlatformKey.key_name, Pipeline.pipeline_name, AdapterInstance.adapter_name, etc.). Per the May 2026 US-prod scan (see KB Obsidian Vault/zipstuff/UN-3393-input-validation/05-prod-baseline.md), zero legitimate user content uses <…> in these columns — every match was pentest leftover. Migration risk: ~zero.
  • LLM fields explicitly opted out: prompt, assert_prompt, assertion_failure_prompt, output, summarize_prompt, preamble, postamble are listed in Meta.html_safe_fields so they continue to accept arbitrary text.
  • Direct DRF-base-class users: Serializers that inherit directly from serializers.ModelSerializer / serializers.Serializer / serializers.HyperlinkedModelSerializer (i.e. not via AuditSerializer) are not touched by this PR. They remain unchanged. A follow-up sweep will convert them; their absence here means write paths through them retain pre-PR behaviour.

Database Migrations

  • None.

Env Config

  • None.

Relevant Docs

  • KB: Obsidian Vault/zipstuff/UN-3393-input-validation/.
  • ADR 0003 (adr/0003-base-mixin-default-on-opt-out.md) — opt-out semantics.

Related Issues or PRs

  • Jira: UN-3393.
  • Stacked on: UN-3393 [FEAT] Add SanitizedSerializerMixin foundation for input validation #1965 (foundation).
  • Follow-up: a separate PR that converts the remaining ~20 direct DRF-base-class users (tags/serializers.py, scheduler/serializer.py, tenant_account_v2/serializer.py, pipeline_v2/serializers/{internal,update,sharing,execute}.py, response-only and list serializers across the codebase, …).
  • Follow-up: cover backend/pluggable_apps/ (lives in the unstract-cloud repo; separate PR there).

Dependencies Versions

  • None.

Notes on Testing

Local tests in the OSS worktree (cloud-deps not installed, so manage.py check is unavailable here — CI runs the full suite):

cd backend && uv run pytest utils/tests/ -q

→ 85 passed (50 from foundation + 35 pre-existing utils tests).

Manual sanity:

cd backend && uv run python -c "from utils.serializer import ModelSerializer; from backend.serializers import AuditSerializer; assert issubclass(AuditSerializer, ModelSerializer); print('ok')"

Screenshots

n/a (no UI change)

Checklist

I have read and understood the Contribution Guidelines.

@coderabbitai

coderabbitai Bot commented May 14, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2543ddc1-8c78-4113-8bcd-0b6dbd8083d3

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/UN-3393-input-validation-sweep

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

greptile-apps Bot commented May 14, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR routes additional backend serializers through centralized text sanitization and updates Prompt Studio error handling for rejected edits. The previously reported PromptStudioOutputSerializer regression is resolved.

  • Adds explicit markup opt-outs for legitimate Prompt Studio prompt, output, and context fields.
  • Applies sanitized serializer bases to audit-backed resources, notifications, and tags.
  • Displays prompt-key validation failures inline while preserving rejected text for correction.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
backend/prompt_studio/prompt_studio_output_manager_v2/serializers.py Adds correctly named output and context exemptions, resolving the previously reported rejection of legitimate LLM markup.
backend/utils/serializer/sanitization.py Uses user-visible field labels in validation errors without changing sanitizer coverage.
backend/backend/serializers.py Routes AuditSerializer subclasses through the centralized sanitized ModelSerializer.
frontend/src/components/custom-tools/prompt-card/PromptCard.jsx Adds latest-attempt tracking, rollback behavior, and inline prompt-key validation state.
frontend/src/components/custom-tools/document-parser/DocumentParser.jsx Parses structured field errors and falls back to global alerts when no inline renderer handles them.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    Request[Serializer input] --> OutputSerializer[PromptStudioOutputSerializer]
    OutputSerializer --> Audit[AuditSerializer]
    Audit --> Sanitized[Sanitized ModelSerializer]
    Sanitized --> Exemption{Field in html_safe_fields?}
    Exemption -->|output or context| Accept[Skip HTML validator]
    Exemption -->|other writable text| Validate[Apply HTML validator]
Loading

Reviews (12): Last reviewed commit: "UN-3393 [FIX] Keep the prompt card's sta..." | Re-trigger Greptile

Comment thread backend/backend/serializers.py
@chandrasekharan-zipstack
chandrasekharan-zipstack force-pushed the feat/UN-3393-input-validation-sweep branch from 93b7ecd to 1e8e4ef Compare May 15, 2026 10:43
chandrasekharan-zipstack added a commit that referenced this pull request May 15, 2026
Greptile P1 review comment on PR #1966. `PromptStudioOutputManager`
stores raw LLM responses (`output`: CharField) and document chunks
(`context`: TextField) that routinely contain <thinking>, <context>,
and other XML-like tags. The serializer is mounted on a ModelViewSet
with no class-level HTTP-method restriction, so write paths through
DRF admin / browsable API / any future write endpoint would
incorrectly reject legitimate LLM output without this opt-out.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@chandrasekharan-zipstack
chandrasekharan-zipstack force-pushed the feat/UN-3393-input-validation-sweep branch from 1e8e4ef to 6ce8ea8 Compare May 15, 2026 11:46
@sonarqubecloud

Copy link
Copy Markdown

@Deepak-Kesavan Deepak-Kesavan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated review

Automated review by Unstract PR review kit (Claude Code). Each finding below was reproduced against the code rather than inferred, so treat it as something to resolve before merge. If one is wrong, disagree on the thread and close it — that is the expected way to clear a finding. Anything tagged [unverified] was not reproduced and is flagged for your judgement instead.

3 inline comment(s) · 1 finding(s) not on changed lines (below).

Findings off the diff

  • backend/adapter_processor_v2/serializers.py:59
    [minor] The PR lists BaseAdapterSerializer as newly covered, but AdapterInstanceSerializer.to_internal_value returns the raw dict without calling super(), so the mixin's validators never run on the adapter write path — the coverage is nominal, not real.

AdapterInstanceSerializer is what get_serializer_class returns for every non-list action (adapter_processor_v2/views.py:167-172), and it's what create/update feed request data to. Its to_internal_value override (serializers.py:59-67) encrypts adapter_metadata and then return data — it never calls super().to_internal_value(data).

In DRF 3.14.0 (pinned in backend/uv.lock), Serializer.run_validation calls to_internal_value(data) and field-level validators run exclusively inside it. So the validate_no_html_tags partials this PR attaches are never executed on this path — along with adapter_id's max_length and adapter_type's choice check. I confirmed it: a serializer differing only by def to_internal_value(self, data): return data accepted adapter_id="<script>alert(1)</script>" as valid, while the parent rejected it.

In practice adapter_name and description are still covered, because BaseAdapterSerializer.validate() (serializers.py:32-44) hand-checks exactly those two after to_internal_value — which is why nothing visibly breaks. The point is the claim: this serializer is not sanitized by the mixin, and the two hand-checks are load-bearing, not redundant. If a later cleanup removes them the way this PR removed the three validate_description methods, the coverage goes to zero silently.

This file is not in the diff (git diff --name-only over backend/adapter_processor_v2/ is empty; last touched in bf15e8d2e), so the bypass is pre-existing — raising it only because the PR description asserts coverage this path doesn't have.

Suggested fix: Delegate, then encrypt: pop adapter_metadata, call super().to_internal_value(data), and set ADAPTER_METADATA_B on the validated dict. Once field validators actually run, the adapter_name / description hand-checks in BaseAdapterSerializer.validate become genuinely redundant and can go the same way as the three validate_description methods. Alternatively, just drop BaseAdapterSerializer from the "affected" list in the PR body.

🤖 Unstract PR review kit (Claude Code) · review-pr-bot:52134af5ef8c

review-pr-bot:review

Comment thread backend/backend/serializers.py
Comment thread frontend/src/components/custom-tools/document-parser/DocumentParser.jsx Outdated
Comment thread frontend/src/components/custom-tools/prompt-card/PromptCard.jsx Outdated
chandrasekharan-zipstack added a commit that referenced this pull request Aug 28, 2026
Greptile P1 review comment on PR #1966. `PromptStudioOutputManager`
stores raw LLM responses (`output`: CharField) and document chunks
(`context`: TextField) that routinely contain <thinking>, <context>,
and other XML-like tags. The serializer is mounted on a ModelViewSet
with no class-level HTTP-method restriction, so write paths through
DRF admin / browsable API / any future write endpoint would
incorrectly reject legitimate LLM output without this opt-out.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@chandrasekharan-zipstack
chandrasekharan-zipstack force-pushed the feat/UN-3393-input-validation-sweep branch from c9f058e to be37d40 Compare August 28, 2026 06:33

@Deepak-Kesavan Deepak-Kesavan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated review (re-review)

Automated review by Unstract PR review kit (Claude Code). Each finding below was reproduced against the code rather than inferred, so treat it as something to resolve before merge. If one is wrong, disagree on the thread and close it — that is the expected way to clear a finding. Anything tagged [unverified] was not reproduced and is flagged for your judgement instead.

2 inline comment(s) · 2 finding(s) not on changed lines (below).

Findings off the diff

  • backend/utils/input_sanitizer.py:17
    [critical] HTML_TAG_PATTERN's ambiguous <\s*/?\s* prefix backtracks quadratically, and this PR is what attaches it to every writable CharField/TextField on ~18 OSS serializers plus every enterprise AuditSerializer subclass — so one small authenticated request pins a gunicorn worker for minutes of pure CPU.

Any authenticated org member PATCHes a covered field with "<" + " "*N + "x" (no leading/trailing whitespace, so DRF's trim_whitespace does not shorten it). Measured end-to-end through the real serializer field in the PR checkout, using the checkout's own venv (DRF 3.17.1 per backend/uv.lock):

  2 KB body ->  0.16 s   8 KB ->  2.51 s   16 KB -> 10.37 s   32 KB -> 40.16 s

Clean O(n^2) — and is_valid() returns True. The value is not even rejected; it is pure wasted CPU.

Django's default DATA_UPLOAD_MAX_MEMORY_SIZE is 2.5 MB (no override in backend/backend/settings/), so a single 2.5 MB description extrapolates to ~65 CPU-hours in one request. CPython's regex engine does not release the GIL, and backend/entrypoint.sh:81-85 runs gunicorn --worker-class gthread --threads 512 --workers 2 --timeout 600 — so one such request freezes an entire worker process (all 512 threads, every tenant) and is not reaped for 10 minutes. Two concurrent requests take the shared backend down.

Reachable fields include CustomTool.description and ToolStudioPrompt.prompt_key (both models.TextField with no max_length, so no MaxLengthValidator in front — and DRF's run_validators collects all errors rather than short-circuiting, so the sanitizer runs even on bounded fields), plus Tag.description, Notification.*, and every enterprise AuditSerializer CharField (CannedQuestion.question, ChatHistory.label, AppDeployment.description) — the enterprise repo declares no html_safe_fields anywhere.

origin/main was not vulnerable: its HTML_TAG_PATTERN = r"<[^>]*>|<[a-zA-Z/!]" is linear (0.036 ms at 32 KB).

Suggested fix: Collapse the ambiguous prefix into one unambiguous character class:

HTML_TAG_PATTERN = re.compile(rf"<[\s/]*({_SCRIPTABLE_TAGS})\b", re.IGNORECASE)

Verified in the checkout: 200 000 spaces -> 16 ms (linear), and it still matches <script>x, </ script >, < / SCRIPT, <svg onload=1 while still allowing qty < 500 and the <invoice_no> field. Also cap input length before running any regex, and add a regression test asserting the validator completes in bounded time on "<" + " "*50000 + "x".

🤖 Unstract PR review kit (Claude Code) · review-pr-bot:fd36ebbebd19

  • backend/utils/input_sanitizer.py:38
    [major] Follow-up to your reply on commit 5ffaa0af1: narrowing the tag rule to 15 script-capable tags leaves EVENT_HANDLER_PATTERN's hand-maintained _DOM_EVENTS allow-list as the only guard on inert carrier tags — and it omits every modern no-interaction event, so payloads origin/main rejected are now accepted on fields main already protected.

Your reply says "<img src=x onerror=alert(1)> is still rejected, via the event-handler rule" — true for onerror, but the rule is a fixed ~60-name list and the tag rule no longer backstops it. Executed against the shipped validate_no_html_tags / validate_name_field in the PR checkout, compared with git show origin/main:backend/utils/input_sanitizer.py:

  main=rejected  PR=ACCEPTED  <video src=1 onloadstart=alert(1)>      media autoload, zero interaction
  main=rejected  PR=ACCEPTED  <audio src=x onloadedmetadata=alert(1)> zero interaction
  main=rejected  PR=ACCEPTED  <b onbeforetoggle=alert(1)>             popover API
  main=rejected  PR=ACCEPTED  <b onanimationend=alert(1)>             CSS animation
  main=rejected  PR=ACCEPTED  <body onpageshow=alert(1)>              bfcache
  main=rejected  PR=ACCEPTED  <x onauxclick=alert(1)>
  main=rejected  PR=ACCEPTED  <img src=x onerror/**/=alert(1)>        comment between name and '='

Two independent lenses reproduced this by importing the module from the checkout, not by reading it. Also missing from the list: oncanplay, onended, ontimeupdate, ontransitionend, onbeforeinput, onslotchange, onhashchange, onpopstate, onbeforeunload.

The sharp part is that this is a regression on fields main already guarded, not just an incomplete new guard: validate_name_field("<b onbeforetoggle=alert(1)>") now returns ACCEPTED, and it fronts APIDeployment.display_name (api_v2/serializers.py:83), Workflow.workflow_name (workflow_v2:65), CustomTool.tool_name (prompt_studio_core_v2:127), AdapterInstance.adapter_name (adapter_processor_v2:51), connector_v2:57, notification_v2:126, account_v2/serializer.py:15,18. The three description fields whose validate_description this PR deletes are in the same position: the mixin calls the identical function, but that function is now weaker than the one main ran.

Not currently exploitable, and I want to be straight about that: I found no dangerouslySetInnerHTML or rehype-raw in OSS frontend/src (CustomMarkdown.jsx is token-based, MarkdownRenderer.jsx uses remarkGfm only), no mark_safe/format_html on these fields in either backend, and the one unstract-cloud hit (plugins/verticals/.../ValidatedTextArea.jsx:198) escapes at line 87 and is not fed by these fields. This is defence-in-depth that got thinner, under a PR titled as stored-XSS prevention — not a live XSS path.

Suggested fix: Make the event rule structural instead of an allow-list, so it cannot rot as new DOM events ship:

EVENT_HANDLER_PATTERN = re.compile(r"<[a-zA-Z][^>]*\bon[a-z]{3,}\s*=", re.IGNORECASE | re.DOTALL)

Benign prose (onboarding=, connection=) still passes because it has no leading <tag, which is what the allow-list was built to protect. Add onloadstart, onloadedmetadata, onbeforetoggle, onanimationend, onpageshow and onerror/**/= as regression tests alongside the four false-positive cases from 5ffaa0af1.

🤖 Unstract PR review kit (Claude Code) · review-pr-bot:ec64e2a0c2b4

review-pr-bot:review

Comment thread frontend/src/components/custom-tools/prompt-card/PromptCard.jsx Outdated
Comment thread frontend/src/components/custom-tools/prompt-card/PromptCard.jsx Outdated

@Deepak-Kesavan Deepak-Kesavan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated review (re-review)

Automated review by Unstract PR review kit (Claude Code). Each finding below was reproduced against the code rather than inferred, so treat it as something to resolve before merge. If one is wrong, disagree on the thread and close it — that is the expected way to clear a finding. Anything tagged [unverified] was not reproduced and is flagged for your judgement instead.

1 inline comment(s) · 1 finding(s) not on changed lines (below).

Findings off the diff

  • frontend/src/components/custom-tools/prompt-card/PromptCard.jsx:164
    [major] Making the rollback unconditional is the right call, but its target is wrong: prevPromptDetailsState is a whole-object snapshot of the card's own optimistic state, not a read of the stored row — so the rollback reverts sibling fields the server accepted in the meantime, and across overlapping rejections it settles on a value the server rejected.

Both halves were reproduced independently by two lenses, one of them by rendering the real PromptCard under vitest with handleChangePromptCard returning deferred promises.

(a) A prompt_key rejection reverts a sibling field the server just accepted — this half is new in 06fad5445.
Stored row: prompt_key="A", active=true. User edits the key; after the 1000 ms debounce (EditableText.jsx:57-62) the PATCH goes out and prevPromptDetailsState is snapshotted as the whole object. While it is in flight the user flips the disable toggle — Header.jsx:158-176 calls handleChange with no debounce at all — and the server accepts it, so active=false locally and remotely. The key PATCH then 400s. attemptGen is keyed per field name, so an accepted PATCH on active never bumps the prompt_key generation: isLatestAttempt() is true and PromptCard.jsx:229 writes the whole stale snapshot back, flipping active to true. Header.jsx:201-206 (useEffect(..., [promptDetails, details])) re-derives isDisablePrompt/required/webhookEnabled/webhookUrl off the new object identity, so the toggle visibly reverts while the server holds the new value. Same shape with the prompt body: PromptCardItems.jsx:218-229 feeds defaultText={promptDetails?.prompt} to the textarea EditableText, which is never passed error, so the new early return at EditableText.jsx:29-33 does not protect it — its [defaultText] effect calls setText and the body the user just saved disappears from the screen.
Probe output at 06fad5445: after toggle accepted: {..."active":false} -> after key 400 rollback: {..."active":true}; and body accepted, card.prompt = P1-new body -> after key 400 rollback, card.prompt = P0. At e6e048a61 neither happened, because if (!handledInline) skipped the rollback entirely for a renderable prompt_key error. This path is created by making the rollback unconditional.

(b) Across two overlapping rejections the card settles on a rejected value — the invariant your commit message states does not hold.
Stored prompt_key="good". Attempt 1 PATCHes bad1 and writes it optimistically at :190. A render happens, so attempt 2 — fired ~1 s later by the same debounce, with the input not disabled while saving (EditableText.jsx:118-120 disables only on isCoverageLoading/isSinglePassExtractLoading/isPublicSource) — captures prevPromptDetailsState = {prompt_key: "bad1"}. Both 400. Attempt 1's rejection is correctly ignored. Attempt 2 rolls back to its snapshot, so the card settles on bad1, which the server rejected and never stored.
Rendered-component probe: 06fad5445 -> bad1; e6e048a61 -> bad2; correct answer good. The generation counter suppresses the superseded attempt's write but never corrects the surviving attempt's target. Nothing repairs it afterwards: the success path only writes updatedPromptsCopy (:206-209), and the resync effect at :83-92 is one-shot behind isPromptDetailsStateUpdated.
Downstream this is the same consequence as the finding this commit closes — PromptOutput.jsx:242/246/249 key highlightData/confidenceData/wordConfidenceData on promptDetails.prompt_key inside the (singlePassExtractMode || isSimplePromptStudio) branch (:189-192), and PromptCard.jsx:449 passes it as OutputForDocModal's promptKey.

Both halves need a PATCH round-trip to outlast the next user action — ordinary on a loaded backend, and exactly the race your new comment at PromptCard.jsx:60-63 acknowledges. Neither is a deterministic failure on every rejection.

Suggested fix: Roll back to the last server-confirmed value, and only for the field this attempt owns:

// seeded from `promptDetails` in the :83-92 effect, refreshed in .then from res.data
const lastSaved = useRef({});
// ...
setPromptDetailsState((prev) => ({ ...prev, [name]: lastSaved.current[name] }));

That fixes both halves at once: sibling fields are never touched, and the rollback becomes idempotent across any number of overlapping rejections. Reading the stored value out of useCustomToolStore.getState().details.prompts by prompt_id at rejection time works equally well — DocumentParser.jsx:207-224 keeps that copy current on every success. frontend/ has vitest 3.2.6 wired up with a test script, and the deferred-promise harness both lenses used to reproduce this is about 30 lines, so the overlapping-rejection case is cheap to lock down.

🤖 Unstract PR review kit (Claude Code) · review-pr-bot:bc01ad124064

review-pr-bot:review

Comment thread frontend/src/components/custom-tools/prompt-card/PromptCard.jsx
@chandrasekharan-zipstack
chandrasekharan-zipstack force-pushed the feat/UN-3393-input-validation-sweep branch 2 times, most recently from 998e071 to 6166b4b Compare September 2, 2026 05:23
Builds on the foundation PR by routing every `AuditSerializer` subclass
through `SanitizedSerializerMixin`. ~18 write-path serializers
(`WorkflowSerializer`, `CustomToolSerializer`, `APIDeploymentSerializer`,
`APIKeySerializer`, `ConnectorInstanceSerializer`, `BaseAdapterSerializer`,
`PlatformKeySerializer`, `PlatformApiKey{Create,Update}Serializer`,
`PipelineSerializer`, `ToolInstanceSerializer`, `ToolStudioPromptSerializer`,
`PromptStudioOutputSerializer`, `ProfileManagerSerializer`,
`IndexManagerSerializer`, `PromptStudioRegistry{,Info}Serializer`,
`PromptStudioDocumentManagerSerializer`) now auto-reject HTML/JS-shaped
input on every writable `CharField` / `TextField`.

- `backend/backend/serializers.py`: `AuditSerializer` now inherits
  `utils.serializer.ModelSerializer` (the pre-mixed variant) instead of
  `rest_framework.serializers.ModelSerializer`. `create` / `update`
  semantics unchanged.

- `prompt_studio/prompt_studio_v2.ToolStudioPromptSerializer`: declares
  `Meta.html_safe_fields = ("prompt", "assert_prompt",
  "assertion_failure_prompt", "output")`. LLM prompt text legitimately
  contains XML/HTML-like markup (e.g. `<context>`, `<thinking>`); LLM
  `output` may include anything the model produced.

- `prompt_studio/prompt_studio_core_v2.CustomToolSerializer`: declares
  `Meta.html_safe_fields = ("summarize_prompt", "preamble", "postamble",
  "output")`. Tool-level LLM context fields and stored LLM output.

- Removes redundant manual `validate_description(self, value)` methods
  in `api_v2.APIDeploymentSerializer`,
  `workflow_v2.WorkflowSerializer`, and
  `prompt_studio_core_v2.CustomToolSerializer`. The mixin now covers
  these via the `AuditSerializer` base. `validate_<name>` methods that
  use `validate_name_field` are retained because they also strip
  whitespace and reject empty values — behaviour the mixin doesn't
  duplicate.

- Imports of `validate_no_html_tags` are dropped from the three files
  whose `validate_description` methods were removed.

Files that inherit DRF base classes directly (without going through
`AuditSerializer`) are tracked as a follow-up sweep. The AuditSerializer
path already covers the highest-value write-path entities.

Test coverage: `cd backend && uv run pytest utils/tests/ -q` → 85 passed.
Full Django check requires cloud-deps; PR CI exercises the full suite.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extends PR2 coverage to two user-write-path serializers that don't go
through `AuditSerializer`:

- `tags.TagSerializer` (user-create with `name` + `description`) now
  inherits `utils.serializer.ModelSerializer`. Sister
  `TagParamsSerializer` is a query-param parser with strict regex
  validation; left as-is.

- `notification_v2.NotificationSerializer` now inherits
  `utils.serializer.ModelSerializer`. The model's `name`,
  `authorization_key`, `authorization_header`, and `url` (URLField is a
  CharField subclass) are auto-sanitized. `notification_type`,
  `authorization_type`, `platform` are ChoiceField in the serializer,
  not CharField; the mixin skips them. The existing manual
  `validate_name` keeps its uniqueness + strip-whitespace logic; the
  mixin's HTML check runs alongside as redundant defence.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Greptile P1 review comment on PR #1966. `PromptStudioOutputManager`
stores raw LLM responses (`output`: CharField) and document chunks
(`context`: TextField) that routinely contain <thinking>, <context>,
and other XML-like tags. The serializer is mounted on a ModelViewSet
with no class-level HTTP-method restriction, so write paths through
DRF admin / browsable API / any future write endpoint would
incorrectly reject legitimate LLM output without this opt-out.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Mixin now emits "Prompt key …" instead of "prompt_key …" by using
  `field.label` or a title-cased fallback when invoking the validator.
- DocumentParser re-throws DRF field-keyed validation errors so the
  prompt card can render them inline instead of showing only a transient
  toast. Non-field errors continue to surface via the existing toast.
- PromptCard tracks per-field error state, keeps the typed value so
  users can edit-and-fix in place, and clears the error on the next
  edit attempt for the same field.
- EditableText accepts an `error` prop, applies Ant `status="error"`
  on the input, and renders the message in a danger Text node below.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
handleChangePromptCard decided that a field-keyed validation error would be
rendered inline purely from the error's shape, and skipped the global alert
on that basis. Two callers made that wrong:

- NotesCard has no inline renderer, so a rejected note title reverted with
  no feedback at all where it previously showed a toast.
- PromptCard rendered only prompt_key, so a rejection on any other attr
  (postprocessing_webhook_url, enforce_type, non_field_errors) produced no
  inline error, no rollback and no alert.

The caller now passes an onFieldError callback and returns whether it can
render the errors; anything it declines falls back to the alert. PromptCard
gates on RENDERABLE_FIELD_ERRORS and rolls back when nothing was rendered.
handleChangePromptCard re-throws in both paths so callers still see the
failure.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MbK3GDtfcExzb6kceRAdhu
Header keeps its own copy of the webhook and toggle values, and its rollback
handlers only ran if handleChange rejected. Call sites with no local state to
restore opt out explicitly.
The fixture relied on a plain h1 tag, which the narrowed sanitizer allows.
…edit is rejected

An inline field error used to suppress the rollback of promptDetailsState, so
a rejected prompt_key stayed in the card's copy of the row while the database
kept the old one. Outputs, highlight and confidence data are keyed by
prompt_key in single-pass extract and Simple Prompt Studio, so they stopped
resolving for that prompt until the card unmounted.

The rollback is now unconditional and EditableText keeps the typed text on
screen while its inline error is showing, which is the part the user needs to
correct in place.

Each attempt also carries a per-field generation, so a rejection that lands
after a newer attempt was accepted no longer paints a permanent inline error
over a value the server took, and a success clears the field's error.
@chandrasekharan-zipstack
chandrasekharan-zipstack force-pushed the feat/UN-3393-input-validation-sweep branch from 6166b4b to dc25298 Compare September 2, 2026 05:34
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Frontend Lint Report (Biome)

All checks passed! No linting or formatting issues found.

@sonarqubecloud

sonarqubecloud Bot commented Sep 2, 2026

Copy link
Copy Markdown

@github-actions

github-actions Bot commented Sep 2, 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 21.0
e2e-coowners e2e 1 0 0 0 1.8
e2e-etl e2e 1 0 0 0 8.3
e2e-login e2e 2 0 0 0 1.2
e2e-prompt-studio e2e 1 0 0 0 4.6
e2e-smoke e2e 2 0 0 0 1.3
e2e-workflow e2e 1 0 0 0 16.4
frontend unit 0 1 0 0 0.0
integration-backend integration 310 0 0 26 46.7
integration-connectors integration 1 0 0 7 8.0
integration-workers integration 157 0 0 1 50.8
ui e2e 0 1 0 0 0.0
unit-backend unit 1181 0 0 1 43.1
unit-connectors unit 63 0 0 0 9.9
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.6
unit-runner unit 5 0 0 0 2.9
unit-sdk1 unit 563 0 0 0 29.5
unit-workers unit 1397 0 0 1 130.2
TOTAL 3856 2 0 36 384.3

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants