-
Notifications
You must be signed in to change notification settings - Fork 470
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
Support for self-signed certificates #565
Support for self-signed certificates #565
Conversation
…y skipping certs validation.
@robertomg: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/ |
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 working on this! I left a few comments, but otherwise looks great.
packages/apollo-cli/src/config.ts
Outdated
@@ -19,6 +19,7 @@ export interface SchemaDependency { | |||
engineKey?: string; | |||
extends?: string; | |||
clientSide?: boolean; | |||
skipsSSLValidation?: boolean; |
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 think this should probably be part of EndpointConfig
, similar to endpoint-specific headers.
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 think this is a great suggestion and actually I thought about implementing this way on the first place. 👍
@@ -35,6 +35,10 @@ export default class SchemaDownload extends Command { | |||
description: | |||
"The URL of the server to fetch the schema from or path to ./your/local/schema.graphql" | |||
}), | |||
insecure: flags.boolean({ |
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.
Wondering about the naming, because it's a bit confusing to have insecure
, skipsSSLValidation
, and rejectUnauthorized
referring to the same option.
skipSSLValidation
seems the most descriptive (I prefer skip vs. skips, but that's no biggie). I understand wanting to follow curl
in using insecure
, but consistency between command flag and config is more important here I think.
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 agree on this that giving a more self-explanatory name is more important than keeping old names. So I decided to change this (also to skip instead of skips). Thanks for the feedback
): Promise<GraphQLSchema | undefined> => { | ||
if (!url) throw new Error("No endpoint provided when fetching schema"); | ||
const filePath = projectFolder ? path.resolve(projectFolder, url) : url; | ||
if (fs.existsSync(filePath)) return fromFile(filePath); | ||
|
||
const insecureOptionActive = insecureEnabled ? insecureEnabled : false; |
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 think we can avoid an extra variable here by using a default parameter value (insecureEnabled?: boolean = false
).
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.
Just discovered that checking a (boolean | undefined
) variable on an if would only accept it if the variable is true
so this line is not necessary anymore.
@@ -17,7 +17,7 @@ export async function loadSchema( | |||
|
|||
if (dependency.endpoint && dependency.endpoint.url) { | |||
try { | |||
return await fetchSchema(dependency.endpoint, config.projectFolder); | |||
return await fetchSchema(dependency.endpoint, config.projectFolder, dependency.skipsSSLValidation); |
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 we put skipSSLValidation
under endpoint
we can avoid passing in an extra parameter.
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.
Done
Thanks for the quick response! |
Adds support for skipping certificate validation
Often developers use a self-signed certificate for local services. In development process is not uncommon to see those kind of https requests without a Root CA signed certificate. This PR tries to solve the issue reported on #199 when using
apollo-cli
for downloading the GraphQL schema.schema:download
has a new option--insecure
or equally-k
for skipping certificate validation. These names were chosen following the popular unix commandcurl
convention.It is my first PR on the project so any kind of feedback is more than welcome.
Fixes #199