From 951fdc1a1b8504df28c0a9cf0aa82b4b62631e44 Mon Sep 17 00:00:00 2001
From: Michael Hamann <michael.hamann@xwiki.com>
Date: Mon, 20 Jan 2025 16:42:00 +0100
Subject: [PATCH] XWIKI-22799: Make the required rights analyzers of the Cache,
HTML and RAW macro case-insensitive
* Ignore the case of parameter names.
* Add tests.
(cherry picked from commit 3d451e957fe2b14459e9ac64172b4a0e4c46971c)
---
.../CacheMacroRequiredRightsAnalyzer.java | 7 +-
.../CacheMacroRequiredRightsAnalyzerTest.java | 39 +++++++++--
.../HTMLMacroRequiredRightsAnalyzer.java | 15 +++--
.../macro/RawMacroRequiredRightsAnalyzer.java | 22 +++++--
.../HTMLMacroRequiredRightsAnalyzerTest.java | 65 +++++++++++++++++++
.../RawMacroRequiredRightsAnalyzerTest.java | 36 +++++++---
6 files changed, 158 insertions(+), 26 deletions(-)
diff --git a/xwiki-platform-core/xwiki-platform-rendering/xwiki-platform-rendering-macros/xwiki-platform-rendering-macro-cache/src/main/java/org/xwiki/rendering/internal/macro/cache/CacheMacroRequiredRightsAnalyzer.java b/xwiki-platform-core/xwiki-platform-rendering/xwiki-platform-rendering-macros/xwiki-platform-rendering-macro-cache/src/main/java/org/xwiki/rendering/internal/macro/cache/CacheMacroRequiredRightsAnalyzer.java
index 725f03eeb2b..c2117b91643 100644
--- a/xwiki-platform-core/xwiki-platform-rendering/xwiki-platform-rendering-macros/xwiki-platform-rendering-macro-cache/src/main/java/org/xwiki/rendering/internal/macro/cache/CacheMacroRequiredRightsAnalyzer.java
+++ b/xwiki-platform-core/xwiki-platform-rendering/xwiki-platform-rendering-macros/xwiki-platform-rendering-macro-cache/src/main/java/org/xwiki/rendering/internal/macro/cache/CacheMacroRequiredRightsAnalyzer.java
@@ -23,8 +23,8 @@
import javax.inject.Singleton;
import org.xwiki.component.annotation.Component;
-import org.xwiki.platform.security.requiredrights.MacroRequiredRightsAnalyzer;
import org.xwiki.platform.security.requiredrights.MacroRequiredRightReporter;
+import org.xwiki.platform.security.requiredrights.MacroRequiredRightsAnalyzer;
import org.xwiki.rendering.block.MacroBlock;
/**
@@ -43,7 +43,10 @@ public class CacheMacroRequiredRightsAnalyzer implements MacroRequiredRightsAnal
@Override
public void analyze(MacroBlock macroBlock, MacroRequiredRightReporter reporter)
{
- reporter.analyzeContent(macroBlock, macroBlock.getParameter("id"));
+ macroBlock.getParameters().entrySet().stream()
+ .filter(entry -> "id".equalsIgnoreCase(entry.getKey()))
+ .forEach(entry -> reporter.analyzeContent(macroBlock, entry.getValue()));
+
reporter.analyzeContent(macroBlock, macroBlock.getContent());
}
}
diff --git a/xwiki-platform-core/xwiki-platform-rendering/xwiki-platform-rendering-macros/xwiki-platform-rendering-macro-cache/src/test/java/org/xwiki/rendering/internal/macro/cache/CacheMacroRequiredRightsAnalyzerTest.java b/xwiki-platform-core/xwiki-platform-rendering/xwiki-platform-rendering-macros/xwiki-platform-rendering-macro-cache/src/test/java/org/xwiki/rendering/internal/macro/cache/CacheMacroRequiredRightsAnalyzerTest.java
index 3a635a391ce..9060d7e29bd 100644
--- a/xwiki-platform-core/xwiki-platform-rendering/xwiki-platform-rendering-macros/xwiki-platform-rendering-macro-cache/src/test/java/org/xwiki/rendering/internal/macro/cache/CacheMacroRequiredRightsAnalyzerTest.java
+++ b/xwiki-platform-core/xwiki-platform-rendering/xwiki-platform-rendering-macros/xwiki-platform-rendering-macro-cache/src/test/java/org/xwiki/rendering/internal/macro/cache/CacheMacroRequiredRightsAnalyzerTest.java
@@ -19,7 +19,14 @@
*/
package org.xwiki.rendering.internal.macro.cache;
+import java.util.Map;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
import org.xwiki.platform.security.requiredrights.MacroRequiredRightReporter;
import org.xwiki.rendering.block.MacroBlock;
import org.xwiki.test.junit5.mockito.ComponentTest;
@@ -27,7 +34,6 @@
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.when;
/**
* Unit tests for {@link CacheMacroRequiredRightsAnalyzer}.
@@ -40,14 +46,13 @@ class CacheMacroRequiredRightsAnalyzerTest
@InjectMockComponents
private CacheMacroRequiredRightsAnalyzer analyzer;
- @Test
- void analyze()
+ @ParameterizedTest
+ @MethodSource("idValuesProvider")
+ void analyze(String parameterName)
{
- MacroBlock macroBlock = mock();
String idValue = "idValue";
String contentValue = "contentValue";
- when(macroBlock.getParameter("id")).thenReturn(idValue);
- when(macroBlock.getContent()).thenReturn(contentValue);
+ MacroBlock macroBlock = new MacroBlock("cache", Map.of(parameterName, idValue), contentValue, false);
MacroRequiredRightReporter reporter = mock();
this.analyzer.analyze(macroBlock, reporter);
@@ -55,4 +60,26 @@ void analyze()
verify(reporter).analyzeContent(macroBlock, idValue);
verify(reporter).analyzeContent(macroBlock, contentValue);
}
+
+ @Test
+ void analyzeAllInOne()
+ {
+ Map<String, String> parameters = idValuesProvider().collect(Collectors.toMap(Function.identity(),
+ Function.identity()));
+ String content = "content";
+ MacroBlock macroBlock = new MacroBlock("cache", parameters, content, false);
+ MacroRequiredRightReporter reporter = mock();
+
+ this.analyzer.analyze(macroBlock, reporter);
+
+ verify(reporter).analyzeContent(macroBlock, content);
+ for (String value : parameters.values()) {
+ verify(reporter).analyzeContent(macroBlock, value);
+ }
+ }
+
+ static Stream<String> idValuesProvider()
+ {
+ return Stream.of("id", "ID", "Id", "iD");
+ }
}
diff --git a/xwiki-platform-core/xwiki-platform-rendering/xwiki-platform-rendering-xwiki/src/main/java/org/xwiki/rendering/internal/macro/HTMLMacroRequiredRightsAnalyzer.java b/xwiki-platform-core/xwiki-platform-rendering/xwiki-platform-rendering-xwiki/src/main/java/org/xwiki/rendering/internal/macro/HTMLMacroRequiredRightsAnalyzer.java
index 79737a8209e..165c20225c4 100644
--- a/xwiki-platform-core/xwiki-platform-rendering/xwiki-platform-rendering-xwiki/src/main/java/org/xwiki/rendering/internal/macro/HTMLMacroRequiredRightsAnalyzer.java
+++ b/xwiki-platform-core/xwiki-platform-rendering/xwiki-platform-rendering-xwiki/src/main/java/org/xwiki/rendering/internal/macro/HTMLMacroRequiredRightsAnalyzer.java
@@ -24,6 +24,7 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.stream.Stream;
import javax.inject.Inject;
import javax.inject.Named;
@@ -67,11 +68,10 @@ public class HTMLMacroRequiredRightsAnalyzer implements MacroRequiredRightsAnaly
@Override
public void analyze(MacroBlock macroBlock, MacroRequiredRightReporter reporter)
{
- boolean wiki = Boolean.TRUE.equals(this.converter.convert(Boolean.class, macroBlock.getParameter("wiki")));
- String cleanParameter = macroBlock.getParameter("clean");
+ boolean wiki = getBooleanParameterValues(macroBlock, "wiki").anyMatch(Boolean.TRUE::equals);
+
// Cleaning is enabled by default.
- boolean clean =
- cleanParameter == null || Boolean.TRUE.equals(this.converter.convert(Boolean.class, cleanParameter));
+ boolean clean = getBooleanParameterValues(macroBlock, "clean").noneMatch(Boolean.FALSE::equals);
if (wiki) {
reporter.analyzeContent(macroBlock, macroBlock.getContent());
@@ -105,4 +105,11 @@ public void analyze(MacroBlock macroBlock, MacroRequiredRightReporter reporter)
}
}
}
+
+ private Stream<Boolean> getBooleanParameterValues(MacroBlock macroBlock, String parameterName)
+ {
+ return macroBlock.getParameters().entrySet().stream()
+ .filter(entry -> parameterName.equalsIgnoreCase(entry.getKey()))
+ .map(entry -> this.converter.convert(Boolean.class, entry.getValue()));
+ }
}
diff --git a/xwiki-platform-core/xwiki-platform-rendering/xwiki-platform-rendering-xwiki/src/main/java/org/xwiki/rendering/internal/macro/RawMacroRequiredRightsAnalyzer.java b/xwiki-platform-core/xwiki-platform-rendering/xwiki-platform-rendering-xwiki/src/main/java/org/xwiki/rendering/internal/macro/RawMacroRequiredRightsAnalyzer.java
index e44b1abfa0b..f4a82ce026c 100644
--- a/xwiki-platform-core/xwiki-platform-rendering/xwiki-platform-rendering-xwiki/src/main/java/org/xwiki/rendering/internal/macro/RawMacroRequiredRightsAnalyzer.java
+++ b/xwiki-platform-core/xwiki-platform-rendering/xwiki-platform-rendering-xwiki/src/main/java/org/xwiki/rendering/internal/macro/RawMacroRequiredRightsAnalyzer.java
@@ -50,15 +50,25 @@ public class RawMacroRequiredRightsAnalyzer implements MacroRequiredRightsAnalyz
@Override
public void analyze(MacroBlock macroBlock, MacroRequiredRightReporter reporter)
+ {
+ // Check if any parameter that is equal to "syntax" ignoring case is an HTML syntax.
+ boolean isHTML = macroBlock.getParameters().entrySet().stream()
+ .filter(entry -> "syntax".equalsIgnoreCase(entry.getKey()))
+ .anyMatch(entry -> isHTMLSyntax(entry.getValue()));
+
+ if (isHTML) {
+ reporter.report(macroBlock, List.of(MacroRequiredRight.SCRIPT), "rendering.macro.rawMacroRequiredRights");
+ }
+ }
+
+ private boolean isHTMLSyntax(String syntaxValue)
{
try {
- SyntaxType syntax = this.syntaxRegistry.resolveSyntax(macroBlock.getParameter("syntax")).getType();
- if (SyntaxType.HTML_FAMILY_TYPES.contains(syntax)) {
- reporter.report(macroBlock, List.of(MacroRequiredRight.SCRIPT),
- "rendering.macro.rawMacroRequiredRights");
- }
+ SyntaxType syntax = this.syntaxRegistry.resolveSyntax(syntaxValue).getType();
+ return SyntaxType.HTML_FAMILY_TYPES.contains(syntax);
} catch (ParseException e) {
- // Ignore, this should fail the macro or at least won't produce HTML output.
+ // Values that can't be parsed also won't be considered as HTML by the macro itself.
+ return false;
}
}
}
diff --git a/xwiki-platform-core/xwiki-platform-rendering/xwiki-platform-rendering-xwiki/src/test/java/org/xwiki/rendering/internal/macro/HTMLMacroRequiredRightsAnalyzerTest.java b/xwiki-platform-core/xwiki-platform-rendering/xwiki-platform-rendering-xwiki/src/test/java/org/xwiki/rendering/internal/macro/HTMLMacroRequiredRightsAnalyzerTest.java
index c7d51927f77..8e29b277a1c 100644
--- a/xwiki-platform-core/xwiki-platform-rendering/xwiki-platform-rendering-xwiki/src/test/java/org/xwiki/rendering/internal/macro/HTMLMacroRequiredRightsAnalyzerTest.java
+++ b/xwiki-platform-core/xwiki-platform-rendering/xwiki-platform-rendering-xwiki/src/test/java/org/xwiki/rendering/internal/macro/HTMLMacroRequiredRightsAnalyzerTest.java
@@ -21,10 +21,13 @@
import java.util.List;
import java.util.Map;
+import java.util.stream.Stream;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.CsvSource;
+import org.junit.jupiter.params.provider.MethodSource;
import org.mockito.ArgumentCaptor;
import org.xwiki.platform.security.requiredrights.MacroRequiredRight;
import org.xwiki.platform.security.requiredrights.MacroRequiredRightReporter;
@@ -130,4 +133,66 @@ void analyzeWithoutParameters(String content, boolean scriptRequired)
}
verifyNoMoreInteractions(reporter);
}
+
+ /**
+ * Test the behavior when a parameter occurs several times but with different case. Ensure that the "most
+ * dangerous" value wins.
+ */
+ @ParameterizedTest
+ @MethodSource("analyzeWithDuplicateParametersDataProvider")
+ void analyzeWithDuplicateParameters(Map<String, String> arguments, boolean wiki, boolean clean)
+ {
+ String content = "content";
+ MacroBlock macroBlock = new MacroBlock(HTML_MACRO_ID, arguments, content, false);
+
+ MacroRequiredRightReporter reporter = mock();
+ this.htmlMacroRequiredRightsAnalyzer.analyze(macroBlock, reporter);
+
+ if (wiki) {
+ verify(reporter).analyzeContent(macroBlock, content);
+ }
+
+ if (!clean || wiki) {
+ ArgumentCaptor<List<MacroRequiredRight>> argumentCaptor = ArgumentCaptor.captor();
+ verify(reporter).report(eq(macroBlock), argumentCaptor.capture(), anyString());
+ List<MacroRequiredRight> requiredRights = argumentCaptor.getValue();
+ if (wiki && clean) {
+ assertEquals(List.of(MacroRequiredRight.MAYBE_SCRIPT), requiredRights);
+ } else {
+ assertEquals(List.of(MacroRequiredRight.SCRIPT), requiredRights);
+ }
+ } else {
+ verifyNoMoreInteractions(reporter);
+ }
+ }
+
+ private static Stream<Arguments> analyzeWithDuplicateParametersDataProvider()
+ {
+ return Stream.of(
+ Arguments.of(Map.of(
+ "wiki", Boolean.TRUE.toString(),
+ "WIKI", Boolean.FALSE.toString(),
+ "clean", Boolean.TRUE.toString(),
+ "CLEAN", Boolean.FALSE.toString()
+ ), true, false),
+ Arguments.of(Map.of(
+ "wiki", Boolean.FALSE.toString(),
+ "WIKI", Boolean.TRUE.toString(),
+ "clean", Boolean.FALSE.toString(),
+ "CLEAN", Boolean.TRUE.toString()
+ ), true, false),
+ Arguments.of(Map.of(
+ "wiKi", Boolean.TRUE.toString(),
+ "clEan", Boolean.FALSE.toString()
+ ), true, false),
+ Arguments.of(Map.of(
+ "wikI", Boolean.FALSE.toString(),
+ "Wiki", Boolean.FALSE.toString(),
+ "clean", Boolean.TRUE.toString()
+ ), false, true),
+ Arguments.of(Map.of(
+ "cleaN", Boolean.FALSE.toString()
+ ), false, false)
+ );
+ }
}
diff --git a/xwiki-platform-core/xwiki-platform-rendering/xwiki-platform-rendering-xwiki/src/test/java/org/xwiki/rendering/internal/macro/RawMacroRequiredRightsAnalyzerTest.java b/xwiki-platform-core/xwiki-platform-rendering/xwiki-platform-rendering-xwiki/src/test/java/org/xwiki/rendering/internal/macro/RawMacroRequiredRightsAnalyzerTest.java
index 5b10712f513..e25c9a07797 100644
--- a/xwiki-platform-core/xwiki-platform-rendering/xwiki-platform-rendering-xwiki/src/test/java/org/xwiki/rendering/internal/macro/RawMacroRequiredRightsAnalyzerTest.java
+++ b/xwiki-platform-core/xwiki-platform-rendering/xwiki-platform-rendering-xwiki/src/test/java/org/xwiki/rendering/internal/macro/RawMacroRequiredRightsAnalyzerTest.java
@@ -20,8 +20,10 @@
package org.xwiki.rendering.internal.macro;
import java.util.List;
+import java.util.Map;
import java.util.stream.Stream;
+import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;
@@ -35,6 +37,7 @@
import org.xwiki.test.junit5.mockito.InjectMockComponents;
import org.xwiki.test.junit5.mockito.MockComponent;
+import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoInteractions;
@@ -50,33 +53,50 @@ class RawMacroRequiredRightsAnalyzerTest
{
private static final String SYNTAX_PARAMETER = "syntax";
+ private static final String PLAIN_VALUE = "plain";
+
+ private static final String HTML_VALUE = "html";
+
@MockComponent
private SyntaxRegistry syntaxRegistry;
@InjectMockComponents
private RawMacroRequiredRightsAnalyzer analyzer;
+ @BeforeEach
+ void setUp() throws ParseException
+ {
+ when(this.syntaxRegistry.resolveSyntax(anyString())).then(invocationOnMock -> {
+ String syntaxValue = invocationOnMock.getArgument(0);
+ return switch (syntaxValue) {
+ case PLAIN_VALUE -> Syntax.PLAIN_1_0;
+ case HTML_VALUE -> Syntax.HTML_5_0;
+ default -> throw new ParseException("Unknown syntax: " + syntaxValue);
+ };
+ });
+ }
+
private static Stream<Arguments> analyzeTestCases()
{
return Stream.of(
- Arguments.of("plain", Syntax.PLAIN_1_0, null),
- Arguments.of("html", Syntax.HTML_5_0, MacroRequiredRight.SCRIPT)
+ Arguments.of(Map.of(SYNTAX_PARAMETER, PLAIN_VALUE), null),
+ Arguments.of(Map.of(SYNTAX_PARAMETER, HTML_VALUE), MacroRequiredRight.SCRIPT),
+ Arguments.of(Map.of("sYnTaX", HTML_VALUE), MacroRequiredRight.SCRIPT),
+ Arguments.of(Map.of("sYntax", HTML_VALUE, SYNTAX_PARAMETER, PLAIN_VALUE), MacroRequiredRight.SCRIPT),
+ Arguments.of(Map.of("Syntax", PLAIN_VALUE, "SYNTAX", HTML_VALUE), MacroRequiredRight.SCRIPT),
+ Arguments.of(Map.of("syntaX", PLAIN_VALUE), null)
);
}
@ParameterizedTest
@MethodSource("analyzeTestCases")
- void analyze(String syntaxValue, Syntax expectedSyntax, MacroRequiredRight expectedRight) throws ParseException
+ void analyze(Map<String, String> parameters, MacroRequiredRight expectedRight)
{
- when(this.syntaxRegistry.resolveSyntax(syntaxValue)).thenReturn(expectedSyntax);
-
- MacroBlock macroBlock = mock();
- when(macroBlock.getParameter(SYNTAX_PARAMETER)).thenReturn(syntaxValue);
+ MacroBlock macroBlock = new MacroBlock("raw", parameters, false);
MacroRequiredRightReporter reporter = mock();
this.analyzer.analyze(macroBlock, reporter);
- verify(this.syntaxRegistry).resolveSyntax(syntaxValue);
if (expectedRight != null) {
verify(reporter).report(macroBlock, List.of(expectedRight), "rendering.macro.rawMacroRequiredRights");
} else {
--
GitLab