-
Notifications
You must be signed in to change notification settings - Fork 921
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
fix(grpcHeaders): accept values with "=" symbols #8047
fix(grpcHeaders): accept values with "=" symbols #8047
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, thank you for the contribution!
This piece of code was never tested, but the functionality was straightforward. Now that it got more complicated, can you please add a suite of tests for various header values? It will greatly eliminate the risk of introducing bugs in case we need to add more code on top.
Hi @rkapka, Do you know if there's any faster way to invoke the tests than this? $ go test github.com/prysmaticlabs/prysm/validator/client
ok github.com/prysmaticlabs/prysm/validator/client 44.623s If not, is it okay if we pull the snippet of code into something that can be unit tested? |
Ah, it looks like when I run individual tests, it goes way faster. |
# What Before this commit, it was not possible to pass in base64-encoded content as a header value if it contained an equals sign. This commit changes the behavior slightly. Rather than ignore key/value pairs where the value happens to have an equals sign, we assume the first equals sign delimits the key from the value and pass in the rest of the value as-is. Also, instead of printing the header name along with its value, we print the name, so there is less risk of leaking information into logs that shouldn't be there. # Testing This has not been tested in the context of the full validator client. Instead, a small example was made to demonstrate the feasibility. The example is shown here: ```go package main import ( "log" "os" "strings" "github.com/davecgh/go-spew/spew" "github.com/urfave/cli/v2" ) type Config struct { GrpcHeadersFlag string } func main() { app := &cli.App{ Action: func(c *cli.Context) error { for _, hdr := range strings.Split(c.String("grpc-headers"), ",") { if hdr != "" { ss := strings.Split(hdr, "=") spew.Dump(ss[0]) spew.Dump(strings.Join(ss[1:], "=")) } } return nil }, Flags: []cli.Flag{ &cli.StringFlag{ Name: "grpc-headers", Usage: "A comma-separated list of key value pairs to pass as gRPC headers for all gRPC " + "calls. Example: --grpc-headers=key=value", }, }, } err := app.Run(os.Args) if err != nil { log.Fatal(err) } } ``` Example invocation: ```command ❯ go run main.go --grpc-headers=key=value,Authorization="Basic $(echo -n hello:world | base64)" (string) (len=3) "key" (string) (len=5) "value" (string) (len=13) "Authorization" (string) (len=22) "Basic aGVsbG86d29ybGQ=" ```
2e00c9a
to
d131573
Compare
We've added 5 new tests cases. 👍 |
Ooops, I forgot to add a failure case. One moment please. |
d131573
to
3fe4935
Compare
for input, output := range map[string][]string{ | ||
"should-break": []string{}, | ||
"key=value": []string{"key", "value"}, | ||
"": []string{}, | ||
",": []string{}, | ||
"key=value,Authorization=Q=": []string{ | ||
"key", "value", "Authorization", "Q=", | ||
}, | ||
"Authorization=this is a valid value": []string{ | ||
"Authorization", "this is a valid value", | ||
}, | ||
} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is considered too hacky, we can take the time to improve it.
3fe4935
to
2e43d6d
Compare
@artburkart. Please run gazelle. I tried to push to your branch but i dont have access.
|
Requested changes were addressed
Thank you! 🙏 |
What type of PR is this?
Bug fix
What does this PR do? Why is it needed?
Before this commit, it was not possible to pass in base64-encoded
content as a header value if it contained an equals sign. This commit
changes the behavior slightly. Rather than ignore key/value pairs where
the value happens to have an equals sign, we assume the first equals
sign delimits the key from the value and pass in the rest of the value
as-is.
Also, instead of printing the header name along with its value, we only
print the name, so there is less risk of leaking information into logs
that shouldn't be there.
Which issues(s) does this PR fix?
N/A
Other notes for review
This has not been tested in the context of the full validator client.
Instead, a small example was made to demonstrate the feasibility. The
example is shown here:
Example invocation: