Skip to content

Commit

Permalink
Merge pull request from GHSA-599v-w48h-rjrm
Browse files Browse the repository at this point in the history
* Add a page test.
  • Loading branch information
michitux committed Mar 24, 2022
1 parent f4d4b56 commit 3b50f39
Show file tree
Hide file tree
Showing 3 changed files with 241 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -121,5 +121,12 @@
<scope>test</scope>
<type>test-jar</type>
</dependency>
<!-- Mail script service for Page Tests. -->
<dependency>
<groupId>org.xwiki.platform</groupId>
<artifactId>xwiki-platform-mail-script</artifactId>
<version>${project.version}</version>
<scope>test</scope>
</dependency>
</dependencies>
</project>
Original file line number Diff line number Diff line change
Expand Up @@ -88,37 +88,63 @@ $response.setContentType("text/xml")
#set($query = "select distinct list from BaseObject as obj, DBStringListProperty as prop join prop.list list where obj.name <> :templatename and obj.className = :classname and prop.id.id = obj.id and prop.id.name = :fieldname and lower(list) like :input ESCAPE '!'")
#set($queryResult = $services.query.hql($query).setLimit(30).bindValues({'templatename' : ${templatename}, 'classname' : ${classname}, 'fieldname' : ${fieldname}, 'input' : "%${input}%"}).execute())
#set($results = $queryResult.toArray())
#set($documentQuery = 'select obj.name from BaseObject as obj, DBStringListProperty as prop join prop.list list
where obj.name <> :templateName and obj.className = :className and prop.id.id = obj.id
and prop.id.name = :fieldname and list = :propertyValue')
<?xml version="1.0" encoding="UTF-8"?>
<results type="3">
#foreach($res in $results)
<rs id="$foreach.count" info="">$!{escapetool.xml($res)}</rs>
#set($documentQueryResult = $services.query.hql($documentQuery).setLimit(1).bindValues({'templateName' :

This comment has been minimized.

Copy link
@sdumitriu

sdumitriu Mar 28, 2022

Member

This is partially wrong. If a value is used in two documents, and the user has access to only one of them, the value may or may not be suggested depending on which of the documents matches the query first. On the plus side, it does not offer suggestions that should not be offered, but it may not offer valid suggestions.

Also, this is inefficient, running an extra query for every possible result, but given that at most 30 values are checked, it's not too bad.

But this is better than before, so it's good enough for now.

Possible improvement: have only one query like select distinct list, obj.name..., check the rights on $res[1], display the value of $res[0]; but this introduces the bug that duplicate values may be displayed since the distinct applies to the value - document pair, not just the value.

Another bug: if the input would match 60 values, 30 of which are accessible, instead of 30 suggestions, the user may see 0 if the first 30 matches of the query happen to be the inaccessible ones. The workaround for that is to raise the limit and break out of the loop when 30 values have been printed, with the disadvantage that the query may take much longer if there are many values to match on.

And one more bug: this doesn't fix the case where $hibquery != "", so an xclass that suggests values from other documents will allow exposing values.

This comment has been minimized.

Copy link
@michitux

michitux Mar 29, 2022

Author Contributor

I would suggest to continue this discussion on the respective Jira issue, which is https://jira.xwiki.org/browse/XWIKI-18849. For the public record: these limitations are known and expected, there is just currently no really better solution. For the last point, in this case we are only returning the values that have been configured with the query and we are checking the rights of the class where the property is defined. It is the job of the user creating the query to make sure that it is not exposing any sensitive data that shouldn't be accessible when the class definition is not accessible. Therefore, this is not a bug from my point of view. If you believe otherwise, please create a confidential Jira issue so we can discuss the details there.

${templatename}, 'className' : ${classname}, 'fieldname' : ${fieldname}, 'propertyValue' : $res}).execute())
#if (!$documentQueryResult.isEmpty() && $xwiki.hasAccessLevel('view', $documentQueryResult.get(0)))
<rs id="$foreach.count" info="">$!{escapetool.xml($res)}</rs>
#end
#end
</results>
######################### StringListProperty query
#elseif($p.isMultiSelect())
#set($query = "select distinct prop.textValue from BaseObject as obj, StringListProperty as prop where obj.name <> :templatename and obj.className = :classname and prop.id.id = obj.id and prop.name = :fieldname and lower(prop.textValue) like :input ESCAPE '!'")
#set($queryResult = $services.query.hql($query).setLimit(30).bindValues({'templatename' : ${templatename}, 'classname' : ${classname}, 'fieldname' : ${fieldname}, 'input' : "%${input}%"}).execute())
#set($results = $queryResult.toArray())
#set($documentQuery = 'select obj.name from BaseObject as obj, StringListProperty as prop where obj.name <>
:templateName and obj.className = :className and prop.id.id = obj.id and prop.name = :fieldname and
prop.textValue = :propertyValue')
<?xml version="1.0" encoding="UTF-8"?>
<results type="5">
#foreach($res in $results)
#set($list = $p.getListFromString($res, $sep, false))
#set($list = $list.toArray())
#foreach($word in $list)
#set($documentQueryResult = $services.query.hql($documentQuery).setLimit(1).bindValues({'templateName' :
${templatename}, 'className' : ${classname}, 'fieldname' : ${fieldname}, 'propertyValue' : $res}).execute())
#if (!$documentQueryResult.isEmpty() && $xwiki.hasAccessLevel('view', $documentQueryResult.get(0)))
#set($list = $p.getListFromString($res, $sep, false))
#set($list = $list.toArray())
#foreach($word in $list)
<rs id="$foreach.count" info="">$!{escapetool.xml($word)}</rs>
#end
#end
#end
</results>
######################### StringProperty query
#else
#elseif($p && ($p.getClassType() != "Email" || !$services.mail.general.shouldObfuscate()) && $p.getClassType() != "Password")
#set($query = "select distinct prop.value from BaseObject as obj, StringProperty as prop where obj.className = :classname and obj.name <> :templatename and prop.id.id = obj.id and prop.id.name = :fieldname and lower(prop.value) like :input ESCAPE '!'")
#set($queryResult = $services.query.hql($query).setLimit(30).bindValues({'templatename' : ${templatename}, 'classname' : ${classname}, 'fieldname' : ${fieldname}, 'input' : "%${input}%"}).execute())
#set($results = $queryResult.toArray())
#set($documentQuery = 'select obj.name from BaseObject as obj, StringProperty as prop where
obj.className = :className and obj.name <> :templateName and prop.id.id = obj.id and prop.id.name = :fieldname and
prop.value = :propertyValue')
<?xml version="1.0" encoding="UTF-8"?>
<results type="8">
#foreach($res in $results)
<rs id="$foreach.count" info="">$!{escapetool.xml($res)}</rs>
#set($documentQueryResult = $services.query.hql($documentQuery).setLimit(1).bindValues({'templateName' :
${templatename}, 'className' : ${classname}, 'fieldname' : ${fieldname}, 'propertyValue' : $res}).execute())
#if (!$documentQueryResult.isEmpty() && $xwiki.hasAccessLevel('view', $documentQueryResult.get(0)))
<rs id="$foreach.count" info="">$!{escapetool.xml($res)}</rs>
#end
#end
</results>
## Inaccessible property, return empty results.
#else
<?xml version="1.0" encoding="UTF-8"?>
<results type="8">
</results>
#end
#end ## if hibquery
Original file line number Diff line number Diff line change
@@ -0,0 +1,202 @@
/*
* See the NOTICE file distributed with this work for additional
* information regarding copyright ownership.
*
* This is free software; you can redistribute it and/or modify it
* under the terms of the GNU Lesser General Public License as
* published by the Free Software Foundation; either version 2.1 of
* the License, or (at your option) any later version.
*
* This software is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public
* License along with this software; if not, write to the Free
* Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
* 02110-1301 USA, or see the FSF site: http://www.fsf.org.
*/
package org.xwiki.web;

import java.io.ByteArrayInputStream;
import java.nio.charset.StandardCharsets;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;

import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;
import org.w3c.dom.Document;
import org.w3c.dom.NodeList;
import org.xwiki.mail.script.GeneralMailScriptService;
import org.xwiki.mail.script.MailScriptService;
import org.xwiki.model.reference.DocumentReference;
import org.xwiki.query.internal.ScriptQuery;
import org.xwiki.query.script.QueryManagerScriptService;
import org.xwiki.script.service.ScriptService;
import org.xwiki.template.TemplateManager;
import org.xwiki.test.page.PageTest;
import org.xwiki.velocity.tools.EscapeTool;

import com.xpn.xwiki.doc.XWikiDocument;
import com.xpn.xwiki.objects.classes.BaseClass;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyInt;
import static org.mockito.ArgumentMatchers.anyMap;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

/**
* Tests the {@code suggest.vm} template.
*
* @version $Id$
* @since 14.2
* @since 13.10.4
*/
class SuggestPageTest extends PageTest
{
private static final String EMAIL_FIELD = "email";

private static final String PASSWORD_FIELD = "password";

private static final String VIEW_ACTION = "view";

private static final String RESULTS_TAG = "rs";

private TemplateManager templateManager;

private ScriptQuery query;

@BeforeEach
void setUp() throws Exception
{
this.templateManager = this.oldcore.getMocker().getInstance(TemplateManager.class);
registerVelocityTool("escapetool", new EscapeTool());

// suggest.vm needs programming rights and normally has them as a Velocity template.
when(this.xwiki.getRightService().hasProgrammingRights(any())).thenReturn(true);

// Set the query manager to basically ignore any parameters set, the individual tests set the results
// independently of them.
QueryManagerScriptService qmss = mock(QueryManagerScriptService.class);
this.oldcore.getMocker().registerComponent(ScriptService.class, "query", qmss);
this.query = mock(ScriptQuery.class);
when(qmss.hql(anyString())).thenReturn(this.query);
when(this.query.setLimit(anyInt())).thenReturn(this.query);
when(this.query.bindValues(anyMap())).thenReturn(this.query);

// Create a document with some properties to test. Most of them are inspired by XWiki.XWikiUsers, thus the
// naming.
XWikiDocument classDocument =
new XWikiDocument(new DocumentReference("xwiki", "XWiki", "XWikiUsers"));
BaseClass xclass = classDocument.getXClass();
xclass.addTextField("first_name", "First Name", 30);
xclass.addEmailField(EMAIL_FIELD, "e-Mail", 30);
xclass.addPasswordField(PASSWORD_FIELD, "Password", 10);
xclass.addStaticListField("imtype", "IM Type", 30, true, "AIM|Yahoo|Jabber|MSN|Skype|ICQ");
xclass.addStaticListField("tags", "Tags", 30, true, true, "", "checkbox", ",");
this.xwiki.saveDocument(classDocument, this.context);

// Set this document as default class name.
this.request.put("classname", "XWiki.XWikiUsers");
}

@Test
void hidesPassword() throws Exception
{
NodeList resultElements = getResult(PASSWORD_FIELD).getElementsByTagName(RESULTS_TAG);

verify(this.query, never()).execute();
assertEquals(0, resultElements.getLength());
}

@ParameterizedTest
@CsvSource({ "first_name, 8", "imtype, 5", "tags, 3", "email, 8" })
void accessCheck(String fieldName, String resultsType) throws Exception
{
String documentU1Reference = "XWiki.U1";
String documentU2Reference = "XWiki.U2";

List<Object> values = Arrays.asList("U1", "U2");
when(this.query.execute()).thenReturn(values)
.thenReturn(Collections.singletonList(documentU1Reference))
.thenReturn(Collections.singletonList(documentU2Reference));

when(this.xwiki.getRightService().hasAccessLevel(eq(VIEW_ACTION), any(), eq(documentU1Reference), any()))
.thenReturn(true);
when(this.xwiki.getRightService().hasAccessLevel(eq(VIEW_ACTION), any(), eq(documentU2Reference), any()))
.thenReturn(false);

Document result = getResult(fieldName);
assertEquals(resultsType, result.getDocumentElement().getAttribute("type"));
NodeList resultElements = result.getElementsByTagName(RESULTS_TAG);

assertEquals(1, resultElements.getLength());
assertEquals(values.get(0), resultElements.item(0).getTextContent());
}

/**
* Check that the email is displayed when the script service is available but returns false (vs. does not exist
* as in the tests where it is not explicitly configured).
*
* @throws Exception when the test fails
*/
@Test
void emailVisibleWhenNotObfuscated() throws Exception
{
mockEmailObfuscation(false);

String documentReference = "XWiki.Admin";
String email = "user@example.com";
when(this.query.execute()).thenReturn(Collections.singletonList(email))
.thenReturn(Collections.singletonList(documentReference));
when(this.xwiki.getRightService().hasAccessLevel(eq(VIEW_ACTION), any(), eq(documentReference), any()))
.thenReturn(true);

NodeList resultElements = getResult(EMAIL_FIELD).getElementsByTagName(RESULTS_TAG);
assertEquals(1, resultElements.getLength());
assertEquals(email, resultElements.item(0).getTextContent());
}

@Test
void emailHiddenWhenObfuscated() throws Exception
{
mockEmailObfuscation(true);

NodeList resultElements = getResult(EMAIL_FIELD).getElementsByTagName(RESULTS_TAG);
verify(this.query, never()).execute();
assertEquals(0, resultElements.getLength());
}

private void mockEmailObfuscation(boolean shallObfuscate) throws Exception
{
GeneralMailScriptService generalMailScriptService = mock(GeneralMailScriptService.class);
MailScriptService mailScriptService = mock(MailScriptService.class);
when(mailScriptService.get("general")).thenReturn(generalMailScriptService);
this.oldcore.getMocker().registerComponent(ScriptService.class, "mail", mailScriptService);
when(generalMailScriptService.shouldObfuscate()).thenReturn(shallObfuscate);
}

private Document getResult(String fieldName) throws Exception
{
this.request.put("fieldname", fieldName);

String xmlResult = this.templateManager.render("suggest.vm").trim();

DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactory.newInstance();
DocumentBuilder documentBuilder = documentBuilderFactory.newDocumentBuilder();
return documentBuilder.parse(new ByteArrayInputStream(xmlResult.getBytes(StandardCharsets.UTF_8)));
}
}

0 comments on commit 3b50f39

Please sign in to comment.