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

Headers prefixed with X- are mistakenly ignored #24

Closed
danielgtaylor opened this issue Dec 1, 2022 · 8 comments
Closed

Headers prefixed with X- are mistakenly ignored #24

danielgtaylor opened this issue Dec 1, 2022 · 8 comments
Assignees

Comments

@danielgtaylor
Copy link
Contributor

danielgtaylor commented Dec 1, 2022

While RFC 6648 deprecated the X- prefix we still have an API that includes the header (and some common proxies still include stuff like X-Forwarded-For). Interestingly, these seem to be completely ignored by libopenapi. I haven't dug into it yet but suspect this might clash with code handling the OpenAPI extensions which start with x- as well. Notably the spec does not list headers as extensible via this method even though the response object itself does support it, so these should just be parsed as normal keys.

$ go run ./bug
Headers [Last-Modified]
package main

import (
	"fmt"

	"github.com/pb33f/libopenapi"
	"golang.org/x/exp/maps"
)

var data = `
openapi: "3.1"
paths:
  /fails:
    get:
      responses:
        "200":
          headers:
            'Last-Modified':
              description: desc
              schema:
                type: string
            X-fails:
              description: desc
              schema:
                type: string
`

func main() {
	doc, err := libopenapi.NewDocument([]byte(data))
	if err != nil {
		panic(err)
	}

	result, errs := doc.BuildV3Model()
	if len(errs) > 0 {
		panic(errs)
	}

	m := result.Model

	fmt.Println("Headers", maps.Keys(m.Paths.PathItems["/fails"].Get.Responses.Codes["200"].Headers))
}

Part of danielgtaylor/restish#115

@TristanSpeakEasy
Copy link
Contributor

Have just run into this myself

@daveshanley
Copy link
Member

No problem, a simple fix in a couple of places. I'll work on this shortly.

@daveshanley daveshanley self-assigned this Dec 1, 2022
@daveshanley
Copy link
Member

Available in v0.3.1

@JoeReid
Copy link

JoeReid commented Apr 19, 2023

@daveshanley There seems to be a regression of this issue in the v0.7.0 release.
However, I have not checked previous versions to see if this issue regressed prior to the latest release.

I have also not checked if #54 has also re-surfaced, but I assume it is likely.

Happy to help with a fix, if this would be useful. I would need some guidance with the codebase though.

@daveshanley
Copy link
Member

@daveshanley There seems to be a regression of this issue in the v0.7.0 release. However, I have not checked previous versions to see if this issue regressed prior to the latest release.

I have also not checked if #54 has also re-surfaced, but I assume it is likely.

Happy to help with a fix, if this would be useful. I would need some guidance with the codebase though.

Hi Joe,

Generally with each bug fix, I add a test to validate and prevent regressions. I may have missed this one, but I haven't heard any other comments from folks about it regressing.

I do remember seeing that code again on my travels recently, but I didn't touch it, at least I can't remember touching it. So what would be helpful is a reproducible sample?

@daveshanley daveshanley reopened this Apr 19, 2023
@JoeReid
Copy link

JoeReid commented Apr 19, 2023

@daveshanley There seems to be a regression of this issue in the v0.7.0 release. However, I have not checked previous versions to see if this issue regressed prior to the latest release.
I have also not checked if #54 has also re-surfaced, but I assume it is likely.
Happy to help with a fix, if this would be useful. I would need some guidance with the codebase though.

Hi Joe,

Generally with each bug fix, I add a test to validate and prevent regressions. I may have missed this one, but I haven't heard any other comments from folks about it regressing.

I do remember seeing that code again on my travels recently, but I didn't touch it, at least I can't remember touching it. So what would be helpful is a reproducible sample?

Thanks for your swift response 👍

Under further investigation, the issue seems to only apply to response headers, and request headers are working as expected.
In which case this may be an issue that has not be reported before.

The following is a main.go file that parses two minimal openapi files, one with x- prefixed headers, one without.
(I would've provided a playground link, but alas, the library doesn't pull before the execution timeout)

package main

import (
	"fmt"

	"github.com/pb33f/libopenapi"
)

func main() {
	fmt.Println("Spec with x-:")
	printHeaders(`openapi: 3.0.0
paths:
  /pets:
    get:
      parameters:
        - name: x-api-key
          in: header
      responses:
        '200':
          headers:
            x-next:
              schema:
                type: string
          content:
            application/json:
              schema:
                type: object
`)

	fmt.Println("Spec without x-:")
	printHeaders(`openapi: 3.0.0
paths:
  /pets:
    get:
      parameters:
        - name: api-key
          in: header
      responses:
        '200':
          headers:
            next:
              schema:
                type: string
          content:
            application/json:
              schema:
                type: object
`)
}

func printHeaders(spec string) {
	document, err := libopenapi.NewDocument([]byte(spec))
	if err != nil {
		panic(err)
	}

	v3Model, errs := document.BuildV3Model()
	if len(errs) > 0 {
		panic(errs)
	}

	for _, pathItem := range v3Model.Model.Paths.PathItems {
		for _, op := range pathItem.GetOperations() {
			fmt.Printf("\tRequest headers:\n")

			for _, param := range op.Parameters {
				if param.In == "header" {
					fmt.Printf("\t\t- %s\n", param.Name)
				}
			}

			fmt.Printf("\tResponse headers:\n")
			for _, resp := range op.Responses.Codes {
				for headerName, _ := range resp.Headers {
					fmt.Printf("\t\t- %s\n", headerName)
				}
			}
		}
	}
}

As can be seen in the output of this file's execution, request headers where correctly found, but the response headers were
only provided when not prefixed.

$ go run main.go
Spec with x-:
        Request headers:
                - x-api-key
        Response headers:
Spec without x-:
        Request headers:
                - api-key
        Response headers:
                - next

Thanks for your time on this.

@daveshanley
Copy link
Member

daveshanley commented Apr 19, 2023

Oh, I see, yeah, that should not happen. Will circle back to this on the next bug run at the end of the month.

daveshanley added a commit that referenced this issue May 17, 2023
New extraction functions added (that just wrap the old ones). The difference here is that the extensions can be included or ignored now. This has been added to address issue #24 where header keys can in fact include an extension prefix.

Signed-off-by: Dave Shanley <dave@quobix.com>
daveshanley added a commit that referenced this issue May 17, 2023
New extraction functions added (that just wrap the old ones). The difference here is that the extensions can be included or ignored now. This has been added to address issue #24 where header keys can in fact include an extension prefix.

Signed-off-by: Dave Shanley <dave@quobix.com>
@daveshanley
Copy link
Member

daveshanley commented May 17, 2023

@JoeReid Can you try v0.8.3

Header keys in responses can now use extension prefixes.

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

4 participants