Skip to content

Fix double-encoding of escaped string variables in multipart uploads#9839

Merged
michaelstaib merged 4 commits into
mainfrom
multipart-text-encoding
Jun 4, 2026
Merged

Fix double-encoding of escaped string variables in multipart uploads#9839
michaelstaib merged 4 commits into
mainfrom
multipart-text-encoding

Conversation

@michaelstaib

Copy link
Copy Markdown
Member

Summary

Escaped string variables in graphql-multipart-request-spec uploads were being double-encoded when the request JSON was rewritten to inject file references. Any escape sequence (\", \\, \n, \uXXXX, ...) in a string variable was doubled, so resolvers (and forwarded subgraph requests) received corrupted values.

This fixes it across every affected path.

Root cause

While rewriting the variables/operations JSON, already-escaped bytes from Utf8JsonReader.ValueSpan were written back through a writer that escapes again, doubling every escape sequence.

Fix

  • HotChocolate.Text.Json.JsonWriter (WriteStringValue(ReadOnlySpan<byte>, skipEscaping: true)): the raw-write path now adds the surrounding quotes automatically when the value is not already wrapped in them. Detection is a single leading-byte check in WriteStringValueRaw (sound because a JSON string's content can never start with a raw ", a literal quote is always backslash-escaped), threaded into both the minimized and indented writers. This lets callers pass Utf8JsonReader.ValueSpan straight through as a zero-copy, zero-allocation verbatim copy, no GetString() and no re-escape.
  • Fusion FileEntryBuilder: the string-value branch is now a single WriteStringValue(reader.ValueSpan, skipEscaping: true) passthrough (no buffer, no decode), and property names decode once and reuse for both the writer and the file-map path. Fixes double-encoding of escaped string values and escaped property names.
  • Fusion InMemorySourceSchemaClient: its StripFileMarkers value path already used skipEscaping: true with an unquoted span and was emitting invalid JSON; the writer change corrects it automatically.
  • AspNetCore HttpMultipartMiddleware: escaped string variables are decoded via Utf8JsonReader.CopyString into a pooled buffer (cleared after use, since variable values may carry secrets) and written once, keeping the unescaped fast path. System.Text.Json's Utf8JsonWriter has no skipEscaping, so decode-then-write is required there.

Tests

  • JsonWriter framing: bare content gets framed, already-quoted literals stay single-framed, empty maps to "", plus the indented path.
  • FileEntryBuilder: escaped value and unicode-escaped property name round-trip single-encoded.
  • HttpMultipartMiddleware: escaped and mixed-escape (", \, newline, unicode) string variables reach the resolver intact.
  • Green on net8.0 / net9.0 / net10.0. All existing skipEscaping call sites were audited and are unaffected.

Out of scope (follow-ups)

  • The escaped-property-name path in InMemorySourceSchemaClient (WritePropertyName has no skipEscaping) and the multi-segment ValueSpan case (value split across ReadOnlySequence segments) remain. Both are narrow and pre-existing.

Supersedes #9810. Original fix by @BickelLukas (Consolidate-Software); this branch carries that work plus the Fusion and JsonWriter follow-ups.

Copilot AI 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.

Pull request overview

Fixes incorrect double-encoding of escaped GraphQL string variables during multipart upload JSON rewrites by enabling a zero-copy “verbatim” write path (no re-escaping) and updating multipart rewrite codepaths to use it safely.

Changes:

  • Updated HotChocolate.Text.Json.JsonWriter.WriteStringValue(..., skipEscaping: true) to automatically add surrounding quotes when callers pass unquoted, pre-escaped UTF-8 content (e.g., Utf8JsonReader.ValueSpan).
  • Adjusted Fusion multipart variable rewriting (FileEntryBuilder) to passthrough escaped string values verbatim and to decode property names once for both writing and file-map path building.
  • Updated ASP.NET Core multipart middleware to decode escaped string variables before writing via System.Text.Json (which lacks a skipEscaping equivalent), and added regression tests across layers.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/HotChocolate/Json/test/Json.Tests/JsonWriterTests.cs Adds coverage for quote-framing behavior when writing pre-escaped UTF-8 string content with skipEscaping: true (minimized + indented).
src/HotChocolate/Json/src/Json/JsonWriter.WriteValues.String.cs Implements conditional quote-framing in the raw write path to prevent double-encoding and invalid JSON.
src/HotChocolate/Fusion/test/Fusion.Execution.Tests/Transport/Http/FileEntryBuilderTests.cs Adds regression tests for escaped string values and unicode-escaped property names in Fusion multipart variable rewriting.
src/HotChocolate/Fusion/src/Fusion.Execution/Transport/Http/FileEntryBuilder.cs Writes string values using skipEscaping: true passthrough and avoids double-decoding property names.
src/HotChocolate/AspNetCore/test/AspNetCore.Tests/HttpMultipartMiddlewareTests.cs Adds multipart upload tests ensuring escaped/mixed-escape string variables reach resolvers intact.
src/HotChocolate/AspNetCore/test/AspNetCore.Tests.Utilities/UploadQuery.cs Adds resolver used by new multipart upload tests (UploadWithText).
src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/HttpMultipartMiddleware.cs Decodes escaped string values using Utf8JsonReader.CopyString into a pooled buffer before writing with Utf8JsonWriter.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@michaelstaib michaelstaib merged commit c26e2f7 into main Jun 4, 2026
280 of 282 checks passed
@michaelstaib michaelstaib deleted the multipart-text-encoding branch June 4, 2026 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants