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

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

Open
code-bunny opened this issue Jan 19, 2025 · 3 comments
Open

Comments

@code-bunny
Copy link

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?

@stas
Copy link
Owner

stas commented Jan 20, 2025

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

@code-bunny
Copy link
Author

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
Copy link
Author

@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.

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

No branches or pull requests

2 participants