michalmuskala / jason

A blazing fast JSON parser and generator in pure Elixir.
Other
1.6k stars 170 forks source link

Parsing breaks with "unexpected byte at position ..." when string value contains newline #103

Closed hubertlepicki closed 4 years ago

hubertlepicki commented 4 years ago

I am using Jason 1.1.2 on Ubuntu Linux, and this is the behavior that works:

iex(15)> Jason.decode!("{\"message\": \"no newline ok\"}")  
%{"message" => "no newline ok"}

Yet, if a message I receive contains a newline, suddenly the decoding breaks:

iex(16)> Jason.decode!("{\"message\": \"newline\n not ok\"}")
** (Jason.DecodeError) unexpected byte at position 13: 0x6E ('n')
    (jason 1.1.2) lib/jason.ex:78: Jason.decode!/2

Switching to Poison fixes the issue:

Poison.decode!("{\"message\": \"newline\n not ok\"}")
%{"message" => "newline\n not ok"}

I think Jason expects the newline to be additionally escaped, because this works the same in Jason and Poison:

Jason.decode!("{\"message\": \"newline\\n not ok\"}")
%{"message" => "newline\n not ok"}

It may very well be that the newline character has to be escaped in JSON strings but that's not what I am getting from external source.

Is this a bug in Jason or I need to look for a workaround outside?

ericmj commented 4 years ago

According to the JSON specification all control characters need to be escaped inside strings.

hubertlepicki commented 4 years ago

@ericmj I did suspect that this is the case, yet we don't live in ideal world and often source of JSON may not entirely comply with the spec.

Poison seems to be handling that case and I was wondering if Jason shouldn't do it too.

In my very case it's the source of this broken JSON is Mailgun webhook callbacks, but I can't imagine they are the only ones doing that.

OvermindDL1 commented 4 years ago

I would personally opt for following the spec to the letter. A preprocessing step can always escape control characters before passing it to Jason. It's also not hard to fork Jason into your own project to make some minor changes. :-)

michalmuskala commented 4 years ago

The biggest risk I see with relaxing the spec here the fact that different parsers could produce different output. There are known security vulnerabilities caused by those differences in other areas (order of elements in objects), so I would be hesitant here without a good understanding that this can't cause similar issues. Especially that "newline-delimited JSON" is an existing format that relies on that property that "raw" newlines are invalid inside JSON.

hubertlepicki commented 4 years ago

Alright, ok I think it makes sense to close it then without fixing. As I said, I wasn't sure if that should be handled. Poison does, Jason does not.

Anyway I am sure someone in the future will stumble upon this issue and get their problem solved (or atleast understood) so not an entirely wasted effort.

hubertlepicki commented 4 years ago

Just an update in case someone stumbles upon this too: we have never solved the external service sending us invalid JSON. I don't have great solution either, but I came up with a hack that seems to work: a simple proxy which tries Jason and if exception happens retries using Poison. This makes 99% of our requests go to Jason, and the faulty 1% goes to Poison.

defmodule JasonWithPoisonFallback do
  @moduledoc """
  A JSON parser wrapper which *on decoding only* first tries Jason,
  if it crashes falls back to Poison.

  It uses Jason only for encoding.
  """

  def decode(input, opts \\ []) do
    case Jason.decode(input, opts) do
      {:ok, value} -> {:ok, value}
      _ -> Poison.decode(input, opts)
    end
  end

  def decode!(input, opts \\ []) do
    try do
      Jason.decode!(input, opts)
    catch
      _kind, _error ->
        Poison.decode!(input, opts)
    end
  end

  defdelegate encode(term, opts \\ []), to: Jason
  defdelegate encode!(term, opts \\ []), to: Jason
  defdelegate encode_to_iodata(input, opts \\ []), to: Jason
  defdelegate encode_to_iodata!(input, opts \\ []), to: Jason
end

and then set it up to be used by Phoenix:

config :phoenix, :json_library, JasonWithPoisonFallback
bitboxer commented 3 years ago

Sadly this won't work anymore with poison 4.x. Poison now also follows the spec.