fix: show proper `req.url` in HTTPS mode by Skn0tt · Pull Request #5968 · netlify/cli

Skn0tt · 2023-08-28T10:15:50Z

🎉 Thanks for submitting a pull request! 🎉

Summary

Together with https://github.com/netlify/edge-functions-bootstrap/pull/294, this PR fixes what req.url shows in HTTPS mode.

It passes the original hostname and protocol into the Edge Function via X-Forwarded-Host and X-Forwarded-Proto. https://github.com/netlify/edge-functions-bootstrap/pull/294 reads this and shows it in req.url.

Bootstrap uses this URL for passthrough requests as well. In order for the Deno runtime to accept the self-signed certificate, we pass it into edge-bundler. This also allows us to remove the secondary HTTP server for passthrough requests.

For us to review and ship your PR efficiently, please perform the following steps:

Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
Read the contribution guidelines 📖. This ensures your code follows our style guide and
passes our tests.
Update or add tests (if any source code was changed or added) 🧪
Update or add documentation (if features were changed or added) 📝
Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

Skn0tt · 2023-08-28T10:16:23Z

@@ -1,5 +1,5 @@
 import { env } from 'process'

-const latestBootstrapURL = 'https://64c264287e9cbb0008621df3--edge.netlify.com/bootstrap/index-combined.ts'


We need to merge the ef-bootstrap PR first, so we can update the URL here.

github-actions · 2023-08-28T10:17:37Z

📊 Benchmark results

Comparing with 4a7d9df

Dependency count: 1,331 ⬆️ 1.58% increase vs. 4a7d9df
Package size: 295 MB ⬆️ 7.52% increase vs. 4a7d9df

Skn0tt · 2023-08-28T10:21:23Z


-  // If we're running the main server on HTTPS, we need to start a secondary
-  // server on HTTP for receiving passthrough requests from edge functions.
-  // This lets us run the Deno server on HTTP and avoid the complications of


With this PR, we're passing the "potentially untrusted certificate" into Deno via the --certs flag.

This functionality was originally implemented in denoland/deno#3972, and it works by adding the cert as a trusted root certificate to the HTTP client:

https://github.com/denoland/deno/pull/3972/files#diff-5c6fb275a66a1b02f99597f6e9b36297274abd4b16926b9fb2d412f94ee0700eR52

From my perspective, this should be fine. Maybe Eduardo can elaborate on the complications they had in mind when they're back.

Have you seen the PR that introduced that functionality? #5409

sarahetter

LGTM, but it'd be great to wait for @eduardoboucas 's feedback on the comment above ^

eduardoboucas

Since you're essentially reverting #5409, can you add some colour to why you think we won't reintroduce the problems people were having with untrusted certificates? There's a whole Slack thread (internal to Netlify only) with a discussion on why running Deno on HTTP would be preferable.

I understand that the behaviour described in #5945 doesn't feel ideal, but is it breaking anything? I'd like to better understand what exactly we're looking to fix before we risk breaking other things.

Skn0tt · 2023-09-04T09:50:16Z

Wasn't aware of the Slack thread and the nuance around Deno requiring the CA cert. Agree that this isn't the right fix, in that case.

The thing that's broken is that req.url shows the wrong protocol and hostname. We can fix that by reading the the X-Forwarded-Proto and X-Forwarded-Host headers from inside bootstrap, and that's probably the right way forward. Closing!

Skn0tt · 2023-09-05T08:37:25Z

Found a way of fixing this without removing the second passthrough server :) It works by adding a X-NF-Passthrough-Proto header to ef-bootstrap, so we can change protocol of the passthrough independently of the protocol of the protocol that's shown in req.url.

eduardoboucas · 2023-09-05T08:43:45Z

    req[headersSymbol] = {
      [headers.FeatureFlags]: getFeatureFlagsHeader(featureFlags),
-      [headers.ForwardedHost]: forwardedHost,
+      [headers.ForwardedProtocol]: req.socket.encrypted ? 'https:' : 'http:',


Can we make the caller method pass settings.https rather than using req.socket.encrypted here? That way we have a unified detection mechanism for whether we're running on HTTPS.

done in dfa73c9

eduardoboucas · 2023-09-05T08:45:26Z

+              try {
+                await fetch(url, {})
+              } catch (error) {
+                return new Response(error)


nit: I think using an error code here might come in handy.

Suggested change

return new Response(error)

return new Response(error, { status: 500 })

done in 3c9a98c

eduardoboucas · 2023-09-05T08:47:25Z

  DeployID: 'x-nf-deploy-id',
  FeatureFlags: 'x-nf-feature-flags',
  ForwardedHost: 'x-forwarded-host',
+  ForwardedProtocol: 'x-forwarded-proto',


Is this one still needed?

We're still using x-forwarded-proto in the code. x-forwarded-host could be removed, if we wanted - but I think it's also fine to leave it in, so it's easier to discover the headers that ef-bootstrap takes in the future 🤷

eduardoboucas

LGTM! I haven't approved because we're still using a deploy preview for the bootstrap URL. Once that's changed, we're good to roll.

eduardoboucas · 2023-09-05T09:34:45Z

 import { env } from 'process'

-const latestBootstrapURL = 'https://64e7783fce8cfe0008496c72--edge.netlify.com/bootstrap/index-combined.ts'
+const latestBootstrapURL = 'https://deploy-preview-294--edge.netlify.app/bootstrap/index-combined.ts'


Let's not forget to change this before merging.

done in bb5f238

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`6.5.0` -> `6.6.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/6.5.0/6.6.0) | | [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`6.5.0` -> `6.6.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/6.5.0/6.6.0) | | [@typescript-eslint/typescript-estree](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`6.5.0` -> `6.6.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2ftypescript-estree/6.5.0/6.6.0) | | [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.48.0` -> `8.49.0`](https://renovatebot.com/diffs/npm/eslint/8.48.0/8.49.0) | | [netlify-cli](https://github.com/netlify/cli) | devDependencies | minor | [`16.2.0` -> `16.3.1`](https://renovatebot.com/diffs/npm/netlify-cli/16.2.0/16.3.1) | --- ### Release Notes <details> <summary>typescript-eslint/typescript-eslint (@typescript-eslint/eslint-plugin)</summary> ### [`v6.6.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#660-2023-09-04) [Compare Source](typescript-eslint/typescript-eslint@v6.5.0...v6.6.0) ##### Bug Fixes - **eslint-plugin:** \[key-spacing] consider properties with parens and comments ([#7525](typescript-eslint/typescript-eslint#7525)) ([7012279](typescript-eslint/typescript-eslint@7012279)) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website. </details> <details> <summary>typescript-eslint/typescript-eslint (@typescript-eslint/parser)</summary> ### [`v6.6.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#660-2023-09-04) [Compare Source](typescript-eslint/typescript-eslint@v6.5.0...v6.6.0) **Note:** Version bump only for package [@typescript-eslint/parser](https://github.com/typescript-eslint/parser) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website. </details> <details> <summary>typescript-eslint/typescript-eslint (@typescript-eslint/typescript-estree)</summary> ### [`v6.6.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/typescript-estree/CHANGELOG.md#660-2023-09-04) [Compare Source](typescript-eslint/typescript-eslint@v6.5.0...v6.6.0) **Note:** Version bump only for package [@typescript-eslint/typescript-estree](https://github.com/typescript-eslint/typescript-estree) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website. </details> <details> <summary>eslint/eslint (eslint)</summary> ### [`v8.49.0`](https://github.com/eslint/eslint/releases/tag/v8.49.0) [Compare Source](eslint/eslint@v8.48.0...v8.49.0) #### Features - [`da09f4e`](eslint/eslint@da09f4e) feat: Implement onUnreachableCodePathStart/End ([#17511](eslint/eslint#17511)) (Nicholas C. Zakas) - [`32b2327`](eslint/eslint@32b2327) feat: Emit deprecation warnings in RuleTester ([#17527](eslint/eslint#17527)) (Nicholas C. Zakas) - [`acb7df3`](eslint/eslint@acb7df3) feat: add new `enforce` option to `lines-between-class-members` ([#17462](eslint/eslint#17462)) (Nitin Kumar) #### Documentation - [`ecfb54f`](eslint/eslint@ecfb54f) docs: Update README (GitHub Actions Bot) - [`de86b3b`](eslint/eslint@de86b3b) docs: update `no-promise-executor-return` examples ([#17529](eslint/eslint#17529)) (Nitin Kumar) - [`032c4b1`](eslint/eslint@032c4b1) docs: add typescript template ([#17500](eslint/eslint#17500)) (James) - [`cd7da5c`](eslint/eslint@cd7da5c) docs: Update README (GitHub Actions Bot) #### Chores - [`b7621c3`](eslint/eslint@b7621c3) chore: remove browser test from `npm test` ([#17550](eslint/eslint#17550)) (Milos Djermanovic) - [`cac45d0`](eslint/eslint@cac45d0) chore: upgrade [@eslint/js](https://github.com/eslint/js)[@8](https://github.com/8).49.0 ([#17549](eslint/eslint#17549)) (Milos Djermanovic) - [`cd39508`](eslint/eslint@cd39508) chore: package.json update for [@eslint/js](https://github.com/eslint/js) release (ESLint Jenkins) - [`203a971`](eslint/eslint@203a971) ci: bump actions/checkout from 3 to 4 ([#17530](eslint/eslint#17530)) (dependabot\[bot]) - [`a40fa50`](eslint/eslint@a40fa50) chore: use eslint-plugin-jsdoc's flat config ([#17516](eslint/eslint#17516)) (Milos Djermanovic) - [`926a286`](eslint/eslint@926a286) test: replace Karma with Webdriver.IO ([#17126](eslint/eslint#17126)) (Christian Bromann) - [`f591d2c`](eslint/eslint@f591d2c) chore: Upgrade config-array ([#17512](eslint/eslint#17512)) (Nicholas C. Zakas) </details> <details> <summary>netlify/cli (netlify-cli)</summary> ### [`v16.3.1`](https://github.com/netlify/cli/blob/HEAD/CHANGELOG.md#1631-2023-09-06) [Compare Source](netlify/cli@v16.3.0...v16.3.1) ##### Bug Fixes - **deps:** update dependency [@netlify/build](https://github.com/netlify/build) to v29.20.13 ([#5981](netlify/cli#5981)) ([d9545fa](netlify/cli@d9545fa)) - **deps:** update dependency [@netlify/edge-bundler](https://github.com/netlify/edge-bundler) to v8.19.1 ([#5983](netlify/cli#5983)) ([9d86237](netlify/cli@9d86237)) ### [`v16.3.0`](https://github.com/netlify/cli/blob/HEAD/CHANGELOG.md#1630-2023-09-06) [Compare Source](netlify/cli@v16.2.0...v16.3.0) ##### Features - support `context.params` + `config.method` in functions + edge functions ([#5970](netlify/cli#5970)) ([6afe7bd](netlify/cli@6afe7bd)) ##### Bug Fixes - **deps:** update netlify packages ([#5972](netlify/cli#5972)) ([34e0faa](netlify/cli@34e0faa)) - **deps:** update netlify packages ([#5974](netlify/cli#5974)) ([4a7d9df](netlify/cli@4a7d9df)) - make lm:setup test pass ([#5980](netlify/cli#5980)) ([40b3e78](netlify/cli@40b3e78)) - show proper `req.url` in HTTPS mode ([#5968](netlify/cli#5968)) ([da723a1](netlify/cli@da723a1)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - "before 3pm on Sunday" in timezone America/New_York, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Co-authored-by: chris b <[email protected]> Reviewed-on: https://git.chriswb.dev/chrisw-b/PersonalApi/pulls/6 Co-authored-by: Renovate Bot <[email protected]> Co-committed-by: Renovate Bot <[email protected]>

fix: show proper req.url in HTTPS mode

090d94c

Skn0tt requested a review from a team as a code owner August 28, 2023 10:15

Skn0tt self-assigned this Aug 28, 2023

Skn0tt commented Aug 28, 2023

View reviewed changes

sarahetter previously approved these changes Aug 30, 2023

View reviewed changes

eduardoboucas requested changes Sep 4, 2023

View reviewed changes

Skn0tt closed this Sep 4, 2023

fix: use x-nf-passthrough-proto

58b40b3

Skn0tt reopened this Sep 5, 2023

Skn0tt dismissed sarahetter’s stale review via 58b40b3 September 5, 2023 08:31

Merge branch 'main' into fix-5945

d528543

Skn0tt requested a review from eduardoboucas September 5, 2023 08:37

eduardoboucas reviewed Sep 5, 2023

View reviewed changes

Skn0tt added 3 commits September 5, 2023 11:26

refactor: use settings.https to detect https

dfa73c9

refactor: throw 500 error in test

18fe0a1

chore: remove unused import

3c9a98c

eduardoboucas requested changes Sep 5, 2023

View reviewed changes

Skn0tt added 2 commits September 5, 2023 16:08

fix: update bootstrap url

bb5f238

fix: use .com

a60764c

eduardoboucas approved these changes Sep 5, 2023

View reviewed changes

Merge branch 'main' into fix-5945

f57689e

Skn0tt merged commit da723a1 into main Sep 6, 2023

Skn0tt deleted the fix-5945 branch September 6, 2023 08:00

token-generator-app Bot mentioned this pull request Sep 6, 2023

chore(main): release 16.3.0 #5973

Merged

		@@ -1,5 +1,5 @@
		import { env } from 'process'

		const latestBootstrapURL = 'https://64c264287e9cbb0008621df3--edge.netlify.com/bootstrap/index-combined.ts'

	return new Response(error)
	return new Response(error, { status: 500 })

Conversation

Skn0tt commented Aug 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Benchmark results

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarahetter left a comment

Choose a reason for hiding this comment

Uh oh!

eduardoboucas left a comment

Choose a reason for hiding this comment

Uh oh!

Skn0tt commented Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Skn0tt commented Sep 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eduardoboucas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Skn0tt commented Aug 28, 2023 •

edited

Loading

github-actions Bot commented Aug 28, 2023 •

edited

Loading

Skn0tt commented Sep 4, 2023 •

edited

Loading