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

Misc fixes #560

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions api/_responses/errors.go
Original file line number Diff line number Diff line change
@@ -1,19 +1,23 @@
package _responses

import "github.com/t2bot/matrix-media-repo/common"
import (
"fmt"

"github.com/t2bot/matrix-media-repo/common"
)

type ErrorResponse struct {
Code string `json:"errcode"`
Message string `json:"error"`
InternalCode string `json:"mr_errcode"`
}

func InternalServerError(message string) *ErrorResponse {
return &ErrorResponse{common.ErrCodeUnknown, message, common.ErrCodeUnknown}
func InternalServerError(err error) *ErrorResponse {
return &ErrorResponse{common.ErrCodeUnknown, fmt.Sprint(err), common.ErrCodeUnknown}
}

func BadGatewayError(message string) *ErrorResponse {
return &ErrorResponse{common.ErrCodeUnknown, message, common.ErrCodeUnknown}
func BadGatewayError(err error) *ErrorResponse {
return &ErrorResponse{common.ErrCodeUnknown, fmt.Sprint(err), common.ErrCodeUnknown}
}

func MethodNotAllowed() *ErrorResponse {
Expand Down Expand Up @@ -48,8 +52,8 @@ func GuestAuthFailed() *ErrorResponse {
return &ErrorResponse{common.ErrCodeNoGuests, "Guests cannot use this endpoint", common.ErrCodeNoGuests}
}

func BadRequest(message string) *ErrorResponse {
return &ErrorResponse{common.ErrCodeUnknown, message, common.ErrCodeBadRequest}
func BadRequest(err error) *ErrorResponse {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These functions deliberately do not take error objects to avoid accidentally leaking internal details to callers - please revert this change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's an exception to this rule later on in the federation test/info endpoints, as those endpoints are primarily used for debugging or by admins rather than random consumers.

return &ErrorResponse{common.ErrCodeUnknown, fmt.Sprint(err), common.ErrCodeBadRequest}
}

func QuotaExceeded() *ErrorResponse {
Expand Down
14 changes: 2 additions & 12 deletions api/_routers/00-install-params.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,15 @@ package _routers

import (
"context"
"errors"
"net/http"
"regexp"

"github.com/julienschmidt/httprouter"
)

func localCompile(expr string) *regexp.Regexp {
r, err := regexp.Compile(expr)
if err != nil {
panic(errors.New("error compiling expression: " + expr + " | " + err.Error()))
}
return r
}

var ServerNameRegex = localCompile("[a-zA-Z0-9.:\\-_]+")

//var NumericIdRegex = localCompile("[0-9]+")
var ServerNameRegex = regexp.MustCompile("[a-zA-Z0-9.:\\-_]+")

// var NumericIdRegex = regexp.MustCompile("[0-9]+")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this got moved down incorrectly

func GetParam(name string, r *http.Request) string {
p := httprouter.ParamsFromContext(r.Context())
if p == nil {
Expand Down
7 changes: 4 additions & 3 deletions api/_routers/03-host_detection.go
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please revert 8515e0b - the package naming is deliberate

Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"encoding/json"
"errors"
"fmt"
"net"
"net/http"
"strings"
Expand Down Expand Up @@ -60,11 +61,11 @@ func (h *HostRouter) ServeHTTP(w http.ResponseWriter, r *http.Request) {
}).Inc()
logger.Warnf("The server name provided ('%s') in the Host header is not configured, or the request was made directly to the media repo. Please specify a Host header and check your reverse proxy configuration. The request is being rejected.", r.Host)
w.WriteHeader(http.StatusBadGateway)
if b, err := json.Marshal(_responses.BadGatewayError("Review server logs to continue")); err != nil {
panic(errors.New("error preparing BadGatewayError: " + err.Error()))
if b, err := json.Marshal(_responses.BadGatewayError(errors.New("Review server logs to continue"))); err != nil {
panic(fmt.Errorf("error preparing BadGatewayError: %v", err))
} else {
if _, err = w.Write(b); err != nil {
panic(errors.New("error sending BadGatewayError: " + err.Error()))
panic(fmt.Errorf("error sending BadGatewayError: %v", err))
}
}
return // don't call next handler
Expand Down
2 changes: 1 addition & 1 deletion api/_routers/97-optional-access-token.go
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please revert fadeb9a - the package naming is deliberate

Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func OptionalAccessToken(generator GeneratorWithUserFn) GeneratorFn {
if !errors.Is(err, matrix.ErrInvalidToken) {
sentry.CaptureException(err)
ctx.Log.Error("Error verifying token: ", err)
return _responses.InternalServerError("unexpected error validating access token")
return _responses.InternalServerError(errors.New("unexpected error validating access token"))
}

ctx.Log.Warn("Failed to verify token (non-fatal): ", err)
Expand Down
2 changes: 1 addition & 1 deletion api/_routers/97-require-access-token.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ func RequireAccessToken(generator GeneratorWithUserFn) GeneratorFn {
if err != nil && !errors.Is(err, matrix.ErrInvalidToken) {
sentry.CaptureException(err)
ctx.Log.Error("Error verifying token: ", err)
return _responses.InternalServerError("unexpected error validating access token")
return _responses.InternalServerError(errors.New("unexpected error validating access token"))
}
return _responses.AuthFailed()
}
Expand Down
8 changes: 4 additions & 4 deletions api/_routers/98-use-rcontext.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ func (c *RContextRouter) ServeHTTP(w http.ResponseWriter, r *http.Request) {

r = writeStatusCode(w, r, http.StatusOK)
if _, err := w.Write([]byte(htmlRes.HTML)); err != nil {
panic(errors.New("error sending HtmlResponse: " + err.Error()))
panic(fmt.Errorf("error sending HtmlResponse: %w", err))
}
return // don't continue
}
Expand All @@ -104,16 +104,16 @@ beforeParseDownload:
ranges, err := http_range.ParseRange(r.Header.Get("Range"), downloadRes.SizeBytes, rctx.Config.Downloads.DefaultRangeChunkSizeBytes)
if errors.Is(err, http_range.ErrInvalid) {
proposedStatusCode = http.StatusRequestedRangeNotSatisfiable
res = _responses.BadRequest("invalid range header")
res = _responses.BadRequest(errors.New("invalid range header"))
goto beforeParseDownload // reprocess `res`
} else if errors.Is(err, http_range.ErrNoOverlap) {
proposedStatusCode = http.StatusRequestedRangeNotSatisfiable
res = _responses.BadRequest("out of range")
res = _responses.BadRequest(errors.New("out of range"))
goto beforeParseDownload // reprocess `res`
}
if len(ranges) > 1 {
proposedStatusCode = http.StatusRequestedRangeNotSatisfiable
res = _responses.BadRequest("only 1 range is supported")
res = _responses.BadRequest(errors.New("only 1 range is supported"))
goto beforeParseDownload // reprocess `res`
}

Expand Down
26 changes: 14 additions & 12 deletions api/custom/datastores.go
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please revert 87964bb - this is not a change I'm looking to support at the moment, for reasons discussed over chat.

Primarily, time as a package allows mistakes in precision to be made far too easily. We work with a single precision of milliseconds throughout the codebase deliberately - please don't change that.

Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
package custom

import (
"errors"
"fmt"
"net/http"
"strconv"

"github.com/getsentry/sentry-go"
"github.com/t2bot/matrix-media-repo/api/_apimeta"
"github.com/t2bot/matrix-media-repo/api/_responses"
Expand All @@ -9,9 +14,6 @@ import (
"github.com/t2bot/matrix-media-repo/datastores"
"github.com/t2bot/matrix-media-repo/tasks"

"net/http"
"strconv"

"github.com/sirupsen/logrus"
"github.com/t2bot/matrix-media-repo/common/rcontext"
"github.com/t2bot/matrix-media-repo/util"
Expand All @@ -29,7 +31,7 @@ func GetDatastores(r *http.Request, rctx rcontext.RequestContext, user _apimeta.
if err != nil {
sentry.CaptureException(err)
rctx.Log.Error("Error getting datastore URI: ", err)
return _responses.InternalServerError("unexpected error getting datastore information")
return _responses.InternalServerError(errors.New("unexpected error getting datastore information"))
}
dsMap := make(map[string]interface{})
dsMap["type"] = ds.Type
Expand All @@ -47,7 +49,7 @@ func MigrateBetweenDatastores(r *http.Request, rctx rcontext.RequestContext, use
if beforeTsStr != "" {
beforeTs, err = strconv.ParseInt(beforeTsStr, 10, 64)
if err != nil {
return _responses.BadRequest("Error parsing before_ts: " + err.Error())
return _responses.BadRequest(fmt.Errorf("Error parsing before_ts: %w", err))
}
}

Expand All @@ -61,28 +63,28 @@ func MigrateBetweenDatastores(r *http.Request, rctx rcontext.RequestContext, use
})

if sourceDsId == targetDsId {
return _responses.BadRequest("Source and target datastore cannot be the same")
return _responses.BadRequest(errors.New("Source and target datastore cannot be the same"))
}
if _, ok := datastores.Get(rctx, sourceDsId); !ok {
return _responses.BadRequest("Source datastore does not appear to exist")
return _responses.BadRequest(errors.New("Source datastore does not appear to exist"))
}
if _, ok := datastores.Get(rctx, targetDsId); !ok {
return _responses.BadRequest("Target datastore does not appear to exist")
return _responses.BadRequest(errors.New("Target datastore does not appear to exist"))
}

estimate, err := datastores.SizeOfDsIdWithAge(rctx, sourceDsId, beforeTs)
if err != nil {
rctx.Log.Error(err)
sentry.CaptureException(err)
return _responses.InternalServerError("Unexpected error getting storage estimate")
return _responses.InternalServerError(errors.New("Unexpected error getting storage estimate"))
}

rctx.Log.Infof("User %s has started a datastore media transfer", user.UserId)
task, err := tasks.RunDatastoreMigration(rctx, sourceDsId, targetDsId, beforeTs)
if err != nil {
rctx.Log.Error(err)
sentry.CaptureException(err)
return _responses.InternalServerError("Unexpected error starting migration")
return _responses.InternalServerError(errors.New("Unexpected error starting migration"))
}

migration := &DatastoreMigration{
Expand All @@ -100,7 +102,7 @@ func GetDatastoreStorageEstimate(r *http.Request, rctx rcontext.RequestContext,
if beforeTsStr != "" {
beforeTs, err = strconv.ParseInt(beforeTsStr, 10, 64)
if err != nil {
return _responses.BadRequest("Error parsing before_ts: " + err.Error())
return _responses.BadRequest(fmt.Errorf("Error parsing before_ts: %w", err))
}
}

Expand All @@ -115,7 +117,7 @@ func GetDatastoreStorageEstimate(r *http.Request, rctx rcontext.RequestContext,
if err != nil {
rctx.Log.Error(err)
sentry.CaptureException(err)
return _responses.InternalServerError("Unexpected error getting storage estimate")
return _responses.InternalServerError(errors.New("Unexpected error getting storage estimate"))
}
return &_responses.DoNotCacheResponse{Payload: result}
}
Loading