Skip to content

Reject empty reference tokens in JsonPatch #68613

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Youssef1313 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Reject empty reference tokens in JsonPatch #68613

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/Features/JsonPatch.SystemTextJson/src/Internal/ParsedPath.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Generic;
using System.Text;
using Microsoft.AspNetCore.JsonPatch.SystemTextJson.Exceptions;
Expand Down Expand Up @@ -36,6 +37,11 @@ public string LastSegment

private static string[] ParsePath(string path)
{
if (path.Length == 0)
{
return Array.Empty<string>();
}
Comment on lines +40 to +43

var strings = new List<string>();
var sb = new StringBuilder(path.Length);

Expand All @@ -48,6 +54,10 @@ private static string[] ParsePath(string path)
strings.Add(sb.ToString());
sb.Length = 0;
}
else if (i != 0)
{
throw new JsonPatchException(Resources.FormatInvalidValueForPath(path), null);
}
}
else if (path[i] == '~')
{
Expand Down Expand Up @@ -80,6 +90,10 @@ private static string[] ParsePath(string path)
{
strings.Add(sb.ToString());
}
else
{
throw new JsonPatchException(Resources.FormatInvalidValueForPath(path), null);

@DeagleGross DeagleGross Aug 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

per RFC 6901 section 5 both "" and "/" are valid and we accept the first one and reject the latter. Should we acccept both instead?

@Youssef1313 Youssef1313 Aug 18, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@DeagleGross From the same spec // is also valid but we are rejecting it.

Maybe we should try to be fully compliant then?

The reason I rejected / is that it essentially means a key which is empty string, which is very similar to // (just that an extra level of nesting).

So I would say we either handle empty keys everywhere, or we don't handle empty keys.

For "" it means whole document, not empty key, which is why I viewed it differently.

@DeagleGross DeagleGross Aug 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe we should try to be fully compliant then?

Ye, I imagined we started to treat none of these as errors.

@Youssef1313 Youssef1313 Aug 18, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Okay. I'll look more into the implementation and the RFC to get it right.

}

return strings.ToArray();
}
Expand Down
18 changes: 18 additions & 0 deletions src/Features/JsonPatch.SystemTextJson/test/Internal/ParsedPathTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ public class ParsedPathTests
[InlineData("foo/bar~1baz", new string[] { "foo", "bar/baz" })]
[InlineData("foo/bar~0/~0/~1~1/~0~0/baz", new string[] { "foo", "bar~", "~", "//", "~~", "baz" })]
[InlineData("~0~1foo", new string[] { "~/foo" })]
[InlineData("/foo/bar", new string[] { "foo", "bar" })]
[InlineData("/foo", new string[] { "foo" })]
[InlineData("", new string[] { })]
public void ParsingValidPathShouldSucceed(string path, string[] expected)
{
// Arrange & Act
Expand All @@ -38,4 +41,19 @@ public void PathWithInvalidEscapeSequenceShouldFail(string path)
var parsedPath = new ParsedPath(path);
});
}

[Theory]
[InlineData("//isSmth")]
[InlineData("//")]
[InlineData("/foo/")]
[InlineData("/foo//bar")]
[InlineData("foo//bar")]
[InlineData("foo/")]
public void PathWithEmptyReferenceTokenShouldFail(string path)
Comment on lines +45 to +52
{
Assert.Throws<JsonPatchException>(() =>
{
var parsedPath = new ParsedPath(path);
});
}
}
13 changes: 13 additions & 0 deletions src/Features/JsonPatch/src/Internal/ParsedPath.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@ public string LastSegment

private static string[] ParsePath(string path)
{
if (path.Length == 0)
{
return Array.Empty<string>();
}
Comment on lines +44 to +47

var strings = new List<string>();
var sb = new StringBuilder(path.Length);

Expand All @@ -53,6 +58,10 @@ private static string[] ParsePath(string path)
strings.Add(sb.ToString());
sb.Length = 0;
}
else if (i != 0)
{
throw new JsonPatchException(Resources.FormatInvalidValueForPath(path), null);
}
}
else if (path[i] == '~')
{
Expand Down Expand Up @@ -85,6 +94,10 @@ private static string[] ParsePath(string path)
{
strings.Add(sb.ToString());
}
else
{
throw new JsonPatchException(Resources.FormatInvalidValueForPath(path), null);
}

return strings.ToArray();
}
Expand Down
20 changes: 19 additions & 1 deletion src/Features/JsonPatch/test/Internal/ParsedPathTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Licensed to the .NET Foundation under one or more agreements.
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using Microsoft.AspNetCore.JsonPatch.Exceptions;
Expand All @@ -16,6 +16,9 @@ public class ParsedPathTests
[InlineData("foo/bar~1baz", new string[] { "foo", "bar/baz" })]
[InlineData("foo/bar~0/~0/~1~1/~0~0/baz", new string[] { "foo", "bar~", "~", "//", "~~", "baz" })]
[InlineData("~0~1foo", new string[] { "~/foo" })]
[InlineData("/foo/bar", new string[] { "foo", "bar" })]
[InlineData("/foo", new string[] { "foo" })]
[InlineData("", new string[] { })]
public void ParsingValidPathShouldSucceed(string path, string[] expected)
{
// Arrange & Act
Expand All @@ -38,4 +41,19 @@ public void PathWithInvalidEscapeSequenceShouldFail(string path)
var parsedPath = new ParsedPath(path);
});
}

[Theory]
[InlineData("//isSmth")]
[InlineData("//")]

@DeagleGross DeagleGross Aug 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

test [InlineData("/")]?

[InlineData("/foo/")]
[InlineData("/foo//bar")]
[InlineData("foo//bar")]
[InlineData("foo/")]
public void PathWithEmptyReferenceTokenShouldFail(string path)
Comment on lines +45 to +52

@DeagleGross DeagleGross Aug 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

could we add an E2E tests for such scenario to not only asssert the exception, but validate what's the HTTP response?

{
Assert.Throws<JsonPatchException>(() =>
{
var parsedPath = new ParsedPath(path);
});
}
}
Loading