-
Notifications
You must be signed in to change notification settings - Fork 219
feat: add mask request host #1479
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
Conversation
acef439 to
05d4c72
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
05d4c72 to
b65c9d7
Compare
9803f0c to
bb3cae8
Compare
| if t.MaskRequestHost != nil { | ||
| // Mask the request host to bypass source host protections. | ||
| r.Out.Header.Set("X-Forwarded-Host", r.In.Host) | ||
| r.Out.Host = *t.MaskRequestHost |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we certain that we want to change the ip/port to connect to, or do we just want the Host http request header to change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just the http request header to change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the Host field doesn't just set the header, I believe it changes where you send the request as well. I'll see if I can verify that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the req.URL (or r.SetURL(url)) is what controls where the request goes, the r.Out.Host is just the header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh. Turns out that, when writing the http request, it pulls the host from the struct and then writes it out of band. I'll push the test on to your branch, but it just confirms that the code works 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One suggestion: instead of using X-Forwarded-Host, we might want to consider using more widely accepted headers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
X-Forwarded-Host is actually widely accepted header
f3d6f71 to
9d9e7ec
Compare
e2781d7 to
0980901
Compare
tests/integration/internal/tests/proxies/traffic_access_token_test.go
Outdated
Show resolved
Hide resolved
0fb9fe7 to
8d740e0
Compare
Note
Adds
maskRequestHostto sandbox network config and proxies non-envd traffic with a masked Host (with${PORT}substitution), including validation, propagation, and tests.network.maskRequestHosttoNewSandboxhandling with ASCII validation; accept host or host:port (port optional).types.SandboxNetworkConfigand pass through to orchestrator creation.SandboxNetworkIngressConfig.mask_request_host) and generated stubs.X-Forwarded-Hostand overridesHost; supports${PORT}placeholder; logsmask_request_host.MaskRequestHostto proxy poolDestinationand rewrite logic.spec/openapi.yml) and generated client/server types to includemaskRequestHost.maskRequestHostAPI param (valid/invalid), traffic access token paths, sandbox-not-found/closed-port; refactor request helpers.golang.org/x/netto API module; minor go.sum tidy.Written by Cursor Bugbot for commit bf6cbd0. This will update automatically on new commits. Configure here.