fix(serializer): disable MsgpackSerializer and remove shamaton/msgpack dependency#95
fix(serializer): disable MsgpackSerializer and remove shamaton/msgpack dependency#95
Conversation
…k dependency Disable Marshal and Unmarshal in MsgpackSerializer by converting them into stubs that return errors. This addresses a security concern in the upstream shamaton/msgpack library (ref: shamaton/msgpack#60). The type is marked deprecated and will be removed in a future release. - Remove github.com/shamaton/msgpack/v3 from go.mod - Bump github.com/hyp3rd/ewrap from v1.3.8 to v1.3.9
There was a problem hiding this comment.
Pull request overview
Disables the Msgpack-based serializer due to an upstream security concern and removes the shamaton/msgpack module dependency, while bumping ewrap to the latest patch version.
Changes:
- Convert
MsgpackSerializer.Marshal/Unmarshalinto deprecated stubs that return errors. - Remove
github.com/shamaton/msgpack/v3fromgo.mod. - Bump
github.com/hyp3rd/ewrapfromv1.3.8tov1.3.9(and updatego.sumaccordingly).
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/libs/serializer/msgpack.go | Deprecates and disables msgpack serialization/deserialization by returning errors. |
| go.mod | Removes msgpack dependency and bumps ewrap. |
| go.sum | Updates checksums for the ewrap version bump. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // MsgpackSerializer leverages `msgpack` to serialize the items before storing them in the cache. | ||
| // | ||
| // Deprecated: This serializer is now a shim and will be removed in a future release for security reasons. | ||
| // REF: https://github.com/shamaton/msgpack/pull/60 | ||
| // Please use the `Marshal` method of the `Serializer` interface instead. | ||
| type MsgpackSerializer struct{} |
There was a problem hiding this comment.
The deprecation notice points users to a Serializer interface, but the interface in this package is named ISerializer (see internal/libs/serializer/serializer.go). Consider updating this doc to reference ISerializer and/or direct users to the supported serializer(s) (e.g. DefaultJSONSerializer / serializer.New("default")) so the guidance is accurate.
internal/libs/serializer/msgpack.go
Outdated
| // | ||
| // Deprecated: This method is now a shim and will be removed in a future release for security reasons. | ||
| // REF: https://github.com/shamaton/msgpack/pull/60 | ||
| // Please use the `Marshal` method of the `Serializer` interface instead. |
There was a problem hiding this comment.
Unmarshal's deprecation comment currently says to use the Marshal method instead, which is misleading. Please update it to refer to Unmarshal (and fix the referenced interface name if needed) so users know what to call as a replacement.
| // Please use the `Marshal` method of the `Serializer` interface instead. | |
| // Please use the `Unmarshal` method of the `Serializer` interface instead. |
| func (*MsgpackSerializer) Marshal(_ any) ([]byte, error) { // receiver omitted (unused) | ||
| // data, err := msgpack.Marshal(&v) | ||
| // if err != nil { | ||
| // return nil, ewrap.Wrap(err, "failed to marshal msgpack") | ||
| // } | ||
|
|
||
| return data, nil | ||
| // return data, nil | ||
| return nil, ewrap.New("msgpack serialization is deprecated and has been disabled for security reasons") | ||
| } |
There was a problem hiding this comment.
As of this change, MsgpackSerializer.Marshal/Unmarshal always return an error. The Redis and RedisCluster backends currently default to serializer.New("msgpack") when no serializer is provided (pkg/backend/redis.go and pkg/backend/redis_cluster.go), so this will cause cache operations to fail at runtime for default-configured users. Consider switching the backend default serializer to "default" (JSON) and/or removing msgpack from the default registry to avoid breaking the default path.
| github.com/cespare/xxhash/v2 v2.3.0 | ||
| github.com/goccy/go-json v0.10.6 | ||
| github.com/gofiber/fiber/v3 v3.1.0 | ||
| github.com/hyp3rd/ewrap v1.3.8 | ||
| github.com/hyp3rd/ewrap v1.3.9 | ||
| github.com/hyp3rd/sectools v1.2.3 | ||
| github.com/longbridgeapp/assert v1.1.0 | ||
| github.com/redis/go-redis/v9 v9.18.0 | ||
| github.com/shamaton/msgpack/v3 v3.1.0 | ||
| github.com/ugorji/go/codec v1.3.1 | ||
| go.opentelemetry.io/otel v1.43.0 |
There was a problem hiding this comment.
go.mod removes github.com/shamaton/msgpack/v3, but go.sum still contains checksums for it (e.g. lines 63-64). Please run go mod tidy (or otherwise clean go.sum) so the dependency removal is reflected consistently.
…k dependency Disable Marshal and Unmarshal in MsgpackSerializer by converting them into stubs that return errors. This addresses a security concern in the upstream shamaton/msgpack library (ref: shamaton/msgpack#60). The type is marked deprecated and will be removed in a future release. - Remove github.com/shamaton/msgpack/v3 from go.mod - Bump github.com/hyp3rd/ewrap from v1.3.8 to v1.3.9
Disable Marshal and Unmarshal in MsgpackSerializer by converting them into stubs that return errors. This addresses a security concern in the upstream shamaton/msgpack library (ref: shamaton/msgpack#60). The type is marked deprecated and will be removed in a future release.