-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Return 401 for GET request to /v2 API for public artifacts. #14768
Conversation
This commits make sure when the request does not carry authorization headers, the HEAD and GET will get the same response code. This change should be made due to goharbor#14711 Signed-off-by: Daniel Jiang <jiangd@vmware.com>
Codecov Report
@@ Coverage Diff @@
## master #14768 +/- ##
==========================================
+ Coverage 66.22% 66.42% +0.20%
==========================================
Files 940 937 -3
Lines 74477 75355 +878
Branches 2140 2194 +54
==========================================
+ Hits 49320 50055 +735
- Misses 21256 21381 +125
- Partials 3901 3919 +18
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@@ -60,7 +60,8 @@ func (rc *reqChecker) check(req *http.Request) (string, error) { | |||
return getChallenge(req, al), fmt.Errorf("unauthorized to list catalog") | |||
} | |||
} | |||
if a.target == repository && req.Header.Get(authHeader) == "" && req.Method == http.MethodHead { // make sure 401 is returned for CLI HEAD, see #11271 | |||
if a.target == repository && req.Header.Get(authHeader) == "" && | |||
(req.Method == http.MethodHead || req.Method == http.MethodGet) { // make sure 401 is returned for CLI HEAD, see #11271 | |||
return getChallenge(req, al), fmt.Errorf("authorize header needed to send HEAD to repository") |
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.
The error message here says authorize header needed to send HEAD to repository
but this is true for GET
too (with this PR), right? With (req.Method == http.MethodHead || req.Method == http.MethodGet)
cc @reasonerjt
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.
Should it says HEAD or GET
? Or maybe interpolate the req.Method
in the string for accuracy? Something like this maybe? -
fmt.Errorf("authorize header needed to send %s to repository", req.Method)
This commits make sure when the request does not carry authorization
headers, the HEAD and GET will get the same response code. This change
should be made due to #14711
Signed-off-by: Daniel Jiang jiangd@vmware.com