-
Notifications
You must be signed in to change notification settings - Fork 16
feat: support running extensions on private code without a private Sourcegraph instance #249
Conversation
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.
@sqs I'd like your input on the interaction between e-c-c and the browser extension in "feat: allow GraphQL requests that are known to not contain private info" and "feat: block queryLSP from a private repo to Sourcegraph.com" and whether or not this approach makes sense
Makes sense to me. 👍
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.
Overall nice work. I've got some suggestions/requests in the commit you asked me to review. The changes to request logic will conflict with changes I've made in my auth token PR. The changes look nice but unnecessary for the changes intended in this PR. Could we break that out separately so we can easily merge our PRs and then get those changes in afterwards?
src/libs/gitlab/file_info.ts
Outdated
) | ||
) | ||
) | ||
} | ||
const commitID = commitIDFromPermalink({ |
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.
I purposefully kept DOM stuff outside of this file. Could you move this into scrape
? Why is this needed if we get the rev from getFilePageInfo
anyways?
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.
Moved to scrape
. getFilePageInfo
doesn't resolve to a commit ID, does it? It looks like it only gives you the revision (i.e. it could be a ref like master
rather than a commit ID).
Thanks, @ijsnow, I accepted all of your suggestions except the
Mind if I try resolving the conflicts? No conflict is too difficult to resolve when equipped with the almighty |
2237ec3
to
db539b1
Compare
I resolved the conflicts in |
db539b1
to
2899060
Compare
2899060
to
b8b1aa1
Compare
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 PR is included in version 1.17.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Motivation This adds support for running extensions (e.g. Codecov) on private code even when the repository does not exist on the Sourcegraph instance. Doing so will resolve this issue: https://github.com/sourcegraph/browser-extensions/issues/234#issuecomment-430429015
Problem The browser extension must not let arbitrary GraphQL requests through to the Sourcegraph instance (which could be Sourcegraph.com) because they might contain private info (such as repo names), but some GraphQL requests are necessary to run extensions at all (e.g. fetching the extension manifests).
Implementation I've marked some GraphQL requests as safe by passing in
requestMightContainPrivateInfo: false
(defaults totrue
to keep old behavior for unmarked requests). The browser extension will allow those requests to be sent to the Sourcegraph instance. I also blockedqueryLSP
when sent from a private repo to Sourcegraph.com.Reviewers The main file to review is
src/shared/backend/extensions.ts
. If you want to review the rest, I'd recommend reviewing commit-by-commit.Testing
git checkout allow-known-safe-gql-requests
yarn ; yarn run dev
Depends on sourcegraph/sourcegraph-langserver-http#8
Depends on sourcegraph/extensions-client-common#57