Existing sanitized URIs or calls to URI templates should still be esc…

…aped in

the query.

Examples:

   /redirect?continue={$uri}
   /redirect?continue={call .destination data="all" /}

In both of those, it doesn't really make any sense for ampersands in the URI to
break out of the continue param.

This will affect existing templates if they do something like:

   /action?{$query}
   /action?{call .queryParams data="all" /}

However, most examples I see (and during my last large scale change I ended up
doing this anyway) are like this:

   {let $queryString kind="uri"}?a={$a}&b={$b}{/let}
   /action{$queryString}

which is still handled. It's a question whether we still want to explicitly
support the above syntax in the general case, but in that specific case, it's a
pretty good idea.
-------------
Created by MOE: http://code.google.com/p/moe-java
MOE_MIGRATED_REVID=96124596

java/src/com/google/template/soy/shared/restricted/Sanitizers.java

@@ -352,18 +352,15 @@ public static String filterCssValue(String value) {
 
 
   /**
-   * Converts the input to a piece of a URI by percent encoding assuming a UTF-8 encoding.
+   * Converts the input to a piece of a URI by percent encoding the value as UTF-8 bytes.
    */
   public static String escapeUri(SoyValue value) {
-    if (isSanitizedContentOfKind(value, SanitizedContent.ContentKind.URI)) {
-      return normalizeUri(value);
-    }
     return escapeUri(value.coerceToString());
   }
 
 
   /**
-   * Converts plain text to a piece of a URI by percent encoding assuming a UTF-8 encoding.
+   * Converts plain text to a piece of a URI by percent encoding the string as UTF-8 bytes.
    */
   public static String escapeUri(String value) {
     return uriEscaper().escape(value);

java/tests/com/google/template/soy/basicdirectives/BasicEscapeDirectiveTest.java

@@ -160,8 +160,10 @@ public final void testEscapeUri() {
         "a%25bc%20%3E%20d",
         UnsafeSanitizedContentOrdainer.ordainAsSafe("a%bc > d", SanitizedContent.ContentKind.HTML),
         escapeUri);
+    // NOTE: URIs are not treated specially (e.g. /redirect?continue={$url} should not allow $url
+    // to break out and add other query params, and would be unexpected.)
     assertTofuOutput(
-        "a%bc%20%3E%20d",
+        "a%25bc%20%3E%20d",
         UnsafeSanitizedContentOrdainer.ordainAsSafe("a%bc > d", SanitizedContent.ContentKind.URI),
         escapeUri);
 
@@ -170,7 +172,8 @@ public final void testEscapeUri() {
         .addTest("a%25b%20%3E%20c", " 'a%b > c' ", escapeUri)
         .addTest("a%25bc%20%3E%20d",
             "soydata.VERY_UNSAFE.ordainSanitizedHtml('a%bc > d')", escapeUri)
-        .addTest("a%bc%20%3E%20d", "soydata.VERY_UNSAFE.ordainSanitizedUri('a%bc > d')", escapeUri)
+        .addTest("a%25bc%20%3E%20d", "soydata.VERY_UNSAFE.ordainSanitizedUri('a%bc > d')",
+            escapeUri)
         .runTests();
   }
 

java/tests/com/google/template/soy/shared/restricted/SanitizersTest.java

@@ -265,12 +265,11 @@ public final void testEscapeUri() {
     assertEquals("%EF%BC%83%EF%BC%9A", Sanitizers.escapeUri("\uff03\uff1a"));
     // Test other unicode codepoints.
     assertEquals("%C2%85%E2%80%A8", Sanitizers.escapeUri("\u0085\u2028"));
-    // Test Sanitized Content of the right kind. Note that some characters are still normalized --
-    // specifically, ones that are allowed in URI's but not attributes but don't change meaning
-    // (and parentheses since they're technically reserved).
-    assertEquals("foo%28%27&%27%29", Sanitizers.escapeUri(
+    // SanitizedUris are not special in URIs. For example, in /redirect?continue={$url}, we clearly
+    // don't want ampersands in the continue URL to break out of the continue param.
+    assertEquals("foo%20%28%2527%26%27%29", Sanitizers.escapeUri(
         UnsafeSanitizedContentOrdainer.ordainAsSafe(
-            "foo(%27&')", SanitizedContent.ContentKind.URI)));
+            "foo (%27&')", SanitizedContent.ContentKind.URI)));
     // Test SanitizedContent of the wrong kind -- it should be completely escaped.
     assertEquals("%2528%2529", Sanitizers.escapeUri(
         UnsafeSanitizedContentOrdainer.ordainAsSafe(

javascript/soyutils_usegoog.js

@@ -1395,13 +1395,12 @@ soy.$$pctEncode_ = function(ch) {
  * @return {string} An escaped version of value.
  */
 soy.$$escapeUri = function(value) {
-  if (soydata.isContentKind(value, soydata.SanitizedContentKind.URI)) {
-    goog.asserts.assert(value.constructor === soydata.SanitizedUri);
-    return soy.$$normalizeUri(value);
-  }
-  if (value instanceof goog.html.SafeUrl) {
-    return soy.$$normalizeUri(goog.html.SafeUrl.unwrap(value));
-  }
+  // NOTE: We don't check for SanitizedUri or SafeUri, because just because
+  // something is already a valid complete URL doesn't mean we don't want to
+  // encode it as a component.  For example, it would be bad if
+  // ?redirect={$url} didn't escape ampersands, because in that template, the
+  // continue URL should be treated as a single unit.
+
   // Apostophes and parentheses are not matched by encodeURIComponent.
   // They are technically special in URIs, but only appear in the obsolete mark
   // production in Appendix D.2 of RFC 3986, so can be encoded without changing

python/sanitize.py

@@ -161,9 +161,6 @@ def escape_js_value(value):
 
 
 def escape_uri(value):
-  if is_content_kind(value, CONTENT_KIND.URI):
-    return normalize_uri(value)
-
   return generated_sanitize.escape_uri_helper(value)