fix: parse boolean values in URL query parameters

[glowcloud]

Refs #9674

[heldersepu]

Was trying to test this and I realised that the URL params are not enabled by default:

• https://raw.githack.com/swagger-api/swagger-ui/issue-9674/dist/index.html?docExpansion=None&defaultModelsExpandDepth=None

• https://raw.githack.com/swagger-api/swagger-ui/issue-9674/dist/index.html?syntaxHighlight.activated=false

Any reason why is that not enabled by default?

seems the petstore has it:
https://petstore.swagger.io/swagger-initializer.js (queryConfigEnabled: true)
but not in the code:
https://github.com/swagger-api/swagger-ui/blob/master/dist/swagger-initializer.js

[heldersepu]

Added fix to my fork, it works as intended:
https://raw.githack.com/heldersepu/swagger-ui/test/dist/index.html?docExpansion=None&defaultModelsExpandDepth=None&syntaxHighlight.activated=false#/pet/findPetsByStatus

[heldersepu]

This is such an easy fix ... Why does it takes so long to merge?

[char0n]

Hi @heldersepu,

Any reason why is that not enabled by default?

Please have a look at GHSA-qrmm-w75w-3wpx

Long story short, it's a security concern, and there must be an explicit decision to enable the query config. We've explicitly decided to enable it on deployments we control like petstore.

[char0n]

This is such an easy fix ... Why does it takes so long to merge?

Please be patient. This is huge and complex software and it's just one piece in the entire Swagger ecosystem. We expedite features and bug-fixes with the speed that matches our resources.

[char0n]

I'm suggesting to rewrite the functions into following form:

export const parseSearch = () => {
  const searchParams = new URLSearchParams(win.location.search)

  if (searchParams.size === 0) return {}

  for (const [key, value] of searchParams) {
    searchParams.set(key, value  === 'true' ? true : value === 'false' ? false : value)
  }

  return Object.fromEntries(searchParams)
}

export const serializeSearch = (searchMap) => {
  const searchParams = new URLSearchParams(Object.entries(searchMap))
  return String(searchParams)
}

Explanation: instead of using custom search params parsing, the functions now use standardized WHATWG spec compliant parser.

[char0n]

But still IMHO we shouldn't be doing this in this function as this function was designed as general purpose search parameters parser. This should be done only depending on the context. Why only handle boolean if there is also a number and null ?

for (const [key, value] of searchParams) {
    searchParams.set(key, value  === 'true' ? true : value === 'false' ? false : value)
}

[glowcloud]

I'm suggesting to rewrite the functions into following form:

export const parseSearch = () => {
  const searchParams = new URLSearchParams(win.location.search)

  if (searchParams.size === 0) return {}

  for (const [key, value] of searchParams) {
    searchParams.set(key, value  === 'true' ? true : value === 'false' ? false : value)
  }

  return Object.fromEntries(searchParams)
}

export const serializeSearch = (searchMap) => {
  const searchParams = new URLSearchParams(Object.entries(searchMap))
  return String(searchParams)
}

Explanation: instead of using custom search params parsing, the functions now use standardized WHATWG spec compliant parser.

I see that two unit tests fail with this: the one I added in this PR and:

  it("encode url components", () => {
        expect(serializeSearch({foo: "foo bar"})).toEqual("foo=foo%20bar")
      })

Will check out how to modify these to make it work.

[char0n]

I see that two unit tests fail with this: the one I added in this PR and:

Yes because the encoding in the test is no longer compatible with WHATWG spec and how url search params SHOULD be encoded apart from how path segment is encoded. To say it simply - how the path and search params are encoded diverged in WHATWG spec.

[glowcloud]

But still IMHO we shouldn't be doing this in this function as this function was designed as general purpose search parameters parser. This should be done only depending on the context. Why only handle boolean if there is also a number and null ?

for (const [key, value] of searchParams) {
    searchParams.set(key, value  === 'true' ? true : value === 'false' ? false : value)
}

Yes, I think we should also handle these. For number it can seem like there's no issue there, for example with defaultModelExpandDepth but in our prop types we expect a number so we get this error (only in dev):

Warning: Failed prop type: Invalid prop `expandDepth` of type `string` supplied to `Model`, expected `number`.

The setting will work properly in this case, I'm guessing because of type coercion, here when I set it to 1:

[char0n]

But still IMHO we shouldn't be doing this in this function as this function was designed as general purpose search parameters parser. This should be done only depending on the context. Why only handle boolean if there is also a number and null ?

What I mean by this is that we shouldn't actually do it at all (inside the parseSearch function). I'll reiterate that the parseSearch is a general purpose function and is used in multiple places in the codebase. And depending where it's used, we apply coercion differently. Converting string to number is very complex task; well not the conversion but rather detection what possibly is a number.

[char0n]

We can do this as two PRs. First PR is aligning those two functions with WHATWG spec. The seconds one would actually do the context dependent coercion somewhere in

swagger-ui/src/core/index.js

Line 139 in 3b72ee1

let queryConfig = opts.queryConfigEnabled ? parseSearch() : {}

[glowcloud]

Reverted the changes to parseSearch and added new functions for conversion.

I went through the config options and hopefully didn't miss anything. I skipped the options that allow functions.

Also tested it manually and an example for these options:

?deepLinking=true&defaultModelExpandDepth=5&syntaxHighlight={"theme":"monokai"}&supportedSubmitMethods=["get"]

We can see that:

• try it out is not enabled for the POST request, is enabled for the GET request
• syntax highlight theme is set to monokai
• model is expanded according to defaultModelExpandDepth

Deep linking also works, example URL:

http://localhost:3200/?deepLinking=true&defaultModelExpandDepth=5&syntaxHighlight={%22theme%22:%22monokai%22}&supportedSubmitMethods=[%22get%22]#/pet/updatePet

Let me know if it's intended to support arrays and objects in the URL as well.

[glowcloud]

Superseded by #9829