Skip to content

Conversation

N-Olbert
Copy link
Contributor

@N-Olbert N-Olbert commented Aug 6, 2025

Summary of the changes (Less than 80 chars)

  • Test to verify that control characters are handled correctly

Closes #7759 (can not reproduce)

@michaelstaib
Copy link
Member

I think the issue is in the transport layer. So, this needs a full integration test. You can add that here:
https://github.com/ChilliCream/graphql-platform/blob/main/src/HotChocolate/AspNetCore/test/AspNetCore.Tests/GraphQLOverHttpSpecTests.cs

I have a hunch what is causing this :) but lets see once you added the test.

@michaelstaib michaelstaib marked this pull request as draft August 28, 2025 06:33
@N-Olbert
Copy link
Contributor Author

@michaelstaib
I created the test, but it produced the same outcome. After investigating further, it seems to be a real issue.

My tests worked because I used properly encoded values, but in fact this shouldn’t be required.

If I don`t escape the DEL-char it is indeed valid json:

const string unescaped = "\"\u007f\""; // 3 chars, "[DEL]"
const string escaped = "\"\\u007F\"";  // 8 chars, "\u007F"
Assert.NotEqual(unescaped.Length, escaped.Length);
Assert.Equal(
    JsonSerializer.Deserialize<char>(unescaped),
    JsonSerializer.Deserialize<char>(escaped)); // passes

(For values between \u000 and \u001F one get a proper exception, like
System.Text.Json.JsonException: ''0x1F' is invalid within a JSON string. The string should be correctly escaped.)

Anyways, if I use a not escaped DEL-char with HC I do get the error.

After some digging, I believe this should be valid according to the GraphQL spec as well.
The spec states in 2.9.4

StringCharacter ::

  • SourceCharacter but not " or \ or LineTerminator
  • \u EscapedUnicode
  • \ EscapedCharacter

Spec

Where SourceCharacter is defined as:

SourceCharacter ::

  • "U+0009"
  • "U+000A"
  • "U+000D"
  • "U+0020–U+FFFF"

Spec

Soooo... 'DEL'/'\u007F' is a member of [U+0020, U+FFFF], so it should be valid.

This statement: "Since {SourceCharacter} must not contain some ASCII control characters, escape
sequences must be used to represent these characters
" only refers to disallowed SourceCharacters, which means escaping should only be mandatory for [U+0000, U+001F] (except for TAB, LF and CR) in GraphQL.

Do you agree with my reading of the spec?

I have adjusted the PR so that DEL is treated as a normal input now. DEL was special-cased before, I removed that handling since, according to the spec, DEL is not a special character.

If this change is desired, I would also add tests for the other special characters and convert the current integration test into a proper unit test that directly verifies the Utf8GraphQLReader.

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.

Allow DEL character in request variables
2 participants