From 5744758f8311ab9cc78016b435d8aaa607cccea1 Mon Sep 17 00:00:00 2001 From: Alexandre Chatiron Date: Wed, 22 Apr 2015 20:25:01 +0800 Subject: [PATCH 1/5] * fix(router): make some improvement in router --- framework/src/play/mvc/Router.java | 24 +++++++++++-------- .../app/controllers/security/XssAttempts.java | 16 +++++++++++++ .../security/XssAttempts/testUrlParam.html | 8 +++++++ .../XssAttempts/xssAttempts.test.html | 10 ++++++++ 4 files changed, 48 insertions(+), 10 deletions(-) create mode 100644 samples-and-tests/just-test-cases/app/controllers/security/XssAttempts.java create mode 100644 samples-and-tests/just-test-cases/app/views/security/XssAttempts/testUrlParam.html create mode 100644 samples-and-tests/just-test-cases/test/security/XssAttempts/xssAttempts.test.html diff --git a/framework/src/play/mvc/Router.java b/framework/src/play/mvc/Router.java index 80ddd45263..a2007eeeda 100644 --- a/framework/src/play/mvc/Router.java +++ b/framework/src/play/mvc/Router.java @@ -509,11 +509,13 @@ public static ActionDefinition reverse(String action, Map args) try { queryString.append(URLEncoder.encode(key, encoding)); queryString.append("="); - if (object.toString().startsWith(":")) { - queryString.append(object.toString()); - } else { - queryString.append(URLEncoder.encode(object.toString() + "", encoding)); - } + String objStr = object.toString(); + // Special case to handle jsAction tag + if (objStr.startsWith(":") && objStr.length() > 1) { + queryString.append(':'); + objStr = objStr.substring(1); + } + queryString.append(URLEncoder.encode(objStr + "", encoding)); queryString.append("&"); } catch (UnsupportedEncodingException ex) { } @@ -524,11 +526,13 @@ public static ActionDefinition reverse(String action, Map args) try { queryString.append(URLEncoder.encode(key, encoding)); queryString.append("="); - if (value.toString().startsWith(":")) { - queryString.append(value.toString()); - } else { - queryString.append(URLEncoder.encode(value.toString() + "", encoding)); - } + String objStr = value.toString(); + // Special case to handle jsAction tag + if (objStr.startsWith(":") && objStr.length() > 1) { + queryString.append(':'); + objStr = objStr.substring(1); + } + queryString.append(URLEncoder.encode(objStr + "", encoding)); queryString.append("&"); } catch (UnsupportedEncodingException ex) { } diff --git a/samples-and-tests/just-test-cases/app/controllers/security/XssAttempts.java b/samples-and-tests/just-test-cases/app/controllers/security/XssAttempts.java new file mode 100644 index 0000000000..e2ca9e0f4e --- /dev/null +++ b/samples-and-tests/just-test-cases/app/controllers/security/XssAttempts.java @@ -0,0 +1,16 @@ +package controllers.security; + +import play.mvc.Controller; + + + +/** + * Test controller to check response to Xss attempt. + */ +public class XssAttempts extends Controller { + + public static void testUrlParam(String url){ + render(url); + } + +} diff --git a/samples-and-tests/just-test-cases/app/views/security/XssAttempts/testUrlParam.html b/samples-and-tests/just-test-cases/app/views/security/XssAttempts/testUrlParam.html new file mode 100644 index 0000000000..7b805bc882 --- /dev/null +++ b/samples-and-tests/just-test-cases/app/views/security/XssAttempts/testUrlParam.html @@ -0,0 +1,8 @@ +#{extends 'main.html' /} +#{set title:'Home' /} + +URL param value : ${url}
+ +XSS test on +asd + diff --git a/samples-and-tests/just-test-cases/test/security/XssAttempts/xssAttempts.test.html b/samples-and-tests/just-test-cases/test/security/XssAttempts/xssAttempts.test.html new file mode 100644 index 0000000000..597665d35a --- /dev/null +++ b/samples-and-tests/just-test-cases/test/security/XssAttempts/xssAttempts.test.html @@ -0,0 +1,10 @@ +#{selenium 'Test the cache tag'} + %{ def customCode = "\">" ; }% + + open('@{security.XssAttempts.testUrlParam(customCode )}') + assertHtmlSource('*href="/security.xssattempts/testurlparam?url=${customCode.urlEncode()}*') + + open('@{security.XssAttempts.testUrlParam(":" + customCode )}') + assertHtmlSource('*href="/security.xssattempts/testurlparam?url=:${customCode.urlEncode()}*') + +#{/selenium} \ No newline at end of file From b74813ba501c4d79c702b2b3121ff620176f2b41 Mon Sep 17 00:00:00 2001 From: Robert Bakker Date: Fri, 24 Apr 2015 23:15:49 +0200 Subject: [PATCH 2/5] added selenium test for router fix --- .../app/views/security/XssAttempts/testUrlParam.html | 4 ++-- samples-and-tests/just-test-cases/conf/routes | 1 + .../just-test-cases/test/xssurltest.test.html | 7 +++++++ 3 files changed, 10 insertions(+), 2 deletions(-) create mode 100644 samples-and-tests/just-test-cases/test/xssurltest.test.html diff --git a/samples-and-tests/just-test-cases/app/views/security/XssAttempts/testUrlParam.html b/samples-and-tests/just-test-cases/app/views/security/XssAttempts/testUrlParam.html index 7b805bc882..2e6f018c08 100644 --- a/samples-and-tests/just-test-cases/app/views/security/XssAttempts/testUrlParam.html +++ b/samples-and-tests/just-test-cases/app/views/security/XssAttempts/testUrlParam.html @@ -3,6 +3,6 @@ URL param value : ${url}
-XSS test on -asd +XSS test on <a> +actionxsstest diff --git a/samples-and-tests/just-test-cases/conf/routes b/samples-and-tests/just-test-cases/conf/routes index 1f39745eee..33090edabc 100644 --- a/samples-and-tests/just-test-cases/conf/routes +++ b/samples-and-tests/just-test-cases/conf/routes @@ -53,6 +53,7 @@ GET static2.foo.com/x staticDir:public2/assets2 #bug in router.reverse GET /notmatch/{name}/{action} Application.{action} GET /index2 Application.index2 +GET /xsstest security.XssAttempts.testUrlParam # Catch all * /sample/{controller}/{action} sample.{controller}.{action} diff --git a/samples-and-tests/just-test-cases/test/xssurltest.test.html b/samples-and-tests/just-test-cases/test/xssurltest.test.html new file mode 100644 index 0000000000..ab947a2d5b --- /dev/null +++ b/samples-and-tests/just-test-cases/test/xssurltest.test.html @@ -0,0 +1,7 @@ +#{selenium 'Test xss attack'} + + open('@{security.XssAttempts.testUrlParam(':">')}') + // assert that script tags are properly escaped + assertAttribute('link=actionxsstest@href','/xsstest?url=:%22%3E%3Cscript%3Ealert%28%22oh%22%29%3C%2Fscript%3E') + +#{/selenium} From 75087e68d51030a6a60d21c8952b5cdb326499ea Mon Sep 17 00:00:00 2001 From: Robert Bakker Date: Fri, 24 Apr 2015 23:29:00 +0200 Subject: [PATCH 3/5] better selenium test --- samples-and-tests/just-test-cases/test/xssurltest.test.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/samples-and-tests/just-test-cases/test/xssurltest.test.html b/samples-and-tests/just-test-cases/test/xssurltest.test.html index ab947a2d5b..654d9a1132 100644 --- a/samples-and-tests/just-test-cases/test/xssurltest.test.html +++ b/samples-and-tests/just-test-cases/test/xssurltest.test.html @@ -2,6 +2,6 @@ open('@{security.XssAttempts.testUrlParam(':">')}') // assert that script tags are properly escaped - assertAttribute('link=actionxsstest@href','/xsstest?url=:%22%3E%3Cscript%3Ealert%28%22oh%22%29%3C%2Fscript%3E') + assertAttribute('xpath=//a[1]@href','/xsstest?url=:%22%3E%3Cscript%3Ealert%28%22oh%22%29%3C%2Fscript%3E') #{/selenium} From ef18cc1ef645f438cd4f6d6678d2c13f5b513860 Mon Sep 17 00:00:00 2001 From: Robert Bakker Date: Sun, 26 Apr 2015 10:30:03 +0200 Subject: [PATCH 4/5] make sure diacritics is supported for jsaction (again) Conflicts: framework/src/play/templates/FastTags.java samples-and-tests/just-test-cases/app/views/Users/showAll.html Conflicts: framework/src/play/templates/FastTags.java --- framework/src/play/templates/FastTags.java | 23 ++++- .../app/controllers/Users.java | 84 ++++++++++++------- .../app/views/Users/showAll.html | 54 ++++++++++++ .../XssAttempts/xssAttempts.test.html | 10 ++- .../just-test-cases/test/xssurltest.test.html | 7 -- 5 files changed, 137 insertions(+), 41 deletions(-) create mode 100644 samples-and-tests/just-test-cases/app/views/Users/showAll.html delete mode 100644 samples-and-tests/just-test-cases/test/xssurltest.test.html diff --git a/framework/src/play/templates/FastTags.java b/framework/src/play/templates/FastTags.java index b799649b3d..c811c5a737 100644 --- a/framework/src/play/templates/FastTags.java +++ b/framework/src/play/templates/FastTags.java @@ -59,7 +59,28 @@ public static void _verbatim(Map args, Closure body, PrintWriter out, Exec } public static void _jsAction(Map args, Closure body, PrintWriter out, ExecutableTemplate template, int fromLine) { - out.println("function(options) {var pattern = '" + args.get("arg").toString().replace("&", "&") + "'; for(var key in options) { pattern = pattern.replace(':'+key, options[key] || ''); } return pattern }"); + String html = ""; + String minimize = ""; + if(args.containsKey("minimize") && Boolean.FALSE.equals(Boolean.valueOf(args.get("minimize").toString()))){ + minimize = "\n"; + } + html += "function(options) {" + minimize; + html += "var pattern = '" + args.get("arg").toString().replace("&", "&") + "';" + minimize;; + html += "for(key in options) {" + minimize;; + html += "var val = options[key];" + minimize; + // Encode URI script + if(args.containsKey("encodeURI") && Boolean.TRUE.equals(Boolean.valueOf(args.get("encodeURI").toString()))){ + html += "val = encodeURIComponent(val.replace('&', '&'));" + minimize; + } + //Custom script + if(args.containsKey("customScript")){ + html += "val = " + args.get("customScript") + minimize; + } + html += "pattern = pattern.replace(':' + encodeURIComponent(key), val || '');"+ minimize; + html += "}" + minimize;; + html += "return pattern;" + minimize;; + html += "}" + minimize; + out.println(html); } public static void _jsRoute(Map args, Closure body, PrintWriter out, ExecutableTemplate template, int fromLine) { diff --git a/samples-and-tests/just-test-cases/app/controllers/Users.java b/samples-and-tests/just-test-cases/app/controllers/Users.java index b5bd5b5a41..5fd0a1a8e1 100644 --- a/samples-and-tests/just-test-cases/app/controllers/Users.java +++ b/samples-and-tests/just-test-cases/app/controllers/Users.java @@ -38,39 +38,59 @@ public static void changeColors(List colors) { } public static void edit() { - User u = fresh(); - render(u); - } - - public static void save() { - User u = fresh(); - u.edit(params.getRootParamNode(), "u"); - render(u); - } - - static User fresh() { - try { - User u = new User(); - u.name = "Guillaume"; - u.b = true; - u.l = 356L; - u.birth = new SimpleDateFormat("dd/MM/yyyy").parse("21/12/1980"); - return u; - } catch(Exception e) { - throw new RuntimeException(e); - } - } - - public static void wbyte(byte a, Byte b) { - renderText(a+","+b); - } - - public static void newUser(String name) { - User u = new User(); - u.name = name; - u.save(); - renderText("Created user with name %s", u.name); + User u = fresh(); + render(u); + } + + public static void save() { + User u = fresh(); + u.edit(params.getRootParamNode(), "u"); + render(u); + } + + static User fresh() { + try { + User u = new User(); + u.name = "Guillaume"; + u.b = true; + u.l = 356L; + u.birth = new SimpleDateFormat("dd/MM/yyyy").parse("21/12/1980"); + return u; + } catch (Exception e) { + throw new RuntimeException(e); } + } + + public static void wbyte(byte a, Byte b) { + renderText(a + "," + b); + } + + public static void newUser(String name) { + User u = new User(); + u.name = name; + u.save(); + renderText("Created user with name %s", u.name); + } + public static void showAll() { + List users = User.find("order by name ASC").fetch(); + render(users); + } + + public static void showId(Long id) { + User u = User.findById(id); + if (u != null) { + renderText(u.name); + } + renderText(""); + } + + public static void showName(String name) { + User u = User.find("byName", name).first(); + if (u != null) { + renderText(u.name); + } + renderText(""); + } } diff --git a/samples-and-tests/just-test-cases/app/views/Users/showAll.html b/samples-and-tests/just-test-cases/app/views/Users/showAll.html new file mode 100644 index 0000000000..bb42d3fe53 --- /dev/null +++ b/samples-and-tests/just-test-cases/app/views/Users/showAll.html @@ -0,0 +1,54 @@ +#{extends 'main.html' /} + + + + + + + + +
NameName From ID UrlEncodejsAction urlEncodejsAction customScript
+ diff --git a/samples-and-tests/just-test-cases/test/security/XssAttempts/xssAttempts.test.html b/samples-and-tests/just-test-cases/test/security/XssAttempts/xssAttempts.test.html index 597665d35a..b95e4dbc46 100644 --- a/samples-and-tests/just-test-cases/test/security/XssAttempts/xssAttempts.test.html +++ b/samples-and-tests/just-test-cases/test/security/XssAttempts/xssAttempts.test.html @@ -7,4 +7,12 @@ open('@{security.XssAttempts.testUrlParam(":" + customCode )}') assertHtmlSource('*href="/security.xssattempts/testurlparam?url=:${customCode.urlEncode()}*') -#{/selenium} \ No newline at end of file +#{/selenium} + +#{selenium 'Test xss attack'} + + open('@{security.XssAttempts.testUrlParam(':">')}') + // assert that script tags are properly escaped + assertAttribute('xpath=//a[1]@href','/xsstest?url=:%22%3E%3Cscript%3Ealert%28%22oh%22%29%3C%2Fscript%3E') + +#{/selenium} diff --git a/samples-and-tests/just-test-cases/test/xssurltest.test.html b/samples-and-tests/just-test-cases/test/xssurltest.test.html deleted file mode 100644 index 654d9a1132..0000000000 --- a/samples-and-tests/just-test-cases/test/xssurltest.test.html +++ /dev/null @@ -1,7 +0,0 @@ -#{selenium 'Test xss attack'} - - open('@{security.XssAttempts.testUrlParam(':">')}') - // assert that script tags are properly escaped - assertAttribute('xpath=//a[1]@href','/xsstest?url=:%22%3E%3Cscript%3Ealert%28%22oh%22%29%3C%2Fscript%3E') - -#{/selenium} From 67a2a777950965eee2e69287aefa579bd664263d Mon Sep 17 00:00:00 2001 From: Alexandre Chatiron Date: Mon, 27 Apr 2015 11:22:31 +0800 Subject: [PATCH 5/5] * Add Travis support * refactor(XML): create documentBuilderFactory to factorize the code of XML file creation * fix(ProxyDriver): add getParentLogger() to compile on JDK7 Conflicts: framework/src/play/libs/XML.java --- .travis.yml | 14 +++++ framework/src/play/db/DBPlugin.java | 15 ++++++ framework/src/play/libs/XML.java | 52 ++++++++++++++----- .../XssAttempts/xssAttempts.test.html | 4 +- 4 files changed, 70 insertions(+), 15 deletions(-) create mode 100644 .travis.yml diff --git a/.travis.yml b/.travis.yml new file mode 100644 index 0000000000..a8899aa734 --- /dev/null +++ b/.travis.yml @@ -0,0 +1,14 @@ +language: java +jdk: + - openjdk6 +script: ant -buildfile ./framework/build.xml test +after_failure: + cat ./samples-and-tests/just-test-cases/test-result/*.failed.html + cat ./samples-and-tests/forum/test-result/*.failed.html + cat ./samples-and-tests/zencontact/test-result/*.failed.html + cat ./samples-and-tests/jobboard/test-result/*.failed.html + cat ./samples-and-tests/yabe/test-result/*.failed.html +notifications: + email: + on_success: change + on_failure: always \ No newline at end of file diff --git a/framework/src/play/db/DBPlugin.java b/framework/src/play/db/DBPlugin.java index ecf02589b7..c2200d7392 100644 --- a/framework/src/play/db/DBPlugin.java +++ b/framework/src/play/db/DBPlugin.java @@ -11,15 +11,19 @@ import java.sql.DriverManager; import java.sql.DriverPropertyInfo; import java.sql.SQLException; +import java.sql.SQLFeatureNotSupportedException; import java.util.HashMap; import java.util.Map; import java.util.Properties; + import javax.naming.Context; import javax.naming.InitialContext; import javax.sql.DataSource; import jregex.Matcher; + import org.apache.commons.lang.StringUtils; + import play.Logger; import play.Play; import play.PlayPlugin; @@ -338,6 +342,17 @@ public DriverPropertyInfo[] getPropertyInfo(String u, Properties p) throws SQLEx public boolean jdbcCompliant() { return this.driver.jdbcCompliant(); } + + // Method not annotated with @Override since getParentLogger() is a new method + // in the CommonDataSource interface starting with JDK7 and this annotation + // would cause compilation errors with JDK6. + public java.util.logging.Logger getParentLogger() throws SQLFeatureNotSupportedException { + try { + return (java.util.logging.Logger) Driver.class.getDeclaredMethod("getParentLogger").invoke(this.driver); + } catch (Throwable e) { + return null; + } + } } public static class PlayConnectionCustomizer implements ConnectionCustomizer { diff --git a/framework/src/play/libs/XML.java b/framework/src/play/libs/XML.java index a7c7c4ad06..bc7b3a09d6 100644 --- a/framework/src/play/libs/XML.java +++ b/framework/src/play/libs/XML.java @@ -1,9 +1,6 @@ package play.libs; -import java.io.File; -import java.io.IOException; -import java.io.StringReader; -import java.io.StringWriter; +import java.io.*; import java.security.Key; import java.security.Provider; import java.security.interfaces.RSAPrivateKey; @@ -26,6 +23,7 @@ import javax.xml.crypto.dsig.keyinfo.KeyValue; import javax.xml.crypto.dsig.spec.C14NMethodParameterSpec; import javax.xml.crypto.dsig.spec.TransformParameterSpec; +import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.ParserConfigurationException; import javax.xml.transform.Transformer; @@ -46,6 +44,25 @@ */ public class XML { + public static DocumentBuilderFactory newDocumentBuilderFactory() { + try { + DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); + dbf.setFeature("http://xml.org/sax/features/external-general-entities", false); + dbf.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + return dbf; + } catch (ParserConfigurationException e) { + throw new RuntimeException(e); + } + } + + public static DocumentBuilder newDocumentBuilder() { + try { + return newDocumentBuilderFactory().newDocumentBuilder(); + } catch (ParserConfigurationException e) { + throw new RuntimeException(e); + } + } + /** * Serialize to XML String * @param document The DOM document @@ -58,7 +75,7 @@ public static String serialize(Document document) { Transformer transformer = factory.newTransformer(); DOMSource domSource = new DOMSource(document); StreamResult streamResult = new StreamResult(writer); - transformer.transform(domSource, streamResult); + transformer.transform(domSource, streamResult); } catch (TransformerException e) { throw new RuntimeException( "Error when serializing XML document.", e); @@ -69,18 +86,15 @@ public static String serialize(Document document) { /** * Parse an XML file to DOM * @return null if an error occurs during parsing. - * + * */ public static Document getDocument(File file) { - DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); try { - return dbf.newDocumentBuilder().parse(file); + return newDocumentBuilder().parse(file); } catch (SAXException e) { Logger.warn("Parsing error when building Document object from xml file '" + file + "'.", e); } catch (IOException e) { Logger.warn("Reading error when building Document object from xml file '" + file + "'.", e); - } catch (ParserConfigurationException e) { - Logger.warn("Parsing error when building Document object from xml file '" + file + "'.", e); } return null; } @@ -91,15 +105,27 @@ public static Document getDocument(File file) { */ public static Document getDocument(String xml) { InputSource source = new InputSource(new StringReader(xml)); - DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); try { - return dbf.newDocumentBuilder().parse(source); + return newDocumentBuilder().parse(source); } catch (SAXException e) { Logger.warn("Parsing error when building Document object from xml data.", e); } catch (IOException e) { Logger.warn("Reading error when building Document object from xml data.", e); - } catch (ParserConfigurationException e) { + } + return null; + } + + /** + * Parse an XML coming from an input stream to DOM + * @return null if an error occurs during parsing. + */ + public static Document getDocument(InputStream stream) { + try { + return newDocumentBuilder().parse(stream); + } catch (SAXException e) { Logger.warn("Parsing error when building Document object from xml data.", e); + } catch (IOException e) { + Logger.warn("Reading error when building Document object from xml data.", e); } return null; } diff --git a/samples-and-tests/just-test-cases/test/security/XssAttempts/xssAttempts.test.html b/samples-and-tests/just-test-cases/test/security/XssAttempts/xssAttempts.test.html index b95e4dbc46..b128c1726e 100644 --- a/samples-and-tests/just-test-cases/test/security/XssAttempts/xssAttempts.test.html +++ b/samples-and-tests/just-test-cases/test/security/XssAttempts/xssAttempts.test.html @@ -2,10 +2,10 @@ %{ def customCode = "\">" ; }% open('@{security.XssAttempts.testUrlParam(customCode )}') - assertHtmlSource('*href="/security.xssattempts/testurlparam?url=${customCode.urlEncode()}*') + assertHtmlSource('*href="/xsstest?url=${customCode.urlEncode()}*') open('@{security.XssAttempts.testUrlParam(":" + customCode )}') - assertHtmlSource('*href="/security.xssattempts/testurlparam?url=:${customCode.urlEncode()}*') + assertHtmlSource('*href="/xsstest?url=:${customCode.urlEncode()}*') #{/selenium}