Skip to content

Refactor to use httputil.ReverseProxy #342

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

Merged
merged 3 commits into from
Oct 13, 2025
Merged

Refactor to use httputil.ReverseProxy #342

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1a6c864
Refactor to use httputil.ReverseProxy
dwrz Oct 9, 2025
9c00da7
Refactor TestProcess_ForceStopWithKill test
dwrz Oct 13, 2025
d3882e2
Fix gin interface conversion panic
dwrz Oct 13, 2025
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
8 changes: 8 additions & 0 deletions proxy/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package config
import (
"fmt"
"io"
"net/url"
"os"
"regexp"
"runtime"
Expand Down Expand Up @@ -342,6 +343,13 @@ func LoadConfigFromReader(r io.Reader) (Config, error) {
}
}

// Validate the proxy URL.
if _, err := url.Parse(modelConfig.Proxy); err != nil {
return Config{}, fmt.Errorf(
"model %s: invalid proxy URL: %w", modelId, err,
)
}
Comment on lines +346 to +351
Copy link

@coderabbitai coderabbitai bot Oct 11, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Verify if stricter URL validation is needed.

The validation correctly catches URL parse errors. However, url.Parse is permissive and accepts empty strings, relative URLs, and URLs without scheme or host. Since the PR introduces httputil.ReverseProxy, which typically requires complete URLs with scheme and host, verify whether additional validation is needed.

Run the following script to check how the reverse proxy handles the parsed URL:

🏁 Script executed:

#!/bin/bash
# Description: Check if reverse proxy validates URL structure (scheme/host)
# Expected: Find validation or error handling for incomplete URLs

# Search for reverse proxy setup and URL usage
ast-grep --pattern 'reverseProxy = &httputil.ReverseProxy{$$$}'

# Search for proxy URL validation in process.go
rg -n --type=go -C5 'modelConfig.Proxy|ProxyURL' proxy/process.go

Length of output: 0

🏁 Script executed:

#!/bin/bash
echo "Searching for NewSingleHostReverseProxy usage..."
rg -nP 'NewSingleHostReverseProxy' -tgo -C3

echo "Searching for ReverseProxy struct literal..."
rg -nP '&httputil.ReverseProxy' -tgo -C3

echo "Searching for modelConfig.Proxy usage..."
rg -n 'modelConfig.Proxy' -tgo -C5

Length of output: 4449

Enforce complete proxy URL validation
After parsing, ensure both u.Scheme and u.Host are non-empty (e.g.

u, err := url.Parse(modelConfig.Proxy)
if err != nil || u.Scheme == "" || u.Host == "" {
    return Config{}, fmt.Errorf("model %s: proxy must include scheme and host", modelId)
}
```)

<details>
<summary>🤖 Prompt for AI Agents</summary>

In proxy/config/config.go around lines 346 to 351, the URL validation only
checks parse error and may accept incomplete URLs; update the logic to parse the
proxy into u and treat it as invalid if err != nil or u.Scheme == "" or u.Host
== "", returning a formatted error like "model %s: proxy must include scheme and
host" with modelId and the underlying error when present; ensure you use the
parsed u variable (not the raw string) for these checks and preserve existing
error-wrapping semantics.


</details>

<!-- This is an auto-generated comment by CodeRabbit -->


config.Models[modelId] = modelConfig
}

Expand Down
83 changes: 28 additions & 55 deletions proxy/process.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,11 @@ import (
"context"
"errors"
"fmt"
"io"
"net"
"net/http"
"net/http/httputil"
"net/url"
"os/exec"
"strconv"
"strings"
"sync"
"syscall"
Expand Down Expand Up @@ -39,9 +38,10 @@ const (
)

type Process struct {
ID string
config config.ModelConfig
cmd *exec.Cmd
ID string
config config.ModelConfig
cmd *exec.Cmd
reverseProxy *httputil.ReverseProxy

// PR #155 called to cancel the upstream process
cancelUpstream context.CancelFunc
Expand Down Expand Up @@ -81,10 +81,29 @@ func NewProcess(ID string, healthCheckTimeout int, config config.ModelConfig, pr
concurrentLimit = config.ConcurrencyLimit
}

// Setup the reverse proxy.
proxyURL, err := url.Parse(config.Proxy)
if err != nil {
proxyLogger.Errorf("<%s> invalid proxy URL %q: %v", ID, config.Proxy, err)
}

var reverseProxy *httputil.ReverseProxy
if proxyURL != nil {
reverseProxy = httputil.NewSingleHostReverseProxy(proxyURL)
reverseProxy.ModifyResponse = func(resp *http.Response) error {
// prevent nginx from buffering streaming responses (e.g., SSE)
if strings.Contains(strings.ToLower(resp.Header.Get("Content-Type")), "text/event-stream") {
resp.Header.Set("X-Accel-Buffering", "no")
}
return nil
}
}

return &Process{
ID: ID,
config: config,
cmd: nil,
reverseProxy: reverseProxy,
cancelUpstream: nil,
processLogger: processLogger,
proxyLogger: proxyLogger,
Expand Down Expand Up @@ -434,56 +453,10 @@ func (p *Process) ProxyRequest(w http.ResponseWriter, r *http.Request) {
startDuration = time.Since(beginStartTime)
}

proxyTo := p.config.Proxy
client := &http.Client{}
req, err := http.NewRequestWithContext(r.Context(), r.Method, proxyTo+r.URL.String(), r.Body)
if err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
req.Header = r.Header.Clone()

contentLength, err := strconv.ParseInt(req.Header.Get("content-length"), 10, 64)
if err == nil {
req.ContentLength = contentLength
}

resp, err := client.Do(req)
if err != nil {
http.Error(w, err.Error(), http.StatusBadGateway)
return
}
defer resp.Body.Close()
for k, vv := range resp.Header {
for _, v := range vv {
w.Header().Add(k, v)
}
}
// prevent nginx from buffering streaming responses (e.g., SSE)
if strings.Contains(strings.ToLower(resp.Header.Get("Content-Type")), "text/event-stream") {
w.Header().Set("X-Accel-Buffering", "no")
}
w.WriteHeader(resp.StatusCode)

// faster than io.Copy when streaming
buf := make([]byte, 32*1024)
for {
n, err := resp.Body.Read(buf)
if n > 0 {
if _, writeErr := w.Write(buf[:n]); writeErr != nil {
return
}
if flusher, ok := w.(http.Flusher); ok {
flusher.Flush()
}
}
if err == io.EOF {
break
}
if err != nil {
http.Error(w, err.Error(), http.StatusBadGateway)
return
}
if p.reverseProxy != nil {
p.reverseProxy.ServeHTTP(w, r)
} else {
http.Error(w, fmt.Sprintf("No reverse proxy available for %s", p.ID), http.StatusInternalServerError)
}

totalTime := time.Since(requestBeginTime)
Expand Down
4 changes: 3 additions & 1 deletion proxy/process_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,9 @@ func TestProcess_ForceStopWithKill(t *testing.T) {
if runtime.GOOS == "windows" {
assert.Contains(t, w.Body.String(), "wsarecv: An existing connection was forcibly closed by the remote host")
} else {
assert.Contains(t, w.Body.String(), "unexpected EOF")
// Upstream may be killed mid-response.
// Assert an incomplete or partial response.
assert.NotEqual(t, "12345", w.Body.String())
}

close(waitChan)
Expand Down
Loading