Page MenuHomePhabricator

Encoding JSON for presentation could use more human-friendly escaping rules
Open, WishlistPublic

Description

Hello! I think I've found an issue in the Instance Configuration page of Phacility Admin. I tried to set the feed.http_hooks setting to the following:

["https://ever-phab-slack.herokuapp.com"]

However, when I pressed save, it appeared like this:

["https:\/\/ever-phab-slack.herokuapp.com"]

Luckily, the correct (non-escaped) value got sent to my Phabricator instance, and the HTTP hook is working properly. Thus, it appears to be merely a cosmetic issue in Phacility Admin.

Event Timeline

cmelbye created this task.Jul 27 2015, 6:09 PM
cmelbye raised the priority of this task from to Needs Triage.
cmelbye updated the task description. (Show Details)
cmelbye added a project: Phacility Support.
cmelbye added a subscriber: cmelbye.

This is a valid, compliant JSON-encoding of the string, which allows / to be escaped -- here's the grammar diagram from json.org:

The reason some encoders (including the builtin PHP encoder, which we use) escape this character is to make sure the string </script> appears as <\/script> in output. This defuses some browser vulnerabilities where an attacker could enter, say, a comment like </script><script>..., get that JSON encoded and echoed into the page, and then trick browsers into interpreting the page source as an abruptly-terminated script tag with a syntax error followed by an attacker-controlled script tag with nearly-arbitrary contents.

This encoding isn't desirable when encoding for human users, and we should probably use alternate encoding rules which prevent backslashes from being escaped in these contexts, but the encoding is proper and valid.

epriestley renamed this task from Instance Configuration Improperly Escapes Characters to Encoding JSON for presentation could use more human-friendly escaping rules.Jul 27 2015, 9:28 PM
epriestley triaged this task as Wishlist priority.
rexzor added a subscriber: rexzor.Jul 28 2015, 5:57 AM
rexzor removed a subscriber: rexzor.
chad added a subscriber: chad.Aug 9 2015, 4:11 AM

Is this complete?

The specific UI (PhacilityInstances(Choose an Instance)Adjust Configuration) in question doesn't actually use PhutilJSON to format the output right now, so it's still rendering the less-preferred form:

I need to swap that over at some point.

epriestley added a subscriber: utrack.

T10358 discusses a case where we use (valid, spec-compliant) JSON for presentation, but could use an alternate form which is more human readable (specifically, when displaying unicode characters).

epriestley moved this task from Backlog to Future on the Customer Impact board.Apr 12 2017, 3:11 PM