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

📝 [Proposal]: Add silent request termination #3252

Closed
4 of 8 tasks
grivera64 opened this issue Dec 17, 2024 · 14 comments · Fixed by #3257
Closed
4 of 8 tasks

📝 [Proposal]: Add silent request termination #3252

grivera64 opened this issue Dec 17, 2024 · 14 comments · Fixed by #3257

Comments

@grivera64
Copy link
Member

grivera64 commented Dec 17, 2024

Feature Proposal Description

This issue proposes to add a method to fiber.Ctx that allows users to silently terminate HTTP requests in Fiber without sending any response headers nor response body. It should be a new method for the Ctx interface.

This feature will be helpful for:

  • DDOS protection (avoid responding to malicious traffic)
  • Hiding endpoints (stop bots from scraping sensitive endpoints by ignoring unauthenticated requests)

Note: While these methods are achievable through using a reverse proxy (e.g. NGINX), providing a native way
of doing this in Fiber will give more control to users of how to implement the above for their use case (e.g. a user's app is running on a low-resource machine).

This feature will work by closing the underlying context's net.Conn using the fasthttp.(*RequestCtx).Conn().Close() method. Fiber provides access to the underlying fasthttp.(*RequestCtx) through c.Context(). Thank you, @guno1928, for helping me find the simplest way to do this on Fiber's Discord Server! Your help is greatly appreciated.

The method's name is currently undecided, but here are a few ideas:

  • c.End() (look at Alignment with Express API) (Drop() is different from the Express equivalent res.end())
  • c.Close()
  • c.Terminate()
  • c.Stop()
  • c.Drop()

Please feel free to give to an opinion of which of these (or any other recommendations) would be a clear name for the method.

Alignment with Express API

The Express API uses the function response.connection.end() response.connection.destroy() to silently terminate HTTP requests:

app.get("/", (request, response) => {
  response.connection.destroy();
  return;
});

To be similar to Express.js, this feature proposes to use a single method for the same functionality, called Drop().

app.Get("/", func (c fiber.Ctx) error {
 return c.Drop()
})

HTTP RFC Standards Compliance

N/A, this feature will not affect how Fiber complies with the HTTP RFC standards.

API Stability

This feature will not require changing any existing methods in fiber.Ctx, so it should be safe to add. As fasthttp's interface is stable, there should be minimal changes or deprecations in the future.

Feature Examples

package main

import (
	"log"
	"net"

	"github.com/gofiber/fiber/v3"
)

func main() {
	app := fiber.New()
	app.Get("/", func (c fiber.Ctx) error {
		name := c.Query("name")
		// Silently terminate requests without name
		if name == "" {
			return c.Drop()
		}
		return c.SendString("Hello, " + name + "!")
	})
	log.Fatal(app.Listen(":3000"))
}

Checklist:

  • I agree to follow Fiber's Code of Conduct.
  • I have searched for existing issues that describe my proposal before opening this one.
  • I understand that a proposal that does not meet these guidelines may be closed without explanation.
@gaby
Copy link
Member

gaby commented Dec 17, 2024

I think c.End() is fine. It's simple and short.

@gaby gaby moved this to Todo in v3 Dec 17, 2024
@grivera64
Copy link
Member Author

I experimented with a few examples of where ExpressJS uses end(), and it looks like there may be confusion to directly translate response.connection.end() as this feature's c.End() function. For example:

app.get("/", (req, res) => {
  res.write("Hello ")
  res.connection.end()
  res.write("World")
}); // => "Hello "

versus

app.Get("/", func (c fiber.Ctx) error {
  c.WriteString("Hello ")
  c.End()
  c.WriteString("World")
  return nil
}) // => No response

Would this cause any confusion to use the same name end()? To truly mimic exactly what the Express example does in the above, it would require this approach:

app.Get("/", func (c fiber.Ctx) error {
  return c.SendStreamWriter(func (w *bufio.Writer) {
    w.WriteString("Hello ")
    w.Flush()
    return
    w.WriteString("World")
  })
}) // => "Hello "

@gaby
Copy link
Member

gaby commented Dec 18, 2024

@grivera64 Hmmm that's confusing. My guess is that connection.end() kills the TCP_CONN to the server. In fasthttp case we dont see the "hello" cause it hasnt been flushed. While express does that on each write()

@grivera64
Copy link
Member Author

@grivera64 Hmmm that's confusing. My guess is that connection.end() kills the TCP_CONN to the server. In fasthttp case we dont see the "hello" cause it hasnt been flushed. While express does that on each write()

@gaby Yes, and since fasthttp doesn't do any work until after the handler finishes, it will not send any data first.

To avoid confusion, would it make sense to use a different method name to match the feature's current behavior? Or would it be better to find a different way to achieve the same behavior as end() in the ExpressJS API?

@gaby
Copy link
Member

gaby commented Dec 18, 2024

@grivera64 Hmmm that's confusing. My guess is that connection.end() kills the TCP_CONN to the server. In fasthttp case we dont see the "hello" cause it hasnt been flushed. While express does that on each write()

@gaby Yes, and since fasthttp doesn't do any work until after the handler finishes, it will not send any data first.

To avoid confusion, would it make sense to use a different method name to match the feature's current behavior? Or would it be better to find a different way to achieve the same behavior as end() in the ExpressJS API?

I wonder if the function fasthttp uses to flush when the handler returns is public. If so we could call it then call close() in a wrapper

@gaby
Copy link
Member

gaby commented Dec 18, 2024

You compared against express.js, how does it compare to net/http?

@grivera64
Copy link
Member Author

grivera64 commented Dec 18, 2024

You compared against express.js, how does it compare to net/http?

In net/http, it looks like it requires:

http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
	w.Write([]byte("Hello "))
	if flusher, ok := w.(http.Flusher); ok {
		flusher.Flush()
	}

	conn, _, err := w.(http.Hijacker).Hijack()
	if err != nil {
			http.Error(w, err.Error(), http.StatusInternalServerError)
			return
	}
	conn.Close()

	w.Write([]byte("World"))
})

To do this in net/http, manual flushing is also required, since the writer is buffered. Otherwise, the request body is dropped (with a 200 status code response header).

@gaby
Copy link
Member

gaby commented Dec 18, 2024

@grivera64 Is there a way to send no data at all?

I'm not aure if we should follow Express.js or net/http in this case.

@grivera64
Copy link
Member Author

grivera64 commented Dec 18, 2024

@grivera64 Is there a way to send no data at all?

I'm not aure if we should follow Express.js or net/http in this case.

Actually, it does not send data at all. I was testing net/http on Replit.com and it was inconsistently sending a 200 OK and no response (maybe has to do with how replit works). After running the test locally, removing the flush causes the server to never respond:

http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
    fmt.Fprintf(w, "Hello ");
                                                                    
    conn, _, err := w.(http.Hijacker).Hijack()
    if err != nil {
        http.Error(w, "Internal Server Error", http.StatusInternalServerError)
        return
    }
    conn.Close()
                                                                    
    fmt.Fprintf(w, "World");
}) // curl address:port => curl: (18) transfer closed with outstanding read data remaining

@grivera64
Copy link
Member Author

grivera64 commented Dec 18, 2024

Here is a better example that matches with the feature example:

// GET "/" (net/http version)
http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
    name := r.URL.Query().Get("name")
    if name == "" {
        conn, _, err := w.(http.Hijacker).Hijack()
        if err != nil {
            http.Error(w, "Internal Server Error", http.StatusInternalServerError)
            return
        }
        conn.Close()
    }
    fmt.Fprintf(w, "Hello, %s!", name)
}) // curl http://localhost:3000 => curl: (52) Empty reply from server
// curl http://localhost:3000/\?name=grivera64 => Hello, grivera64!

@gaby
Copy link
Member

gaby commented Dec 18, 2024

In that case, if we wanted to implement the one with no reply we should name it Drop(). Similar to what iptables names dropped packets.

If we find a way of flushing data and ending the connection then it can be called End() to replicate Express.js.

@grivera64
Copy link
Member Author

In that case, if we wanted to implement the one with no reply we should name it Drop(). Similar to what iptables names dropped packets.

If we find a way of flushing data and ending the connection then it can be called End() to replicate Express.js.

While it is a hidden way, there seems to be a way to flush data to the connection before closing the net.Conn. fasthttp writes to the internal net.Conn using writeResponse. After writing to it via a bufio.Writer, the response is flushed into the net.Conn here.

Even though it is a private method, we can duplicate this code within Fiber using public-facing methods (though accessing the underlying net.Conn warns to be careful when using it).

We could use this to implement End(), though flushing the buffer will still send an empty message (since fasthttp uses a StatusOK response as its default response).

If we want to exactly mimic the ExpressJS API in that only calling End() will drop the request, we would have to use some internal tracking to tell whether or not the response had been written to within a fiber Handler.

What are your thoughts about this?

@gaby
Copy link
Member

gaby commented Dec 20, 2024

@grivera64 If copying the function is the only way, seems reasonable in this case to be able to add the End() functionality.

The functionality for Drop() is covered by #3257

@grivera64
Copy link
Member Author

The functionality for Drop() is covered by #3257

Sounds good!

@grivera64 If copying the function is the only way, seems reasonable in this case to be able to add the End() functionality.

Yeah, fasthttp doesn't provide public access to the underlying writer used for flushing. So I believe the only ways to "end" a connection are to:

  • return early from the handler (does not consider middleware that may write to the response after the handler returns to Next())
  • Write/Flush the response directly to the underlying net.Conn and close the connection (will prevent any further modification to the response)

I could make a separate issue/PR for tracking the End() functionality if that's okay.

@github-project-automation github-project-automation bot moved this from In Progress to Done in v3 Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants