inthefabric / RexConnect

Connect to Rexster/RexPro via TCP connection.
www.inthefabric.com
Other
10 stars 0 forks source link

Incorrect response JSON in exception scenarios #2

Closed zachkinstner closed 11 years ago

zachkinstner commented 11 years ago

Discovered this issue while creating issue https://github.com/inthefabric/Fabric/issues/3.

The "exception" item is wrapped in single quotes:

{
  "request":"20ce544b-6fca-45ed-bb1e-b44d74edb1cd",
  "success":false,
  "queryTime":15492,
  "exception":'An error occurred while processing the script for language [groovy]. All transactions across all graphs in the session have been concluded with failure: javax.script.ScriptException: com.thinkaurelius.titan.core.TitanException: Could not read from storage'
}
zachkinstner commented 11 years ago

The issue occurs in CommandHandler on line 82:

"\"exception\":'"+(msg == null ? e.toString() : msg.replace('\'', '"'))+"'"+
gmaurice commented 11 years ago

Hi, unfortunaltly there are some characters that make issues. Having double-quotes and new lines non escaped will make the returned JSON not compliant to JSON decode. This line will do it :

Finally, we can keep simple quotes because it's not a problem for the JSON decode.

I tested it with this exception :

  • Details: com.tinkerpop.rexster.client.RexProException: An error occurred while processing the script for language > [groovy]. All transactions across all graphs in the session have been concluded with failure: org.codehaus.groovy.control.MultipleCompilationErrorsException: startup failed: Script100.groovy: 1: expecting EOF, found 'iE' @ line 1, column 226. 013-03-26; it.to=2013-04-04};}iE=v1.inE( ^

1 error

zachkinstner commented 11 years ago

Thanks for the bug report, @gmaurice. I can see that my JSON creation is over-simplified in general. The code should escape illegal chars for the "exception" property, but also for the "request" and "result" properties. All have the potential to break the JSON formatting.

My initial purpose here was to be as fast as possible -- I wanted to build the simple JSON string directly, rather than sending it to a JSON serializer. I'm thinking now that I should just use Jackson to serialize this data into JSON. RexConnect already uses Jackson to deserialize parameter data.

zachkinstner commented 11 years ago

RexConnect now uses the Jackson ObjectMapper to build the response JSON. This approach seems cleaner and should resolve any formatting issues vs. the previous direct JSON creation.

A simple test using a request with a dangerous-looking ID:

123'456"789&abc$efg\n0#g

Now returns a correctly-escaped response:

{
  "results":"[\"titangraph[local:data/FabricTest]\"]",
  "request":"123'456\"789&abc$efg\\n0",
  "queryTime":7,
  "success":true
}

Fix available in my next commit.