-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add functionality to make a url signed for a HEAD request to S4 driver #63
Add functionality to make a url signed for a HEAD request to S4 driver #63
Conversation
if method != "GET" && method != "HEAD" { | ||
return "", storagedriver.ErrUnsupportedMethod | ||
} | ||
methodString, _ = method.(string) |
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.
This will panic if the method is not a string. It should be something like this:
m, ok = method.(string)
if !ok || (m != "GET" && m != "HEAD") {
return "", storagedriver.ErrUnsupportedMethod
}
methodString = m
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.
It will not, because you will never reach the line if method is not either the string "GET" or the string "HEAD"
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.
Exactly. That statement continuing to not crash relies on a side-effect of the if statement above. If this condition gets changed in any way such that the guarantee is removed, it will crash, unexpectedly.
a389bf2
to
bdd5d35
Compare
Waiting on LGTM from @BrianBland. |
LGTM, I'll submit a PR in a moment with the remaining changes to LayerHandler |
Verified to work for both v2 and v4 auth. |
Add functionality to make a url signed for a HEAD request to S4 driver
Wait until https://github.com/crowdmob/goamz/pull/319 is merged before merging this.
Update: It was merged.