create allowlist for localhost variants and validate host on requests#7032
create allowlist for localhost variants and validate host on requests#7032EvilGenius13 wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Introduces DNS rebinding protection for the theme dev local development server by validating incoming Host headers against an allowlist of localhost variants, preventing malicious origins from accessing the local server via rebinding.
Changes:
- Add Host-header allowlist validation middleware to the theme dev H3 server.
- Add/adjust Vitest coverage to exercise allowlisted vs rejected Host headers.
- Add a changeset for a patch release of
@shopify/theme.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/theme/src/cli/utilities/theme-environment/theme-environment.ts | Adds allowed-host set construction and Host header validation to block non-local Host values. |
| packages/theme/src/cli/utilities/theme-environment/theme-environment.test.ts | Adds DNS rebinding protection tests and updates request helpers to include Host headers. |
| .changeset/few-maps-melt.md | Declares a patch release entry describing the security hardening. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| function createAllowedHostsSet(host: string, port: string): Set<string> { | ||
| const allowedHosts = new Set<string>() | ||
| const portSuffix = `:${port}` | ||
|
|
||
| allowedHosts.add(`${host}${portSuffix}`) | ||
|
|
||
| // When binding to localhost variants or 0.0.0.0, allow all localhost forms | ||
| const localhostVariants = ['localhost', '127.0.0.1', '::1', '0.0.0.0'] | ||
| if (localhostVariants.includes(host)) { | ||
| allowedHosts.add(`localhost${portSuffix}`) | ||
| allowedHosts.add(`127.0.0.1${portSuffix}`) | ||
| allowedHosts.add(`[::1]${portSuffix}`) | ||
| } |
| const hostHeader = event.node.req.headers.host | ||
| const normalizedHost = hostHeader?.toLowerCase().replace(/\.$/, '') | ||
|
|
||
| if (!normalizedHost || !allowedHosts.has(normalizedHost)) { |
| return resEnd(content) | ||
| } | ||
| test.each([ | ||
| ['localhost:9292', 'configured host'], |
Coverage report
Test suite run success3825 tests passing in 1460 suites. Report generated by 🧪jest coverage report action from b0d6fa9 |
EvilGenius13
force-pushed
the
change-allowed-hosts-theme-dev
branch
from
2e14c6c to
b0d6fa9
Compare
|
|
||
| allowedHosts.add(`${normalizedHost}${portSuffix}`) | ||
|
|
||
| // When binding to localhost variants or 0.0.0.0, allow all localhost forms |
There was a problem hiding this comment.
Either explain more of a why if it's confusing or drop this comment
| // When binding to localhost variants or 0.0.0.0, allow all localhost forms |
There was a problem hiding this comment.
If you think most people would understand I can drop the comment. Otherwise I can add something like browsers will send whatever localhost you typed such as localhost or 127.0.0.1 so we need to cover the different formats.
There was a problem hiding this comment.
I didn't think people would actually type 0.0.0.0 - that's usually an address you bind to accept any request.
However, I can imagine people visiting their local private IP address in their home (like 192.168.2.100). Should we also be white listing private IPs? 🤔
There was a problem hiding this comment.
Wait, i think you added the host you ran the theme dev command with to the allowlist. You should be fine here (ignore previous comment).
There was a problem hiding this comment.
There's a chance I'm blind here: does this work if a user passes in a --host flag?
There was a problem hiding this comment.
Yes it does. In the services/dev.ts file it runs setupDevServer and passes a context in that would include flags like host and port so we would be creating the custom host/port if passed in.
|
|
||
| allowedHosts.add(`${normalizedHost}${portSuffix}`) | ||
|
|
||
| // When binding to localhost variants or 0.0.0.0, allow all localhost forms |
There was a problem hiding this comment.
Wait, i think you added the host you ran the theme dev command with to the allowlist. You should be fine here (ignore previous comment).
WHY are these changes introduced?
Fixes https://github.com/Shopify/developer-tools-team/issues/1092
When running
theme deva potential security vulnerability exists where a developer could visit a malicious site while the CLI is running that uses DNS rebinding to extract data.WHAT is this pull request doing?
On
theme devboot up, create an allow list of localhost forms'localhost', '127.0.0.1', '::1', '0.0.0.0'. When requests come in, we deny those that aren't from the allow list.How to test your changes?
Note This vulnerability does not exist when using an environment. You can test this by running the commands below using an environment and they should return a
401.On any current version of the CLI:
theme devand in a separate terminal, try these curl commands:curl -s -I -H "Host: localhost:9292" http://localhost:9292/ | head -1curl -s -I -H "Host: attacker.com:9292" http://localhost:9292/ | head -1curl -s -I -H "Host: evil.local:9292" http://localhost:9292/ | head -1curl -s -I -H "Host: localhost:1234" http://localhost:9292/ | head -1These should return a
200On the current branch:
theme devand in a separate terminal, try the curl commands again:200as it's on the allowlistThese commands should return a
400curl -s -I -H "Host: attacker.com:9292" http://localhost:9292/ | head -1**This will returncurl -s -I -H "Host: evil.local:9292" http://localhost:9292/ | head -1curl -s -I -H "Host: localhost:1234" http://localhost:9292/ | head -1Post-release steps
Measuring impact
How do we know this change was effective? Please choose one:
Checklist