search

sourcegraph / go-langserver

Go language server to add Go support to editors and other tools that use the Language Server Protocol (LSP)
https://sourcegraph.com
MIT License
1.17k stars 90 forks source link

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

Closed chrismwendt closed 5 years ago

chrismwendt commented 5 years ago

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.

cc @beyang just FYI, no action necessary

chrismwendt commented 5 years ago

@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

chrismwendt commented 5 years ago

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

felixfbecker commented 5 years ago

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 commented 5 years ago

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:

https://github.com/sourcegraph/go-langserver/blob/8e668e29439ada68cd881ec34fc1e11c46129cb6/vfsutil/zip.go#L27-L38