Fixed Relay global ID and cursor helpers throwing on malformed client input by xperiandri · Pull Request #638 · fsprojects/FSharp.Data.GraphQL

xperiandri · 2026-10-10T09:49:34Z

Problem

Relay global IDs and cursors come from the client.

Exceptions on malformed input. fromGlobalId, the GlobalId active pattern built on it, and Cursor.toOffset decoded them with Convert.FromBase64String, which throws FormatException on anything that is not valid base64. A node(id: ...) query with a malformed ID therefore answered with a GraphQL error carrying the exception's message, instead of a null node.
Lenient offsets. Cursor.toOffset parsed the offset with the current culture and NumberStyles.Integer. It accepted negative offsets, which index before the start of the data, as well as signs and surrounding whitespace.

Changes

fromGlobalId:
- decodes with Convert.TryFromBase64String;
- rejects bytes that are not valid UTF-8 instead of reading them as U+FFFD, which mapped different IDs to the same text;
- splits at the first :.
It returns ValueNone for a null, empty, non-base64, non-UTF-8 or separator-less ID. Its signature is unchanged.
Cursor.tryToOffset (new) returns ValueNone for a malformed cursor, so that a resolver can report it as a GraphQL error.

It accepts only a non-negative Int32 written in ASCII decimal digits, parsed with the invariant culture. The digits are checked before parsing, because Int32.TryParse ignores trailing NUL characters even with NumberStyles.None.
Cursor.toOffset uses tryToOffset, and returns the default value for a malformed cursor.
Cursor.ofOffset formats with the invariant culture.
Docs. The docs of NodeField and NodeAsyncField say that the resolver gets the raw client ID and should return None when fromGlobalId yields nothing.
Breaking: Cursor.toOffset no longer reads back a cursor that Cursor.ofOffset wrote for a negative offset. The library itself never writes negative offsets.

Tests

The tests are xUnit, in Relay/NodeTests.fs and Relay/CursorTests.fs:

malformed global IDs and cursors through the helpers and through executed queries: not base64, non-UTF-8, no separator, signs, whitespace, non-ASCII digits, overflow, other prefixes, trailing NULs;
node resolving a malformed ID to null without errors;
a connection reporting a malformed after cursor as a GraphQL error;
round trips of toGlobalId/fromGlobalId and ofOffset/tryToOffset.

Verification:

Unit tests: the whole project passes, 833 tests with 5 skipped.
FAKE: the full pipeline passes, including the integration tests.

🤖 Generated with Claude Code

… input Relay global IDs and connection cursors come from the client, but the helpers decoding them threw on malformed input: - `fromGlobalId` and the `GlobalId` active pattern used `Convert.FromBase64String`, which throws `FormatException` on anything that is not valid base64. A `node` query with a malformed `id` produced a GraphQL error carrying the exception's message instead of a `null` node. - `Cursor.toOffset` decoded through the same pattern, so it threw too, and parsed the offset with the current culture and `NumberStyles.Integer`, accepting negative offsets, signs and surrounding whitespace. Changes: - `fromGlobalId` decodes with `Convert.TryFromBase64String`, rejects bytes that are not valid UTF-8 instead of turning them into U+FFFD, and splits at the first `:` byte; malformed input yields `ValueNone`. Its signature is unchanged. - Added `Cursor.tryToOffset`, which accepts only ASCII decimal digits parsed with `NumberStyles.None` and the invariant culture and returns `ValueNone` otherwise; `Cursor.toOffset` is built on it and keeps its signature. - `Cursor.ofOffset` formats the offset with the invariant culture. - Documented on `NodeField`/`NodeAsyncField` how a resolver turns a malformed ID into a `null` node. - Tests cover empty, whitespace, non-base64, non-ASCII, non-UTF-8, missing separator, negative, signed, overflowing and non-ASCII-digit IDs and cursors, through the helpers and through executed `node` and connection queries. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

`Int32.TryParse` ignores trailing NUL characters even with `NumberStyles.None`, so `Cursor.tryToOffset` read "1" followed by NULs as 1, although it is documented to accept ASCII decimal digits only. The local ID of a cursor is now checked to hold only ASCII digits before it is parsed. The release note of the stricter `Cursor.toOffset` is marked as a breaking change: a cursor that `Cursor.ofOffset` wrote for a negative offset is no longer read back. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

xperiandri · 2026-10-10T09:51:10Z

+        letters
+        |> Array.skip start
+        |> Array.truncate first
+        |> Array.mapi (fun index letter -> { Cursor = Cursor.ofOffset (start + index); Node = letter })


Suggested change

letters

|> Array.skip start

|> Array.truncate first

|> Array.mapi (fun index letter -> { Cursor = Cursor.ofOffset (start + index); Node = letter })

letters

|> Seq.skip start

|> Seq.truncate first

|> Seq.mapi (fun index letter -> { Cursor = Cursor.ofOffset (start + index); Node = letter })

|> Seq.toArray

Applied in 4ff2fdd.

xperiandri · 2026-10-10T09:51:35Z

+                        box <| NameValueLookup.ofList [ "node", upcast "c" ]
+                        upcast NameValueLookup.ofList [ "node", upcast "d" ]


why differently?

No reason: the first item of the list was boxed with box <| and the second with upcast. Both go through box <| now (4ff2fdd).

Copilot

🟢 Approval recommended

The implementation and tests consistently cover the documented malformed-input behavior.

0 open findings

What changed in this PR

Improves Relay helpers so malformed client-provided global IDs and cursors are handled safely and consistently.

Changes:

Adds non-throwing Base64/UTF-8 global ID decoding.
Adds strict cursor offset validation via Cursor.tryToOffset.
Expands documentation, release notes, and malformed-input tests.

File	Description
`src/FSharp.Data.GraphQL.Server.Relay/Node.fs`	Safely decodes and validates global IDs.
`src/FSharp.Data.GraphQL.Server.Relay/Connections.fs`	Adds strict cursor parsing and invariant formatting.
`tests/FSharp.Data.GraphQL.Tests/Relay/NodeTests.fs`	Tests malformed IDs and node resolution.
`tests/FSharp.Data.GraphQL.Tests/Relay/CursorTests.fs`	Tests cursor validation and query behavior.
`RELEASE_NOTES.md`	Documents behavior and breaking changes.

🧠 Review effort: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

The cursor decoder opens `System` instead of qualifying `Int32`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions · 2026-10-10T10:15:12Z

Test Results

9 files 9 suites 14m 25s ⏱️
941 tests 936 ✅ 5 💤 0 ❌
2 823 runs 2 808 ✅ 15 💤 0 ❌

Results for commit 4ff2fdd.

♻️ This comment has been updated with latest results.

…alike Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

xperiandri and others added 2 commits October 3, 2026 04:17

Copilot AI balanced review requested due to automatic review settings October 10, 2026 09:49

Copilot started reviewing on behalf of xperiandri October 10, 2026 09:50 View session

xperiandri commented Oct 10, 2026

View reviewed changes

Copilot AI reviewed Oct 10, 2026

View reviewed changes

Asserted with Assert.Equal and Assert.Empty in the Relay ID tests

fc4a685

The cursor decoder opens `System` instead of qualifying `Int32`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Paged the test connection through Seq and boxed its expected edges …

4ff2fdd

…alike Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Navigation Menu

Repository navigation

Fixed Relay global ID and cursor helpers throwing on malformed client input - #638

Fixed Relay global ID and cursor helpers throwing on malformed client input#638
xperiandri wants to merge 4 commits into
devfrom
relay-safe-ids

xperiandri commented Oct 10, 2026

Uh oh!

xperiandri Oct 10, 2026

Uh oh!

xperiandri Oct 10, 2026

Uh oh!

xperiandri Oct 10, 2026

Uh oh!

xperiandri Oct 10, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

github-actions Bot commented Oct 10, 2026 •

edited

Loading

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-        letters
-        |> Array.skip start
-        |> Array.truncate first
-        |> Array.mapi (fun index letter -> { Cursor = Cursor.ofOffset (start + index); Node = letter })
+        letters
+        |> Seq.skip start
+        |> Seq.truncate first
+        |> Seq.mapi (fun index letter -> { Cursor = Cursor.ofOffset (start + index); Node = letter })
+        |> Seq.toArray

		box <\| NameValueLookup.ofList [ "node", upcast "c" ]
		upcast NameValueLookup.ofList [ "node", upcast "d" ]

Conversation

xperiandri commented Oct 10, 2026

Problem

Changes

Tests

Uh oh!

xperiandri Oct 10, 2026

Choose a reason for hiding this comment

Uh oh!

xperiandri Oct 10, 2026

Choose a reason for hiding this comment

Uh oh!

xperiandri Oct 10, 2026

Choose a reason for hiding this comment

Uh oh!

xperiandri Oct 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

github-actions Bot commented Oct 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Oct 10, 2026 •

edited

Loading