jsonapi.rb icon indicating copy to clipboard operation
jsonapi.rb copied to clipboard

request.fullpath is not unescaped when used with grape and swagger ui

Open code-bunny opened this issue 10 months ago • 3 comments

Looks like we are hoping the request.full_path is good by default for returning. However I've found some libraries and especially calls from swagger ui will escape query strings. This means when we are creating links they'll come in their escaped form into the response:

  "links": {
    "self": "http://localhost:3000/api/bundles?page%5Bnumber%5D=2",
    "current": "http://localhost:3000/api/bundles?page[number]=2",
    "first": "http://localhost:3000/api/bundles?page[number]=1",
    "prev": "http://localhost:3000/api/bundles?page[number]=1"
  }

This appears to only be present in the pagination and could be corrected by just wrapping it in a CGI.unescape block.

    # Generates the pagination links
    #
    # @return [Array]
    def jsonapi_pagination(resources)
      links = { self: request.base_url + CGI.unescape(request.fullpath) }
      pagination = jsonapi_pagination_meta(resources)

Before opening a PR, is there a specific reason we wouldn't want to CGI.unescape a path here?

code-bunny avatar Jan 19 '25 22:01 code-bunny

Before opening a PR, is there a specific reason we wouldn't want to CGI.unescape a path here?

Seems like a valid inconsistency to me and all of these should be escaped. Checked the JSONAPI forums and it looks like we need to escape these

https://discuss.jsonapi.org/t/should-be-escaped-in-response-links/795/3

Appreciate any PRs on this

stas avatar Jan 20 '25 13:01 stas

Reading that guideline it looks like we should go for readable first, so unescaping the request.fullpath from the pagination%5Bpage%5D to pagination[page] (inline with the other outputs) is indeed the right path. I'll get a PR together for you soon.

code-bunny avatar Jan 20 '25 13:01 code-bunny

@stas I've put in a preliminary PR for this as the specs don't currently run on older versions of Ruby/Rails. No Gemfile.lock is committed to the repo so we canny tell the exact dependences you want to run against.

code-bunny avatar Jan 20 '25 14:01 code-bunny