From d8d5818b47009f28433a7e96bcce8d116c8a9769 Mon Sep 17 00:00:00 2001 From: kim <89579420+NyaaaWhatsUpDoc@users.noreply.github.com> Date: Mon, 6 Mar 2023 09:38:43 +0000 Subject: [PATCH] [bugfix] internal server error on search not found (#1590) * add error value wrapping, include status code / not found flags from transport errors, update error usages Signed-off-by: kim * add code commenting for gtserror functions Signed-off-by: kim --------- Signed-off-by: kim --- internal/federation/authenticate.go | 3 +- internal/federation/dereferencing/error.go | 27 +++++----- internal/federation/federatingprotocol.go | 3 +- internal/gtserror/error.go | 60 ++++++++++++++++++++++ internal/transport/deliver.go | 4 +- internal/transport/dereference.go | 20 +++----- internal/transport/derefinstance.go | 10 ++-- internal/transport/derefmedia.go | 5 +- internal/transport/transport.go | 13 ++++- 9 files changed, 108 insertions(+), 37 deletions(-) create mode 100644 internal/gtserror/error.go diff --git a/internal/federation/authenticate.go b/internal/federation/authenticate.go index a966bb26a..32e427542 100644 --- a/internal/federation/authenticate.go +++ b/internal/federation/authenticate.go @@ -25,6 +25,7 @@ import ( "encoding/pem" "errors" "fmt" + "net/http" "net/url" "strings" @@ -226,7 +227,7 @@ func (f *federator) AuthenticateFederatedRequest(ctx context.Context, requestedU // The actual http call to the remote server is made right here in the Dereference function. b, err := trans.Dereference(ctx, requestingPublicKeyID) if err != nil { - if errors.Is(err, transport.ErrGone) { + if gtserror.StatusCode(err) == http.StatusGone { // if we get a 410 error it means the account that owns this public key has been deleted; // we should add a tombstone to our database so that we can avoid trying to deref it in future if err := f.HandleGone(ctx, requestingPublicKeyID); err != nil { diff --git a/internal/federation/dereferencing/error.go b/internal/federation/dereferencing/error.go index 3ba357f7a..530c72b32 100644 --- a/internal/federation/dereferencing/error.go +++ b/internal/federation/dereferencing/error.go @@ -19,10 +19,10 @@ package dereferencing import ( - "errors" "fmt" + "net/http" - "github.com/superseriousbusiness/gotosocial/internal/transport" + "github.com/superseriousbusiness/gotosocial/internal/gtserror" ) // ErrDB denotes that a proper error has occurred when doing @@ -96,22 +96,21 @@ func newErrOther(err error) error { } func wrapDerefError(derefErr error, fluff string) error { - var ( - err error - errWrongType *ErrWrongType - ) - + // Wrap with fluff. + err := derefErr if fluff != "" { err = fmt.Errorf("%s: %w", fluff, derefErr) } - switch { - case errors.Is(derefErr, transport.ErrGone): - err = NewErrNotRetrievable(err) - case errors.As(derefErr, &errWrongType): - err = newErrWrongType(err) - default: - err = newErrTransportError(err) + // Check for unretrievable HTTP status code errors. + if code := gtserror.StatusCode(derefErr); // nocollapse + code == http.StatusGone || code == http.StatusNotFound { + return NewErrNotRetrievable(err) + } + + // Check for other untrievable errors. + if gtserror.NotFound(derefErr) { + return NewErrNotRetrievable(err) } return err diff --git a/internal/federation/federatingprotocol.go b/internal/federation/federatingprotocol.go index e60f278c7..49b38eda4 100644 --- a/internal/federation/federatingprotocol.go +++ b/internal/federation/federatingprotocol.go @@ -31,6 +31,7 @@ import ( "github.com/superseriousbusiness/activity/streams/vocab" "github.com/superseriousbusiness/gotosocial/internal/ap" "github.com/superseriousbusiness/gotosocial/internal/db" + "github.com/superseriousbusiness/gotosocial/internal/gtserror" "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" "github.com/superseriousbusiness/gotosocial/internal/log" "github.com/superseriousbusiness/gotosocial/internal/transport" @@ -210,7 +211,7 @@ func (f *federator) AuthenticatePostInbox(ctx context.Context, w http.ResponseWr transport.WithFastfail(ctx), username, publicKeyOwnerURI, false, ) if err != nil { - if errors.Is(err, transport.ErrGone) { + if gtserror.StatusCode(err) == http.StatusGone { // This is the same case as the http.StatusGone check above. // It can happen here and not there because there's a race // where the sending server starts sending account deletion diff --git a/internal/gtserror/error.go b/internal/gtserror/error.go new file mode 100644 index 000000000..b9299403b --- /dev/null +++ b/internal/gtserror/error.go @@ -0,0 +1,60 @@ +/* + GoToSocial + Copyright (C) 2021-2023 GoToSocial Authors admin@gotosocial.org + + This program is free software: you can redistribute it and/or modify + it under the terms of the GNU Affero General Public License as published by + the Free Software Foundation, either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU Affero General Public License for more details. + + You should have received a copy of the GNU Affero General Public License + along with this program. If not, see . +*/ + +package gtserror + +import ( + "codeberg.org/gruf/go-errors/v2" +) + +// package private error key type. +type errkey int + +const ( + // error value keys. + _ errkey = iota + statusCodeKey + notFoundKey +) + +// StatusCode checks error for a stored status code value. For example +// an error from an outgoing HTTP request may be stored, or an API handler +// expected response status code may be stored. +func StatusCode(err error) int { + i, _ := errors.Value(err, statusCodeKey).(int) + return i +} + +// WithStatusCode will wrap the given error to store provided status code, +// returning wrapped error. See StatusCode() for example use-cases. +func WithStatusCode(err error, code int) error { + return errors.WithValue(err, statusCodeKey, code) +} + +// NotFound checks error for a stored "not found" flag. For example +// an error from an outgoing HTTP request due to DNS lookup. +func NotFound(err error) bool { + _, ok := errors.Value(err, notFoundKey).(struct{}) + return ok +} + +// SetNotFound will wrap the given error to store a "not found" flag, +// returning wrapped error. See NotFound() for example use-cases. +func SetNotFound(err error) error { + return errors.WithValue(err, notFoundKey, struct{}{}) +} diff --git a/internal/transport/deliver.go b/internal/transport/deliver.go index 7db3bf338..b1883b64e 100644 --- a/internal/transport/deliver.go +++ b/internal/transport/deliver.go @@ -29,6 +29,7 @@ import ( "codeberg.org/gruf/go-byteutil" apiutil "github.com/superseriousbusiness/gotosocial/internal/api/util" "github.com/superseriousbusiness/gotosocial/internal/config" + "github.com/superseriousbusiness/gotosocial/internal/gtserror" ) func (t *transport) BatchDeliver(ctx context.Context, b []byte, recipients []*url.URL) error { @@ -96,7 +97,8 @@ func (t *transport) Deliver(ctx context.Context, b []byte, to *url.URL) error { if code := resp.StatusCode; code != http.StatusOK && code != http.StatusCreated && code != http.StatusAccepted { - return fmt.Errorf("POST request to %s failed: %s", urlStr, resp.Status) + err := fmt.Errorf("POST request to %s failed: %s", urlStr, resp.Status) + return gtserror.WithStatusCode(err, resp.StatusCode) } return nil diff --git a/internal/transport/dereference.go b/internal/transport/dereference.go index f42f146ea..d82a1ac8a 100644 --- a/internal/transport/dereference.go +++ b/internal/transport/dereference.go @@ -20,7 +20,6 @@ package transport import ( "context" - "errors" "fmt" "io" "net/http" @@ -28,15 +27,10 @@ import ( apiutil "github.com/superseriousbusiness/gotosocial/internal/api/util" "github.com/superseriousbusiness/gotosocial/internal/config" + "github.com/superseriousbusiness/gotosocial/internal/gtserror" "github.com/superseriousbusiness/gotosocial/internal/uris" ) -// ErrGone is returned from Dereference when the remote resource returns 410 GONE. -// This is useful in cases where we're processing a delete of a resource that's already -// been removed from the remote server, so we know we don't need to keep trying to -// dereference it. -var ErrGone = errors.New("remote resource returned HTTP code 410 GONE") - func (t *transport) Dereference(ctx context.Context, iri *url.URL) ([]byte, error) { // if the request is to us, we can shortcut for certain URIs rather than going through // the normal request flow, thereby saving time and energy @@ -72,12 +66,10 @@ func (t *transport) Dereference(ctx context.Context, iri *url.URL) ([]byte, erro } defer rsp.Body.Close() - switch rsp.StatusCode { - case http.StatusOK: - return io.ReadAll(rsp.Body) - case http.StatusGone: - return nil, ErrGone - default: - return nil, fmt.Errorf("GET request to %s failed: %s", iriStr, rsp.Status) + if rsp.StatusCode != http.StatusOK { + err := fmt.Errorf("GET request to %s failed: %s", iriStr, rsp.Status) + return nil, gtserror.WithStatusCode(err, rsp.StatusCode) } + + return io.ReadAll(rsp.Body) } diff --git a/internal/transport/derefinstance.go b/internal/transport/derefinstance.go index 2dcf367a0..41949f449 100644 --- a/internal/transport/derefinstance.go +++ b/internal/transport/derefinstance.go @@ -30,6 +30,7 @@ import ( apimodel "github.com/superseriousbusiness/gotosocial/internal/api/model" apiutil "github.com/superseriousbusiness/gotosocial/internal/api/util" + "github.com/superseriousbusiness/gotosocial/internal/gtserror" "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" "github.com/superseriousbusiness/gotosocial/internal/id" "github.com/superseriousbusiness/gotosocial/internal/log" @@ -102,7 +103,8 @@ func dereferenceByAPIV1Instance(ctx context.Context, t *transport, iri *url.URL) defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("GET request to %s failed: %s", iriStr, resp.Status) + err := fmt.Errorf("GET request to %s failed: %s", iriStr, resp.Status) + return nil, gtserror.WithStatusCode(err, resp.StatusCode) } b, err := io.ReadAll(resp.Body) @@ -252,7 +254,8 @@ func callNodeInfoWellKnown(ctx context.Context, t *transport, iri *url.URL) (*ur defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("callNodeInfoWellKnown: GET request to %s failed: %s", iriStr, resp.Status) + err := fmt.Errorf("GET request to %s failed: %s", iriStr, resp.Status) + return nil, gtserror.WithStatusCode(err, resp.StatusCode) } b, err := io.ReadAll(resp.Body) @@ -303,7 +306,8 @@ func callNodeInfo(ctx context.Context, t *transport, iri *url.URL) (*apimodel.No defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("callNodeInfo: GET request to %s failed: %s", iriStr, resp.Status) + err := fmt.Errorf("GET request to %s failed: %s", iriStr, resp.Status) + return nil, gtserror.WithStatusCode(err, resp.StatusCode) } b, err := io.ReadAll(resp.Body) diff --git a/internal/transport/derefmedia.go b/internal/transport/derefmedia.go index c8a817eef..88d7f0b32 100644 --- a/internal/transport/derefmedia.go +++ b/internal/transport/derefmedia.go @@ -24,6 +24,8 @@ import ( "io" "net/http" "net/url" + + "github.com/superseriousbusiness/gotosocial/internal/gtserror" ) func (t *transport) DereferenceMedia(ctx context.Context, iri *url.URL) (io.ReadCloser, int64, error) { @@ -46,7 +48,8 @@ func (t *transport) DereferenceMedia(ctx context.Context, iri *url.URL) (io.Read // Check for an expected status code if rsp.StatusCode != http.StatusOK { - return nil, 0, fmt.Errorf("GET request to %s failed: %s", iriStr, rsp.Status) + err := fmt.Errorf("GET request to %s failed: %s", iriStr, rsp.Status) + return nil, 0, gtserror.WithStatusCode(err, rsp.StatusCode) } return rsp.Body, rsp.ContentLength, nil diff --git a/internal/transport/transport.go b/internal/transport/transport.go index b0f68f707..4cb95bd98 100644 --- a/internal/transport/transport.go +++ b/internal/transport/transport.go @@ -25,6 +25,7 @@ import ( "errors" "fmt" "io" + "net" "net/http" "net/url" "strconv" @@ -36,6 +37,7 @@ import ( errorsv2 "codeberg.org/gruf/go-errors/v2" "codeberg.org/gruf/go-kv" "github.com/go-fed/httpsig" + "github.com/superseriousbusiness/gotosocial/internal/gtserror" "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" "github.com/superseriousbusiness/gotosocial/internal/httpclient" "github.com/superseriousbusiness/gotosocial/internal/log" @@ -56,6 +58,7 @@ type Transport interface { // Deliver sends an ActivityStreams object. Deliver(ctx context.Context, b []byte, to *url.URL) error + // BatchDeliver sends an ActivityStreams object to multiple recipients. BatchDeliver(ctx context.Context, b []byte, recipients []*url.URL) error @@ -65,10 +68,13 @@ type Transport interface { // Dereference fetches the ActivityStreams object located at this IRI with a GET request. Dereference(ctx context.Context, iri *url.URL) ([]byte, error) + // DereferenceMedia fetches the given media attachment IRI, returning the reader and filesize. DereferenceMedia(ctx context.Context, iri *url.URL) (io.ReadCloser, int64, error) + // DereferenceInstance dereferences remote instance information, first by checking /api/v1/instance, and then by checking /.well-known/nodeinfo. DereferenceInstance(ctx context.Context, iri *url.URL) (*gtsmodel.Instance, error) + // Finger performs a webfinger request with the given username and domain, and returns the bytes from the response body. Finger(ctx context.Context, targetUsername string, targetDomain string) ([]byte, error) } @@ -207,6 +213,10 @@ func (t *transport) do(r *http.Request, signer func(*http.Request) error) (*http } else if errors.As(err, &x509.UnknownAuthorityError{}) { // Unknown authority errors we do NOT recover from return nil, err + } else if dnserr := (*net.DNSError)(nil); // nocollapse + errors.As(err, &dnserr) && dnserr.IsNotFound { + // DNS lookup failure, this domain does not exist + return nil, gtserror.SetNotFound(err) } if fastFail { @@ -219,7 +229,7 @@ func (t *transport) do(r *http.Request, signer func(*http.Request) error) (*http backoff = time.Duration(i) * baseBackoff } - l.Errorf("backing off for %s after http request error: %v", backoff.String(), err) + l.Errorf("backing off for %s after http request error: %v", backoff, err) select { // Request ctx cancelled @@ -228,7 +238,6 @@ func (t *transport) do(r *http.Request, signer func(*http.Request) error) (*http // Backoff for some time case <-time.After(backoff): - backoff *= 2 } }