Revert PR #171 which auto decodes URL parameters

[ThomasTJdev]

Request to roll back commit a03b5be0b72f1a686b6ca594c145cee665e6db7b which implemented PR #171 which fixed Issue #159.

Scope

The commit changed the behaviour the template @ used to access URL parameters and is noted as a Breaking change. The change auto-decodes the URL parameter using decodeUrl() regardless of the parameter-type.

Exact change: https://github.com/dom96/jester/pull/171/files#diff-6a259fda1df185e57d46cf13c94d178eb6be365815acdf3dc8a132ab58a63f61L640-R640

Problem

When using Javascript to perform POST-request the data will/could be wrongly decoded.

Basic examples, but imagine this being Base64, JSON or other encoded data:

# A1
fetch("/api", {
  method: "POST",
  body: new URLSearchParams({ msg: "My message with plus+++signs" })
});

# A2
$.ajax({
  type: "POST",
  url: "/api",
  data: { msg: "My message with plus+++signs" }
});

import jester
routes:
  post "/api":
    echo @"msg"
    # Actual: My message with plus   signs
    # Expected: My message with plus+++signs

When parsing the the above POST-request with Jester the parameters will be returned after the decodeUrl() and therefor stripped for the +-signs, which in this case is required.

Actual My message with plus signs

Expected result My message with plus+++signs

Suggestion

Roll this commit back and let the URL param be parsed by the end user. In some cases they need to be parsed as traditional URL parameter (example.com?project=hello) or as Form data provided by a Javascript function.

1. Roll back the commit
2. Provide a compile option to opt out

If accepted then I'll be happy to provide the PR.

[dom96]

What does Sinatra do in this case?

[ThomasTJdev]

With Sinatra it differs whether it is pure URL parameter vs Javascript value with POST-request. On GET-request it parses like decodeUrl().

Minimal example with Sinatra

Sinatra backend

require 'sinatra'

post '/' do
  author = params['author']
  msg = params["msg"]

  puts author
  puts msg
  author
end

Javascript frontend

# B1
fetch("/?author=Message with plus+++signs", {
    method: "POST",
    body: new URLSearchParams( { msg: "Message with plus+++signs" })
})

Output on backend The above Javascript (B1) uses inline URL parameters and formatted body. The server echo (puts) these values:

127.0.0.1 - - [23/May/2023:16:02:36 +0200] "POST /?author=Message%20with%20plus+++signs HTTP/1.1" 200 25 0.0034
Message with plus   signs  <== This is the @author
Message with plus+++signs  <== This is the @msg

What does Sinatra?

I'm no Ruby man.. I spent some time going through the code, but with no direct solution. is_frozen, HEADER_PARAMS..

[dom96]

With Sinatra it differs whether it is pure URL parameter vs Javascript value with POST-request

Sounds like that is what we should do then :)

[ThomasTJdev]

With Sinatra it differs whether it is pure URL parameter vs Javascript value with POST-request

Sounds like that is what we should do then :)

Hehe, got you ;) .

Core problem

So, in Jester it all boils down to that non-URL params is already decoded within parseUrlQuery(). So when also decoding within the @-template we double decode - and that's some mess.

https://github.com/dom96/jester/blob/88dad038bc16b228d206c90756906806e7020c95/jester/private/utils.nim#L39

Approaches

I have tried three approaches. But both 1 and 2 have breaking changes...

First try

1. Solution (1) changes how we transport the params around. Instead of a Table[string, string] we now use var Table[string, tuple[value: string, frozen: bool]]. By doing this we identify the frozen parameters when using @. BUT now we will break code with:

   for k, v in request.params:
   # => Currently just a string
   # => New: We now have tuple[string, bool]

https://github.com/dom96/jester/compare/master...ThomasTJdev:jester:decodeUrlParms

Second try

2. Solution (2) removes the default decoding of non-URL params. Then we can always use @ since nothing is decoded as default. BUT this introduces a breaking change for code with:

   for k, v in request.params:
   # => `v` is not decoded, so users have to decode the data

https://github.com/dom96/jester/compare/master...ThomasTJdev:jester:decodeUrlParms2

Third try

3. Solution (3) just ensures that everything is decoded - even though it is not needed. We currently decode() non-URL params by default, so this code then just default decode() URL-params too. This has no breaking changes but adds an overhead with the decodeUrl().

EDIT This is breaking change for users accessing the raw parameters. Example:

Current

  var params: string
  for k, v in c.req.params:
    if params != "":
      params.add("&")
    params.add(k & "=" & v)

New

  var params: string
  for k, v in c.req.params:
    if params != "":
      params.add("&")
    params.add(k & "=" & encodeUrl(v))

https://github.com/dom96/jester/compare/master...ThomasTJdev:jester:decodeUrlParms3

[dom96]

Wow, very thorough. Thank you. I think the "Third try" is the best, but I don't have strong feelings to be honest :)

[ThomasTJdev]

I care a lot about your library here :) ! Third option is added as a PR.