Browse Source

This commit fixes #2 (git.destrealm.org). See notes.

Chiefly, the primary mistake I made was in the capstanContext. More
specifically, it was in the capstanContext.WriteJSON() function which
was itself calling c.Write() rather than c.response.Write(). I believe
this was due to the changes made to wrap the response calls so we could
handle errors internally, allowing callers to return the context from
their controller methods rather than being forced to return an error.

I had a feeling this design would eventually bite me because it requires
a fairly substantial amount of caution in its design.

What eventually happened was that, for whatever reason, a race condition
would (sometimes) trigger if the remote endpoint killed the connection
at just the right time, allowing WriteJSON to set the value of lastError
to the current context because of its call into c.Write() rather than
c.response.Write(). This would cause a cascade of failures the moment
the goroutine stack overflowed bringing the whole application down. This
could have caused a denial-of-service attack by simply combining a) code
that called Context.Error() and b) found an endpoint that delivered a
large enough response that the connection could be torn down before the
server had a chance to finalize its response.

Fortunately, "a" is likely to be uncommon, because there isn't much
reason for clients to call Context.Error(). Since clients are encouraged
to return the context itself in the event of an error (this happens
transparently; some context functions returning errors actually return
an error wrapped by that same context), I don't believe there is
currently any code out there that would use this method directly.

To prevent this from happening again, I have taken the precaution of
implementing the following:

 * ALL calls internal to the Context that set c.lastError now pass the
 value through c.SetError().
 * c.SetError() will now panic if it is passed a Context.
master
Benjamin Shelton 4 weeks ago
parent
commit
e9a94cd234
1 changed files with 12 additions and 9 deletions
  1. +12
    -9
      context.go

+ 12
- 9
context.go View File

@@ -329,7 +329,7 @@ func (c *capstanContext) In(v interface{}) error {
}

if err != nil {
c.lastError = err
c.SetError(err)
// Bad request.
c.code = 400
return c
@@ -362,11 +362,11 @@ func (c *capstanContext) JSON(v interface{}) error {
Get("content-type"), "application/json") || strings.
HasPrefix(c.request.Header.Get("content-type"), "text/json") {
c.code = 400
c.lastError = ErrNotJSON
c.SetError(ErrNotJSON)
return c
}
if err := c.json(v); err != nil {
c.lastError = err
c.SetError(err)
// Bad request.
c.code = 400
return c
@@ -390,7 +390,7 @@ func (c *capstanContext) Render(name string, values interface{}) error {

if c.contextRenderer != nil {
if err := c.contextRenderer.Render(name, values, c); err != nil {
c.lastError = err
c.SetError(err)
c.code = 500
return c
}
@@ -398,7 +398,7 @@ func (c *capstanContext) Render(name string, values interface{}) error {
}
if c.renderer != nil {
if err := c.renderer.Render(name, values, c); err != nil {
c.lastError = err
c.SetError(err)
c.code = 500
return c
}
@@ -496,7 +496,7 @@ func (c *capstanContext) Write(b []byte) (int, error) {
}
n, err := c.response.Write(b)
if err != nil {
c.lastError = err
c.SetError(err)
c.code = 500
return n, c
}
@@ -532,9 +532,9 @@ func (c *capstanContext) WriteJSON(v interface{}) error {
c.response.Header().Add("content-length",
strconv.FormatInt(int64(len(b)), 10))

_, err = c.Write(b)
_, err = c.response.Write(b)
if err != nil {
c.lastError = err
c.SetError(err)
c.code = 500
return c
}
@@ -557,7 +557,7 @@ func (c *capstanContext) HasError() error {

// Panic sets the last error state to the contents of the backtrace.
func (c *capstanContext) Panic(trace string) {
c.lastError = fmt.Errorf(trace)
c.SetError(fmt.Errorf(trace))
}

// Fulfill the error and ErrorTracer interfaces.
@@ -578,6 +578,9 @@ func (c *capstanContext) Error() string {

// SetError changes the value of lastError for this context.
func (c *capstanContext) SetError(err error) {
if _, ok := c.lastError.(Context); ok {
panic("SetError(): context-as-error is invalid here")
}
c.lastError = err
}



Loading…
Cancel
Save