Skip to content

Commit 1c787b5

Browse files
authored
Escape pipe character for injected users (opensearch-project#5175)
Signed-off-by: shikharj05 <8859327+shikharj05@users.noreply.github.com>
1 parent 75f03c7 commit 1c787b5

File tree

3 files changed

+46
-4
lines changed

3 files changed

+46
-4
lines changed

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java

+7-4
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,7 @@
111111

112112
import static org.opensearch.security.OpenSearchSecurityPlugin.traceAction;
113113
import static org.opensearch.security.support.ConfigConstants.OPENDISTRO_SECURITY_USER_INFO_THREAD_CONTEXT;
114+
import static org.opensearch.security.support.SecurityUtils.escapePipe;
114115

115116
public class PrivilegesEvaluator {
116117

@@ -275,12 +276,14 @@ public boolean isInitialized() {
275276
private void setUserInfoInThreadContext(User user) {
276277
if (threadContext.getTransient(OPENDISTRO_SECURITY_USER_INFO_THREAD_CONTEXT) == null) {
277278
StringJoiner joiner = new StringJoiner("|");
278-
joiner.add(user.getName());
279-
joiner.add(String.join(",", user.getRoles()));
280-
joiner.add(String.join(",", user.getSecurityRoles()));
279+
// Escape any pipe characters in the values before joining
280+
joiner.add(escapePipe(user.getName()));
281+
joiner.add(escapePipe(String.join(",", user.getRoles())));
282+
joiner.add(escapePipe(String.join(",", user.getSecurityRoles())));
283+
281284
String requestedTenant = user.getRequestedTenant();
282285
if (!Strings.isNullOrEmpty(requestedTenant)) {
283-
joiner.add(requestedTenant);
286+
joiner.add(escapePipe(requestedTenant));
284287
}
285288
threadContext.putTransient(OPENDISTRO_SECURITY_USER_INFO_THREAD_CONTEXT, joiner.toString());
286289
}

src/main/java/org/opensearch/security/support/SecurityUtils.java

+8
Original file line numberDiff line numberDiff line change
@@ -140,4 +140,12 @@ private static String resolveEnvVar(String envVarName, String mode, boolean bc,
140140
return bc ? Hasher.hash(envVarValue.toCharArray(), settings) : envVarValue;
141141
}
142142
}
143+
144+
// Helper method to escape pipe characters
145+
public static String escapePipe(String input) {
146+
if (input == null) {
147+
return "";
148+
}
149+
return input.replace("|", "\\|");
150+
}
143151
}

src/test/java/org/opensearch/security/support/SecurityUtilsTest.java

+31
Original file line numberDiff line numberDiff line change
@@ -70,4 +70,35 @@ private void checkKeysWithPredicate(Collection<String> keys, String predicateNam
7070
);
7171
});
7272
}
73+
74+
@Test
75+
public void testEscapePipe_NullInput() {
76+
assertThat("Null input should return empty string", SecurityUtils.escapePipe(null), equalTo(""));
77+
}
78+
79+
@Test
80+
public void testEscapePipe_EmptyString() {
81+
assertThat("Empty string should return empty string", SecurityUtils.escapePipe(""), equalTo(""));
82+
}
83+
84+
@Test
85+
public void testEscapePipe_StringWithoutPipe() {
86+
assertThat("String without pipe should remain unchanged", SecurityUtils.escapePipe("normal string"), equalTo("normal string"));
87+
}
88+
89+
@Test
90+
public void testEscapePipe_SinglePipe() {
91+
assertThat("Single pipe should be escaped", SecurityUtils.escapePipe("before|after"), equalTo("before\\|after"));
92+
}
93+
94+
@Test
95+
public void testEscapePipe_MultiplePipes() {
96+
assertThat("Multiple pipes should all be escaped", SecurityUtils.escapePipe("a|b|c"), equalTo("a\\|b\\|c"));
97+
}
98+
99+
@Test
100+
public void testEscapePipe_MultiplePipesConsecutive() {
101+
assertThat("Consecutive pipes should all be escaped", SecurityUtils.escapePipe("|||"), equalTo("\\|\\|\\|"));
102+
}
103+
73104
}

0 commit comments

Comments
 (0)