Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Jason Instead of JSX #179

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

jherdman
Copy link
Contributor

Given that the broader Elixir community is circling their wagons around
Jason, and issue #153, it seems that moving to Jason would be a wise
choice.

NOTE: I'm having some challenges with the test suite that I hope to iron out. This should be considered draft. Help is more than welcome.

@jherdman
Copy link
Contributor Author

One failure left:

  1) test hackney request with ssl options (ExVCR.RecorderHackneyTest)
     test/recorder_hackney_test.exs:157
     ** (Jason.EncodeError) invalid byte 0x82 in <<48, 130, 2, 11, 48, 130, 1, 145, 160, 3, 2, 1, 2, 2, 18, 17, 210, 187, 186, 51, 110, 212, 188, 230, 36, 104, 197, 13, 132, 29, 152, 232, 67, 48, 10, 6, 8, 42, 134, 72, 206, 61, 4, 3, 3, 48, 70, 49, 11, 48, ...>>
     code: use_cassette "record_hackney_with_ssl_options" do
     stacktrace:
       lib/jason.ex:199: Jason.encode_to_iodata!/2
       (exvcr 0.13.2) lib/exvcr/json.ex:14: ExVCR.JSON.save/2
       test/recorder_hackney_test.exs:158: ExVCR.RecorderHackneyTest."-test hackney request with ssl options/1-after$^0/0-0-"/1
       test/recorder_hackney_test.exs:158: (test)

@jherdman
Copy link
Contributor Author

Failing test can be run via mix test --only wip

@swissbeats93
Copy link

swissbeats93 commented Aug 29, 2021

@jherdman I don't have time to test this at the moment but looking through the Jason documentation. More specifically, this part:

`
The generation is controlled by the Jason.Encoder protocol, please refer to the module to read more on how to define the protocol for custom data types.

Options
:escape - controls how strings are encoded. Possible values are:

:json (default) - the regular JSON escaping as defined by RFC 7159.
:javascript_safe - additionally escapes the LINE SEPARATOR (U+2028) and PARAGRAPH SEPARATOR (U+2029) characters to make the produced JSON valid JavaScript.
:html_safe - similar to :javascript_safe, but also escapes the / character to prevent XSS.
:unicode_safe - escapes all non-ascii characters.
`

We need to specify a non-default escape option so that encode! does not raise an error.
Edit: This reinforces the idea that it is an encoding problem

@jherdman
Copy link
Contributor Author

@swissbeats93 thanks for the reply! Yeah, I tried those various options but with no luck. I'll give that forum post a better read ASAP.

Given that the broader Elixir community is circling their wagons around
Jason, and issue parroty#153, it seems that moving to Jason would be a wise
choice.
test "HTTPoison with ssl options" do
use_cassette "record_hackney_with_ssl_options" do
response =
HTTPoison.post!("https://example.com", {:form, []}, [], ssl: [{:versions, [:"tlsv1.2"]}])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this for two reasons:

  1. Jason Encoder needs to be set up for this form of options
  2. This should fail the same as L157 given that HTTPoison uses hackney under the hood

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 @jherdman

Sorry to bother you over an old PR. But I've been looking at it and it seems like the request here is different (according to a rexbug trace):

:hackney.request(:post, "https://example.com", [], {:form, []}, [ssl_options: [versions: [:"tlsv1.2"]]])

whereas in the failing test it's

opts = [
  customize_hostname_check: [
    match_fun: #Function<6.29392970/2 in :public_key.pkix_verify_hostname_match_fun/1>
  ],
  server_name_indication: 'localhost',
  verify: :verify_peer,
  depth: 100,
  cacerts: [
    <<48, 130, 2, 11, 48, 130, 1, 145, 160, 3, 2, 1, 2, 2, 18, 17, 210, 187,
      186, 51, 110, 212, 188, 230, 36, 104, 197, 13, 132, 29, 152, 232, 67, 48,
      10, 6, 8, 42, 134, 72, 206, 61, 4, 3, 3, 48, 70, 49, 11, 48, 9, 6, 3, 85,
      4, 6, 19, 2, 66, 69, 49, 25, 48, 23, 6, 3, 85, 4, 10, 19, 16, 71, 108,
      111, 98, 97, 108, 83, 105, 103, 110, 32, 110, 118, 45, 115, 97, 49, 28,
      48, 26, 6, 3, 85, 4, 3, 19, 19, 71, 108, 111, 98, 97, 108, 83, 105, 103,
      110, 32, 82, 111, 111, 116, 32, 69, 52, 54, 48, 30, 23, 13, 49, 57, 48,
      51, 50, 48, 48, 48, 48, 48, 48, 48, 90, 23, 13, 52, 54, 48, 51, 50, 48,
      48, 48, 48, 48, 48, 48, 90, 48, 70, 49, 11, 48, 9, 6, 3, 85, 4, 6, 19, 2,
      66, 69, 49, 25, 48, 23, 6, 3, 85, 4, 10, 19, 16, 71, 108, 111, 98, 97,
      108, 83, 105, 103, 110, 32, 110, 118, 45, 115, 97, 49, 28, 48, 26, 6, 3,
      85, 4, 3, 19, 19, 71, 108, 111, 98, 97, 108, 83, 105, 103, 110, 32, 82,
      111, 111, 116, 32, 69, 52, 54, 48, 118, 48, 16, 6, 7, 42, 134, 72, 206,
      61, 2, 1, 6, 5, 43, 129, 4, 0, 34, 3, 98, 0, 4, 156, 14, 177, 207, 183,
      232, 158, 82, 119, 117, 52, 250, 165, 70, 167, 173, 50, 25, 50, 180, 7,
      169, 39, 202, 148, 187, 12, 210, 10, 16, 199, 218, 137, 176, 151, 12, 112,
      19, 9, 1, 142, 216, 234, 71, 234, 190, 178, 128, 43, 205, 252, 40, 13,
      219, 172, 188, 164, 134, 55, 237, 112, 8, 0, 117, 234, 147, 11, 123, 46,
      82, 156, 35, 104, 35, 6, 67, 236, 146, 47, 83, 132, 219, 251, 71, 20, 7,
      232, 95, 148, 103, 93, 201, 122, 129, 60, 32, 163, 66, 48, 64, 48, 14, 6,
      3, 85, 29, 15, 1, 1, 255, 4, 4, 3, 2, 1, 134, 48, 15, 6, 3, 85, 29, 19, 1,
      1, 255, 4, 5, 48, 3, 1, 1, 255, 48, 29, 6, 3, 85, 29, 14, 4, 22, 4, 20,
      49, 10, 144, 143, 182, 198, 157, 210, 68, 75, 128, 181, 162, 230, 31, 177,
      18, 79, 27, 149, 48, 10, 6, 8, 42, 134, 72, 206, 61, 4, 3, 3, 3, 104, 0,
      48, 101, 2, 49, 0, 223, 84, 144, 237, 155, 239, 139, 148, 2, 147, 23, 130,
      153, 190, 179, 158, 44, 246, 11, 145, 140, 159, 74, 20, 177, 246, 100,
      188, 187, 104, 81, 19, 12, 3, 247, 21, 139, 132, 96, 185, 139, 255, 82,
      142, 231, 140, 188, 28, 2, 48, 60, 249, 17, 212, 140, 78, 192, 193, 97,
      194, 21, 76, 170, 171, 29, 11, 49, 95, 59, 28, 226, 0, 151, 68, 49, 230,
      254, 115, 150, 47, 218, 150, 211, 254, 8, 7, 179, 52, 137, 188, 5, 159,
      247, 30, 134, 238, 139, 112>>,
     # etc.
  ],
  partial_chain: #Function<0.89509999/1 in :hackney_ssl.partial_chain>,
  verify_fun: {&:ssl_verify_hostname.verify_fun/3, [check_hostname: 'localhost']}
]

:hackney.request(:post, "http://localhost:1234/server", [], [], opts)

and the :cacerts option is what makes it fail.


In non-strict mode jsx seems to escape invalid bytes with :

https://github.com/talentdeficit/jsx/blob/1d3407aa9752430ec0a06111ac1b046ffafdca22/src/jsx_parser.erl#L608C7-L608C14

Here's what the encoded request for the failing test looks like. This is a data loss, so I wonder if request.options is actually used anywhere in ExVCR's logic.

Anyway, we can preprocess the data in the Jason impl for ExVCR.Request to escape all invalid bytes in options with .

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to work:

defmodule ExVCR.Request do
  defstruct url: nil, headers: [], method: nil, body: nil, options: [], request_body: ""

  defimpl Jason.Encoder do
    def encode(request, opts) do
      request
      |> Map.update!(:headers, &Map.new/1)
      |> Map.update!(:options, fn options -> options |> clean_invalid() |> Map.new() end)
      |> Map.take([:url, :headers, :method, :body, :options, :request_body])
      |> Jason.Encode.map(opts)
    end

    defp clean_invalid([{_, _} | _] = kw) do
      kw |> Enum.map(fn {k, v} -> {k, clean_invalid(v)} end) |> Map.new()
    end

    defp clean_invalid([value | rest]) do
      [clean_invalid(value) | clean_invalid(rest)]
    end

    defp clean_invalid([] = empty), do: empty

    defp clean_invalid(map) when is_map(map) do
      map |> Map.to_list() |> clean_invalid() |> Map.new()
    end

    defp clean_invalid(tuple) when is_tuple(tuple) do
      tuple |> Tuple.to_list() |> clean_invalid() |> List.to_tuple()
    end

    defp clean_invalid(bin) when is_binary(bin) do
      clean_bin(bin)
    end

    defp clean_invalid(other), do: other

    defp clean_bin(<<b::utf8, rest::bytes>>), do: <<b::utf8>> <> clean_bin(rest)
    defp clean_bin(<<_invalid, rest::bytes>>), do: "�" <> clean_bin(rest)
    defp clean_bin(<<>> = empty), do: empty
  end
end

Resulting JSON: https://gist.github.com/ruslandoga/be54ef0cbcd69bdb66b01a5e9bd7a062

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's OK, I'll try to finish up this PR in #206

@jherdman
Copy link
Contributor Author

I haven't the time to get this over the hump. If anyone is interested in taking this over please feel free.

@ruslandoga ruslandoga mentioned this pull request Jun 23, 2023
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants