Panics with index out of range error

[aeneasr]

Thanks for creating an issue! Please fill out this form so we can be sure to have all the information we need, and to minimize back and forth.

• What are you trying to do?

$ git clone git@github.com:ory/kratos.git
$ cd kratos
$ git checkout -b nancy 9ea96120451cf93be42c87146dcb9db02c67631d
$ go list -m all | nancy sleuth -q
Error: runtime error: index out of range [4] with length 4

[... rest of help message ...]

• Anything else?

cc @bhamail / @DarthHater

[DarthHater]

Hey @aeneasr , what version of Nancy are you running (and what OS is it on), just want to make sure I replicate it completely.

[aeneasr]

Ah yes of course: 1.0.5 on both linux and macOS

[DarthHater]

Ok! @aeneasr the dependency that it is tripping up on is: github.com/ory/kratos-client-go v0.0.0-00010101000000-000000000000 and it's because the replace you are doing is something we hadn't really expected.

 !  ~/c/g/o/kratos    go list -m all | nancy sleuth
[]string{"github.com/markbates/pkger", "v0.17.1", "=>", "github.com/falafeljan/pkger", "v0.17.1-0.20200722132747-95726f5b9b9b"}
[]string{"github.com/ory/kratos-client-go", "v0.0.0-00010101000000-000000000000", "=>", "./internal/httpclient"}

Essentially that string generally splits on whitespace and has the target version of what is being replaced at s[4]

In this case we have no target version, and a replace for just name, so it's hard to say what to do in reality. Technically you are replacing something with an internal bit of code, so in theory we should just throw this dependency away? Maybe explain a lil bit about what is going on in that situation. I have a commit pushed up that kinda "fixes" it, but I am not entirely sure that we want to fix it that way.

The commit I have (and the offending piece of code) is:

func parseSpaceSeparatedDependency(scanner *bufio.Scanner, deps *types.ProjectList, criteria func(s []string) bool) {
    text := scanner.Text()
    s := strings.Split(text, " ")
    if criteria(s) {
        if len(s) == 4 {
            // Odd case, a replace on just name (not version). Should we even handle this?
            deps.Projects = append(deps.Projects, types.Projects{Name: s[3], Version: s[1]})
        } else if len(s) > 3 {
            deps.Projects = append(deps.Projects, types.Projects{Name: s[0], Version: s[4]})
        } else {
            deps.Projects = append(deps.Projects, types.Projects{Name: s[0], Version: s[1]})
        }
    }
}

cc @zendern @bhamail

[aeneasr]

We are using the internal rewrite because the SDK is generated in a CI which pushes to the repo. We consume the SDK locally so in order to allow development of SDK consumers in the project, we've set up the local rewrite.

The SDK consumers can then again be consumed by 3rd party code so the internal package wouldn't work, which is why we've require the "official" SDK.

I'm not sure if it can be discarded - the internal package has a go.mod file which could be analyzed. However, to resolve the panic, it could be a workaround for now.

[DarthHater]

I mean more so this: you have rewritten a known coordinate into something that's internal (without a version). It'll be hard for us to tell the difference in these cases, and it seems likely best for us to warn a user if we encounter this, something like "Unversioned coordinate found" and give the user an opportunity to look through it?

[aeneasr]

Ah yes, I think that makes sense! It could also make sense to ignore the rewrite, use the original dependency for inspection, and warn the user about it!

[aeneasr]

This also panics when consuming a sub-module from the parent-module:

+ go.mod -> github.com/ory/kratos
+ corp
   + go.mod -> github.com/ory/kratos/corp

github.com/ory/kratos/go.mod

module github.com/ory/kratos

replace github.com/ory/kratos/corp => ./corp

require(
    github.com/ory/kratos/corp v0.0.0-00010101000000-000000000000
)

github.com/ory/kratos/corp/go.mod

module github.com/ory/kratos/corp

go 1.14

replace github.com/ory/kratos => ../

This type of set up is much more common, often also used in big projects like Kubernetes. I think this should be supported and not panic.

[aeneasr]

Just wanted to circle back and see if there's anything I can do to help. If it is implementation, I would need some pointers :)

[zendern]

So I started to take a look at this tonight @aeneasr I think we can fix the panic but honestly i think there might a simpler fix. The README has been updated i think back when 1.0.0 was released to officially suggest that you run go list -json -m all b/c the output is much nicer for the machines to understand :)

You can see I ran both with and without. Without it panics like reported and with -json it does what its supposed to.

If you can't easily switch what you are running I understand and still plan on looking at the panic but just wanted to throw that out there as an option.

[zendern]

Also another question on this..... this is one of the rewrites you have in there.

github.com/ory/kratos-client-go v0.5.4-alpha.1 => ./internal/httpclient

Should nancy a.) Just ignore it since you said rewrite it to this internal thing b.) Still let you know about any issues on the thing on the left side of the =>

My gut says scan cause security ¯_(ツ)_/¯ but maybe that doesn't make any sense.

[aeneasr]

Thank you, go list -json -m all | nancy sleuth -q indeed prevents the panic!

[aeneasr]

b.) Still let you know about any issues on the thing on the left side of the =>

Hm I don't think so, I would expect ./internal/httpclient/go.mod to be checked if it exists or it to be completely ignored (as the dependencies of ./internal/httpclient are in the root go.mod).

[zendern]

@aeneasr I think its probably best to fix the panic. Ignore it like the json version. And we can work on writing up an issue that deals with internal packages with go.mod files as well. The hard part of that one would be to have nancy call itself and ultimately go list .... command b/c today nancy doesn't do that it is just fed details.

So might be a 2 parter. Or maybe @DarthHater or @bhamail have some other ideas on what we could do to make it better for submodules.

---

Edit: Did the ignore thing in this commit 15c75b4

[aeneasr]

Ah, sounds good! Thank you for your hard work on this :)

[DarthHater]

No better ideas! You rule @zendern !!!!

[bhamail]

Noice!

[zendern]

@aeneasr All merged if you want to give it a go. Also spun up this issue #222 to deal with the subprojects. Please let us know if we missed anything.

[aeneasr]

Awesome, thank you! :)