Skip to content
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

Use the workspace trust feature #861

Merged
merged 5 commits into from
Jun 14, 2021
Merged

Conversation

shati-patel
Copy link
Contributor

@shati-patel shati-patel commented May 12, 2021

Will fix #849 and will also (indirectly) fix #509.

Uses the syntax from microsoft/vscode#120251 (comment) to let the CodeQL extension work with workspace trust. For users who have enabled the workspace trust feature:

  • If the user trusts the workspace: all CodeQL features are enabled
  • If the user doesn't trust the workspace: Certain settings (that could execute arbitrary code from the workspace) are disabled at the workspace level, and the extension ignores them. This is what the UI looks like:
    image

⚠️ Note: Currently, you can't use the CodeQL extension at all in untrusted workspaces. This is because it depends on the Test Explorer, which currently doesn't work in untrusted workspaces. PR: hbenl/vscode-test-explorer#204. Fixed!

Checklist

@aeisenberg
Copy link
Contributor

aeisenberg commented May 12, 2021

This looks great! Three extra things:

  1. This PR should also address Allow CodeQL CLI version (not path) to be specified in workspace settings #509 because users can now specify per-workspace cli versions.
  2. We should not merge this until workspace trust is enabled by default in vscode. I'm not sure which version that will be, but I think it's happening soon.
  3. We need to update the minimum vscode version required to run this extension when the appropriate vscode is released. See:
    "engines": {
    "vscode": "^1.43.0"
    },

@aeisenberg
Copy link
Contributor

The upstream PR has been merged and the new version of test explorer has been released. Let's ship this!

@shati-patel
Copy link
Contributor Author

shati-patel commented May 28, 2021

The upstream PR has been merged and the new version of test explorer has been released. Let's ship this!

We should probably still wait until workspace trust is enabled by default in VS Code, since we've changed the CLI executable setting back to "window" scope. (i.e. anyone who hasn't explicitly set "security.workspace.trust.enabled": true doesn't yet get any of the protection offered by workspace trust.)

Points 2 and 3 in your previous comment (#861 (comment)) still hold, I think!

@adityasharad
Copy link
Contributor

Nice! Do we know what version of VS Code will ship with workspace trust?

@shati-patel
Copy link
Contributor Author

Nice! Do we know what version of VS Code will ship with workspace trust?

I've been keeping an eye on microsoft/vscode#120251, but haven't seen anything about an expected release. (Maybe I'm looking in the wrong places though...)

I suppose we could revert the "machine -> window" change and ship the workspace trust implementation itself at any time? That would at least make the extension work for users who have enabled workspace trust and are in an untrusted workspace, without changing the behaviour for anyone else.

@aeisenberg
Copy link
Contributor

We should probably still wait until until workspace trust is enabled by default in VS Code

Yes. Good point.

There is a block on the package.json and we should update this to the version where workspace trust is going to be released. This will ensure that users on older versions on vscode won't accidentally get the new configuration behaviour.

"engines": {
    "vscode": "^1.43.0"
},

@alexet
Copy link
Contributor

alexet commented Jun 11, 2021

Running qtests in untrusted workspaces is also unsafe at the moment and hence also needs to be disabled.

partial cherry-pick from `qc-development` branch
This version of VS Code has workspace trust enabled by default
@shati-patel
Copy link
Contributor Author

shati-patel commented Jun 14, 2021

I've added some more commits to update dependencies, since workspace trust is now fully enabled (v1.57.0). I updated to 1.48.0 separately first, cherry-picked from #874 (which will hopefully minimise conflicts with the qc-development branch), so commit ec4785e shouldn't need a re-review.


Re:

Running qtests in untrusted workspaces is also unsafe at the moment and hence also needs to be disabled.

I think that's what line 30 covers:

"restrictedConfigurations": [
"codeQL.cli.executablePath",
"codeQL.runningTests.additionalTestArguments"
]

(Unless we need to disable qltests completely? I'm not sure how to do that.)

Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just some updates for the changelog.

extensions/ql-vscode/CHANGELOG.md Outdated Show resolved Hide resolved
extensions/ql-vscode/package.json Outdated Show resolved Hide resolved
@shati-patel shati-patel merged commit aa0d844 into main Jun 14, 2021
@shati-patel shati-patel deleted the shati-patel/workspace-trust branch June 14, 2021 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants