Replace ad-hoc JSON parsing/serialising with oatpp ObjectMapper #6
Loading…
Add table
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Location:
JsonErrorHandler.hpp,AuthInterceptor::makeJsonError,ws/Listener::jsonStr/jsonInt,Hub::buildPresenceMsgMultiple sites hand-assemble JSON via string concatenation.
JsonErrorHandler::handleErrorembedsstatus.descriptioninto the body without escaping, the ad-hocjsonStr/jsonIntparsers inws/Listener.hppmishandle escaped quotes and nested objects. Switch tooatpp::parser::json::mapping::ObjectMapper.From audit #1.
Agent Evaluation
Feasibility: Medium. Multiple call sites (
AuthInterceptor::makeJsonError,JsonErrorHandler::handleError, theescapeJsonhelper,Hub::buildPresenceMsg/notifyBooking/notifyPerson,ws/Listener::jsonStr/jsonInt) all hand-roll JSON. Switching them tooatpp::parser::json::mapping::ObjectMapperremoves the escape bugs but means each call site needs a small DTO struct (or a helper that builds anoatpp::Fields<…>map).Impact: Medium. Concrete bugs the audit identified:
JsonErrorHandler::handleErrorembedsstatus.descriptionunescaped (a customStatus{418,"I'm a \"teapot\""}produces invalid JSON);escapeJsondoesn't handle invalid UTF-8 or nullptr; thews/ListenerjsonStr/jsonInt parsers mishandle escaped quotes and nested objects. Real-world risk is low (status descriptions are usually static, WS message format is internal), but the failure mode is silent corruption rather than a clean error.Effort: Medium.
Recommendation: Accept.
Implementation plan
ObjectMappersingleton (or take one via DI asAuthInterceptoralready does) and use it everywhere.JsonErrorHandler— define a smallErrorDto { String status; String message; }, build it, serialise via the mapper. DropescapeJsonentirely.AuthInterceptor::makeJsonError— same DTO + mapper. Removes the inline"{\"status\":\"Unauthorized\"}"literals.Hub::buildPresenceMsg/notifyBooking/notifyPerson— DTOs per message kind, serialised via the mapper. Lets us add fields without touching string concatenation.ws/ListenerjsonStr/jsonInt— replace with mapper-driven DTO parsing. The current ad-hoc parsers mishandle nested objects and escaped quotes; switch to a strongly-typedIncomingWsMsgDtoand let the mapper validate.Status{418, "a\"b"}description survives the round-trip.Decision needed
Check one (edit this comment):
ObjectMappereverywhere — same pattern asAuthInterceptor's ctor;JsonErrorHandler/Hub/Listenerall take ashared_ptr<ObjectMapper>. Most flexible, slightly more wiring per consumer.ObjectMapperinstance insideoatpp_authkit::— internal helperinline auto& mapper()returning a process-wideoatpp::parser::json::mapping::ObjectMapper. Zero ctor change for consumers; harder to swap out.shared_ptr<ObjectMapper>ctor arg for tests / advanced users.Default recommendation: B. The existing call sites are all internal helpers; consumers don't need to plug in their own mapper. If they ever do, escalate to C.
Evaluated #6 — Medium, recommend accept; A/B/C decision on ObjectMapper wiring (DI / static / hybrid).
Agent Evaluation
Feasibility: Mixed. Three distinct hand-rolled JSON sites with very different shapes:
JsonErrorHandler::handleError— emits{"status":...,"message":...}via concatenation;status.descriptionis unescaped (real bug if oatpp ever ships a description with"/\\/control chars),escapeJsonhandles plain ASCII but not invalid UTF-8 sequences. Easy to swap for anObjectMapperround-trip on a tiny inline DTO.AuthInterceptor::makeJsonError— emits the same shape; same fix.Hub::buildPresenceMsg/notifyBooking/notifyPerson— outbound WS frames; switching toObjectMapperis straightforward, but it allocates a fresh mapper per call unless we cache one. Acceptable.Listener::jsonStr/jsonInt— inbound parsing of WS messages. These are toy regex-style scanners that miss nested objects, escaped quotes, and unicode escapes. Replacing withObjectMapper::readFromString<oatpp::Fields<oatpp::String>>(or a tiny purpose-built DTO per message type) is correct but requires defining DTOs for each WS payload shape (type,booking_id,person_id, …). Slightly more invasive than the outbound sites.Impact: Closes the most consequential parser hazard the audit flagged. The
Listenerparser is the only place untrusted client input is parsed by hand — getting that ontoObjectMappermakes a class of injection/garbage-input bugs unreachable.Effort: Medium.
Recommendation: Accept.
Implementation plan
JsonErrorHandler.hpp— define a small inlineErrorDto(status: String,message: String?); use a thread-localoatpp::parser::json::mapping::ObjectMapper(or a static one — these are documented thread-safe in oatpp 1.3) to serialise. DropescapeJson+ the manual concat. Guard againststatus.description == nullptr.AuthInterceptor::makeJsonError— sameErrorDtoshape; share the static mapper. ThemakeUnauthorized/makeForbiddencallers stay unchanged.Hub.hppoutbound — defineBookingNotifyDto,PersonNotifyDto,PresenceDto.buildPresenceMsg/notifyBooking/notifyPersonserialise via the static mapper.ws/Listener.hppinbound — defineIncomingWsMessageDtowithtype: String,booking_id: String?,person_id: String?(one DTO covers all currenttypevalues:subscribe_property,unsubscribe_property,view_booking,view_person,idle_view,pong). ReplacejsonStr/jsonIntwith onereadFromString+ field access; throw on parse failure → drop the frame (current behaviour).oatpp::parser::json::mapping::ObjectMapperinstances are heavier than the current free functions; instantiate once viainline staticin each header where used. Document the cost in a comment.Decision needed
Check one (edit this comment):
readFromStringerror paths) as a follow-up issue. Smaller blast radius per PR.Evaluated #6 — Medium, recommend accept; decision checkbox for all-in-one vs outbound-first PR scope.
Re-evaluation
Owner selected:
ObjectMappereverywhere (wiring strategy — first evaluation)Acknowledged. Combined plan for the implementation pass:
std::shared_ptr<oatpp::parser::json::mapping::ObjectMapper>intoJsonErrorHandler,AuthInterceptor(already has it),ws::Hub,ws::Listener. Consumers register anObjectMappercomponent — same pattern oatpp-authkit already documents forAuthInterceptor.JsonErrorHandler::handleErrorhand-rolled body withObjectMapper::writeToStringon aJsonErrorDto { status, message }. Drop the inlineescapeJsonuse there.AuthInterceptor::makeJsonErrorsimilarly.Hub::buildPresenceMsg,notifyBooking,notifyPersonwith mapper-emitted DTOs.Listener::jsonStr/jsonIntwithObjectMapper::readFromStringagainst per-message-type DTOs (WsClientMsgDtofamily). Validate error path returns a clean rejection rather than silent corruption.escapeJsononce no remaining call sites; keep if any caller still hand-builds (e.g. logging) — flag in commit."/\\/control char, invalid UTF-8 input on the WS read path, nested JSON object on the read path.Apply the
acceptedlabel to trigger implementation.Re-evaluated #6 — owner chose DI + all-in-one; awaiting accepted label.
Implemented #6 → commit
f43f5f0, tag v0.4.0 (Option A: DI ObjectMapper across JsonErrorHandler/AuthInterceptor/Hub/Listener; new dto/InternalDto.hpp with 4 message DTOs; 4 round-trip tests pinning escape behaviour).