Do not include access token in cache key when fetching zip archives

[chrismwendt]

Prior to this change, the cache key was the full URL, including whatever username:password was set (e.g. the Sourcegraph access token).

After this change, the cache key will be the URL but with empty username:password.

This will increase the probability of a cache hit because each repo@revision will be fetched once and used by any user.

A HEAD request is still made for each user x repo@revision pair to make sure the user has access to the repository contents.

• Helps fix https://sourcegraph.slack.com/archives/C0C324C91/p1554154297030300
• Helps fix https://sourcegraph.slack.com/archives/C0C324C91/p1554159963034900
• Helps fix https://github.com/sourcegraph/sourcegraph/issues/1940

cc @beyang just FYI, no action necessary

[chrismwendt]

@slimsag I added you as a reviewer to help expedite the review of this change since it got bumped in priority today https://sourcegraph.slack.com/archives/C0C324C91/p1554427053097800?thread_ts=1554425326.073300&cid=C0C324C91

[emidoots]

It's kind of weird to use urlString and urlStruct naming. I would leave url as-is and rename urlStruct to just u

[chrismwendt]

What do you suggest for naming now that the the url package name from "net/url" conflicts with the local variable url?

[emidoots]

it won't conflict if you do my withoutAuth refactor suggested below

[chrismwendt]

Like this?

func withoutAuth(urlString string) (string, error) {
	u, err := url.Parse(urlString)
...

[emidoots]

some requests inline

[chrismwendt]

Thanks, @slimsag. I left urlString because both u and url are used for other variables.

[felixfbecker]

If the cache key does not include the user/auth info, is there a way as user A I can possibly get data returned that was cached for user B, without being authorised to see that data?

[chrismwendt]

If the cache key does not include the user/auth info, is there a way as user A I can possibly get data returned that was cached for user B, without being authorised to see that data?

go-langserver checks for permission before reading from the cache:

go-langserver/vfsutil/zip.go

Lines 27 to 38 in 8e668e2

	request, err := http.NewRequest("HEAD", url, nil)
	if err != nil {
	return nil, errors.Wrapf(err, "failed to construct a new request with URL %s", url)
	}
	setAuthFromNetrc(request)
	response, err := ctxhttp.Do(ctx, nil, request)
	if err != nil {
	return nil, err
	}
	if response.StatusCode != http.StatusOK {
	return nil, fmt.Errorf("unable to fetch zip from %s (expected HTTP response code 200, but got %d)", url, response.StatusCode)
	}