Skip to content

Commit b7d0889

Browse files
bjones86bjones86
committed
SAK-42253: revert 9c0fa8b 
1 parent 92f043c commit b7d0889

File tree

11 files changed

+77
-124
lines changed

11 files changed

+77
-124
lines changed

‎deploy/pom.xml

-5
Original file line numberDiff line numberDiff line change
@@ -495,11 +495,6 @@
495495
<artifactId>commons-validator</artifactId>
496496
<scope>compile</scope>
497497
</dependency>
498-
<dependency>
499-
<groupId>org.apache.commons</groupId>
500-
<artifactId>commons-text</artifactId>
501-
<scope>compile</scope>
502-
</dependency>
503498
<dependency>
504499
<groupId>org.apache.httpcomponents</groupId>
505500
<artifactId>httpcore</artifactId>

‎kernel/api/src/main/java/org/sakaiproject/util/api/FormattedText.java

-10
Original file line numberDiff line numberDiff line change
@@ -250,7 +250,6 @@ public String processFormattedText(final String strFromBrowser, StringBuilder er
250250
* @param value
251251
* The text containing entity references (e.g., a News item description).
252252
* @return The HTML, ready for processing.
253-
* @see https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/web/util/HtmlUtils.html#htmlUnescape-java.lang.String-
254253
*/
255254
public String unEscapeHtml(String value);
256255

@@ -460,13 +459,4 @@ public String processFormattedText(final String strFromBrowser, StringBuilder er
460459
*/
461460
public String getHtmlBody(String text);
462461

463-
/**
464-
* Sanitizes the user input to prevent XSS attacks.
465-
*
466-
* @param userInput The value to sanitize.
467-
* @return A sanitized user input.
468-
* @see https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/web/util/HtmlUtils.html#htmlEscape-java.lang.String-java.lang.String-
469-
*/
470-
public String sanitizeUserInput(final String userInput);
471-
472462
}

‎kernel/api/src/main/java/org/sakaiproject/util/api/MockFormattedText.java

+1-7
Original file line numberDiff line numberDiff line change
@@ -223,11 +223,5 @@ public String getHtmlBody(String text) {
223223
// TODO Auto-generated method stub
224224
return getHtmlBody(null);
225225
}
226-
227-
@Override
228-
public String sanitizeUserInput(final String userInput){
229-
log.warn(WARNING);
230-
return userInput;
231-
}
232-
226+
233227
}

‎kernel/kernel-impl/pom.xml

-4
Original file line numberDiff line numberDiff line change
@@ -247,10 +247,6 @@
247247
<groupId>org.hibernate</groupId>
248248
<artifactId>hibernate-core</artifactId>
249249
</dependency>
250-
<dependency>
251-
<groupId>org.springframework</groupId>
252-
<artifactId>spring-web</artifactId>
253-
</dependency>
254250
<dependency>
255251
<groupId>org.javassist</groupId>
256252
<artifactId>javassist</artifactId>

‎kernel/kernel-impl/src/main/java/org/sakaiproject/user/impl/BaseUserDirectoryService.java

+61-58
Original file line numberDiff line numberDiff line change
@@ -1519,11 +1519,11 @@ public User addUser(String id, String eid, String firstName, String lastName, St
15191519
UserEdit edit = addUser(id, eid);
15201520

15211521
// fill in the fields
1522-
edit.setLastName(formattedText().sanitizeUserInput(lastName));
1523-
edit.setFirstName(formattedText().sanitizeUserInput(firstName));
1524-
edit.setEmail(formattedText().sanitizeUserInput(email));
1522+
edit.setLastName(lastName);
1523+
edit.setFirstName(firstName);
1524+
edit.setEmail(email);
15251525
edit.setPassword(pw);
1526-
edit.setType(formattedText().sanitizeUserInput(type));
1526+
edit.setType(type);
15271527

15281528
ResourcePropertiesEdit props = edit.getPropertiesEdit();
15291529
if (properties != null)
@@ -1769,43 +1769,44 @@ protected void addLiveUpdateProperties(BaseUserEdit edit)
17691769
edit.m_lastModifiedInstant = Instant.now();
17701770
}
17711771

1772-
/**
1773-
* Adjust the id - trim it to null. Note: eid case insensitive option does NOT apply to id.
1774-
*
1775-
* @param id
1776-
* The id to clean up.
1777-
* @return A cleaned up id.
1778-
*/
1779-
protected String cleanId(String id) {
1780-
if(StringUtils.isEmpty(id)){
1781-
return null;
1782-
}
1783-
// if we are not doing separate id and eid, use the eid rules
1784-
if (!m_separateIdEid) {
1785-
id = cleanEid(id);
1786-
}
1787-
id = formattedText().sanitizeUserInput(id);
1788-
// max length for an id is 99 chars
1772+
/**
1773+
* Adjust the id - trim it to null. Note: eid case insensitive option does NOT apply to id.
1774+
*
1775+
* @param id
1776+
* The id to clean up.
1777+
* @return A cleaned up id.
1778+
*/
1779+
protected String cleanId(String id)
1780+
{
1781+
// if we are not doing separate id and eid, use the eid rules
1782+
if (!m_separateIdEid) {
1783+
id = cleanEid(id);
1784+
}
1785+
id = StringUtils.trimToNull(id);
1786+
// max length for an id is 99 chars
17891787
id = StringUtils.abbreviate(id, 99);
1790-
return id;
1791-
}
1788+
return id;
1789+
}
17921790

1793-
/**
1794-
* Adjust the eid - trim it to null, and lower case IF we are case insensitive.
1795-
*
1796-
* @param eid
1797-
* The eid to clean up.
1798-
* @return A cleaned up eid.
1799-
*/
1800-
protected String cleanEid(String eid) {
1801-
if(StringUtils.isEmpty(eid)){
1802-
return null;
1803-
}
1791+
/**
1792+
* Adjust the eid - trim it to null, and lower case IF we are case insensitive.
1793+
*
1794+
* @param eid
1795+
* The eid to clean up.
1796+
* @return A cleaned up eid.
1797+
*/
1798+
protected String cleanEid(String eid)
1799+
{
18041800
eid = StringUtils.lowerCase(eid);
1805-
eid = formattedText().sanitizeUserInput(eid);
1801+
eid = StringUtils.trimToNull(eid);
1802+
1803+
if (eid != null) {
1804+
// remove all instances of these chars <>,;:\"
1805+
eid = StringUtils.replaceChars(eid, "<>,;:\\/", "");
1806+
}
18061807
// NOTE: length check is handled later on
18071808
return eid;
1808-
}
1809+
}
18091810

18101811
protected UserEdit getCachedUser(String ref)
18111812
{
@@ -2030,7 +2031,7 @@ public boolean updateUserId(String id,String newEmail)
20302031
return false;
20312032
}
20322033
user.setEid(newEmail);
2033-
user.setEmail(formattedText().sanitizeUserInput(newEmail));
2034+
user.setEmail(newEmail);
20342035
((BaseUserEdit) user).setEvent(SECURE_UPDATE_USER_ANY);
20352036
commitEdit(user);
20362037
return true;
@@ -2174,7 +2175,7 @@ public BaseUserEdit(String id, String eid)
21742175
// if the id is not null (a new user, rather than a reconstruction)
21752176
// and not the anon (id == "") user,
21762177
// add the automatic (live) properties
2177-
if (StringUtils.isNotEmpty(m_id)) addLiveProperties(this);
2178+
if ((m_id != null) && (m_id.length() > 0)) addLiveProperties(this);
21782179

21792180
//KNL-567 lazy set the properties to be lazy so they get loaded
21802181
props.setLazy(true);
@@ -2214,13 +2215,13 @@ public BaseUserEdit(Element el)
22142215

22152216
m_id = cleanId(el.getAttribute("id"));
22162217
m_eid = cleanEid(el.getAttribute("eid"));
2217-
m_firstName = formattedText().sanitizeUserInput(el.getAttribute("first-name"));
2218-
m_lastName = formattedText().sanitizeUserInput(el.getAttribute("last-name"));
2219-
setEmail(el.getAttribute("email"));
2218+
m_firstName = StringUtils.trimToNull(el.getAttribute("first-name"));
2219+
m_lastName = StringUtils.trimToNull(el.getAttribute("last-name"));
2220+
setEmail(StringUtils.trimToNull(el.getAttribute("email")));
22202221
m_pw = el.getAttribute("pw");
2221-
m_type = formattedText().sanitizeUserInput(el.getAttribute("type"));
2222-
m_createdUserId = formattedText().sanitizeUserInput(el.getAttribute("created-id"));
2223-
m_lastModifiedUserId = formattedText().sanitizeUserInput(el.getAttribute("modified-id"));
2222+
m_type = StringUtils.trimToNull(el.getAttribute("type"));
2223+
m_createdUserId = StringUtils.trimToNull(el.getAttribute("created-id"));
2224+
m_lastModifiedUserId = StringUtils.trimToNull(el.getAttribute("modified-id"));
22242225

22252226
String time = StringUtils.trimToNull(el.getAttribute("created-time"));
22262227
if (time != null)
@@ -2315,15 +2316,15 @@ public BaseUserEdit(Element el)
23152316
public BaseUserEdit(String id, String eid, String email, String firstName, String lastName, String type, String pw,
23162317
String createdBy, Instant createdOn, String modifiedBy, Instant modifiedOn)
23172318
{
2318-
m_id = cleanId(id);
2319-
m_eid = cleanEid(eid);
2320-
m_firstName = formattedText().sanitizeUserInput(firstName);
2321-
m_lastName = formattedText().sanitizeUserInput(lastName);
2322-
m_type = formattedText().sanitizeUserInput(type);
2319+
m_id = id;
2320+
m_eid = eid;
2321+
m_firstName = firstName;
2322+
m_lastName = lastName;
2323+
m_type = type;
23232324
setEmail(email);
23242325
m_pw = pw;
2325-
m_createdUserId = formattedText().sanitizeUserInput(createdBy);
2326-
m_lastModifiedUserId = formattedText().sanitizeUserInput(modifiedBy);
2326+
m_createdUserId = createdBy;
2327+
m_lastModifiedUserId = modifiedBy;
23272328
if (createdOn != null) m_createdInstant = createdOn;
23282329
if (modifiedBy != null) m_lastModifiedInstant = modifiedOn;
23292330

@@ -2628,7 +2629,7 @@ public String getDisplayId() {
26282629
*/
26292630
public String getFirstName()
26302631
{
2631-
if (StringUtils.isEmpty(m_firstName)) return StringUtils.EMPTY;
2632+
if (m_firstName == null) return "";
26322633
return m_firstName;
26332634
}
26342635

@@ -2637,7 +2638,7 @@ public String getFirstName()
26372638
*/
26382639
public String getLastName()
26392640
{
2640-
if (StringUtils.isEmpty(m_lastName)) return StringUtils.EMPTY;
2641+
if (m_lastName == null) return "";
26412642
return m_lastName;
26422643
}
26432644

@@ -2682,7 +2683,7 @@ public String getSortName()
26822683
*/
26832684
public String getEmail()
26842685
{
2685-
if (StringUtils.isEmpty(m_email)) return StringUtils.EMPTY;
2686+
if (m_email == null) return "";
26862687
return m_email;
26872688
}
26882689

@@ -2800,7 +2801,8 @@ public void setEid(String eid)
28002801
public void setFirstName(String name)
28012802
{
28022803
if(!m_restrictedFirstName) {
2803-
m_firstName = formattedText().sanitizeUserInput(name);
2804+
// https://jira.sakaiproject.org/browse/SAK-20226 - removed html from name
2805+
m_firstName = formattedText().convertFormattedTextToPlaintext(name);
28042806
m_sortName = null;
28052807
}
28062808
}
@@ -2811,7 +2813,8 @@ public void setFirstName(String name)
28112813
public void setLastName(String name)
28122814
{
28132815
if(!m_restrictedLastName) {
2814-
m_lastName = formattedText().sanitizeUserInput(name);
2816+
// https://jira.sakaiproject.org/browse/SAK-20226 - removed html from name
2817+
m_lastName = formattedText().convertFormattedTextToPlaintext(name);
28152818
m_sortName = null;
28162819
}
28172820
}
@@ -2822,7 +2825,7 @@ public void setLastName(String name)
28222825
public void setEmail(String email)
28232826
{
28242827
if(!m_restrictedEmail) {
2825-
m_email = formattedText().sanitizeUserInput(email);
2828+
m_email = email;
28262829
}
28272830
}
28282831

@@ -2857,7 +2860,7 @@ public void setType(String type)
28572860
{
28582861
if(!m_restrictedType) {
28592862

2860-
m_type = formattedText().sanitizeUserInput(type);
2863+
m_type = type;
28612864

28622865
}
28632866
}

‎kernel/kernel-impl/src/main/java/org/sakaiproject/util/impl/FormattedTextImpl.java

+3-12
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,6 @@
5353
import org.sakaiproject.util.ResourceLoader;
5454
import org.sakaiproject.util.Xml;
5555
import org.sakaiproject.util.api.FormattedText;
56-
import org.springframework.web.util.HtmlUtils;
5756

5857
/**
5958
* FormattedText provides support for user entry of formatted text; the formatted text is HTML. This includes text formatting in user input such as bold, underline, and fonts.
@@ -594,8 +593,8 @@ public String escapeHtml(String value, boolean escapeNewlines) {
594593
*/
595594
String val = "";
596595
if (StringUtils.isNotEmpty(value)){
597-
val = HtmlUtils.htmlEscape(value, StandardCharsets.UTF_8.name());
598-
if (escapeNewlines) {
596+
val = StringEscapeUtils.escapeHtml4(value);
597+
if (escapeNewlines && val != null) {
599598
val = val.replace("\n", "<br/>\n");
600599
}
601600
}
@@ -662,7 +661,7 @@ public String encodeUnicode(String value)
662661
public String unEscapeHtml(String value)
663662
{
664663
if (StringUtils.isEmpty(value)) return StringUtils.EMPTY;
665-
return HtmlUtils.htmlUnescape(value);
664+
return StringEscapeUtils.unescapeHtml4(value);
666665
}
667666

668667
/* (non-Javadoc)
@@ -1506,12 +1505,4 @@ protected static final char hexDigit(int i)
15061505
9002, 9674, 9824, 9827, 9829, 9830, 34, 38, 60, 62, 338, 339, 352, 353, 376, 710, 732, 8194, 8195, 8201, 8204, 8205,
15071506
8206, 8207, 8211, 8212, 8216, 8217, 8218, 8220, 8221, 8222, 8224, 8225, 8240, 8249, 8250, 8364 };
15081507

1509-
public String sanitizeUserInput(final String userInput) {
1510-
if(StringUtils.EMPTY.equals(userInput)) {
1511-
return StringUtils.EMPTY;
1512-
}
1513-
String val = StringUtils.trimToNull(userInput);
1514-
val = HtmlUtils.htmlEscape(val, StandardCharsets.UTF_8.name());
1515-
return val;
1516-
}
15171508
}

‎kernel/kernel-impl/src/test/java/org/sakaiproject/user/impl/BaseUserDirectoryServiceTest.java

+3-3
Original file line numberDiff line numberDiff line change
@@ -180,15 +180,15 @@ public void testCleanEid() {
180180
Assert.assertNotNull(cleaned);
181181
Assert.assertEquals(eid, cleaned);
182182

183-
eid = "[email protected]";
183+
eid = "[email protected]";
184184
cleaned = userDirectoryService.cleanEid(eid);
185185
Assert.assertNotNull(cleaned);
186186
Assert.assertEquals(eid, cleaned);
187187

188-
eid = "<script>alert('xss');</script>";
188+
eid = "<script>alert('XSS');</script>";
189189
cleaned = userDirectoryService.cleanEid(eid);
190190
Assert.assertNotNull(cleaned);
191-
Assert.assertEquals("&lt;script&gt;alert(&#39;xss&#39;);&lt;/script&gt;", cleaned);
191+
Assert.assertEquals("scriptalert('xss')script", cleaned);
192192

193193
// empty cases
194194
eid = "";

‎kernel/kernel-impl/src/test/java/org/sakaiproject/user/impl/test/UserDirectoryServiceGetTest.java

+8-8
Original file line numberDiff line numberDiff line change
@@ -241,23 +241,23 @@ public void testLocalUserNameEdit() throws Exception {
241241
userEdit = userDirectoryService.editUser(localuserId);
242242
String originalFirstName = userEdit.getFirstName();
243243
String originalLastName = userEdit.getLastName();
244-
userEdit.setFirstName("John");
245-
userEdit.setLastName("Sakaiger");
244+
userEdit.setFirstName("Aaron");
245+
userEdit.setLastName("Zeckoski");
246246
userDirectoryService.commitEdit(userEdit);
247247

248248
user = userDirectoryService.getUser(localuserId);
249-
Assert.assertEquals("John", user.getFirstName());
250-
Assert.assertEquals("Sakaiger", user.getLastName());
249+
Assert.assertEquals("Aaron", user.getFirstName());
250+
Assert.assertEquals("Zeckoski", user.getLastName());
251251

252252
// check https://jira.sakaiproject.org/browse/SAK-20226
253253
userEdit = userDirectoryService.editUser(localuserId);
254-
userEdit.setFirstName("<b>John</b>");
255-
userEdit.setLastName("Sakaiger<script>alert(document.cookie)</script>");
254+
userEdit.setFirstName("<b>Aaron</b>");
255+
userEdit.setLastName("Zeckoski<script>alert(document.cookie)</script>");
256256
userDirectoryService.commitEdit(userEdit);
257257

258258
user = userDirectoryService.getUser(localuserId);
259-
Assert.assertEquals("&lt;b&gt;John&lt;/b&gt;", user.getFirstName());
260-
Assert.assertEquals("Sakaiger&lt;script&gt;alert(document.cookie)&lt;/script&gt;", user.getLastName());
259+
Assert.assertEquals("Aaron", user.getFirstName());
260+
Assert.assertEquals("Zeckoskialert(document.cookie)", user.getLastName());
261261

262262
// Return to where we were.
263263
userEdit = userDirectoryService.editUser(localuserId);

‎kernel/kernel-impl/src/test/java/org/sakaiproject/util/impl/FormattedTextTest.java

+1-6
Original file line numberDiff line numberDiff line change
@@ -1174,10 +1174,5 @@ public void getHtmlBodyTest() {
11741174
Assert.assertEquals("", result);
11751175
}
11761176

1177-
@Test
1178-
public void testEscapeXSS() {
1179-
String threat = "<img src=a onerror=\"alert(1)\" áéñíòùç %*+,-/;'<=>^|\\";
1180-
Assert.assertEquals("<img src=a onerror=\"alert(1)\" áéñíòùç %*+,-/;'<=>^|\\", threat);
1181-
Assert.assertEquals("&lt;img src=a onerror=&quot;alert(1)&quot; áéñíòùç %*+,-/;&#39;&lt;=&gt;^|\\", formattedText.sanitizeUserInput(threat));
1182-
}
1177+
11831178
}

‎kernel/kernel-util/pom.xml

-4
Original file line numberDiff line numberDiff line change
@@ -69,10 +69,6 @@
6969
<groupId>org.apache.commons</groupId>
7070
<artifactId>commons-lang3</artifactId>
7171
</dependency>
72-
<dependency>
73-
<groupId>org.apache.commons</groupId>
74-
<artifactId>commons-text</artifactId>
75-
</dependency>
7672
<dependency>
7773
<groupId>commons-io</groupId>
7874
<artifactId>commons-io</artifactId>

0 commit comments

Comments
 (0)