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

🐛 BUG: TOML not validated before running Worker + bad Browser Rendering API error #7529

Open
irvinebroque opened this issue Dec 12, 2024 · 1 comment
Labels
bug Something that isn't working

Comments

@irvinebroque
Copy link
Contributor

irvinebroque commented Dec 12, 2024

Which Cloudflare product(s) does this pertain to?

Wrangler

What version(s) of the tool(s) are you using?

3.95.0

What version of Node are you using?

22.3.0

What operating system and version are you using?

Sequoia 15.1.1

Describe the Bug

This TOML:

name = "browser-rendering-remote"
main = "src/index.js"
compatibility_date = "2024-12-05"
compatibility_flags = ["nodejs_compat"]

[observability]
enabled = true

browser = { binding = "MYBROWSER" }

...allows you to run a Worker, but then at runtime, try wrangler dev --remote and you'll see the following runtime error:

[wrangler:inf] GET /favicon.ico 500 Internal Server Error (51ms)
✘ [ERROR] Uncaught (in response) TypeError: Cannot read properties of undefined (reading 'fetch')

      at PuppeteerWorkers.launch
  (file:///Users/brendan/src/test/browser-rendering-remote/.wrangler/tmp/dev-na2A11/index.js:16857:32)
      at Object.fetch
  (file:///Users/brendan/src/test/browser-rendering-remote/.wrangler/tmp/dev-na2A11/index.js:18456:56)
      at fetchDispatcher
  (file:///Users/brendan/src/test/browser-rendering-remote/.wrangler/tmp/dev-na2A11/index.js:18554:19)
      at __facade_invokeChain__
  (file:///Users/brendan/src/test/browser-rendering-remote/.wrangler/tmp/dev-na2A11/index.js:18516:10)

The browser property above is interpreted as a part of [observability] — even though [observability] has no such property that one can configure.

The browser binding is not defined at runtime, leading to the runtime error.

This is likely to happen a lot because the C3 template we give you back uses []:

# Configuration: https://developers.cloudflare.com/workers/observability/logs/workers-logs/#enable-workers-logs
[observability]
enabled = true

so if you drop browser = { binding = "MYBROWSER" } (or similar) under this, then you'll hit some flavor of this issue. And it looks like things are breaking at runtime, which is horribly misleading.

What do we need to do?

  1. Use TOML syntax that is less likely to cause confusing errors cloudflare-docs#18706 — we should give people example TOML that is less prone to this footgun
  2. We should consider starting to loudly raise a warning (not error) in local dev if we see properties within wrangler.toml that we know are not supported as part of the given binding/thing being configured. Ex: if I see that the observability binding has a browser property configured — we should know that that....isn't a thing and warn you — right?
  3. We should likely audit docs for other examples of places where we can do (1)
  4. In the Browser Rendering puppeteer fork, we should be able to check if the binding was actually passed in — and if not, throw a loud and clear error that explains
  5. Anything else?

Please provide a link to a minimal reproduction

https://github.com/irvinebroque/browser-rendering-remote

h/t @rita3ko finding this

@irvinebroque irvinebroque added the bug Something that isn't working label Dec 12, 2024
@github-project-automation github-project-automation bot moved this to Untriaged in workers-sdk Dec 12, 2024
@irvinebroque
Copy link
Contributor Author

@lrapoport-cf recognize one answer to this is — wrangler.json

We also have lot of people using TOML.

If I understand root cause right — seems like there should be a way to validate / warn about fields that we see in TOML config that aren't actually part of that thing? So that we can give people better guidance?

@nevikashah re: footgun getting started

@irvinebroque irvinebroque changed the title 🐛 BUG: TOML not validated before running Worker 🐛 BUG: TOML not validated before running Worker + bad Browser Rendering API error Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that isn't working
Projects
Status: Untriaged
Development

No branches or pull requests

1 participant