fix(proxy): harden response limiter, document header stripping, and exercise body-size test
- Add comments to ModifyResponse explaining why browser security headers are stripped from registry responses (Docker clients, not browsers) - Cap limitedReadCloser.Read return value to guard against misbehaving readers and include remaining bytes in error message for debugging - Rewrite TestHandler_ReadsMaxBodySizeConfig to send an oversized body through a real backend, asserting 413 instead of testing with nil body - Wrap GetImageExposedPorts error with image ref context in target_metadata
- commit
- de49ebd43540f7fee060343a5b139dd3477acc0e
- author
- brice · · 2026-03-16 05:21 UTC
- parent
- 278618d
- signature
- unsigned
3 files · +40 -5
M internal/adapters/in/http/proxy/forwarder.go +18 -1
| @@ -156,10 +156,19 @@ func (h *Handler) forwardToRegistry(w http.ResponseWriter, r *http.Request, regi | |||
| 156 | 156 | log.Error().Err(proxyErr).Int("registry_port", registryPort).Msg("registry proxy error") | |
| 157 | 157 | proxyError(w, "Registry Unavailable", http.StatusServiceUnavailable) | |
| 158 | 158 | }, | |
| 159 | + | // ModifyResponse adjusts registry responses for Docker client consumption. | |
| 160 | + | // Registry API responses are consumed by Docker CLI/daemon, not browsers. | |
| 161 | + | // Upstream registry servers may inject browser-oriented security headers | |
| 162 | + | // (e.g., HSTS, CSP, X-Frame-Options) that are irrelevant for API clients | |
| 163 | + | // and could conflict with Gordon's own security headers on browser-facing | |
| 164 | + | // routes. We strip them here to ensure clean, predictable registry responses. | |
| 159 | 165 | ModifyResponse: func(resp *http.Response) error { | |
| 160 | 166 | resp.Header.Set("X-Proxied-By", "Gordon") | |
| 161 | 167 | resp.Header.Set("X-Registry-Backend", "gordon-registry") | |
| 162 | 168 | ||
| 169 | + | // Remove browser-oriented security headers injected by the upstream | |
| 170 | + | // registry. These are unnecessary for Docker client API traffic and | |
| 171 | + | // would conflict with headers set on browser-facing proxy routes. | |
| 163 | 172 | resp.Header.Del("X-Content-Type-Options") | |
| 164 | 173 | resp.Header.Del("X-Frame-Options") | |
| 165 | 174 | resp.Header.Del("X-XSS-Protection") | |
| @@ -246,14 +255,22 @@ func (l *limitedReadCloser) Read(p []byte) (int, error) { | |||
| 246 | 255 | l.closed = true | |
| 247 | 256 | l.ReadCloser.Close() | |
| 248 | 257 | } | |
| 249 | - | return 0, fmt.Errorf("response body exceeded maximum size limit") | |
| 258 | + | return 0, fmt.Errorf("response body exceeded maximum size limit (remaining: %d bytes)", l.remaining) | |
| 250 | 259 | } | |
| 251 | 260 | toRead := len(p) | |
| 252 | 261 | if int64(toRead) > l.remaining { | |
| 253 | 262 | toRead = int(l.remaining) | |
| 254 | 263 | } | |
| 255 | 264 | n, err := l.ReadCloser.Read(p[:toRead]) | |
| 265 | + | // Cap n to the requested slice to guard against misbehaving readers | |
| 266 | + | // that return more bytes than the buffer length. | |
| 267 | + | if n > toRead { | |
| 268 | + | n = toRead | |
| 269 | + | } | |
| 256 | 270 | l.remaining -= int64(n) | |
| 271 | + | if l.remaining < 0 { | |
| 272 | + | l.remaining = 0 | |
| 273 | + | } | |
| 257 | 274 | return n, err | |
| 258 | 275 | } | |
| 259 | 276 | ||
| @@ -156,10 +156,19 @@ func (h *Handler) forwardToRegistry(w http.ResponseWriter, r *http.Request, regi | |||||
| 156 | log.Error().Err(proxyErr).Int("registry_port", registryPort).Msg("registry proxy error") | 156 | log.Error().Err(proxyErr).Int("registry_port", registryPort).Msg("registry proxy error") | ||
| 157 | proxyError(w, "Registry Unavailable", http.StatusServiceUnavailable) | 157 | proxyError(w, "Registry Unavailable", http.StatusServiceUnavailable) | ||
| 158 | }, | 158 | }, | ||
| 159 | + | // ModifyResponse adjusts registry responses for Docker client consumption. | |||
| 160 | + | // Registry API responses are consumed by Docker CLI/daemon, not browsers. | |||
| 161 | + | // Upstream registry servers may inject browser-oriented security headers | |||
| 162 | + | // (e.g., HSTS, CSP, X-Frame-Options) that are irrelevant for API clients | |||
| 163 | + | // and could conflict with Gordon's own security headers on browser-facing | |||
| 164 | + | // routes. We strip them here to ensure clean, predictable registry responses. | |||
| 159 | ModifyResponse: func(resp *http.Response) error { | 165 | ModifyResponse: func(resp *http.Response) error { | ||
| 160 | resp.Header.Set("X-Proxied-By", "Gordon") | 166 | resp.Header.Set("X-Proxied-By", "Gordon") | ||
| 161 | resp.Header.Set("X-Registry-Backend", "gordon-registry") | 167 | resp.Header.Set("X-Registry-Backend", "gordon-registry") | ||
| 162 | 168 | ||||
| 169 | + | // Remove browser-oriented security headers injected by the upstream | |||
| 170 | + | // registry. These are unnecessary for Docker client API traffic and | |||
| 171 | + | // would conflict with headers set on browser-facing proxy routes. | |||
| 163 | resp.Header.Del("X-Content-Type-Options") | 172 | resp.Header.Del("X-Content-Type-Options") | ||
| 164 | resp.Header.Del("X-Frame-Options") | 173 | resp.Header.Del("X-Frame-Options") | ||
| 165 | resp.Header.Del("X-XSS-Protection") | 174 | resp.Header.Del("X-XSS-Protection") | ||
| @@ -246,14 +255,22 @@ func (l *limitedReadCloser) Read(p []byte) (int, error) { | |||||
| 246 | l.closed = true | 255 | l.closed = true | ||
| 247 | l.ReadCloser.Close() | 256 | l.ReadCloser.Close() | ||
| 248 | } | 257 | } | ||
| 249 | - | return 0, fmt.Errorf("response body exceeded maximum size limit") | 258 | + | return 0, fmt.Errorf("response body exceeded maximum size limit (remaining: %d bytes)", l.remaining) |
| 250 | } | 259 | } | ||
| 251 | toRead := len(p) | 260 | toRead := len(p) | ||
| 252 | if int64(toRead) > l.remaining { | 261 | if int64(toRead) > l.remaining { | ||
| 253 | toRead = int(l.remaining) | 262 | toRead = int(l.remaining) | ||
| 254 | } | 263 | } | ||
| 255 | n, err := l.ReadCloser.Read(p[:toRead]) | 264 | n, err := l.ReadCloser.Read(p[:toRead]) | ||
| 265 | + | // Cap n to the requested slice to guard against misbehaving readers | |||
| 266 | + | // that return more bytes than the buffer length. | |||
| 267 | + | if n > toRead { | |||
| 268 | + | n = toRead | |||
| 269 | + | } | |||
| 256 | l.remaining -= int64(n) | 270 | l.remaining -= int64(n) | ||
| 271 | + | if l.remaining < 0 { | |||
| 272 | + | l.remaining = 0 | |||
| 273 | + | } | |||
| 257 | return n, err | 274 | return n, err | ||
| 258 | } | 275 | } | ||
| 259 | 276 | ||||
M internal/adapters/in/http/proxy/handler_test.go +21 -3
| @@ -1,6 +1,8 @@ | |||
| 1 | 1 | package proxy | |
| 2 | 2 | ||
| 3 | 3 | import ( | |
| 4 | + | "bytes" | |
| 5 | + | "io" | |
| 4 | 6 | "net" | |
| 5 | 7 | "net/http" | |
| 6 | 8 | "net/http/httptest" | |
| @@ -111,22 +113,38 @@ func TestHandler_ProxiesToTarget(t *testing.T) { | |||
| 111 | 113 | } | |
| 112 | 114 | ||
| 113 | 115 | func TestHandler_ReadsMaxBodySizeConfig(t *testing.T) { | |
| 116 | + | // Backend that reads the full request body, triggering MaxBytesReader. | |
| 117 | + | backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 118 | + | _, _ = io.ReadAll(r.Body) | |
| 119 | + | w.WriteHeader(http.StatusOK) | |
| 120 | + | })) | |
| 121 | + | defer backend.Close() | |
| 122 | + | ||
| 114 | 123 | proxySvc := inmocks.NewMockProxyService(t) | |
| 124 | + | backendPort := backend.Listener.Addr().(*net.TCPAddr).Port | |
| 115 | 125 | ||
| 116 | 126 | proxySvc.EXPECT().ProxyConfig().Return(in.ProxyServiceConfig{ | |
| 117 | 127 | MaxBodySize: 1024, | |
| 118 | 128 | }) | |
| 119 | 129 | proxySvc.EXPECT().IsRegistryDomain("app.example.com").Return(false) | |
| 120 | - | proxySvc.EXPECT().GetTarget(mock.Anything, "app.example.com").Return(nil, domain.ErrNoTargetAvailable) | |
| 130 | + | proxySvc.EXPECT().GetTarget(mock.Anything, "app.example.com").Return(&domain.ProxyTarget{ | |
| 131 | + | Host: "127.0.0.1", | |
| 132 | + | Port: backendPort, | |
| 133 | + | ContainerID: "c-1", | |
| 134 | + | Scheme: "http", | |
| 135 | + | }, nil) | |
| 136 | + | proxySvc.EXPECT().TrackInFlight("c-1").Return(func() {}) | |
| 121 | 137 | ||
| 122 | 138 | handler := NewHandler(proxySvc, testLogger()) | |
| 123 | 139 | ||
| 124 | - | req := httptest.NewRequest(http.MethodPost, "http://app.example.com/test", nil) | |
| 140 | + | // Send a body larger than the 1024-byte limit. | |
| 141 | + | largeBody := bytes.NewReader(make([]byte, 2048)) | |
| 142 | + | req := httptest.NewRequest(http.MethodPost, "http://app.example.com/test", largeBody) | |
| 125 | 143 | req.Host = "app.example.com" | |
| 126 | 144 | w := httptest.NewRecorder() | |
| 127 | 145 | handler.ServeHTTP(w, req) | |
| 128 | 146 | ||
| 129 | - | assert.Equal(t, http.StatusNotFound, w.Code) | |
| 147 | + | assert.Equal(t, http.StatusRequestEntityTooLarge, w.Code) | |
| 130 | 148 | } | |
| 131 | 149 | ||
| 132 | 150 | func TestHandler_UpdatedConfigReflected(t *testing.T) { | |
| @@ -1,6 +1,8 @@ | |||||
| 1 | package proxy | 1 | package proxy | ||
| 2 | 2 | ||||
| 3 | import ( | 3 | import ( | ||
| 4 | + | "bytes" | |||
| 5 | + | "io" | |||
| 4 | "net" | 6 | "net" | ||
| 5 | "net/http" | 7 | "net/http" | ||
| 6 | "net/http/httptest" | 8 | "net/http/httptest" | ||
| @@ -111,22 +113,38 @@ func TestHandler_ProxiesToTarget(t *testing.T) { | |||||
| 111 | } | 113 | } | ||
| 112 | 114 | ||||
| 113 | func TestHandler_ReadsMaxBodySizeConfig(t *testing.T) { | 115 | func TestHandler_ReadsMaxBodySizeConfig(t *testing.T) { | ||
| 116 | + | // Backend that reads the full request body, triggering MaxBytesReader. | |||
| 117 | + | backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
| 118 | + | _, _ = io.ReadAll(r.Body) | |||
| 119 | + | w.WriteHeader(http.StatusOK) | |||
| 120 | + | })) | |||
| 121 | + | defer backend.Close() | |||
| 122 | + | ||||
| 114 | proxySvc := inmocks.NewMockProxyService(t) | 123 | proxySvc := inmocks.NewMockProxyService(t) | ||
| 124 | + | backendPort := backend.Listener.Addr().(*net.TCPAddr).Port | |||
| 115 | 125 | ||||
| 116 | proxySvc.EXPECT().ProxyConfig().Return(in.ProxyServiceConfig{ | 126 | proxySvc.EXPECT().ProxyConfig().Return(in.ProxyServiceConfig{ | ||
| 117 | MaxBodySize: 1024, | 127 | MaxBodySize: 1024, | ||
| 118 | }) | 128 | }) | ||
| 119 | proxySvc.EXPECT().IsRegistryDomain("app.example.com").Return(false) | 129 | proxySvc.EXPECT().IsRegistryDomain("app.example.com").Return(false) | ||
| 120 | - | proxySvc.EXPECT().GetTarget(mock.Anything, "app.example.com").Return(nil, domain.ErrNoTargetAvailable) | 130 | + | proxySvc.EXPECT().GetTarget(mock.Anything, "app.example.com").Return(&domain.ProxyTarget{ |
| 131 | + | Host: "127.0.0.1", | |||
| 132 | + | Port: backendPort, | |||
| 133 | + | ContainerID: "c-1", | |||
| 134 | + | Scheme: "http", | |||
| 135 | + | }, nil) | |||
| 136 | + | proxySvc.EXPECT().TrackInFlight("c-1").Return(func() {}) | |||
| 121 | 137 | ||||
| 122 | handler := NewHandler(proxySvc, testLogger()) | 138 | handler := NewHandler(proxySvc, testLogger()) | ||
| 123 | 139 | ||||
| 124 | - | req := httptest.NewRequest(http.MethodPost, "http://app.example.com/test", nil) | 140 | + | // Send a body larger than the 1024-byte limit. |
| 141 | + | largeBody := bytes.NewReader(make([]byte, 2048)) | |||
| 142 | + | req := httptest.NewRequest(http.MethodPost, "http://app.example.com/test", largeBody) | |||
| 125 | req.Host = "app.example.com" | 143 | req.Host = "app.example.com" | ||
| 126 | w := httptest.NewRecorder() | 144 | w := httptest.NewRecorder() | ||
| 127 | handler.ServeHTTP(w, req) | 145 | handler.ServeHTTP(w, req) | ||
| 128 | 146 | ||||
| 129 | - | assert.Equal(t, http.StatusNotFound, w.Code) | 147 | + | assert.Equal(t, http.StatusRequestEntityTooLarge, w.Code) |
| 130 | } | 148 | } | ||
| 131 | 149 | ||||
| 132 | func TestHandler_UpdatedConfigReflected(t *testing.T) { | 150 | func TestHandler_UpdatedConfigReflected(t *testing.T) { | ||
M internal/usecase/proxy/target_metadata.go +1 -1
| @@ -48,7 +48,7 @@ func (s *Service) resolveTargetMetadata(ctx context.Context, imageRef string) (T | |||
| 48 | 48 | if port == 0 { | |
| 49 | 49 | exposedPorts, portsErr := s.runtime.GetImageExposedPorts(ctx, imageRef) | |
| 50 | 50 | if portsErr != nil { | |
| 51 | - | return TargetMetadata{}, portsErr | |
| 51 | + | return TargetMetadata{}, fmt.Errorf("failed to get exposed ports for image %s: %w", imageRef, portsErr) | |
| 52 | 52 | } | |
| 53 | 53 | if len(exposedPorts) == 0 { | |
| 54 | 54 | return TargetMetadata{}, fmt.Errorf("no exposed ports found for image %s", imageRef) | |
| @@ -48,7 +48,7 @@ func (s *Service) resolveTargetMetadata(ctx context.Context, imageRef string) (T | |||||
| 48 | if port == 0 { | 48 | if port == 0 { | ||
| 49 | exposedPorts, portsErr := s.runtime.GetImageExposedPorts(ctx, imageRef) | 49 | exposedPorts, portsErr := s.runtime.GetImageExposedPorts(ctx, imageRef) | ||
| 50 | if portsErr != nil { | 50 | if portsErr != nil { | ||
| 51 | - | return TargetMetadata{}, portsErr | 51 | + | return TargetMetadata{}, fmt.Errorf("failed to get exposed ports for image %s: %w", imageRef, portsErr) |
| 52 | } | 52 | } | ||
| 53 | if len(exposedPorts) == 0 { | 53 | if len(exposedPorts) == 0 { | ||
| 54 | return TargetMetadata{}, fmt.Errorf("no exposed ports found for image %s", imageRef) | 54 | return TargetMetadata{}, fmt.Errorf("no exposed ports found for image %s", imageRef) | ||