Skip to content
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

Add Safety Writer Option #25

Closed
wants to merge 2 commits into from
Closed

Conversation

tigerwill90
Copy link
Owner

@tigerwill90 tigerwill90 commented Aug 15, 2023

Description

This PR introduces a new "Response Writer Safety" feature to address potential panics that can arise when the router or its middlewares make assumptions about the capabilities of custom http.ResponseWriter implementations.

Background

Certain middlewares or handlers, like http.TimeoutHandler, wrap the original http.ResponseWriter and might return a new writer that omits certain interfaces, such as http.Flusher. Routers like Fox, Gin, and Echo traditionally assume these interfaces to be present. The absence of these interfaces can lead to undefined behavior or even cause a panic under specific circumstances.

For example, the following implementation with httputil.ReverseProxy will result in a panic for Fox, Echo and Gin.

func main() {
	url, _ := url.Parse("https://tf1pub.drawapi.com/ssai/multi-slots/16-avec-des-trous")
	proxy := httputil.NewSingleHostReverseProxy(url)

	proxy.Director = func(req *http.Request) {
		req.URL.Scheme = url.Scheme
		req.URL.Host = url.Host
		req.URL.Path = url.Path
		req.Host = url.Host
	}

	f := fox.New()
	f.MustHandle(http.MethodGet, "/", FoxProxyRequestHandler(proxy))

	g := gin.Default()
	g.Handle(http.MethodGet, "/", GinProxyRequestHandler(proxy))

	e := echo.New()
	e.GET("/", EchoProxyRequestHandler(proxy))

	log.Fatalln(http.ListenAndServe(":8080", http.TimeoutHandler(f, 1*time.Second, http.StatusText(http.StatusServiceUnavailable))))
}

func FoxProxyRequestHandler(proxy *httputil.ReverseProxy) fox.HandlerFunc {
	return func(c fox.Context) {
		proxy.ServeHTTP(c.Writer(), c.Request())
	}
}

func GinProxyRequestHandler(proxy *httputil.ReverseProxy) gin.HandlerFunc {
	return func(c *gin.Context) {
		proxy.ServeHTTP(c.Writer, c.Request)
	}
}

func EchoProxyRequestHandler(proxy *httputil.ReverseProxy) echo.HandlerFunc {
	return func(c echo.Context) error {
		proxy.ServeHTTP(c.Response(), c.Request())
		return nil
	}
}

When httputil.ReverseProxy checks if the provided writer implements http.Flusher and the check succeeds (because it use the writer from, say, Echo or Gin), it will attempt to use it (based on the number of bytes already written). But since the underlying writer (from http.TimeoutHandler) doesn't actually support this, a panic occurs.

Introduction of the WithWriterSafety Option:

This PR introduces an option called WithWriterSafety to ensure more predictable behavior when working with different http.ResponseWriter implementations.

Behavior

  • Safe Mode (WithWriterSafety Enabled):
    The router derives the protocol from the request's ProtoMajor and conducts explicit type assertions on the provided http.ResponseWriter. This ensures compatibility across various writer implementations and safeguards against unpredictable outcomes.

  • Optimistic Mode (Default Behavior without WithWriterSafety):
    The router operates under an optimistic assumption that the provided http.ResponseWriter fully supports the necessary interfaces for the request's protocol. Specifically, for HTTP/1.x requests, the writer should implement http.Flusher, http.Hijacker, and io.ReaderFrom. For HTTP/2 requests, the writer should support http.Flusher and http.Pusher.

Example

f := fox.New(fox.WithWriterSafety(true))
f.MustHandle(http.MethodGet, "/", FoxProxyRequestHandler(proxy))
log.Fatalln(http.ListenAndServe(":8080", http.TimeoutHandler(f, 1*time.Second, http.StatusText(http.StatusServiceUnavailable))))

Peformance

By default, the router operates in a mode optimized for performance, which may potentially be less safe. Benchmarks show a performance drop of approximately 30% when switching to Safe Mode due to the multiple interface assertions ensuring safety. Nonetheless, even in this mode, Fox remains as performant as Gin and Echo in most scenarios. As the Fox ecosystem's middleware ensures the necessary interfaces are satisfied, the more performant mode has been chosen as the default. However, for added safety, users can opt-in to the safer mode.

…er capability to avoid UB in certain scenario
@tigerwill90 tigerwill90 requested a review from pawndev August 15, 2023 14:14
@codecov
Copy link

codecov bot commented Aug 15, 2023

Codecov Report

Merging #25 (7e0a085) into master (8e5b17e) will decrease coverage by 2.00%.
The diff coverage is 52.22%.

@@            Coverage Diff             @@
##           master      #25      +/-   ##
==========================================
- Coverage   86.62%   84.62%   -2.00%     
==========================================
  Files          12       12              
  Lines        1787     1841      +54     
==========================================
+ Hits         1548     1558      +10     
- Misses        205      251      +46     
+ Partials       34       32       -2     
Flag Coverage Δ
coverage.txt 84.62% <52.22%> (-2.00%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
response_writer.go 42.46% <7.40%> (-7.79%) ⬇️
context.go 83.33% <67.85%> (-6.24%) ⬇️
fox.go 91.86% <100.00%> (ø)
helpers.go 100.00% <100.00%> (ø)
options.go 100.00% <100.00%> (ø)

@tigerwill90 tigerwill90 deleted the feat/response-writer-safety branch January 24, 2024 10:54
@tigerwill90 tigerwill90 restored the feat/response-writer-safety branch January 24, 2024 10:54
@tigerwill90 tigerwill90 deleted the feat/response-writer-safety branch February 15, 2024 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant