This is an automated email from the ASF dual-hosted git repository.
sergehuber pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/unomi.git
The following commit(s) were added to refs/heads/master by this push:
new 5edd1e9b7 Authorize the executed GraphQL operation, not the first
definition (#874)
5edd1e9b7 is described below
commit 5edd1e9b78b6e078a737cfc50f15642eac42906e
Author: Serge Huber <[email protected]>
AuthorDate: Thu Sep 24 11:08:40 2026 +0200
Authorize the executed GraphQL operation, not the first definition (#874)
Co-authored-by: Claude Opus 4.8 <[email protected]>
---
.../auth/GraphQLServletSecurityValidator.java | 165 +++++++++++++++++----
.../auth/GraphQLServletSecurityValidatorTest.java | 49 ++++++
.../itests/graphql/GraphQLServletSecurityIT.java | 64 ++++++++
.../security/bypass-ambiguous-multi-operation.json | 4 +
.../graphql/security/bypass-extra-root-field.json | 5 +
.../graphql/security/bypass-fragment-spread.json | 5 +
.../security/bypass-introspection-named.json | 5 +
.../graphql/security/bypass-leading-fragment.json | 5 +
.../security/bypass-operation-name-smuggle.json | 5 +
9 files changed, 277 insertions(+), 30 deletions(-)
diff --git
a/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/servlet/auth/GraphQLServletSecurityValidator.java
b/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/servlet/auth/GraphQLServletSecurityValidator.java
index f9b0ea844..d6ef91881 100644
---
a/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/servlet/auth/GraphQLServletSecurityValidator.java
+++
b/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/servlet/auth/GraphQLServletSecurityValidator.java
@@ -41,7 +41,12 @@ import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.Base64;
+import java.util.HashMap;
+import java.util.HashSet;
import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
import static graphql.language.OperationDefinition.Operation.*;
import static org.osgi.service.http.HttpContext.AUTHENTICATION_TYPE;
@@ -86,7 +91,7 @@ public class GraphQLServletSecurityValidator {
}
public boolean validate(String query, String operationName,
HttpServletRequest req, HttpServletResponse res) throws IOException {
- if (isPublicOperation(query)) {
+ if (isPublicOperation(query, operationName)) {
// For public operations, check API key
String apiKey = req.getHeader("X-Unomi-Api-Key");
if (apiKey != null) {
@@ -115,49 +120,149 @@ public class GraphQLServletSecurityValidator {
}
}
- private boolean isPublicOperation(String query) {
- if (query == null) {
+ /**
+ * Decides whether a GraphQL document may run against a public API key.
The check authorizes the
+ * exact operation graphql-java will execute for the request (selected by
{@code operationName}),
+ * evaluates every selected root field against the public allow-list,
detects introspection
+ * structurally, and fails closed on anything it cannot prove public.
+ */
+ private boolean isPublicOperation(String query, String operationName) {
+ if (query == null || query.trim().isEmpty()) {
return false;
}
- final Document queryDoc = parser.parseDocument(query);
- if (queryDoc.getDefinitions().isEmpty()) {
+ final Document queryDoc;
+ try {
+ queryDoc = parser.parseDocument(query);
+ } catch (RuntimeException e) {
+ // Unparseable input is never treated as public; execution will
surface the syntax error.
+ LOG.debug("Failed to parse GraphQL document; refusing public
classification", e);
return false;
}
- final Definition<?> def = queryDoc.getDefinitions().get(0);
- if (def instanceof OperationDefinition) {
- OperationDefinition opDef = (OperationDefinition) def;
- if (SUBSCRIPTION.equals(opDef.getOperation())) {
- // subscriptions are not public
- return false;
- } else if ("IntrospectionQuery".equals(opDef.getName())) {
- // allow introspection query
- return true;
- }
- List<Node> children = opDef.getSelectionSet().getChildren();
- final Field cdp = (Field) children.stream().filter((node) -> {
- return (node instanceof Field) && "cdp".equals(((Field)
node).getName());
- }).findFirst().orElse(null);
- if (cdp == null) {
- // allow not a cdp namespace
- return true;
+ // Separate the document into its operations and a fragment lookup
table.
+ final Map<String, FragmentDefinition> fragments = new HashMap<>();
+ final List<OperationDefinition> operations = new ArrayList<>();
+ for (Definition<?> def : queryDoc.getDefinitions()) {
+ if (def instanceof OperationDefinition) {
+ operations.add((OperationDefinition) def);
+ } else if (def instanceof FragmentDefinition) {
+ final FragmentDefinition fragment = (FragmentDefinition) def;
+ fragments.put(fragment.getName(), fragment);
}
+ }
+ if (operations.isEmpty()) {
+ // No executable operation (e.g. a fragment-only document): fail
closed.
+ return false;
+ }
- final List<String> allowedNodeNames = new ArrayList<>();
- if (QUERY.equals(opDef.getOperation())) {
- allowedNodeNames.add("getProfile");
- } else if (MUTATION.equals(opDef.getOperation())) {
- allowedNodeNames.add("processEvents");
+ // Authorize exactly the operation graphql-java will execute, not
simply the first definition.
+ final OperationDefinition operation =
resolveExecutedOperation(operations, operationName);
+ if (operation == null) {
+ // Ambiguous, duplicated or unknown operation name: fail closed.
+ return false;
+ }
+ return isOperationPublic(operation, fragments);
+ }
+
+ /**
+ * Resolves the operation graphql-java will execute: with an explicit
{@code operationName} exactly
+ * one operation must match; without a name the document must contain
exactly one operation (per the
+ * GraphQL spec). Anything else is invalid input and is refused.
+ */
+ private OperationDefinition
resolveExecutedOperation(List<OperationDefinition> operations, String
operationName) {
+ if (operationName != null && !operationName.trim().isEmpty()) {
+ OperationDefinition match = null;
+ for (OperationDefinition operation : operations) {
+ if (operationName.equals(operation.getName())) {
+ if (match != null) {
+ return null; // duplicate operation name: invalid
document
+ }
+ match = operation;
+ }
}
+ return match;
+ }
+ return operations.size() == 1 ? operations.get(0) : null;
+ }
- return
cdp.getSelectionSet().getChildren().stream().allMatch((node) -> {
- return (node instanceof Field) &&
allowedNodeNames.contains(((Field) node).getName());
- });
+ private boolean isOperationPublic(OperationDefinition operation,
Map<String, FragmentDefinition> fragments) {
+ // Subscriptions are never public.
+ if (SUBSCRIPTION.equals(operation.getOperation())) {
+ return false;
+ }
+
+ final List<Field> topLevelFields =
collectFields(operation.getSelectionSet(), fragments, new HashSet<>());
+ if (topLevelFields.isEmpty()) {
+ return false;
+ }
+
+ // Introspection is detected structurally, never by the operation's
name: a query whose top-level
+ // selections are all meta fields (__schema / __type / __typename) is
public.
+ if (QUERY.equals(operation.getOperation())
+ && topLevelFields.stream().allMatch(field -> field.getName()
!= null && field.getName().startsWith("__"))) {
+ return true;
+ }
+
+ // Every public operation lives entirely under a single "cdp" root;
touching anything else is refused.
+ final List<Field> cdpFields = topLevelFields.stream()
+ .filter(field -> "cdp".equals(field.getName()))
+ .collect(Collectors.toList());
+ if (cdpFields.isEmpty() || cdpFields.size() != topLevelFields.size()) {
+ return false;
+ }
+
+ final List<String> allowedNodeNames = new ArrayList<>();
+ if (QUERY.equals(operation.getOperation())) {
+ allowedNodeNames.add("getProfile");
+ } else if (MUTATION.equals(operation.getOperation())) {
+ allowedNodeNames.add("processEvents");
+ } else {
+ return false;
+ }
+
+ for (Field cdp : cdpFields) {
+ final List<Field> cdpChildren =
collectFields(cdp.getSelectionSet(), fragments, new HashSet<>());
+ if (cdpChildren.isEmpty()) {
+ // An empty cdp selection is not an affirmatively public
request.
+ return false;
+ }
+ for (Field child : cdpChildren) {
+ if (!allowedNodeNames.contains(child.getName())) {
+ return false;
+ }
+ }
}
return true;
}
+ /**
+ * Flattens a selection set into concrete fields, resolving inline
fragments and fragment spreads
+ * (guarding against fragment cycles) so nothing can be hidden from the
allow-list inside a fragment.
+ */
+ private List<Field> collectFields(SelectionSet selectionSet, Map<String,
FragmentDefinition> fragments, Set<String> visitedFragments) {
+ final List<Field> fields = new ArrayList<>();
+ if (selectionSet == null) {
+ return fields;
+ }
+ for (Selection selection : selectionSet.getSelections()) {
+ if (selection instanceof Field) {
+ fields.add((Field) selection);
+ } else if (selection instanceof InlineFragment) {
+ fields.addAll(collectFields(((InlineFragment)
selection).getSelectionSet(), fragments, visitedFragments));
+ } else if (selection instanceof FragmentSpread) {
+ final String name = ((FragmentSpread) selection).getName();
+ if (visitedFragments.add(name)) {
+ final FragmentDefinition fragment = fragments.get(name);
+ if (fragment != null) {
+
fields.addAll(collectFields(fragment.getSelectionSet(), fragments,
visitedFragments));
+ }
+ }
+ }
+ }
+ return fields;
+ }
+
/**
* Authenticates a Basic credential that did not arrive as a request
header — used by the WebSocket
* {@code connection_init} handshake, which is the only way a browser
client can present credentials
diff --git
a/graphql/cxs-impl/src/test/java/org/apache/unomi/graphql/servlet/auth/GraphQLServletSecurityValidatorTest.java
b/graphql/cxs-impl/src/test/java/org/apache/unomi/graphql/servlet/auth/GraphQLServletSecurityValidatorTest.java
index c8a150e02..0f842f220 100644
---
a/graphql/cxs-impl/src/test/java/org/apache/unomi/graphql/servlet/auth/GraphQLServletSecurityValidatorTest.java
+++
b/graphql/cxs-impl/src/test/java/org/apache/unomi/graphql/servlet/auth/GraphQLServletSecurityValidatorTest.java
@@ -223,6 +223,55 @@ class GraphQLServletSecurityValidatorTest {
verify(securityService, never()).setCurrentSubject(any());
}
+ @Test
+ void validate_operationAuthorizationBypasses_areRejectedForPublicApiKey()
throws IOException {
+ // Even holding a valid public API key, none of these documents may
run: each is a way the
+ // public-operation gate could be tricked into classifying a
privileged or ambiguous document as
+ // public. They must all fail closed - no public branch, and with no
Authorization header, 401.
+
lenient().when(request.getHeader("X-Unomi-Api-Key")).thenReturn("public-api-key");
+
+ // A privileged mutation smuggled as a second operation and selected
by operationName.
+ assertNotPublic("query GetProfile { cdp {
getProfile(profileID:{id:\"x\"}) { id } } } "
+ + "mutation Pwn { cdp {
deleteAllPersonalData(profileID:{id:\"v\"}) } }", "Pwn");
+ // A privileged query whose operation is merely NAMED
IntrospectionQuery.
+ assertNotPublic("query IntrospectionQuery { cdp {
findProfiles(first:1000) { edges { node { properties } } } } }", null);
+ // A leading fragment definition in front of a privileged operation.
+ assertNotPublic("fragment f on Query { __typename } query Q { cdp {
deleteProfile(profileID:{id:\"v\"}) } }", "Q");
+ // A privileged field hidden inside a fragment spread.
+ assertNotPublic("query Q { cdp { ...priv } } fragment priv on
CDP_Query { findProfiles(first:10) { edges { node { id } } } }", "Q");
+ // An extra non-cdp root field alongside an allowed one.
+ assertNotPublic("query Q { cdp { getProfile(profileID:{id:\"x\"}) { id
} } segments { edges { node { id } } } }", "Q");
+ // Multiple operations with no operationName: the executed operation
is ambiguous.
+ assertNotPublic("query A { cdp { getProfile(profileID:{id:\"x\"}) { id
} } } "
+ + "query B { cdp { findProfiles(first:1) { edges { node { id }
} } } }", null);
+ // A syntactically invalid document.
+ assertNotPublic("query { cdp { getProfile ", null);
+
+ verify(securityService, never()).setCurrentSubject(any());
+ }
+
+ @Test
+ void validate_legitimatePublicOperation_isAuthenticatedWithPublicApiKey()
throws IOException {
+ Tenant tenant = new Tenant();
+ tenant.setItemId("pub-tenant");
+
when(request.getHeader("X-Unomi-Api-Key")).thenReturn("public-api-key");
+ when(tenantService.getTenantByApiKey("public-api-key",
ApiKey.ApiKeyType.PUBLIC)).thenReturn(tenant);
+ when(securityService.createSubject("pub-tenant",
false)).thenReturn(new Subject());
+ ExecutionContext context = new ExecutionContext("pub-tenant", null,
null);
+
when(executionContextManager.createContext("pub-tenant")).thenReturn(context);
+
+ boolean authenticated = validator.validate(
+ "query { cdp { getProfile(profileID:{id:\"x\"}) { id } } }",
null, request, response);
+
+ assertTrue(authenticated);
+ verify(executionContextManager).setCurrentContext(context);
+ }
+
+ private void assertNotPublic(String query, String operationName) throws
IOException {
+ assertFalse(validator.validate(query, operationName, request,
response),
+ "Expected document to be denied for a public API key: " +
query);
+ }
+
/**
* An unset {@code org.apache.unomi.security.root.password} resolves to
the empty string, which
* {@code PropertiesLoginModule} accepts as the shipped administrator's
password (UNOMI-974).
diff --git
a/itests/src/test/java/org/apache/unomi/itests/graphql/GraphQLServletSecurityIT.java
b/itests/src/test/java/org/apache/unomi/itests/graphql/GraphQLServletSecurityIT.java
index 4288ceaaf..6cb2e44aa 100644
---
a/itests/src/test/java/org/apache/unomi/itests/graphql/GraphQLServletSecurityIT.java
+++
b/itests/src/test/java/org/apache/unomi/itests/graphql/GraphQLServletSecurityIT.java
@@ -84,4 +84,68 @@ public class GraphQLServletSecurityIT extends BaseGraphQLIT {
Assert.assertEquals(401, response.getStatusLine().getStatusCode());
}
}
+
+ //
---------------------------------------------------------------------------------------------
+ // Operation-authorization bypasses.
+ //
+ // Each payload below is a way a caller holding only the page-embedded
public API key could try to
+ // get a privileged operation executed. The public allow-list is
getProfile and processEvents only,
+ // so every one of these must be refused. They are written as end-to-end
requests deliberately: the
+ // decision has to hold against the real parser and the real servlet, not
just in a unit test.
+ //
---------------------------------------------------------------------------------------------
+
+ /** An operation merely NAMED IntrospectionQuery must not be treated as
introspection. */
+ @Test
+ public void
testPublicKeyCannotRunPrivilegedOperationNamedIntrospectionQuery() throws
Exception {
+
assertRefusedForPublicKey("graphql/security/bypass-introspection-named.json");
+ }
+
+ /** A privileged operation selected by operationName, hidden behind a
benign first operation. */
+ @Test
+ public void
testPublicKeyCannotSmugglePrivilegedOperationViaOperationName() throws
Exception {
+
assertRefusedForPublicKey("graphql/security/bypass-operation-name-smuggle.json");
+ }
+
+ /** A leading fragment definition must not make the document look
non-executable. */
+ @Test
+ public void testPublicKeyCannotBypassWithLeadingFragment() throws
Exception {
+
assertRefusedForPublicKey("graphql/security/bypass-leading-fragment.json");
+ }
+
+ /** A privileged field hidden inside a fragment spread must still be seen.
*/
+ @Test
+ public void testPublicKeyCannotHidePrivilegedFieldInFragmentSpread()
throws Exception {
+
assertRefusedForPublicKey("graphql/security/bypass-fragment-spread.json");
+ }
+
+ /** Several operations and no operationName: which one executes is
ambiguous, so it must be refused. */
+ @Test
+ public void testPublicKeyCannotUseAmbiguousMultiOperationDocument() throws
Exception {
+
assertRefusedForPublicKey("graphql/security/bypass-ambiguous-multi-operation.json");
+ }
+
+ /** An allowed field does not license a second root field alongside it. */
+ @Test
+ public void testPublicKeyCannotAddExtraRootFieldBesideAllowedOne() throws
Exception {
+
assertRefusedForPublicKey("graphql/security/bypass-extra-root-field.json");
+ }
+
+ /**
+ * A public-key caller must not get privileged data out of this document.
Refusal is either a 401 or
+ * a 200 carrying no data — both are acceptable outcomes, what matters is
that nothing privileged is
+ * returned. Asserting only on the status code would let a 200-with-data
regression pass unnoticed.
+ */
+ private void assertRefusedForPublicKey(final String resource) throws
Exception {
+ try (CloseableHttpResponse response = postWithAuthType(resource,
AuthType.PUBLIC_KEY)) {
+ final int status = response.getStatusLine().getStatusCode();
+ if (status == 401) {
+ return;
+ }
+ Assert.assertEquals("Expected the request to be refused (401) or
to return no privileged data",
+ 200, status);
+ final ResponseContext context =
ResponseContext.parse(response.getEntity());
+ Assert.assertNull("A public API key must not be able to read
profiles through " + resource,
+ context.getValue("data.cdp.findProfiles"));
+ }
+ }
}
diff --git
a/itests/src/test/resources/graphql/security/bypass-ambiguous-multi-operation.json
b/itests/src/test/resources/graphql/security/bypass-ambiguous-multi-operation.json
new file mode 100644
index 000000000..6270eaece
--- /dev/null
+++
b/itests/src/test/resources/graphql/security/bypass-ambiguous-multi-operation.json
@@ -0,0 +1,4 @@
+{
+ "variables": {},
+ "query": "query Public {\n cdp {\n getProfile(profileID: {id:
\"nobody\"}) {\n cdp_profileIDs {\n id\n }\n }\n
}\n}\n\nquery Privileged {\n cdp {\n findProfiles(first: 100) {\n
edges {\n node {\n cdp_profileIDs {\n id\n
}\n }\n }\n }\n }\n}\n"
+}
diff --git
a/itests/src/test/resources/graphql/security/bypass-extra-root-field.json
b/itests/src/test/resources/graphql/security/bypass-extra-root-field.json
new file mode 100644
index 000000000..b35f870db
--- /dev/null
+++ b/itests/src/test/resources/graphql/security/bypass-extra-root-field.json
@@ -0,0 +1,5 @@
+{
+ "operationName": "Mixed",
+ "variables": {},
+ "query": "query Mixed {\n cdp {\n getProfile(profileID: {id:
\"nobody\"}) {\n cdp_profileIDs {\n id\n }\n }\n }\n
__schema {\n types {\n name\n }\n }\n}\n"
+}
diff --git
a/itests/src/test/resources/graphql/security/bypass-fragment-spread.json
b/itests/src/test/resources/graphql/security/bypass-fragment-spread.json
new file mode 100644
index 000000000..65297540b
--- /dev/null
+++ b/itests/src/test/resources/graphql/security/bypass-fragment-spread.json
@@ -0,0 +1,5 @@
+{
+ "operationName": "Privileged",
+ "variables": {},
+ "query": "query Privileged {\n cdp {\n ...privileged\n }\n}\n\nfragment
privileged on CDP_Query {\n findProfiles(first: 100) {\n edges {\n
node {\n cdp_profileIDs {\n id\n }\n }\n }\n
}\n}\n"
+}
diff --git
a/itests/src/test/resources/graphql/security/bypass-introspection-named.json
b/itests/src/test/resources/graphql/security/bypass-introspection-named.json
new file mode 100644
index 000000000..09a8b50a6
--- /dev/null
+++ b/itests/src/test/resources/graphql/security/bypass-introspection-named.json
@@ -0,0 +1,5 @@
+{
+ "operationName": "IntrospectionQuery",
+ "variables": {},
+ "query": "query IntrospectionQuery {\n cdp {\n findProfiles(first: 100)
{\n edges {\n node {\n cdp_profileIDs {\n id\n
}\n }\n }\n }\n }\n}\n"
+}
diff --git
a/itests/src/test/resources/graphql/security/bypass-leading-fragment.json
b/itests/src/test/resources/graphql/security/bypass-leading-fragment.json
new file mode 100644
index 000000000..91518038a
--- /dev/null
+++ b/itests/src/test/resources/graphql/security/bypass-leading-fragment.json
@@ -0,0 +1,5 @@
+{
+ "operationName": "Privileged",
+ "variables": {},
+ "query": "fragment ignored on CDP_Query {\n __typename\n}\n\nquery
Privileged {\n cdp {\n findProfiles(first: 100) {\n edges {\n
node {\n cdp_profileIDs {\n id\n }\n }\n
}\n }\n }\n}\n"
+}
diff --git
a/itests/src/test/resources/graphql/security/bypass-operation-name-smuggle.json
b/itests/src/test/resources/graphql/security/bypass-operation-name-smuggle.json
new file mode 100644
index 000000000..a3824c8ba
--- /dev/null
+++
b/itests/src/test/resources/graphql/security/bypass-operation-name-smuggle.json
@@ -0,0 +1,5 @@
+{
+ "operationName": "Privileged",
+ "variables": {},
+ "query": "query Public {\n cdp {\n getProfile(profileID: {id:
\"nobody\"}) {\n cdp_profileIDs {\n id\n }\n }\n
}\n}\n\nquery Privileged {\n cdp {\n findProfiles(first: 100) {\n
edges {\n node {\n cdp_profileIDs {\n id\n
}\n }\n }\n }\n }\n}\n"
+}
