Skip to content

Commit e1fc978

Browse files
committed
feat(graphql): validate a caller-supplied select locally before invoking
graphql-js is already a runtime dependency, so parse-check the assembled operation when a `select` is provided and fail fast with a `graphql_invalid_selection` error instead of building a request. This rejects a malformed selection (and any attempt to break out of the field's selection set) with a precise local error, and never reaches the network for those. Field- and argument-level validity stays with the upstream server (which returns verbatim errors): a local GraphQLSchema would need a full introspection snapshot, but the stored snapshot is the plugin's reduced shape, which buildClientSchema cannot consume. The select-to-operation assembly is extracted to a shared `effectiveOperationString` so the validated string and the sent string are identical.
1 parent bd999ff commit e1fc978

4 files changed

Lines changed: 91 additions & 13 deletions

File tree

packages/plugins/graphql/src/sdk/invoke.ts

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,25 @@ export const endpointForTelemetry = (endpoint: string): string => {
2121
return url.toString();
2222
};
2323

24+
/** The operation string to send for a call. A caller-supplied `select` overrides
25+
* the default scalar-leaf selection: it is spliced into the field's selection
26+
* set (`field { <select> }`) so nested/list data can be requested per call. Falls
27+
* back to the stored default operation when `select` is absent or the binding
28+
* predates the prefix/suffix split. `select` is a control input, never a GraphQL
29+
* variable. Shared with plugin.invokeTool so the validated string matches the
30+
* sent string exactly. */
31+
export const effectiveOperationString = (
32+
operation: OperationBinding,
33+
args: Record<string, unknown>,
34+
): string => {
35+
const customSelect = typeof args.select === "string" ? args.select.trim() : "";
36+
return customSelect.length > 0 &&
37+
operation.operationPrefix != null &&
38+
operation.operationSuffix != null
39+
? `${operation.operationPrefix} { ${customSelect} }${operation.operationSuffix}`
40+
: operation.operationString;
41+
};
42+
2443
// ---------------------------------------------------------------------------
2544
// Response helpers
2645
// ---------------------------------------------------------------------------
@@ -71,18 +90,10 @@ export const invoke = Effect.fn("GraphQL.invoke")(function* (
7190
Object.assign(variables, args.variables);
7291
}
7392

74-
// A caller-supplied `select` overrides the default scalar-leaf selection: it is
75-
// spliced into the field's selection set so nested/list data can be requested
76-
// per call. `select` is a control input, not a GraphQL variable, so it never
77-
// enters `variables`. Falls back to the stored default operation when absent or
78-
// when the binding predates the prefix/suffix split.
79-
const customSelect = typeof args.select === "string" ? args.select.trim() : "";
80-
const operationString =
81-
customSelect.length > 0 &&
82-
operation.operationPrefix != null &&
83-
operation.operationSuffix != null
84-
? `${operation.operationPrefix} { ${customSelect} }${operation.operationSuffix}`
85-
: operation.operationString;
93+
// `select` (a control input, not a GraphQL variable) is applied here and never
94+
// enters `variables`. plugin.invokeTool validates this same string before we
95+
// reach the network.
96+
const operationString = effectiveOperationString(operation, args);
8697

8798
let request = HttpClientRequest.post(requestEndpoint).pipe(
8899
HttpClientRequest.setHeader("Content-Type", "application/json"),

packages/plugins/graphql/src/sdk/plugin.test.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -991,4 +991,27 @@ describe("graphqlPlugin generates valid operations against rich schemas (#1146)"
991991
});
992992
}),
993993
);
994+
995+
it.effect("rejects a malformed `select` locally, before any network call", () =>
996+
Effect.gen(function* () {
997+
const { server, executor } = yield* setup("gitlab_syntax");
998+
// Warm the binding cache (the first invoke introspects to materialize
999+
// bindings) so the malformed call below has no legitimate reason to hit the
1000+
// wire: if it does, validation failed to short-circuit.
1001+
yield* executor.execute(toolAddr("gitlab_syntax", "main", "query.currentUser"), {});
1002+
yield* server.clearRequests;
1003+
1004+
const result = yield* executor.execute(
1005+
toolAddr("gitlab_syntax", "main", "query.currentUser"),
1006+
{ select: "active mergeRequests { nodes {" },
1007+
);
1008+
1009+
expect(result).toMatchObject({
1010+
ok: false,
1011+
error: { code: "graphql_invalid_selection" },
1012+
});
1013+
// Parse-check happens before the request is built, so nothing reached the server.
1014+
expect((yield* server.requests).length).toBe(0);
1015+
}),
1016+
);
9941017
});

packages/plugins/graphql/src/sdk/plugin.ts

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,8 @@ import {
4747
GraphqlIntrospectionError,
4848
GraphqlInvocationError,
4949
} from "./errors";
50-
import { invokeWithLayer } from "./invoke";
50+
import { effectiveOperationString, invokeWithLayer } from "./invoke";
51+
import { validateOperationString } from "./validate-selection";
5152
import { graphqlPresets } from "./presets";
5253
import { makeDefaultGraphqlStore, type GraphqlStore, type StoredOperation } from "./store";
5354
import {
@@ -1046,6 +1047,27 @@ export const graphqlPlugin = definePlugin((options?: GraphqlPluginOptions) => {
10461047
});
10471048
}
10481049

1050+
// Parse-check a caller-supplied `select` locally, before any network
1051+
// call: reject a malformed selection (and any attempt to break out of the
1052+
// field's selection set) with a precise error instead of a confusing
1053+
// server response. Field- and argument-level validity is left to the
1054+
// server, which returns verbatim errors.
1055+
const selectArg = (args as Record<string, unknown> | undefined)?.select;
1056+
if (typeof selectArg === "string" && selectArg.trim().length > 0) {
1057+
const operationString = effectiveOperationString(
1058+
op.binding,
1059+
(args ?? {}) as Record<string, unknown>,
1060+
);
1061+
const selectionErrors = validateOperationString(operationString);
1062+
if (selectionErrors.length > 0) {
1063+
return ToolResult.fail({
1064+
code: "graphql_invalid_selection",
1065+
message: selectionErrors[0]!,
1066+
details: { errors: selectionErrors },
1067+
});
1068+
}
1069+
}
1070+
10491071
const headers: Record<string, string> = { ...(config.headers ?? {}) };
10501072
const queryParams: Record<string, string> = {
10511073
...(config.queryParams ?? {}),
Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
import { parse } from "graphql";
2+
3+
// Local validation for a caller-supplied `select` (see invoke / plugin.invokeTool).
4+
// graphql-js is already a runtime dependency of this plugin, so we reuse its
5+
// parser to reject a malformed selection before any network round trip, and to
6+
// catch any attempt to break out of the field's selection set (the spliced text
7+
// must parse as part of a single operation). Field- and argument-level validity
8+
// is left to the upstream server, which returns verbatim errors: building a local
9+
// GraphQLSchema would need a full introspection snapshot, but the stored snapshot
10+
// is a reduced shape (see introspect.ts) that buildClientSchema cannot consume.
11+
12+
/** Parse-check an assembled operation string. Returns a one-element error array
13+
* when the selection is not valid GraphQL, or an empty array when it parses. */
14+
export const validateOperationString = (operationString: string): readonly string[] => {
15+
// oxlint-disable-next-line executor/no-try-catch-or-throw -- boundary: graphql `parse` throws on a syntax error; the thrown value is reported generically (its message is not extracted, per the typed-error lint rules)
16+
try {
17+
parse(operationString);
18+
return [];
19+
} catch {
20+
return ["`select` is not a valid GraphQL selection set"];
21+
}
22+
};

0 commit comments

Comments
 (0)