Skip to content

Add https support in dev local mode#3529

Merged
mrbbot merged 1 commit intomainfrom
jspspike/https-server
Jun 30, 2023
Merged

Add https support in dev local mode#3529
mrbbot merged 1 commit intomainfrom
jspspike/https-server

Conversation

@jspspike
Copy link
Copy Markdown
Contributor

@jspspike jspspike commented Jun 27, 2023

Fixes #3353.

PR adds support for https in local mode for dev. Adds new options to provide custom key and cert for https server.

Docs issue

Author has included the following, where applicable:

Reviewer is to perform the following, as applicable:

  • Checked for inclusion of relevant tests
  • Checked for inclusion of a relevant changeset
  • Checked for creation of associated docs updates
  • Manually pulled down the changes and spot-tested

@jspspike jspspike requested a review from mrbbot June 27, 2023 18:32
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Jun 27, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 555548b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
wrangler Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@jspspike
Copy link
Copy Markdown
Contributor Author

jspspike commented Jun 27, 2023

Waiting on miniflare release before PR can be merged

mrbbot
mrbbot reviewed Jun 28, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@mrbbot mrbbot left a comment

Choose a reason for hiding this comment

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

I don't think we want to add new local-key/local-cert flags for this. We should be very careful about adding new flags, especially those that are local specific.

Instead, Wrangler actually already includes that selfsigned package we talked about for use in remote mode. 🙂 See https://github.com/cloudflare/workers-sdk/blob/8d1521e9ce77136f6da6a1313748e597b3622f8b/packages/wrangler/src/https-options.ts and:

workers-sdk/packages/wrangler/src/proxy.ts

Lines 569 to 572 in 8d1521e

const server: HttpServer | HttpsServer =
localProtocol === "https"
? createHttpsServer(await getHttpsOptions())
: createHttpServer();

We should be able to call this same function in local mode somewhere around here:

workers-sdk/packages/wrangler/src/dev/miniflare.ts

Lines 374 to 378 in 8d1521e

if (config.localProtocol === "https") {
logger.warn(
"Miniflare 3 does not support HTTPS servers yet, starting an HTTP server instead..."
);
}

@jspspike jspspike force-pushed the jspspike/https-server branch from 27ddd3e to 2143d1f Compare June 28, 2023 15:49
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jun 28, 2023 •
edited
Loading

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/5417071771/npm-package-wrangler-3529

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/3529/npm-package-wrangler-3529

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/5417071771/npm-package-wrangler-3529 dev path/to/script.js
Additional artifacts: 
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/5417071771/npm-package-cloudflare-pages-shared-3529

Note that these links will no longer work once the GitHub Actions artifact expires.

mrbbot
mrbbot reviewed Jun 28, 2023
View reviewed changes
@codecov
Copy link
Copy Markdown

codecov bot commented Jun 28, 2023 •
edited
Loading

Codecov Report

Merging #3529 (555548b) into main (4b83918) will decrease coverage by 0.01%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3529      +/-   ##
==========================================
- Coverage   75.09%   75.09%   -0.01%     
==========================================
  Files         190      190              
  Lines       11108    11110       +2     
  Branches     2919     2919              
==========================================
+ Hits         8342     8343       +1     
- Misses       2766     2767       +1
Impacted Files Coverage Δ
packages/wrangler/src/dev/miniflare.ts 64.59% <50.00%> (-0.19%) ⬇️

@jspspike jspspike force-pushed the jspspike/https-server branch from 2143d1f to e539092 Compare June 28, 2023 16:20
@jspspike jspspike requested a review from mrbbot June 28, 2023 16:21
mrbbot
mrbbot reviewed Jun 29, 2023
View reviewed changes
@jspspike jspspike force-pushed the jspspike/https-server branch from e539092 to 555548b Compare June 29, 2023 21:53
@jspspike jspspike marked this pull request as ready for review June 29, 2023 21:54
@jspspike jspspike requested review from a team as code owners June 29, 2023 21:54
@jspspike jspspike requested a review from mrbbot June 29, 2023 21:54
@mrbbot mrbbot merged commit bcdc1fe into main Jun 30, 2023
@mrbbot mrbbot deleted the jspspike/https-server branch June 30, 2023 09:33
@github-actions github-actions bot mentioned this pull request Jun 30, 2023
Merged
@sdarnell
Copy link
Copy Markdown

sdarnell commented Jun 30, 2023

I don't think we want to add new local-key/local-cert flags for this.

@mrbbot / @jspspike The 'selfsigned' package generates a cert, but that cert is not trusted by the system browser as it isn't added to the system keychain etc., and it is a real pain to have to regularly accept the 'here be dragons' warnings or add the freshly updated certs to the appropriate keychains.

For Wrangler 2 I had to hack around and overwrite the wrangler generated cert my my own trusted one (generated by mkcert fwiw). This is definitely a hack. Most other systems allow the cert files to be specified, either via a command line option or environment var. So, I'd request that an option of some sort is provided to override the wrangler/miniflare generated cert.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@mrbbot mrbbot mrbbot approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

🐛 Bug: v3 HTTPS support

3 participants

@jspspike @sdarnell @mrbbot