Skip to content

fix: propagate url.Parse error in buildURL to prevent nil pointer pan… #270

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.

Sign up for GitHub

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

Closed
sicaario wants to merge 6 commits into from
+114 −22
Closed

fix: propagate url.Parse error in buildURL to prevent nil pointer pan… #270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b9d9179
fix: propagate url.Parse error in buildURL to prevent nil pointer pan…
sicaario Apr 13, 2026
08363e4
fix: compare RSA key components in TestGetPrivateKeyPEM instead of wh…
sicaario Apr 13, 2026
02fae19
fix: include parse error in 500 response and add test for empty entry…
sicaario Apr 13, 2026
aa52d24
fix: validate scheme and host in buildURL and expand edge case tests
sicaario Apr 13, 2026
9dfb8a4
fix: validate proxy scheme whitelist and port range in buildURL
sicaario Apr 13, 2026
ad148fb
fix: use generic 500 message, t.Setenv in tests, and Cmp for big.Int N
sicaario Apr 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 41 additions & 10 deletions pkg/router/handlers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,12 @@ limitations under the License.
package router

import (
"errors"
"fmt"
"net/http"
"net/http/httputil"
"net/url"
"strconv"
"strings"
"time"

Expand Down Expand Up @@ -103,27 +105,54 @@ func (s *Server) handleGetSandboxError(c *gin.Context, err error) {
c.JSON(http.StatusInternalServerError, gin.H{"error": "internal server error"})
}

// errNoEntryPoint is returned when a sandbox has no entry points configured.
var errNoEntryPoint = errors.New("no entry point found for sandbox")

func determineUpstreamURL(sandbox *types.SandboxInfo, path string) (*url.URL, error) {
// prefer matched entrypoint by path
for _, ep := range sandbox.EntryPoints {
if strings.HasPrefix(path, ep.Path) {
return buildURL(ep.Protocol, ep.Endpoint), nil
return buildURL(ep.Protocol, ep.Endpoint)
}
}
// fallback to first entrypoint
if len(sandbox.EntryPoints) == 0 {
return nil, fmt.Errorf("no entry point found for sandbox")
return nil, errNoEntryPoint
}
ep := sandbox.EntryPoints[0]
return buildURL(ep.Protocol, ep.Endpoint), nil
return buildURL(ep.Protocol, ep.Endpoint)
}

// validProxySchemes are the URL schemes accepted for reverse-proxy targets.
// ws/wss are included to support WebSocket upgrades.
var validProxySchemes = map[string]bool{
"http": true, "https": true, "ws": true, "wss": true,
}

func buildURL(protocol, endpoint string) *url.URL {
func buildURL(protocol, endpoint string) (*url.URL, error) {
Copy link
Copy Markdown
Member

@hzxuzhonghu hzxuzhonghu Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If iirc， the endpoint is constructed by us in buildSandboxInfor, it is podIP:port. And also the protocol can be only http and https now.

So i think this is actually not worth verifying.

if protocol != "" && !strings.Contains(endpoint, "://") {
endpoint = (strings.ToLower(protocol) + "://" + endpoint)
endpoint = strings.ToLower(protocol) + "://" + endpoint
}
u, err := url.Parse(endpoint)
if err != nil {
return nil, fmt.Errorf("invalid endpoint URL %q: %w", endpoint, err)
}
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

url.Parse can return a non-nil *url.URL without error for inputs that are still unusable for httputil.NewSingleHostReverseProxy (e.g., empty endpoint, http:// with empty host, missing scheme/host). To fully prevent runtime failures, validate the parsed URL has the required components (at least u.Scheme and u.Host), and return an error when they’re missing.

Suggested change
}
}
if u.Scheme == "" || u.Host == "" {
return nil, fmt.Errorf("invalid endpoint URL %q: missing scheme or host", endpoint)
}

Copilot uses AI. Check for mistakes.
if u.Scheme == "" {
return nil, fmt.Errorf("invalid endpoint URL %q: missing scheme", endpoint)
}
url, _ := url.Parse(endpoint)
Copy link
Copy Markdown
Member

@hzxuzhonghu hzxuzhonghu Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

donot we have already guranteed this always succeed in

func buildSandboxInfo(sandbox *sandboxv1alpha1.Sandbox, podIP string, entry *sandboxEntry) *types.SandboxInfo {
	createdAt := sandbox.GetCreationTimestamp().Time
	expiresAt := createdAt.Add(DefaultSandboxTTL)
	if sandbox.Spec.ShutdownTime != nil {
		expiresAt = sandbox.Spec.ShutdownTime.Time
	}
	accesses := make([]types.SandboxEntryPoint, 0, len(entry.Ports))
	for _, port := range entry.Ports {
		accesses = append(accesses, types.SandboxEntryPoint{
			Path:     port.PathPrefix,
			Protocol: string(port.Protocol),
			Endpoint: net.JoinHostPort(podIP, strconv.Itoa(int(port.Port))),
		})

Copy link
Copy Markdown
Author

@sicaario sicaario Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BuildSandboxInfo does guarantee the format. The validation still handles the case where Protocol is empty: url.Parse("host:port") succeeds but parses it as scheme="host" with an empty Host, giving a silently unusable proxy target. Also keeps buildURL safe for any future callers. Happy to move the validation upstream if you prefer though.

Copy link
Copy Markdown
Member

@hzxuzhonghu hzxuzhonghu Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot get what you mean by but parses it as scheme="host" with an empty Host,.

From the api object validation, we have a hard limit that it must be set to http or https now

Copy link
Copy Markdown
Author

@sicaario sicaario Apr 15, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hzxuzhonghu to address this buildURL now takes protocol as a separate parameter and prepends it explicitly before parsing — so "HTTP" + "host:port" becomes "http://host:port" before url.Parse is called. This removes the misparse concern entirely.

The scheme == "" check is still kept because SandboxEntryPoint.Protocol is a plain string (not the API-validated ProtocolType), and the session manager can produce entries with an empty Protocol field — as seen in several tests. So it's a real path

return url
if !validProxySchemes[u.Scheme] {
return nil, fmt.Errorf("invalid endpoint URL %q: unsupported scheme %q, must be http, https, ws, or wss", endpoint, u.Scheme)
}
if u.Host == "" {
return nil, fmt.Errorf("invalid endpoint URL %q: missing host", endpoint)
}
if portStr := u.Port(); portStr != "" {
port, err := strconv.Atoi(portStr)
if err != nil || port < 1 || port > 65535 {
return nil, fmt.Errorf("invalid endpoint URL %q: port %q out of range (1-65535)", endpoint, portStr)
}
}
return u, nil
}

// handleAgentInvoke handles agent invocation requests
Expand All @@ -148,9 +177,11 @@ func (s *Server) forwardToSandbox(c *gin.Context, sandbox *types.SandboxInfo, pa
targetURL, err := determineUpstreamURL(sandbox, path)
if err != nil {
klog.Errorf("Failed to get sandbox access address %s: %v", sandbox.SandboxID, err)
c.JSON(http.StatusNotFound, gin.H{
"error": err.Error(),
})
if errors.Is(err, errNoEntryPoint) {
Copy link
Copy Markdown
Member

@acsoto acsoto Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this branch is newly distinguishing missing entry points from malformed URLs, it would be good to add a small test that covers EntryPoints == nil/empty and asserts we now return 404 here.

Copy link
Copy Markdown
Author

@sicaario sicaario Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added TestForwardToSandbox_NoEntryPoints in the latest commit (02fae19) which sets EntryPoints: [] and asserts a 404 Not Found response, covering the errNoEntryPoint path.

c.JSON(http.StatusNotFound, gin.H{"error": err.Error()})
} else {
c.JSON(http.StatusInternalServerError, gin.H{"error": "invalid sandbox endpoint configuration"})
}
Comment on lines +180 to +184
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description says the 500 response will include a “descriptive message”, but the current body always returns the generic string "invalid sandbox endpoint configuration". If the intent is to be descriptive to clients, consider including the underlying error message (or a sanitized subset) in the JSON response; alternatively, update the PR description to reflect that the detailed reason is only available in logs.

Copilot uses AI. Check for mistakes.
Comment on lines 179 to +184
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 500 response here includes the detailed endpoint parsing error (and potentially the raw configured endpoint string) in the user-facing JSON. Given handleGetSandboxError masks 500s as "internal server error", consider logging the detailed error but returning a more generic 500 message (optionally with a stable error code) to avoid leaking internal configuration details.

Copilot uses AI. Check for mistakes.
return
}

Expand Down
74 changes: 63 additions & 11 deletions pkg/router/handlers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,8 +393,63 @@ func TestHandleCodeInterpreterInvoke(t *testing.T) {
}

func TestForwardToSandbox_InvalidEndpoint(t *testing.T) {
setupEnv()
defer teardownEnv()
cases := []struct {
name string
endpoint string
}{
{"malformed scheme", "://invalid-url"},
{"empty endpoint", ""},
{"scheme only no host", "http://"},
{"no scheme", "localhost:8080"},
{"unsupported scheme ftp", "ftp://host:21"},
{"unsupported scheme file", "file:///etc/passwd"},
{"port out of range", "http://host:99999"},
}

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
t.Setenv("REDIS_ADDR", "localhost:6379")
t.Setenv("REDIS_PASSWORD", "test-password")
t.Setenv("WORKLOAD_MANAGER_URL", "http://localhost:8080")

config := &Config{Port: "8080"}
server, err := NewServer(config)
if err != nil {
t.Fatalf("Failed to create server: %v", err)
}

server.sessionManager = &mockSessionManager{
sandbox: &types.SandboxInfo{
SandboxID: "test-sandbox",
SessionID: "test-session",
Name: "test-sandbox",
EntryPoints: []types.SandboxEntryPoint{
{Endpoint: tc.endpoint, Path: "/test"},
},
},
}

routerServer := httptest.NewServer(server.engine)
defer routerServer.Close()

client := &http.Client{Timeout: 5 * time.Second}
resp, err := client.Post(routerServer.URL+"/v1/namespaces/default/agent-runtimes/test-agent/invocations/test", "application/json", nil)
if err != nil {
t.Fatalf("Failed to make request: %v", err)
}
defer resp.Body.Close()

if resp.StatusCode != http.StatusInternalServerError {
t.Errorf("[%s] expected %d, got %d", tc.name, http.StatusInternalServerError, resp.StatusCode)
}
})
}
}

func TestForwardToSandbox_NoEntryPoints(t *testing.T) {
t.Setenv("REDIS_ADDR", "localhost:6379")
t.Setenv("REDIS_PASSWORD", "test-password")
t.Setenv("WORKLOAD_MANAGER_URL", "http://localhost:8080")

config := &Config{Port: "8080"}
server, err := NewServer(config)
Expand All @@ -404,16 +459,13 @@ func TestForwardToSandbox_InvalidEndpoint(t *testing.T) {

server.sessionManager = &mockSessionManager{
sandbox: &types.SandboxInfo{
SandboxID: "test-sandbox",
SessionID: "test-session",
Name: "test-sandbox",
EntryPoints: []types.SandboxEntryPoint{
{Endpoint: "://invalid-url", Path: "/test"},
},
SandboxID: "test-sandbox",
SessionID: "test-session",
Name: "test-sandbox",
EntryPoints: []types.SandboxEntryPoint{},
},
}

// run via real server to avoid CloseNotifier panic
routerServer := httptest.NewServer(server.engine)
defer routerServer.Close()

Expand All @@ -424,8 +476,8 @@ func TestForwardToSandbox_InvalidEndpoint(t *testing.T) {
}
defer resp.Body.Close()

if resp.StatusCode != http.StatusInternalServerError {
t.Errorf("Expected status code %d, got %d", http.StatusInternalServerError, resp.StatusCode)
if resp.StatusCode != http.StatusNotFound {
t.Errorf("Expected status code %d, got %d", http.StatusNotFound, resp.StatusCode)
}
}

Expand Down
11 changes: 10 additions & 1 deletion pkg/router/jwt_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,16 @@ func TestGetPrivateKeyPEM(t *testing.T) {
privateKey, err := x509.ParsePKCS1PrivateKey(block.Bytes)
assert.NoError(t, err)
assert.NotNil(t, privateKey)
assert.Equal(t, manager.privateKey, privateKey)

// Compare key components instead of whole struct — ParsePKCS1PrivateKey may
// not precompute Dp/Dq/Qinv identically to the original key.
assert.Equal(t, 0, manager.privateKey.PublicKey.N.Cmp(privateKey.PublicKey.N), "Public key N should match")
assert.Equal(t, manager.privateKey.PublicKey.E, privateKey.PublicKey.E, "Public key E should match")
assert.Equal(t, 0, manager.privateKey.D.Cmp(privateKey.D), "Private exponent D should match")
assert.Equal(t, len(manager.privateKey.Primes), len(privateKey.Primes), "Number of primes should match")
for i := range manager.privateKey.Primes {
assert.Equal(t, 0, manager.privateKey.Primes[i].Cmp(privateKey.Primes[i]), "Prime %d should match", i)
}
}

func TestLoadPrivateKeyPEM(t *testing.T) {
Expand Down
Loading