-
Notifications
You must be signed in to change notification settings - Fork 191
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
Allow HTTP connections to fetch database #2332
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.
Thanks for your contribution. This looks reasonable to me. I have a few suggestions.
extensions/ql-vscode/package.json
Outdated
"codeQL.allowHttp": { | ||
"type": "boolean", | ||
"default": false, | ||
"description": "Allow databases to be downloaded via HTTP" |
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.
"description": "Allow databases to be downloaded via HTTP" | |
"description": "Allow databases to be downloaded via HTTP. Warning: enabling this option will allow downloading from insecure servers." |
extensions/ql-vscode/package.json
Outdated
@@ -293,6 +293,11 @@ | |||
"scope": "window", | |||
"minimum": 0, | |||
"description": "Report a warning for any join order whose metric exceeds this value." | |||
}, | |||
"codeQL.allowHttp": { |
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.
Please make it clear that this is related to database downloads only.
"codeQL.allowHttp": { | |
"codeQL.databaseDownload.allowHttp": { |
extensions/ql-vscode/src/config.ts
Outdated
export const ALLOW_HTTP = new Setting( | ||
"allowHttp", | ||
ROOT_SETTING, | ||
); |
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.
export const ALLOW_HTTP = new Setting( | |
"allowHttp", | |
ROOT_SETTING, | |
); | |
const DATABASE_DOWNLOAD_SETTING = new Setting("databaseDownload", ROOT_SETTING); | |
export const ALLOW_HTTP_SETTING = new Setting( | |
"allowHttp", | |
DATABASE_DOWNLOAD_SETTING, | |
); |
@@ -27,6 +27,7 @@ import { | |||
} from "./common/github-url-identifier-helper"; | |||
import { Credentials } from "./common/authentication"; | |||
import { AppCommandManager } from "./common/commands"; | |||
import { ALLOW_HTTP } from "./config"; |
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.
import { ALLOW_HTTP } from "./config"; | |
import { ALLOW_HTTP_SETTING } from "./config"; |
@@ -49,7 +50,9 @@ export async function promptImportInternetDatabase( | |||
return; | |||
} | |||
|
|||
validateHttpsUrl(databaseUrl); | |||
if (!ALLOW_HTTP.getValue()) { |
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.
if (!ALLOW_HTTP.getValue()) { | |
if (!ALLOW_HTTP_SETTING.getValue()) { |
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.
Actually, on a deeper thought, can you push this check into the validateHttpsUrl
method? Rename the method to validateUrl
? This method parses the URL as well as check for https. It's probably better to continue to check that the URL parses regardless of whether or not it is https.
Something like this maybe:
function validateUrl(databaseUrl: string) {
let uri;
try {
uri = Uri.parse(databaseUrl, true);
} catch (e) {
throw new Error(`Invalid url: ${databaseUrl}`);
}
if (!ALLOW_HTTP_SETTING.getValue() && uri.scheme !== "https") {
throw new Error("Must use https for downloading a database.");
}
}
All changes made and fixed the linting issue from before. Ready for review |
Introduce a new config option to allow requests over HTTP when fetching a database from a URL.
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.
Thanks for the quick turnaround. Typo found.
I pushed a few minor changes to your branch: changelog note, typo fix in config description, and invalid variable reference fix. |
Awesome. Changes look good. Anything else you need from me? |
Nope. Let's wait for the checks to pass... |
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.
Thanks again for your contribution! I hope this is what you need.
Introduce a new config option to allow requests over HTTP when fetching a database from a URL.
Closes #2324
Checklist
ready-for-doc-review
label there.