This is an automated email from the ASF dual-hosted git repository. jsinovassin pushed a commit to branch backport/simplify-id-patterns-3.0.x in repository https://gitbox.apache.org/repos/asf/unomi.git
commit 2e448a52bd23da7d4cbc3d51d226cf31bbe8191a Author: jsinovassin <[email protected]> AuthorDate: Fri Sep 25 18:58:14 2026 +0200 Simplify identifier validation patterns to a single character class (unomi-3.0.x) Backport of #873 to unomi-3.0.x, re-cut from the current branch head. The identifier pattern `^(\w|[-_@\.]){0,60}$` has two alternatives that both accept `_`, so a run of underscores followed by one rejected character makes the matcher explore every division of the run. The equivalent `^[\[email protected]]{0,60}$` accepts the same identifiers and evaluates in linear time. The three shipped schemas that carry the pattern are updated: requestIds.json, event.json and item.json. Differences from master, because this line has no tenancy: - SchemaIdPatternSafetyIT sends no API key, since the context endpoint is public here. That is the surface the unbounded evaluation was reachable from on this line. - AllITs registers SchemaIdPatternSafetyIT alone; the other suites the master commit lists do not exist here. - RequestIdsSchemaPatternSafetyTest is added. The unit test that comes with #873 lives in the json-schema services module and covers event.json and item.json, while requestIds.json ships from the rest module and is the schema ContextRequestDeserializer applies to a public POST /cxs/context.json. It gets a guard of its own here rather than integration coverage only. Tests: neither module had unit tests, so junit is added at test scope to both. --- extensions/json-schema/services/pom.xml | 5 + .../META-INF/cxs/schemas/events/event.json | 10 +- .../resources/META-INF/cxs/schemas/items/item.json | 2 +- .../unomi/schema/impl/SchemaPatternSafetyTest.java | 116 ++++++++++++++++++++ .../test/java/org/apache/unomi/itests/AllITs.java | 1 + .../unomi/itests/SchemaIdPatternSafetyIT.java | 103 ++++++++++++++++++ rest/pom.xml | 5 + .../META-INF/cxs/schemas/rest/requestIds.json | 6 +- .../schema/RequestIdsSchemaPatternSafetyTest.java | 118 +++++++++++++++++++++ 9 files changed, 357 insertions(+), 9 deletions(-) diff --git a/extensions/json-schema/services/pom.xml b/extensions/json-schema/services/pom.xml index 7b41dcedf..e3fc11006 100644 --- a/extensions/json-schema/services/pom.xml +++ b/extensions/json-schema/services/pom.xml @@ -121,6 +121,11 @@ <groupId>org.yaml</groupId> <artifactId>snakeyaml</artifactId> </dependency> + <dependency> + <groupId>junit</groupId> + <artifactId>junit</artifactId> + <scope>test</scope> + </dependency> </dependencies> <build> diff --git a/extensions/json-schema/services/src/main/resources/META-INF/cxs/schemas/events/event.json b/extensions/json-schema/services/src/main/resources/META-INF/cxs/schemas/events/event.json index b4673a760..2e9f75c6c 100644 --- a/extensions/json-schema/services/src/main/resources/META-INF/cxs/schemas/events/event.json +++ b/extensions/json-schema/services/src/main/resources/META-INF/cxs/schemas/events/event.json @@ -13,24 +13,24 @@ "properties" : { "eventType" : { "type" : "string", - "pattern" : "^(\\w|[-_@\\.]){0,60}$" + "pattern" : "^[\\[email protected]]{0,60}$" }, "profileId" : { "type" : [ "string"], - "pattern" : "^(\\w|[-_@\\.]){0,60}$" + "pattern" : "^[\\[email protected]]{0,60}$" }, "sessionId" : { "type" : [ "string"], - "pattern" : "^(\\w|[-_@\\.]){0,60}$" + "pattern" : "^[\\[email protected]]{0,60}$" }, "scope" : { "type" : [ "string"], "validateScope": true, - "pattern" : "^(\\w|[-_@\\.]){0,60}$" + "pattern" : "^[\\[email protected]]{0,60}$" }, "sourceId" : { "type" : [ "string"], - "pattern" : "^(\\w|[-_@\\.]){0,60}$" + "pattern" : "^[\\[email protected]]{0,60}$" }, "persistent" : { "type" : "boolean" diff --git a/extensions/json-schema/services/src/main/resources/META-INF/cxs/schemas/items/item.json b/extensions/json-schema/services/src/main/resources/META-INF/cxs/schemas/items/item.json index b5035f831..239d87750 100644 --- a/extensions/json-schema/services/src/main/resources/META-INF/cxs/schemas/items/item.json +++ b/extensions/json-schema/services/src/main/resources/META-INF/cxs/schemas/items/item.json @@ -12,7 +12,7 @@ "properties" : { "itemId" : { "type" : ["null","string"], - "pattern" : "^(\\w|[-_@\\.]){0,60}$", + "pattern" : "^[\\[email protected]]{0,60}$", "description" : "The identifier for the item" }, "itemType" : { diff --git a/extensions/json-schema/services/src/test/java/org/apache/unomi/schema/impl/SchemaPatternSafetyTest.java b/extensions/json-schema/services/src/test/java/org/apache/unomi/schema/impl/SchemaPatternSafetyTest.java new file mode 100644 index 000000000..db9591779 --- /dev/null +++ b/extensions/json-schema/services/src/test/java/org/apache/unomi/schema/impl/SchemaPatternSafetyTest.java @@ -0,0 +1,116 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.unomi.schema.impl; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import org.junit.Test; + +import java.io.InputStream; +import java.util.ArrayList; +import java.util.Iterator; +import java.util.List; +import java.util.Map; +import java.util.regex.Pattern; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +/** + * Guards the identifier-validation patterns in the shipped schemas against a linear-time regression. + * + * These patterns run on identifiers taken from public tracking requests, so they must evaluate in + * linear time. A pattern with overlapping quantified alternatives (for example + * {@code ^(\w|[-_@\.]){0,60}$}, where {@code _} matches both branches) degrades to exponential + * backtracking on the java.util.regex engine, so a bounded id can pin a validation thread. This test + * fails if any shipped id pattern exhibits that behaviour, and also pins down the accepted/rejected + * identifier space so a "simplification" cannot quietly change validation semantics. + */ +public class SchemaPatternSafetyTest { + + private static final ObjectMapper MAPPER = new ObjectMapper(); + + private static final String[] SCHEMA_RESOURCES = { + "/META-INF/cxs/schemas/events/event.json", + "/META-INF/cxs/schemas/items/item.json" + }; + + private List<String> collectPatterns(String resource) throws Exception { + try (InputStream is = getClass().getResourceAsStream(resource)) { + assertTrue("Missing shipped schema resource " + resource, is != null); + List<String> patterns = new ArrayList<>(); + collectPatterns(MAPPER.readTree(is), patterns); + return patterns; + } + } + + private void collectPatterns(JsonNode node, List<String> patterns) { + if (node.isObject()) { + for (Iterator<Map.Entry<String, JsonNode>> it = node.fields(); it.hasNext(); ) { + Map.Entry<String, JsonNode> field = it.next(); + if ("pattern".equals(field.getKey()) && field.getValue().isTextual()) { + patterns.add(field.getValue().asText()); + } else { + collectPatterns(field.getValue(), patterns); + } + } + } else if (node.isArray()) { + for (JsonNode child : node) { + collectPatterns(child, patterns); + } + } + } + + @Test(timeout = 5000) + public void idPatternsAreLinearTimeAndKeepSemantics() throws Exception { + // The classic exponential-backtracking probe for the old id pattern: a run of characters that + // are valid under both alternatives, followed by one invalid character forcing full backtracking. + StringBuilder probe = new StringBuilder(); + for (int i = 0; i < 59; i++) { + probe.append('_'); + } + probe.append('!'); + + List<String> patterns = new ArrayList<>(); + for (String resource : SCHEMA_RESOURCES) { + patterns.addAll(collectPatterns(resource)); + } + assertFalse("Expected id patterns in the shipped schemas", patterns.isEmpty()); + + for (String patternString : patterns) { + Pattern pattern = Pattern.compile(patternString); + + // Must finish effectively instantly; the test-level timeout catches exponential backtracking. + assertFalse("Probe input must be rejected by " + patternString, + pattern.matcher(probe).matches()); + + // Previously accepted identifiers must still be accepted. + assertTrue(pattern.matcher("[email protected]").matches()); + assertTrue(pattern.matcher("").matches()); + assertTrue(pattern.matcher("a.b-c_d@e").matches()); + + // Previously rejected identifiers must still be rejected. + assertFalse(pattern.matcher("white space").matches()); + assertFalse(pattern.matcher("slash/id").matches()); + StringBuilder tooLong = new StringBuilder(); + for (int i = 0; i < 61; i++) { + tooLong.append('a'); + } + assertFalse(pattern.matcher(tooLong).matches()); + } + } +} diff --git a/itests/src/test/java/org/apache/unomi/itests/AllITs.java b/itests/src/test/java/org/apache/unomi/itests/AllITs.java index be8f596a8..4fad7c472 100644 --- a/itests/src/test/java/org/apache/unomi/itests/AllITs.java +++ b/itests/src/test/java/org/apache/unomi/itests/AllITs.java @@ -53,6 +53,7 @@ import org.junit.runners.Suite.SuiteClasses; ModifyConsentIT.class, PatchIT.class, ContextServletIT.class, + SchemaIdPatternSafetyIT.class, RuleServiceIT.class, PrivacyServiceIT.class, GroovyActionsServiceIT.class, diff --git a/itests/src/test/java/org/apache/unomi/itests/SchemaIdPatternSafetyIT.java b/itests/src/test/java/org/apache/unomi/itests/SchemaIdPatternSafetyIT.java new file mode 100644 index 000000000..1f5167e71 --- /dev/null +++ b/itests/src/test/java/org/apache/unomi/itests/SchemaIdPatternSafetyIT.java @@ -0,0 +1,103 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.unomi.itests; + +import org.apache.http.client.methods.CloseableHttpResponse; +import org.apache.http.client.methods.HttpPost; +import org.apache.http.entity.ContentType; +import org.apache.http.entity.StringEntity; +import org.junit.Assert; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.ops4j.pax.exam.junit.PaxExam; +import org.ops4j.pax.exam.spi.reactors.ExamReactorStrategy; +import org.ops4j.pax.exam.spi.reactors.PerSuite; + +/** + * Identifiers arriving on the public tracking endpoints are matched against a shipped validation + * pattern before anything else happens, without administrator credentials. If that match were + * unbounded, one small request would occupy a request thread indefinitely. + * <p> + * These tests assert the property that matters operationally — the request <em>returns</em> — rather + * than any particular status code. The inputs used are ones that took exponential time before the fix + * (seconds at 26 characters, and far beyond that at the lengths used here), so a regression does not + * produce a wrong answer, it produces a request that never comes back. Each is therefore bounded by a + * timeout: if the boundary regresses, the test fails by timing out rather than by assertion. + */ +@RunWith(PaxExam.class) +@ExamReactorStrategy(PerSuite.class) +public class SchemaIdPatternSafetyIT extends BaseIT { + + private final static String CONTEXT_URL = "/cxs/context.json"; + + /** Generous next to the sub-millisecond a bounded evaluation needs, decisive against an unbounded one. */ + private final static int RESPONSE_TIMEOUT_MS = 30_000; + + /** + * An identifier built to defeat the shipped id-validation pattern: a run of characters that the + * pattern's alternatives both accept, followed by one it rejects. Before the fix this forced the + * matcher to explore every division of the run. + */ + @Test(timeout = RESPONSE_TIMEOUT_MS) + public void testMaliciousProfileIdIsValidatedInBoundedTime() throws Exception { + final StringBuilder maliciousId = new StringBuilder(); + for (int i = 0; i < 55; i++) { + maliciousId.append('_'); + } + maliciousId.append('!'); + + final String payload = "{\"profileId\":\"" + maliciousId + "\",\"sessionId\":\"regex-safety-session\"," + + "\"requiredProfileProperties\":[\"*\"],\"events\":[]}"; + + assertReturnsPromptly(payload); + } + + /** The same shape supplied as a sessionId, which is validated by the same pattern. */ + @Test(timeout = RESPONSE_TIMEOUT_MS) + public void testMaliciousSessionIdIsValidatedInBoundedTime() throws Exception { + final StringBuilder maliciousId = new StringBuilder(); + for (int i = 0; i < 55; i++) { + maliciousId.append('_'); + } + maliciousId.append('!'); + + final String payload = "{\"sessionId\":\"" + maliciousId + "\",\"requiredProfileProperties\":[\"*\"]," + + "\"events\":[]}"; + + assertReturnsPromptly(payload); + } + + /** + * Sends the payload to the public context endpoint and requires an answer. Any HTTP status is + * acceptable — rejecting the input and processing it are both fine outcomes. What is not acceptable + * is the request not completing, which is exactly what an unbounded evaluation causes. + */ + private void assertReturnsPromptly(final String payload) throws Exception { + final HttpPost request = new HttpPost(getFullUrl(CONTEXT_URL)); + // No credential: on this line the context endpoint is public, which is the surface the + // unbounded evaluation was reachable from. + request.setEntity(new StringEntity(payload, ContentType.APPLICATION_JSON)); + + final long startedAt = System.currentTimeMillis(); + try (CloseableHttpResponse response = httpClient.execute(request)) { + final long elapsed = System.currentTimeMillis() - startedAt; + Assert.assertNotNull("The public endpoint must answer rather than hang", response); + Assert.assertTrue("The public endpoint answered, but took " + elapsed + + "ms, which suggests the evaluation is no longer bounded", elapsed < RESPONSE_TIMEOUT_MS); + } + } +} diff --git a/rest/pom.xml b/rest/pom.xml index 9c90d7f1b..4e6ea0cb3 100644 --- a/rest/pom.xml +++ b/rest/pom.xml @@ -168,6 +168,11 @@ <artifactId>slf4j-api</artifactId> <scope>provided</scope> </dependency> + <dependency> + <groupId>junit</groupId> + <artifactId>junit</artifactId> + <scope>test</scope> + </dependency> </dependencies> <build> diff --git a/rest/src/main/resources/META-INF/cxs/schemas/rest/requestIds.json b/rest/src/main/resources/META-INF/cxs/schemas/rest/requestIds.json index 366a79524..ec1cf3808 100644 --- a/rest/src/main/resources/META-INF/cxs/schemas/rest/requestIds.json +++ b/rest/src/main/resources/META-INF/cxs/schemas/rest/requestIds.json @@ -12,15 +12,15 @@ "properties": { "sessionId": { "type": ["null", "string"], - "pattern" : "^(\\w|[-_@\\.]){0,60}$" + "pattern" : "^[\\[email protected]]{0,60}$" }, "profileId": { "type": ["null", "string"], - "pattern" : "^(\\w|[-_@\\.]){0,60}$" + "pattern" : "^[\\[email protected]]{0,60}$" }, "personaId" : { "type" : ["null", "string"], - "pattern" : "^(\\w|[-_@\\.]){0,60}$" + "pattern" : "^[\\[email protected]]{0,60}$" } } } diff --git a/rest/src/test/java/org/apache/unomi/rest/schema/RequestIdsSchemaPatternSafetyTest.java b/rest/src/test/java/org/apache/unomi/rest/schema/RequestIdsSchemaPatternSafetyTest.java new file mode 100644 index 000000000..645a71de6 --- /dev/null +++ b/rest/src/test/java/org/apache/unomi/rest/schema/RequestIdsSchemaPatternSafetyTest.java @@ -0,0 +1,118 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.unomi.rest.schema; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import org.junit.Test; + +import java.io.InputStream; +import java.util.ArrayList; +import java.util.Iterator; +import java.util.List; +import java.util.Map; +import java.util.regex.Pattern; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +/** + * Guards the identifier-validation patterns of the requestIds schema against a linear-time + * regression. + * + * The companion test in the json-schema services module covers event.json and item.json, which ship + * from that module. requestIds.json ships from this one, and it is the schema a public + * {@code POST /cxs/context.json} applies to its sessionId, profileId and personaId, so it needs its + * own guard here. + * + * A pattern with overlapping quantified alternatives (for example {@code ^(\w|[-_@\.]){0,60}$}, + * where {@code _} matches both branches) degrades to exponential backtracking on the + * java.util.regex engine, so a bounded identifier can pin a validation thread. + */ +public class RequestIdsSchemaPatternSafetyTest { + + private static final ObjectMapper MAPPER = new ObjectMapper(); + + private static final String SCHEMA_RESOURCE = "/META-INF/cxs/schemas/rest/requestIds.json"; + + /** sessionId, profileId and personaId. A dropped guard on any of the three is a live defect. */ + private static final int EXPECTED_PATTERN_COUNT = 3; + + @Test(timeout = 5000) + public void idPatternsAreLinearTimeAndKeepSemantics() throws Exception { + // The classic exponential-backtracking probe: a run of characters that are valid under both + // alternatives of the old pattern, followed by one invalid character forcing full backtracking. + StringBuilder probe = new StringBuilder(); + for (int i = 0; i < 59; i++) { + probe.append('_'); + } + probe.append('!'); + + List<String> patterns = collectPatterns(); + assertEquals("every identifier of the requestIds schema must carry a pattern", + EXPECTED_PATTERN_COUNT, patterns.size()); + + for (String patternString : patterns) { + Pattern pattern = Pattern.compile(patternString); + + // Must finish effectively instantly; the test-level timeout catches exponential backtracking. + assertFalse("Probe input must be rejected by " + patternString, + pattern.matcher(probe).matches()); + + // Previously accepted identifiers must still be accepted. + assertTrue(pattern.matcher("[email protected]").matches()); + assertTrue(pattern.matcher("").matches()); + assertTrue(pattern.matcher("a.b-c_d@e").matches()); + + // Previously rejected identifiers must still be rejected. + assertFalse(pattern.matcher("white space").matches()); + assertFalse(pattern.matcher("slash/id").matches()); + StringBuilder tooLong = new StringBuilder(); + for (int i = 0; i < 61; i++) { + tooLong.append('a'); + } + assertFalse(pattern.matcher(tooLong).matches()); + } + } + + private List<String> collectPatterns() throws Exception { + try (InputStream is = getClass().getResourceAsStream(SCHEMA_RESOURCE)) { + assertTrue("Missing shipped schema resource " + SCHEMA_RESOURCE, is != null); + List<String> patterns = new ArrayList<>(); + collectPatterns(MAPPER.readTree(is), patterns); + return patterns; + } + } + + private void collectPatterns(JsonNode node, List<String> patterns) { + if (node.isObject()) { + for (Iterator<Map.Entry<String, JsonNode>> it = node.fields(); it.hasNext(); ) { + Map.Entry<String, JsonNode> field = it.next(); + if ("pattern".equals(field.getKey()) && field.getValue().isTextual()) { + patterns.add(field.getValue().asText()); + } else { + collectPatterns(field.getValue(), patterns); + } + } + } else if (node.isArray()) { + for (JsonNode child : node) { + collectPatterns(child, patterns); + } + } + } +}
