Conversation
PR SummaryMedium Risk Overview Reviewed by Cursor Bugbot for commit a0fe0c0. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Prevents HTTP header injection via network transform rules by checking for CR and LF bytes in both header names and values before storing or forwarding them.
Avoid aliasing the internal cache struct by cloning the headers map before exposing it in the API response. Also fixes a nil pointer to nil map serializing as "headers":null — when headers are nil the pointer is left nil so omitempty correctly omits the field.
PUT handler was emitting "sandbox with network transform rules created" instead of "updated", mislabeling update operations in analytics.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c96456b90
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
maxNetworkRuleHeaderValueLen was set to 256, which is too small for common base64-encoded values such as Bearer tokens or API keys that can easily exceed that length. Raise it to 2048. Add maxNetworkRuleHeadersPerRule (20) and enforce it in validateNetworkRules before iterating individual headers. Without a per-rule header count limit a single rule could carry an unbounded number of headers.
buildEgressConfig unconditionally allocated an empty map for orchRules even when rules was nil. Downstream consumers check `if egress.Rules != nil` to detect whether transform rules are configured; a non-nil empty map caused a false positive for every sandbox that was created without any transform rules. Guard the allocation behind `if rules != nil` so the field stays nil when no rules were provided.
… version Adds same envd version logic we already have for volumes. Re-worked logic a bit so we have helper function that handles all corner cases for us.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a0fe0c0. Configure here.
| } | ||
|
|
||
| return | ||
| } |
There was a problem hiding this comment.
Unnecessary GetSandbox call on every network update
Low Severity
The GetSandbox call is made unconditionally on every network update request, but sbxInfo.EnvdVersion is only needed when body.Rules is non-nil. When no rules are provided (the common case of just updating allow/deny lists), validateNetworkRules returns nil immediately, making the GetSandbox round-trip pure overhead. The call could be moved inside a body.Rules != nil guard.
Reviewed by Cursor Bugbot for commit a0fe0c0. Configure here.
| Err: err, | ||
| ClientMsg: "internal error while validating network rules", | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing envd version returns 500 instead of 400
Medium Severity
When envdVersion is empty, checkEnvdVersionRequirement returns errNoEnvdVersion. In validateNetworkRules, this error does not match the errors.Is(err, errNetworkRulesNotSupported) check, so it falls through to the catch-all that returns http.StatusInternalServerError. A missing envd version is a client-side condition (old template that needs rebuilding), not a server error, so returning 500 is misleading to API consumers.
Reviewed by Cursor Bugbot for commit a0fe0c0. Configure here.
There was a problem hiding this comment.
This is actually reasonable point, I think. Why are we returning internal server error for envd version validation? This would not tell users that they cannot do it for old templates, which might be the case where this is triggered, right? Hope I understand it correctly.
3 participants
Support for setting injection headers. Sandbox creation and network update are supported. Configuration is stored in the database to support the sandbox resume flow.