fix(certifier): check wire message size before JSON deserialisation #1523
fix(certifier): check wire message size before JSON deserialisation #1523atharrva01 wants to merge 4 commits intohyperledger-labs:mainfrom
Conversation
|
caught oversized payloads before deserialization using |
|
@atharrva01 thanks a lot for opening the PR. Also, could you please have a look at the failing tests? |
|
Hi @atharrva01 , thanks for the effort. |
|
hi @adecaro you are right, per-view patching doesn't scale. In d5feb20 I've extracted the size guard into a reusable
This stops short of injecting into the FSC comm stack directly (that would need upstream changes), but it gives a single, reusable session-layer abstraction any view in this repo can opt into. Happy to discuss if a deeper comm-stack hook makes more sense. |
1d4acb2 to
2ba1638
Compare
Adds MaxWireMessageBytes (2 MiB) and uses ReceiveRaw() to reject oversized messages before json.Unmarshal, preventing heap spikes from allocate-then-reject. Injects sessionFactory for unit testability. Signed-off-by: atharrva01 <atharvaborade568@gamil.com>
Signed-off-by: atharrva01 <atharvaborade568@gamil.com>
…sion Create session.SizeLimitedJsonSession, a JsonSession wrapper that enforces a configurable byte limit on every received message before JSON decoding. Expose it via session.NewSizeLimitedSession and session.JSONWithLimit so any view can opt in without duplicating the size-check logic. CertificationService now defaults its sessionFactory to JSONWithLimit, and Call() reverts to the simpler s.Receive() call — the guard is transparent. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
0b0a346 to
0054c64
Compare
|
Hi @atharrva01 , I think this check needs to be handled as earlier as possible. If we check at the application level, we risk resource exhaustion anyway. So, I would close this PR, but I would like you to ask to check the fabric smart client (
What do you think? Thanks for your effort in any case 🙏 |
|
Hi @adecaro investigated Does FSC have a max message size limit?Yes both backends enforce a 10 MiB cap at the transport layer, before any application-level deserialization:
Both checks happen as early as possible: the varint/length header is read first, and if it exceeds 10 MiB the message is rejected immediately no heap spike from body allocation. Is the limit configurable?No. Both constants are hardcoded. The comm ConclusionThe protection the maintainer was looking for already exists at the FSC comm layer. The only missing piece is configurability that would require an upstream FSC change to expose the limit via Closing this PR as requested. Thanks for the guidance 🙏 |
Closes #1557
Follow-on to #1498 based on your @adecaro's review feedback.
The
MaxRequestBytescheck I added in #1498 ran after full JSON deserialization , so a 500 MiB payload would still spike the heap before getting rejected. The fix needs to happen earlier.I can't touch the transport layer directly, but
ReceiveRaw()gets us close enough: now we checklen(raw) > MaxWireMessageBytesbefore ever callingjson.Unmarshal, so an oversized message gets dropped before any allocations for the IDs slice orRequestfield.To keep this unit-testable without a real FSC session, I added a
sessionFactoryfield (defaults tosession.JSON) so tests can inject a fake that returns preset raw bytes.Changes:
config.go, addsMaxWireMessageBytes = MaxRequestBytes * 2service.go, switches toReceiveRaw+ size guard + manual unmarshal; addssessionFactoryservice_test.go, 4 new tests: oversized reject, at-limit pass-through, transport errors, constant relationship