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

Expose config.kit.server.https so that svelte-kit preview --https can use user-defined cert #1659

Closed
alipi opened this issue Jun 9, 2021 · 9 comments

Comments

@alipi
Copy link

alipi commented Jun 9, 2021

Is your feature request related to a problem? Please describe.
Expose config.kit.server.https so that svelte-kit preview --https can use user-defined cert.

Describe the solution you'd like
Not tested, but suggested fix:

diff --git a/packages/kit/src/core/load_config/options.js b/packages/kit/src/core/load_config/options.js
index c763465..ba8eb88 100644
--- a/packages/kit/src/core/load_config/options.js
+++ b/packages/kit/src/core/load_config/options.js
@@ -139,6 +139,19 @@ const options = {
 
                        router: expect_boolean(true),
 
+                       server: {
+                               type: 'leaf',
+                               default: null,
+                               validate: (option, keypath) => {
+
+                                       if (!(option && option.https && option.https.cert && option.https.key)) {
+                                               throw new Error(`${keypath} server https cert & key not defined`);
+                                       }
+
+                                       return option;
+                               }
+                       },
+
                        ssr: expect_boolean(true),
 
                        target: expect_string(null),

However I've tested the patched built version:

diff --git a/node_modules/@sveltejs/kit/dist/cli.js b/node_modules/@sveltejs/kit/dist/cli.js
index 9914595..e829be3 100644
--- a/node_modules/@sveltejs/kit/dist/cli.js
+++ b/node_modules/@sveltejs/kit/dist/cli.js
@@ -277,6 +277,19 @@ const options = {
 
 			router: expect_boolean(true),
 
+                       server: {
+                               type: 'leaf',
+                               default: null,
+                               validate: (option, keypath) => {
+
+                                       if (!(option && option.https && option.https.cert && option.https.key)) {
+                                               throw new Error(`${keypath} server https cert & key not defined`);
+                                       }
+
+                                       return option;
+                               }
+                       },
+
 			ssr: expect_boolean(true),
 
 			target: expect_string(null),

Describe alternatives you've considered
N/A

How important is this feature to you?
Good to have, to be able to use existing self-signed certs during development.

@JBusillo
Copy link
Contributor

JBusillo commented Jun 9, 2021

If you use a user-defined certificate, you should place it under the 'vite' option:

	kit: {
		target: '#svelte',
	        vite: {
		      server: {
			     https: {
				   key: fs.readFileSync('/path-to-key'),
				   cert: fs.readFileSync('/path-to-cert')
			     },
		        }

Your change will require the user to specify a key and cert, which shouldn't be required -- Kit will generate a self-signed certificate of its own if a key and cert aren't specified. In this case, the user must accept URL via the browser's "privacy error" notification.

You may want to wait until the next release of svelte-kit. It will include a change that allows HMR to work properly with https.

Perhaps the FAQ in the documentation should be updated to show how to set up https with your own certificates.

@alipi
Copy link
Author

alipi commented Jun 9, 2021

Your change will require the user to specify a key and cert,

Thanks for the feedback. For me the auto generated one is used if it's not specified in the config when running svelte-kit preview --https. So it should not break anything. Regarding config.kit.vite.server, yes it works when running svelte-kit dev --https.

@JBusillo
Copy link
Contributor

JBusillo commented Jun 9, 2021

Ah, I understand.
I see the problem -- Preview is looking for kit.server, and Dev is looking for kit.vite.server. Here are my recommended changes. In start/index.js, from:
const server = await get_server(use_https, config.kit, (req, res) => { ...
To:
const server = await get_server(use_https, config.kit.vite, (req, res) => {...
and in server/index.js from:

if (
	user_config.server &&
	user_config.server.https &&
	user_config.server.https.key &&
	user_config.server.https.cert
) {...

To:

if (
        user_config &&
	user_config.server &&
	user_config.server.https &&
	user_config.server.https.key &&
	user_config.server.https.cert
) {...

I can do the PR and modify this.

As a side note -- I think the option for specifying the key and cert is used often enough that it seems it should be a "kit option" rather than a "kit.vite option". Kit is responsible for creating the server, since it runs in Vite's "middlewares" mode. However, this recommendation should be reviewed by the maintainers. I'll bring this up in the PR.

@alipi
Copy link
Author

alipi commented Jun 9, 2021

Thanks @JBusillo, I'm not familiar with the implementation details, but from a user perspective having one setting for both dev and preview would be even better, although it would break the current config.kit.vite.server variant.

@JBusillo
Copy link
Contributor

JBusillo commented Jun 9, 2021

That all depends on what the maintainers/architects think. And, perhaps to support migration during beta, a console message could be displayed when the old option is used.
But, for sure, I can at least get the preview to use config.kit.vite.server. I'll work on it this evening.

@s-sankar
Copy link

s-sankar commented Aug 13, 2021

Still no proper information about how to deploy the svelte app with node adapter in HTTPS

@EirikFA
Copy link

EirikFA commented Oct 10, 2021

HTTPS is working fine for me with preview, but the certificate specified in the configuration is not used when running dev..

@alipi
Copy link
Author

alipi commented Oct 19, 2021

With the latest version my previous workaround no longer works. Now config.kit.vite.server.https can be used for dev and preview with the workaround below.

diff --git a/packages/kit/src/core/dev/index.js b/packages/kit/src/core/dev/index.js
index 08fa1754..099b2911 100644
--- a/packages/kit/src/core/dev/index.js
+++ b/packages/kit/src/core/dev/index.js
@@ -156,7 +156,7 @@ class Watcher extends EventEmitter {
                [merged_config] = deep_merge(merged_config, {
                        server: {
                                host: this.host,
-                               https: this.https,
+                               https: this.https && merged_config && merged_config.server && merged_config.server.https || this.https,
                                port: this.port
                        }
                });

Patch for built version:

diff --git a/node_modules/@sveltejs/kit/dist/chunks/index.js b/node_modules/@sveltejs/kit/dist/chunks/index.js
index 036a731..ca69c44 100644
--- a/node_modules/@sveltejs/kit/dist/chunks/index.js
+++ b/node_modules/@sveltejs/kit/dist/chunks/index.js
@@ -4332,7 +4332,7 @@ class Watcher extends EventEmitter {
 		[merged_config] = deep_merge(merged_config, {
 			server: {
 				host: this.host,
-				https: this.https,
+				https: this.https && merged_config && merged_config.server && merged_config.server.https || this.https,
 				port: this.port
 			}
 		});

@alipi
Copy link
Author

alipi commented Oct 22, 2021

This was properly fixed in #2622 available in 1.0.0-next.188.

@alipi alipi closed this as completed Oct 22, 2021
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