migrate filtering expressions to safer CEL and add a query builder

This commit is contained in:
Athou
2026-02-15 10:20:50 +01:00
parent 08bfcded7f
commit d444a7080d
46 changed files with 862 additions and 590 deletions

View File

@@ -392,15 +392,9 @@
<version>3.6.1</version>
</dependency>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-jexl</artifactId>
<version>2.1.1</version>
<exclusions>
<exclusion>
<artifactId>commons-logging</artifactId>
<groupId>commons-logging</groupId>
</exclusion>
</exclusions>
<groupId>dev.cel</groupId>
<artifactId>cel</artifactId>
<version>0.11.1</version>
</dependency>
<dependency>
<groupId>org.passay</groupId>

View File

@@ -43,4 +43,7 @@ public class FeedSubscription extends AbstractModel {
@Column(name = "filtering_expression", length = 4096)
private String filter;
@Column(name = "filtering_expression_legacy", length = 4096)
private String filterLegacy;
}

View File

@@ -1,7 +1,7 @@
package com.commafeed.backend.service;
import java.time.Year;
import java.util.concurrent.Callable;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
@@ -11,92 +11,71 @@ import java.util.concurrent.TimeoutException;
import jakarta.inject.Singleton;
import org.apache.commons.jexl2.JexlContext;
import org.apache.commons.jexl2.JexlEngine;
import org.apache.commons.jexl2.JexlException;
import org.apache.commons.jexl2.JexlInfo;
import org.apache.commons.jexl2.MapContext;
import org.apache.commons.jexl2.Script;
import org.apache.commons.jexl2.introspection.JexlMethod;
import org.apache.commons.jexl2.introspection.JexlPropertyGet;
import org.apache.commons.jexl2.introspection.Uberspect;
import org.apache.commons.jexl2.introspection.UberspectImpl;
import org.apache.commons.lang3.StringUtils;
import org.apache.commons.logging.LogFactory;
import org.jsoup.Jsoup;
import com.commafeed.CommaFeedConfiguration;
import com.commafeed.backend.model.FeedEntry;
import dev.cel.common.CelAbstractSyntaxTree;
import dev.cel.common.CelValidationException;
import dev.cel.common.types.SimpleType;
import dev.cel.compiler.CelCompiler;
import dev.cel.compiler.CelCompilerFactory;
import dev.cel.runtime.CelEvaluationException;
import dev.cel.runtime.CelRuntime;
import dev.cel.runtime.CelRuntimeFactory;
import lombok.RequiredArgsConstructor;
@RequiredArgsConstructor
@Singleton
public class FeedEntryFilteringService {
private static final JexlEngine ENGINE = initEngine();
private static final CelCompiler CEL_COMPILER = CelCompilerFactory.standardCelCompilerBuilder()
.addVar("title", SimpleType.STRING)
.addVar("titleLower", SimpleType.STRING)
.addVar("author", SimpleType.STRING)
.addVar("authorLower", SimpleType.STRING)
.addVar("content", SimpleType.STRING)
.addVar("contentLower", SimpleType.STRING)
.addVar("url", SimpleType.STRING)
.addVar("urlLower", SimpleType.STRING)
.addVar("categories", SimpleType.STRING)
.addVar("categoriesLower", SimpleType.STRING)
.build();
private static final CelRuntime CEL_RUNTIME = CelRuntimeFactory.standardCelRuntimeBuilder().build();
private final ExecutorService executor = Executors.newCachedThreadPool();
private final CommaFeedConfiguration config;
private static JexlEngine initEngine() {
// classloader that prevents object creation
ClassLoader cl = new ClassLoader() {
@Override
protected Class<?> loadClass(String name, boolean resolve) {
return null;
}
};
// uberspect that prevents access to .class and .getClass()
Uberspect uberspect = new UberspectImpl(LogFactory.getLog(JexlEngine.class)) {
@Override
public JexlPropertyGet getPropertyGet(Object obj, Object identifier, JexlInfo info) {
if ("class".equals(identifier)) {
return null;
}
return super.getPropertyGet(obj, identifier, info);
}
@Override
public JexlMethod getMethod(Object obj, String method, Object[] args, JexlInfo info) {
if ("getClass".equals(method)) {
return null;
}
return super.getMethod(obj, method, args, info);
}
};
JexlEngine engine = new JexlEngine(uberspect, null, null, null);
engine.setStrict(true);
engine.setClassLoader(cl);
return engine;
}
public boolean filterMatchesEntry(String filter, FeedEntry entry) throws FeedEntryFilterException {
if (StringUtils.isBlank(filter)) {
return true;
}
Script script;
try {
script = ENGINE.createScript(filter);
} catch (JexlException e) {
throw new FeedEntryFilterException("Exception while parsing expression " + filter, e);
}
String title = entry.getContent().getTitle() == null ? "" : Jsoup.parse(entry.getContent().getTitle()).text();
String author = entry.getContent().getAuthor() == null ? "" : entry.getContent().getAuthor();
String content = entry.getContent().getContent() == null ? "" : Jsoup.parse(entry.getContent().getContent()).text();
String url = entry.getUrl() == null ? "" : entry.getUrl();
String categories = entry.getContent().getCategories() == null ? "" : entry.getContent().getCategories();
JexlContext context = new MapContext();
context.set("title", entry.getContent().getTitle() == null ? "" : Jsoup.parse(entry.getContent().getTitle()).text().toLowerCase());
context.set("author", entry.getContent().getAuthor() == null ? "" : entry.getContent().getAuthor().toLowerCase());
context.set("content",
entry.getContent().getContent() == null ? "" : Jsoup.parse(entry.getContent().getContent()).text().toLowerCase());
context.set("url", entry.getUrl() == null ? "" : entry.getUrl().toLowerCase());
context.set("categories", entry.getContent().getCategories() == null ? "" : entry.getContent().getCategories().toLowerCase());
Map<String, Object> data = new HashMap<>();
data.put("title", title);
data.put("titleLower", title.toLowerCase());
context.set("year", Year.now().getValue());
data.put("author", author);
data.put("authorLower", author.toLowerCase());
Callable<Object> callable = script.callable(context);
Future<Object> future = executor.submit(callable);
data.put("content", content);
data.put("contentLower", content.toLowerCase());
data.put("url", url);
data.put("urlLower", url.toLowerCase());
data.put("categories", categories);
data.put("categoriesLower", categories.toLowerCase());
Future<Object> future = executor.submit(() -> evaluateCelExpression(filter, data));
Object result;
try {
result = future.get(config.feedRefresh().filteringExpressionEvaluationTimeout().toMillis(), TimeUnit.MILLISECONDS);
@@ -108,11 +87,15 @@ public class FeedEntryFilteringService {
} catch (TimeoutException e) {
throw new FeedEntryFilterException("Took too long evaluating expression " + filter, e);
}
try {
return (boolean) result;
} catch (ClassCastException e) {
throw new FeedEntryFilterException(e.getMessage(), e);
}
return Boolean.TRUE.equals(result);
}
private Object evaluateCelExpression(String expression, Map<String, Object> data)
throws CelValidationException, CelEvaluationException {
CelAbstractSyntaxTree ast = CEL_COMPILER.compile(expression).getAst();
CelRuntime.Program program = CEL_RUNTIME.createProgram(ast);
return program.eval(data);
}
@SuppressWarnings("serial")

View File

@@ -59,9 +59,12 @@ public class Subscription implements Serializable {
@Schema(description = "date of the newest item", type = SchemaType.INTEGER)
private Instant newestItemTime;
@Schema(description = "JEXL string evaluated on new entries to mark them as read if they do not match")
@Schema(description = "CEL string evaluated on new entries to mark them as read if they do not match")
private String filter;
@Schema(description = "JEXL legacy filter")
private String filterLegacy;
public static Subscription build(FeedSubscription subscription, UnreadCount unreadCount) {
FeedCategory category = subscription.getCategory();
Feed feed = subscription.getFeed();
@@ -81,6 +84,7 @@ public class Subscription implements Serializable {
sub.setNewestItemTime(unreadCount.getNewestItemTime());
sub.setCategoryId(category == null ? null : String.valueOf(category.getId()));
sub.setFilter(subscription.getFilter());
sub.setFilterLegacy(subscription.getFilterLegacy());
return sub;
}

View File

@@ -27,7 +27,7 @@ public class FeedModificationRequest implements Serializable {
@Schema(description = "new display position, null if not changed")
private Integer position;
@Schema(description = "JEXL string evaluated on new entries to mark them as read if they do not match")
@Schema(description = "CEL string evaluated on new entries to mark them as read if they do not match")
@Size(max = 4096)
private String filter;

View File

@@ -431,7 +431,12 @@ public class FeedREST {
User user = authenticationContext.getCurrentUser();
FeedSubscription subscription = feedSubscriptionDAO.findById(user, req.getId());
subscription.setFilter(req.getFilter());
if (StringUtils.isNotBlank(subscription.getFilter())) {
// if the new filter is filled, remove the legacy filter
subscription.setFilterLegacy(null);
}
if (StringUtils.isNotBlank(req.getName())) {
subscription.setTitle(req.getName());

View File

@@ -0,0 +1,21 @@
<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<databaseChangeLog xmlns="http://www.liquibase.org/xml/ns/dbchangelog"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog https://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-latest.xsd">
<changeSet id="cel-filtering-expressions" author="athou">
<addColumn tableName="FEEDSUBSCRIPTIONS">
<column name="filtering_expression_legacy" type="VARCHAR(4096)">
<constraints nullable="true" />
</column>
</addColumn>
<update tableName="FEEDSUBSCRIPTIONS">
<column name="filtering_expression_legacy" valueComputed="filtering_expression" />
</update>
<update tableName="FEEDSUBSCRIPTIONS">
<column name="filtering_expression" valueComputed="NULL" />
</update>
</changeSet>
</databaseChangeLog>

View File

@@ -37,5 +37,6 @@
<include file="changelogs/db.changelog-5.8.xml" />
<include file="changelogs/db.changelog-5.11.xml" />
<include file="changelogs/db.changelog-5.12.xml" />
<include file="changelogs/db.changelog-7.0.xml" />
</databaseChangeLog>

View File

@@ -4,6 +4,7 @@ import java.time.Duration;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.mockito.Mockito;
@@ -48,49 +49,219 @@ class FeedEntryFilteringServiceTest {
}
@Test
void simpleExpression() throws FeedEntryFilterException {
Assertions.assertTrue(service.filterMatchesEntry("author.toString() eq 'athou'", entry));
void simpleEqualsExpression() throws FeedEntryFilterException {
Assertions.assertTrue(service.filterMatchesEntry("author == \"Athou\"", entry));
}
@Test
void newIsDisabled() {
Assertions.assertThrows(FeedEntryFilterException.class,
() -> service.filterMatchesEntry("null eq new ('java.lang.String', 'athou')", entry));
void simpleNotEqualsExpression() throws FeedEntryFilterException {
Assertions.assertTrue(service.filterMatchesEntry("author != \"other\"", entry));
}
@Test
void getClassMethodIsDisabled() {
Assertions.assertThrows(FeedEntryFilterException.class, () -> service.filterMatchesEntry("null eq ''.getClass()", entry));
void containsExpression() throws FeedEntryFilterException {
Assertions.assertTrue(service.filterMatchesEntry("author.contains(\"Athou\")", entry));
}
@Test
void dotClassIsDisabled() throws FeedEntryFilterException {
Assertions.assertTrue(service.filterMatchesEntry("null eq ''.class", entry));
void titleContainsExpression() throws FeedEntryFilterException {
Assertions.assertTrue(service.filterMatchesEntry("title.contains(\"Merge\")", entry));
}
@Test
void cannotLoopForever() {
Mockito.when(config.feedRefresh().filteringExpressionEvaluationTimeout()).thenReturn(Duration.ofMillis(200));
service = new FeedEntryFilteringService(config);
Assertions.assertThrows(FeedEntryFilterException.class, () -> service.filterMatchesEntry("while(true) {}", entry));
void urlContainsExpression() throws FeedEntryFilterException {
Assertions.assertTrue(service.filterMatchesEntry("url.contains(\"github\")", entry));
}
@Test
void handlesNullCorrectly() {
entry.setUrl(null);
entry.setContent(new FeedEntryContent());
Assertions.assertDoesNotThrow(() -> service.filterMatchesEntry("author eq 'athou'", entry));
void andExpression() throws FeedEntryFilterException {
Assertions.assertTrue(service.filterMatchesEntry("author == \"Athou\" && url.contains(\"github\")", entry));
}
@Test
void incorrectScriptThrowsException() {
Assertions.assertThrows(FeedEntryFilterException.class, () -> service.filterMatchesEntry("aa eqz bb", entry));
void orExpression() throws FeedEntryFilterException {
Assertions.assertTrue(service.filterMatchesEntry("author == \"other\" || url.contains(\"github\")", entry));
}
@Test
void incorrectReturnTypeThrowsException() {
Assertions.assertThrows(FeedEntryFilterException.class, () -> service.filterMatchesEntry("1", entry));
void notExpression() throws FeedEntryFilterException {
Assertions.assertTrue(service.filterMatchesEntry("!(author == \"other\")", entry));
}
@Test
void incorrectExpressionThrowsException() {
Assertions.assertThrows(FeedEntryFilterException.class, () -> service.filterMatchesEntry("not valid cel", entry));
}
@Test
void falseValueReturnsFalse() throws FeedEntryFilterException {
Assertions.assertFalse(service.filterMatchesEntry("false", entry));
}
@Test
void trueValueReturnsTrue() throws FeedEntryFilterException {
Assertions.assertTrue(service.filterMatchesEntry("true", entry));
}
@Test
void startsWithExpression() throws FeedEntryFilterException {
Assertions.assertTrue(service.filterMatchesEntry("title.startsWith(\"Merge\")", entry));
}
@Test
void endsWithExpression() throws FeedEntryFilterException {
Assertions.assertTrue(service.filterMatchesEntry("url.endsWith(\"commafeed\")", entry));
}
@Test
void categoriesContainsExpression() throws FeedEntryFilterException {
FeedEntryContent content = entry.getContent();
content.setCategories("tech, programming, java");
entry.setContent(content);
Assertions.assertTrue(service.filterMatchesEntry("categories.contains(\"programming\")", entry));
}
@Test
void caseInsensitiveAuthorMatchUsingLowerVariable() throws FeedEntryFilterException {
Assertions.assertTrue(service.filterMatchesEntry("authorLower == \"athou\"", entry));
}
@Test
void caseInsensitiveTitleMatchUsingLowerVariable() throws FeedEntryFilterException {
Assertions.assertTrue(service.filterMatchesEntry("titleLower.contains(\"merge\")", entry));
}
@Test
void caseInsensitiveUrlMatchUsingLowerVariable() throws FeedEntryFilterException {
Assertions.assertTrue(service.filterMatchesEntry("urlLower.contains(\"github\")", entry));
}
@Test
void caseInsensitiveContentMatchUsingLowerVariable() throws FeedEntryFilterException {
Assertions.assertTrue(service.filterMatchesEntry("contentLower.contains(\"merge\")", entry));
}
@Test
void caseInsensitiveCategoriesMatchUsingLowerVariable() throws FeedEntryFilterException {
FeedEntryContent content = entry.getContent();
content.setCategories("Tech, Programming, Java");
entry.setContent(content);
Assertions.assertTrue(service.filterMatchesEntry("categoriesLower.contains(\"tech\")", entry));
}
@Nested
class Sandbox {
@Test
void sandboxBlocksSystemPropertyAccess() {
Assertions.assertThrows(FeedEntryFilterException.class,
() -> service.filterMatchesEntry("java.lang.System.getProperty(\"user.home\")", entry));
}
@Test
void sandboxBlocksRuntimeExec() {
Assertions.assertThrows(FeedEntryFilterException.class,
() -> service.filterMatchesEntry("java.lang.Runtime.getRuntime().exec(\"calc\")", entry));
}
@Test
void sandboxBlocksProcessBuilder() {
Assertions.assertThrows(FeedEntryFilterException.class,
() -> service.filterMatchesEntry("new java.lang.ProcessBuilder(\"cmd\").start()", entry));
}
@Test
void sandboxBlocksClassLoading() {
Assertions.assertThrows(FeedEntryFilterException.class,
() -> service.filterMatchesEntry("java.lang.Class.forName(\"java.lang.Runtime\")", entry));
}
@Test
void sandboxBlocksReflection() {
Assertions.assertThrows(FeedEntryFilterException.class,
() -> service.filterMatchesEntry("title.getClass().getMethods()", entry));
}
@Test
void sandboxBlocksFileAccess() {
Assertions.assertThrows(FeedEntryFilterException.class,
() -> service.filterMatchesEntry("new java.io.File(\"/etc/passwd\").exists()", entry));
}
@Test
void sandboxBlocksFileRead() {
Assertions.assertThrows(FeedEntryFilterException.class,
() -> service.filterMatchesEntry("java.nio.file.Files.readString(java.nio.file.Paths.get(\"/etc/passwd\"))", entry));
}
@Test
void sandboxBlocksNetworkAccess() {
Assertions.assertThrows(FeedEntryFilterException.class,
() -> service.filterMatchesEntry("new java.net.URL(\"http://evil.com\").openConnection()", entry));
}
@Test
void sandboxBlocksScriptEngine() {
Assertions.assertThrows(FeedEntryFilterException.class, () -> service
.filterMatchesEntry("new javax.script.ScriptEngineManager().getEngineByName(\"js\").eval(\"1+1\")", entry));
}
@Test
void sandboxBlocksThreadCreation() {
Assertions.assertThrows(FeedEntryFilterException.class,
() -> service.filterMatchesEntry("new java.lang.Thread().start()", entry));
}
@Test
void sandboxBlocksEnvironmentVariableAccess() {
Assertions.assertThrows(FeedEntryFilterException.class,
() -> service.filterMatchesEntry("java.lang.System.getenv(\"PATH\")", entry));
}
@Test
void sandboxBlocksExitCall() {
Assertions.assertThrows(FeedEntryFilterException.class, () -> service.filterMatchesEntry("java.lang.System.exit(0)", entry));
}
@Test
void sandboxBlocksUndeclaredVariables() {
Assertions.assertThrows(FeedEntryFilterException.class, () -> service.filterMatchesEntry("unknownVariable == \"test\"", entry));
}
@Test
void sandboxBlocksMethodInvocationOnStrings() {
Assertions.assertThrows(FeedEntryFilterException.class, () -> service.filterMatchesEntry("title.toCharArray()", entry));
}
@Test
void sandboxBlocksArbitraryJavaMethodCalls() {
Assertions.assertThrows(FeedEntryFilterException.class, () -> service.filterMatchesEntry("title.getBytes()", entry));
}
@Test
void sandboxOnlyAllowsDeclaredVariables() {
Assertions.assertThrows(FeedEntryFilterException.class, () -> service.filterMatchesEntry("System", entry));
}
@Test
void sandboxBlocksConstructorCalls() {
Assertions.assertThrows(FeedEntryFilterException.class, () -> service.filterMatchesEntry("new String(\"test\")", entry));
}
@Test
void sandboxBlocksStaticMethodCalls() {
Assertions.assertThrows(FeedEntryFilterException.class, () -> service.filterMatchesEntry("String.valueOf(123)", entry));
}
@Test
void sandboxBlocksLambdaExpressions() {
Assertions.assertThrows(FeedEntryFilterException.class, () -> service.filterMatchesEntry("() -> true", entry));
}
@Test
void sandboxBlocksObjectInstantiation() {
Assertions.assertThrows(FeedEntryFilterException.class, () -> service.filterMatchesEntry("java.util.HashMap{}", entry));
}
}
}

View File

@@ -103,7 +103,7 @@ class WebSocketIT extends BaseIT {
FeedModificationRequest req = new FeedModificationRequest();
req.setId(subscriptionId);
req.setName("feed-name");
req.setFilter("!title.contains('item 4')");
req.setFilter("!titleLower.contains('item 4')");
RestAssured.given().body(req).contentType(ContentType.JSON).post("rest/feed/modify").then().statusCode(HttpStatus.SC_OK);
AtomicBoolean connected = new AtomicBoolean();

View File

@@ -23,6 +23,7 @@ import org.junit.jupiter.api.Test;
import org.xml.sax.InputSource;
import com.commafeed.TestConstants;
import com.commafeed.frontend.model.Entries;
import com.commafeed.frontend.model.Entry;
import com.commafeed.frontend.model.FeedInfo;
import com.commafeed.frontend.model.Subscription;
@@ -263,4 +264,47 @@ class FeedIT extends BaseIT {
}
}
@Nested
class Filter {
@Test
void filterEntriesOnNewFeedItems() throws IOException {
// subscribe and wait for initial 2 entries
Long subscriptionId = subscribeAndWaitForEntries(getFeedUrl());
Entries initialEntries = getFeedEntries(subscriptionId);
Assertions.assertEquals(2, initialEntries.getEntries().size());
// set up a filter that excludes entries with "item 4" in the title
Subscription subscription = getSubscription(subscriptionId);
FeedModificationRequest req = new FeedModificationRequest();
req.setId(subscriptionId);
req.setName(subscription.getName());
req.setCategoryId(subscription.getCategoryId());
req.setPosition(subscription.getPosition());
req.setFilter("!titleLower.contains('item 4')");
RestAssured.given().body(req).contentType(ContentType.JSON).post("rest/feed/modify").then().statusCode(HttpStatus.SC_OK);
// verify filter is set
subscription = getSubscription(subscriptionId);
Assertions.assertEquals("!titleLower.contains('item 4')", subscription.getFilter());
// feed now returns 2 more entries (Item 3 and Item 4)
feedNowReturnsMoreEntries();
forceRefreshAllFeeds();
// wait for new entries to be fetched
Awaitility.await().atMost(Duration.ofSeconds(15)).until(() -> getCategoryEntries("all"), e -> e.getEntries().size() == 4);
// verify that Item 4 was marked as read because it matches the filter
Entries unreadEntries = RestAssured.given()
.get("rest/feed/entries?id={id}&readType=unread", subscriptionId)
.then()
.statusCode(HttpStatus.SC_OK)
.extract()
.as(Entries.class);
Assertions.assertEquals(3, unreadEntries.getEntries().size());
Assertions.assertTrue(unreadEntries.getEntries().stream().noneMatch(e -> e.getTitle().toLowerCase().contains("item 4")),
"Item 4 should be filtered out (marked as read)");
}
}
}