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

Gorilla server invalid code due to unsanitised template #874

Closed
KenxinKun opened this issue Nov 28, 2022 · 4 comments
Closed

Gorilla server invalid code due to unsanitised template #874

KenxinKun opened this issue Nov 28, 2022 · 4 comments

Comments

@KenxinKun
Copy link
Contributor

Problem

When generating the gorilla-server code, invalid Go is generated in the Scopes context key.

Detail + fixes

When trying to generate gorilla-server code I am getting invalid Go code generated such as:

ctx = context.WithValue(ctx, Api-KeyScopes, []string{"global"})

Which, should in fact have been as follows, looking at the generated consts at the top of the file:

ctx = context.WithValue(ctx, Api_KeyScopes, []string{"global"})

PS: note that the hyphen - changed to an underscore _

I think the fix to this is pretty straight forward though, looking at other templates. Currently line 43 in pkg/codegen/templates/gorilla/gorilla-middleware.tmpl (link) has:

ctx = context.WithValue(ctx, {{.ProviderName | ucFirst}}Scopes, {{toStringArray .Scopes}})

But I think it should be:

ctx = context.WithValue(ctx, {{.ProviderName | sanitizeGoIdentity | ucFirst}}Scopes, {{toStringArray .Scopes}})

Is this a quick fix that could be merged in?

Additionally, when fixing the above the linter complains that the used value for the context Key is just a string, not a custom type, so I'd suggest also changing pkg/codegen/templates/constants.tmpl to include:
type ScopeKey string
and use said type in line 4 as:

{{- $ProviderName | ucFirst}}Scopes ScopeKey= "{{$ProviderName}}.Scopes"

Contributing

I'd be happy to make a PR myself but I don't have permissions to push a branch 🥲

@jamietanna
Copy link
Collaborator

Hey, thanks for the report, and a good spot. Please fork and raise a PR, we only allow write access to branches to maintainers :)

I'd be happy helping you get this over the line to unblock you, especially as a quick fix! I'll also release as a patch bump, for anyone who's currently seeing this issue with the latest released code.

@KenxinKun
Copy link
Contributor Author

Hi @jamietanna here's the fork and PR :)

#877

It only fixes the breaking change with the context key. I quickly tried adding an alias type for it not to use string but it started breaking elsewhere (when trying ctx.Set(...), so thought to keep it minimal since this later bit was not breaking as such.

@jamietanna
Copy link
Collaborator

Thanks very much @KenxinKun 🙌 Did you fancy raising a PR for the other cases this occurs?

git grep 'ProviderName | ucF'
pkg/codegen/templates/chi/chi-middleware.tmpl:  ctx = context.WithValue(ctx, {{.ProviderName | ucFirst}}Scopes, {{toStringArray .Scopes}})
pkg/codegen/templates/constants.tmpl:    {{- $ProviderName | ucFirst}}Scopes = "{{$ProviderName}}.Scopes"
pkg/codegen/templates/gin/gin-wrappers.tmpl:  c.Set({{.ProviderName | ucFirst}}Scopes, {{toStringArray .Scopes}})

No need to raise a new issue.

I'm happy to raise the PR for this, thought I'd see if you fancied another contribution!

@KenxinKun
Copy link
Contributor Author

Missing PR :) #881

adrianpk pushed a commit to foorester/oapi-codegen that referenced this issue Jan 16, 2024
…pe-variable-template

Fixes missing sanitation of context key variable in gorilla server

Closes deepmap#874.
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