usememos / memos

An open-source, lightweight note-taking solution. The pain-less way to create your meaningful notes. Your Notes, Your Way.
https://usememos.com
MIT License
34.06k stars 2.47k forks source link

Panic when accessing an non-public resource from API without authentication #3458

Closed RyoJerryYu closed 5 months ago

RyoJerryYu commented 5 months ago

Describe the bug

I run a memos server with docker image with tag 0.22.1 .

version: "3.0"
services:
  memos:
    image: neosmemo/memos:0.22.1
    ...

When I access a resource attached to a private memo, without authentication, it causes a panic, with log as below:

2024/05/26 12:27:21 INFO OK method=/memos.api.v1.ResourceService/GetResourceBinary
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xb65258]

goroutine 1752 [running]:
github.com/usememos/memos/server/router/api/v1.(*APIV1Service).GetResourceBinary(0x400018a3c0, {0x1a7fe78, 0x40002ba8a0}, 0x4000769590)
        /backend-build/server/router/api/v1/resource_service.go:183 +0x2e8
github.com/usememos/memos/proto/gen/api/v1._ResourceService_GetResourceBinary_Handler.func1({0x1a7fe78?, 0x40002ba8a0?}, {0xfbe760?, 0x4000769590?})
        /backend-build/proto/gen/api/v1/resource_service_grpc.pb.go:268 +0xd0
github.com/usememos/memos/server/router/api/v1.(*GRPCAuthInterceptor).AuthenticationInterceptor(0x4000180048, {0x1a7fe78, 0x40002ba8a0}, {0xfbe760, 0x4000769590}, 0x4000904520, 0x400012d8c0)
        /backend-build/server/router/api/v1/acl.go:58 +0xf8
google.golang.org/grpc.getChainUnaryHandler.func1({0x1a7fe78, 0x40002ba8a0}, {0xfbe760, 0x4000769590})
        /go/pkg/mod/google.golang.org/grpc@v1.64.0/server.go:1196 +0xa0
github.com/usememos/memos/server/router/api/v1.(*LoggerInterceptor).LoggerInterceptor(0x2501da0, {0x1a7fe78, 0x40002ba8a0}, {0xfbe760?, 0x4000769590?}, 0x4000904520, 0x18?)
        /backend-build/server/router/api/v1/logger_interceptor.go:20 +0x4c
google.golang.org/grpc.NewServer.chainUnaryServerInterceptors.chainUnaryInterceptors.func1({0x1a7fe78, 0x40002ba8a0}, {0xfbe760, 0x4000769590}, 0x4000904520, 0x102fae0?)
        /go/pkg/mod/google.golang.org/grpc@v1.64.0/server.go:1187 +0x88
github.com/usememos/memos/proto/gen/api/v1._ResourceService_GetResourceBinary_Handler({0x10848e0, 0x400018a3c0}, {0x1a7fe78, 0x40002ba8a0}, 0x400020ca80, 0x40001a2060)
        /backend-build/proto/gen/api/v1/resource_service_grpc.pb.go:270 +0x148
google.golang.org/grpc.(*Server).processUnaryRPC(0x40001a6000, {0x1a7fe78, 0x40002ba7b0}, {0x1a88a20, 0x40007ae000}, 0x40003c1200, 0x400018a720, 0x2485700, 0x4000f64140)
        /go/pkg/mod/google.golang.org/grpc@v1.64.0/server.go:1379 +0xb58
google.golang.org/grpc.(*Server).handleStream(0x40001a6000, {0x1a88a20, 0x40007ae000}, 0x40003c1200)
        /go/pkg/mod/google.golang.org/grpc@v1.64.0/server.go:1790 +0xb20
google.golang.org/grpc.(*Server).serveStreams.func2.1()
        /go/pkg/mod/google.golang.org/grpc@v1.64.0/server.go:1029 +0x8c
created by google.golang.org/grpc.(*Server).serveStreams.func2 in goroutine 493
        /go/pkg/mod/google.golang.org/grpc@v1.64.0/server.go:1040 +0x13c

I dug into the source code, It seems to be happen in server/router/api/v1/resource_service.go:183 , when user is nil and try to access user.ID.

user, err := getCurrentUser(ctx, s.Store)
if err != nil {
    return nil, status.Errorf(codes.Internal, "failed to get current user: %v", err)
}
if memo.Visibility == store.Private && user.ID != resource.CreatorID { // this line
    return nil, status.Errorf(codes.Unauthenticated, "unauthorized access")
}

Steps to reproduce

Access the GetResourceBinary method from API without authentication, can always reproduce this bug.

  1. Post a memo with visibility == Private
  2. Call the GetResourceBinary method from API without authentication. For example, as curl command below:
    curl -X GET http://localhost:5230/file/resources/1/xxx.jpg
  3. curl produce error, and could see the panic log on server.

The version of Memos you're using

v0.22.1

Screenshots or additional context

In addition:

I notice that the docker container shut down after panic. I'm wondering why there is no a panic recover interceptor to avoid a complete server shutdown? It seems not so difficult to implement it.

RyoJerryYu commented 5 months ago

OK, I found it was fixed in the latest version of main branch. So I close this issue. Hope to get the new version as soon as possible.