PathParameterShouldBeInThePath false positive

[davidrea-MS]

Describe the bug
OpenApiParameterRules.PathParameterShouldBeInThePath, compares the context.PathString, which is uses the JSON pointer-encoded path segments, to the unencoded parameter.Name.
This means that a path parameter which contains a / character (or after #2780 is fixed, a ~ character as well) doesn't get detected when referenced in the path. This causes a false positive detection in PathParameterShouldBeInThePath.

OpenApi File To Reproduce

{
    "openapi": "3.0.0",
    "info": {
      "title": "Test",
      "version": "1.0.0"
    },
    "paths": {
      "/{my/path/param}": {
        "get": {
          "parameters": [
            {
              "name": "my/path/param",
              "in": "path",
              "required": true,
              "schema": {
                "type": "string"
              }
            }
          ],
          "responses": {
            "200": {
              "description": "OK"
            }
          }
        }
      }
    }
  }

Expected behavior
When running validation with the default rule set, I don't expect a 'PathParameterShouldBeInThePath' error. I expect either no error, or an error indicating some kind of path template format issue (e.g. no forward-slashes inside braces).

Additional context
In my reading of the OpenAPI specs, there is never any clear guidance as to whether a path /{a/b} is invalid, should be interpreted as a literal string, or contains a reference to parameter {a/b}.
For what it's worth, my team interprets such paths as invalid due to the ambiguity.

Note that if #2780 is fixed, this issue will be effected - replace the parameter name with my~path~param in the above example.

[baywet]

Hi @davidrea-MS
Thank you for using the SDK and for reaching out.

This path parameter is invalid. While the ABNF was only added with version 3.2.0 of the specification, it was added to address a lack of clarity with previous versions of the specification.

https://spec.openapis.org/oas/v3.2.0.html#path-templating

Let me know if you have any additional comments or questions.

[davidrea-MS]

The ABNF doesn't seem to rule out the scenario I mentioned. Am I missing something?
template-expression-param-name = 1*( %x00-7A / %x7C / %x7E-10FFFF ) ; every Unicode character except { and }
This syntax doesn't seem to rule out a / char (%x2F) inside a template-expression-param-name. This appears to be an oversight in the spec, I can't imagine this would be intentional.

Regardless, I believe we have the same problem with the ~ character as soon as this proposed fix #2783 goes in (just use parameter name my~path~param instead).

I believe the easiest fix is that the rule should encode the parameter name before looking for it in the PathString.

[baywet]

Thank you for the additional information.

You are correct, the current version does not prevent the usage of slash in a parameter name, which is odd because a parameter needs to be within a path segment, and the path segments are delimited by /.

Full disclosure, I wrote this ABNF (I'm also a contributor to the OpenAPI specification itself), and my original draft did exclude slash in the parameter name.
OAI/OpenAPI-Specification#4244

I think something got lost through the review process of the PR. I did ask the question here to get the input of other specification contributors OAI/OpenAPI-Specification#5062 (comment)

That effectively leaves us with two avenues depending on the answer:

• slash is forbidden, error/warning when encountering this scenario
• slash is allowed, some kind of fix to encode the slash in the parameter name when emitting the path (as in JSON pointer) for the error message.

Thoughts?

[davidrea-MS]

Thanks for the context. My team's scenarios actually involve 'path template parameter matching' which as you called out recently can't be guaranteed to be non-ambiguous with the spec as it is. That's fine from my perspective, we run additional validation to avoid this problem.

If the OAI spec continues to allow the slash character in path parameter names, it makes parsing the path template in preparation for expansion slightly more difficult, as it can only be done correctly with knowledge of the path parameters. (Otherwise we don't know if {a/b} is a path parameter called a/b or two literal segments {a and b}). If slashes were banned, the correct parse could be achieved without this information. Additionally because path parameter names don't become a part of the resultant request, dropping support for any individual character shouldn't effect how expressive the spec is.
But this is a relatively minor point, as regardless of whether the slash is allowed or not, constructing a url from a path template and parameter values is still simple - it's just the path parsing logic that is a bit trickier, which effects validation scenarios and anyone using a class like UriTemplate.
If it was up to me I'd ban it just to remove any confusion.

Regardless of the outcome of that decision, I think this rule needs to be updated.