Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Stop analysis on 3rd party code #9

Open
arxeiss opened this issue Sep 30, 2022 · 9 comments
Open

Stop analysis on 3rd party code #9

arxeiss opened this issue Sep 30, 2022 · 9 comments

Comments

@arxeiss
Copy link

arxeiss commented Sep 30, 2022

I have some false positives on the code, which is not in my control. Can you stop analysis, if the error occurs in 3rd party libraries?

Example

import (
    "github.com/lestrrat-go/jwx/jwt"
    ...
)

func verifyTokenFormat(bearerToken string) error {
    _, err := jwt.ParseString(bearerToken, jwt.WithValidate(true), jwt.WithAcceptableSkew(time.Second))
    return err
}

This is source of ParseString, still does not accept context, but is already in 3rd party code:
https://github.com/lestrrat-go/jwx/blob/develop/v2/jwt/jwt.go#L87

func ParseString(s string, options ...ParseOption) (Token, error) {
	return parseBytes([]byte(s), options...)
}

And here is code of parseBytes which does not accept context, but has variable named ctx.
https://github.com/lestrrat-go/jwx/blob/develop/v2/jwt/jwt.go#L158

func parseBytes(data []byte, options ...ParseOption) (Token, error) {
	var ctx parseCtx

	// Validation is turned on by default. You need to specify
	// jwt.WithValidate(false) if you want to disable it
	ctx.validate = true
...

Error

The code above is causing this issue

identity/authorization.go:123:30: Function `verifyTokenFormat->parseBytes` should pass the context parameter (contextcheck)
                if err := verifyTokenFormat(sub.AccessToken); err != nil {
@arxeiss arxeiss changed the title Stop on 3rd party code Stop analysis on 3rd party code Sep 30, 2022
@kkHAIKE
Copy link
Owner

kkHAIKE commented Oct 4, 2022

try this command
contextcheck -pkgprefix=<your go.mod pkgname> ./...

golang-ci long time no release, 😢

@arxeiss
Copy link
Author

arxeiss commented Oct 4, 2022

Thank you, this works. However, contextcheck has no configuration in golangci-lint and we use that.

@kkHAIKE
Copy link
Owner

kkHAIKE commented Oct 4, 2022

'pkgprefix' option is not need in golang-ci, it work for standalone cli only. new version released https://github.com/golangci/golangci-lint/releases/tag/v1.50.0

@gonzaloserrano
Copy link

I'm testing golangci-int 1.50.0 with contextcheck and I have the same issue with the very same jwt library. Any advice?

@kkHAIKE
Copy link
Owner

kkHAIKE commented Oct 7, 2022

I'm testing golangci-int 1.50.0 with contextcheck and I have the same issue with the very same jwt library. Any advice?

show me some code

@arxeiss
Copy link
Author

arxeiss commented Oct 11, 2022

So I upgraded to 1.50 and only change is, that error format is changed too:

golangci-lint run --timeout 2m0s ./...
identity/authorization.go:123:30: Function `verifyTokenFormat->ParseString->parseBytes->parse->Decrypt->parseJSONOrCompact->parseCompact->makeDummyRecipient` should pass the context parameter (contextcheck)
                if err := verifyTokenFormat(sub.AccessToken); err != nil {
                                           ^

You can verify that with this repository https://github.com/indykite/jarvis-sdk-go
It still uses 1.49 with ignoring the old message format: https://github.com/indykite/jarvis-sdk-go/blob/master/.golangci.yml#L145

@kkHAIKE
Copy link
Owner

kkHAIKE commented Oct 11, 2022

I think the pkgpath filtering function is invalid..

it's a bug.

@arxeiss
Copy link
Author

arxeiss commented Oct 11, 2022

I'm not really sure. Because when I execute this contextcheck -pkgprefix="github.com/indykite/jarvis-sdk-go" ./... I got no errors

@kkHAIKE
Copy link
Owner

kkHAIKE commented Oct 11, 2022

I'm sure, because contextcheck -pkgprefix=xxx is hard filter by xxx..

d335867

wait next golang-ci release...

DmitriyMV added a commit to DmitriyMV/talos that referenced this issue Oct 20, 2022
I had to do several things:
- contextcheck now supports Go 1.18 generics, but I had to disable it because of this kkHAIKE/contextcheck#9
- dupword produces to many false positives, so it's also disabled
- revive found all packages which didn't have a documentation comment before. And tehre is A LOT of them. I updated some of them, but gave up at some point and just added them to exclude rules for now.

Signed-off-by: Dmitriy Matrenichev <dmitry.matrenichev@siderolabs.com>
DmitriyMV added a commit to DmitriyMV/talos that referenced this issue Oct 20, 2022
I had to do several things:
- contextcheck now supports Go 1.18 generics, but I had to disable it because of this kkHAIKE/contextcheck#9
- dupword produces to many false positives, so it's also disabled
- revive found all packages which didn't have a documentation comment before. And tehre is A LOT of them. I updated some of them, but gave up at some point and just added them to exclude rules for now.
- change lint-vulncheck to use `base` stage as base

Signed-off-by: Dmitriy Matrenichev <dmitry.matrenichev@siderolabs.com>
DJAlPee pushed a commit to DJAlPee/talos that referenced this issue May 22, 2023
I had to do several things:
- contextcheck now supports Go 1.18 generics, but I had to disable it because of this kkHAIKE/contextcheck#9
- dupword produces to many false positives, so it's also disabled
- revive found all packages which didn't have a documentation comment before. And tehre is A LOT of them. I updated some of them, but gave up at some point and just added them to exclude rules for now.
- change lint-vulncheck to use `base` stage as base

Signed-off-by: Dmitriy Matrenichev <dmitry.matrenichev@siderolabs.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants