Safer handling of Accept headers #84
Conversation
crane/views/v2.py
Outdated
| @@ -77,7 +78,8 @@ def name_redirect(relative_path): | |||
| if schema2_data or manifest_list_data: | |||
| # if it is a newer docker client it sets accept headers to manifest schema 1, 2 and list | |||
| # if it is an older docker client, he doesnot set any of accept headers | |||
| accept_headers = request.headers.get('Accept') | |||
| accept_headers = get_accept_headers(request) | |||
| log.debug("Accept: %s", accept_headers) | |||
There was a problem hiding this comment.
please remove the log statement, this information will not help anyone without a deep understanding of docker engine insides
There was a problem hiding this comment.
I don't anything about docker/crane, but I think someone that someone who turns on debug logging would have that deep understanding and want logging like this.
There was a problem hiding this comment.
I agree with bmbouter. More than once I wanted to have more information, and I would prefer to do that without changing code (just changing config). That being said, I shouldn't log.debug twice.
There was a problem hiding this comment.
it is not about docker/crane or pulp related but about docker-engine, the client for pulling the docker content and docker api. I still kinda think this is not very helpful, but if we want to keep this then please at least update to a more meaningful message something like " Accept headers sent from docker client" or "Accept header sent during docker pull"
crane/views/v2.py
Outdated
| accept_headers = request.headers.get('Accept') | ||
| if not accept_headers: | ||
| return [] | ||
| log.debug("Accept: %s", accept_headers) |
There was a problem hiding this comment.
what about leaving just one log statement with "Received Accept headers" or similar?
crane/views/v2.py
Outdated
| return [] | ||
| log.debug("Accept: %s", accept_headers) | ||
| accept_headers = accept_headers.split(',') | ||
| return set(x.strip() for x in accept_headers) |
There was a problem hiding this comment.
for header in accept_header i find easier to read, in general i would avoid x,y,i, etc variable names.
There was a problem hiding this comment.
I would agree, but for comprehension I find it concise enough. If you care deeply I can change it.
There was a problem hiding this comment.
not really. it's just my personal preference of the pattern.
| @@ -125,3 +127,12 @@ def handle_error(error): | |||
| response.headers['Content-Type'] = 'application/json' | |||
| response.status_code = error.status_code | |||
| return response | |||
|
|
|||
|
|
|||
| def get_accept_headers(request): | |||
| headers = {'Accept': 'application/vnd.docker.distribution.manifest.v2+json'} | ||
| response = self.test_client.get('/v2/redhat/foo/manifests/1.25.1-musl', headers=headers) | ||
| # #3303: verify multi-valued headers too | ||
| # manifest lists are evaluated first, so pass a longer media type that |
There was a problem hiding this comment.
can you elaborate more on this please?
There was a problem hiding this comment.
The code first checks if manifest_list_mediatype is in accept_headers. If the client sent a string that includes 'application/vnd.docker.distribution.manifest.list.v2+json like in my +junk test, as well as a valid manifest v2 media type, then you'd get a false match for a manifest list.
| # #3303: verify multi-valued headers too | ||
| # manifest lists are evaluated first, so pass a longer media type that | ||
| # matches the manifest list as a prefix | ||
| headers = {'Accept': 'application/vnd.docker.distribution.manifest.list.v2+jsonjunk,application/vnd.docker.distribution.manifest.v2+json'} # noqa |
There was a problem hiding this comment.
i don't think +jsonjunk would be a valid mediatype sent from docker client :) max what you can have is +json or +prettyjws
There was a problem hiding this comment.
That's with a civilized client. As a server, you should always check the inputs from the client. This is not a realistic example, just like this patch is not fixing something broken now.
There was a problem hiding this comment.
please address https://github.com/pulp/crane/pull/84/files#r162663154 and https://github.com/pulp/crane/pull/84/files#r162658437
and we're good to merge
mibanescu
force-pushed
the
3303
branch
3 times, most recently
from
34797c4
to
e495a6a
Compare
|
All requested changes have been done. |
closes #3303
https://pulp.plan.io/issues/3303