Repository: bazelbuild/bazel
Issue: struct.to_json doesn't escape keys
Labels: type: bug
P3
good first issue

### Description of the problem / feature request:

struct.to_json doesn't escape keys.

### Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

```python
print(struct(**{ "\"": 1 }).to_json()) # outputs {""":1}
```

### What operating system are you running Bazel on?

macOS

### What's the output of `bazel info release`?

release 0.27.0

###  Have you found anything relevant by searching the web?

N/A

### Any other information, logs, or outputs that you want to share?

https://github.com/bazelbuild/bazel/blob/a3421e204f22e478382bb8487b96df9dcd8e463a/src/main/java/com/google/devtools/build/lib/packages/StructImpl.java#L300

Keys are not escaped.

Issue comments:

Pull request review comments:
This is a new dependency, and it's totally unnecessary. There is already code in StructImpl.java to escape Java strings into JSON notation (see L319); please re-use it.
Don't use String.valueOf(x) here; it's equivalent to x.toString() except when x is null, which can't happen because this statement is dominated by the check on L291 (and even if it could, I would prefer to see a crash in that case than to silently emit incorrect JSON).

Use: (String) entry.getKey()
Could you also apply escaping to the field names on L280? Field names should be valid identifiers, but this is not guaranteed, and we don't want to emit corrupt JSON in that case. 

At that point, there are three places that emit quote escape(s) quote; could you refactor jsonEscapeString to accept a StringBuilder and emit the quotes too? Thanks.
Sounds good.
Done.
The appendColon logic has no business being in here, as it can be composed separately an no extra cost, resulting in a simpler function.
If you like, use append('"') to avoid the internal loop.
Let's call this function appendJSONStringLiteral, the StringBuilder 'out', and the String s.
Sounds good.
Not sure I completely understand your suggestion. Was my change what you had in mind?
Done.

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/packages/StructImpl.java b/src/main/java/com/google/devtools/build/lib/packages/StructImpl.java
index 0b0cc35526cd19..5030af2ca86f6e 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/StructImpl.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/StructImpl.java
@@ -270,9 +270,8 @@ private static void printJson(Object value, StringBuilder sb, String container,
       for (String field : ((ClassObject) value).getFieldNames()) {
         sb.append(join);
         join = ",";
-        sb.append("\"");
-        sb.append(field);
-        sb.append("\":");
+        appendJSONStringLiteral(sb, field);
+        sb.append(':');
         printJson(((ClassObject) value).getValue(field), sb, "struct field", field);
       }
       sb.append("}");
@@ -289,9 +288,8 @@ private static void printJson(Object value, StringBuilder sb, String container,
               container,
               key != null ? " '" + key + "'" : "");
         }
-        sb.append("\"");
-        sb.append(entry.getKey());
-        sb.append("\":");
+        appendJSONStringLiteral(sb, (String) entry.getKey());
+        sb.append(':');
         printJson(entry.getValue(), sb, "dict value", String.valueOf(entry.getKey()));
       }
       sb.append("}");
@@ -305,9 +303,7 @@ private static void printJson(Object value, StringBuilder sb, String container,
       }
       sb.append("]");
     } else if (value instanceof String) {
-      sb.append("\"");
-      sb.append(jsonEscapeString((String) value));
-      sb.append("\"");
+      appendJSONStringLiteral(sb, (String) value);
     } else if (value instanceof Integer || value instanceof Boolean) {
       sb.append(value);
     } else {
@@ -318,10 +314,12 @@ private static void printJson(Object value, StringBuilder sb, String container,
     }
   }
 
-  private static String jsonEscapeString(String string) {
-    return escapeDoubleQuotesAndBackslashesAndNewlines(string)
+  private static void appendJSONStringLiteral(StringBuilder out, String s) {
+    out.append('"');
+    out.append(escapeDoubleQuotesAndBackslashesAndNewlines(s)
         .replace("\r", "\\r")
-        .replace("\t", "\\t");
+        .replace("\t", "\\t"));
+    out.append('"');
   }
 
   @Override


Test patch: