fix: accepts unbounded nested payload - #2262
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces size validation guards for nested payloads in client-supplied game actions, such as sideboard submissions and debug actions, to prevent large allocations and resource exhaustion. The review feedback identifies a resource exhaustion vulnerability in bound_serialized_json where full in-memory serialization can be replaced with a custom byte-limiting writer, a performance bottleneck in the sideboard validation loop due to eager string formatting, and the opportunity to clean up the unused guard_deck_card_count_payload helper.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| fn bound_serialized_json<T: Serialize>(field: &str, value: &T) -> Result<(), String> { | ||
| let serialized = serde_json::to_string(value) | ||
| .map_err(|err| format!("{field} could not be serialized for size validation: {err}"))?; | ||
| if serialized.len() > MAX_DEBUG_AST_JSON_LEN { | ||
| return Err(format!( | ||
| "{field} is {} serialized bytes; at most {MAX_DEBUG_AST_JSON_LEN} allowed", | ||
| serialized.len() | ||
| )); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Resource Exhaustion Vector in Security Guard
Using serde_json::to_string allocates the entire serialized JSON string in memory. If an adversary sends a deeply nested or extremely large AST payload, this will allocate a huge string and consume significant CPU/memory before the length check is even evaluated, defeating the purpose of the guard.
We can serialize directly to a custom std::io::Write that counts bytes and returns an error immediately when MAX_DEBUG_AST_JSON_LEN is exceeded, completely avoiding any memory allocation for the serialized string.
fn bound_serialized_json<T: Serialize>(field: &str, value: &T) -> Result<(), String> {
struct LimitingWriter {
written: usize,
limit: usize,
}
impl std::io::Write for LimitingWriter {
fn write(&mut self, buf: &[u8]) -> std::io::Result<usize> {
self.written += buf.len();
if self.written > self.limit {
return Err(std::io::Error::new(
std::io::ErrorKind::InvalidData,
"limit exceeded",
));
}
Ok(buf.len())
}
fn flush(&mut self) -> std::io::Result<()> {
Ok(())
}
}
let mut writer = LimitingWriter {
written: 0,
limit: MAX_DEBUG_AST_JSON_LEN,
};
serde_json::to_writer(&mut writer, value)
.map_err(|err| format!("{field} size validation failed or exceeded limit: {err}"))
}| for (index, card) in main.iter().enumerate() { | ||
| guard_deck_card_count_payload(&format!("SubmitSideboard.main[{index}]"), card)?; | ||
| } | ||
| for (index, card) in sideboard.iter().enumerate() { | ||
| guard_deck_card_count_payload(&format!("SubmitSideboard.sideboard[{index}]"), card)?; | ||
| } |
There was a problem hiding this comment.
Allocation Bottleneck in Validation Loop
Looping over main and sideboard cards and calling guard_deck_card_count_payload with a formatted string allocates a new String via format! on every single iteration in the happy path. Since MAX_ACTION_LIST_LEN is 10,000, a valid but large sideboard action could trigger up to 20,000 string allocations, creating a significant performance bottleneck and a potential CPU/memory amplification vector.
We should perform the length check directly on the card name and only allocate/format the error string on failure.
for (index, card) in main.iter().enumerate() {
if card.name.len() > MAX_CHOICE_LEN {
return Err(format!(
"SubmitSideboard.main[{index}].name is {} bytes; at most {MAX_CHOICE_LEN} allowed",
card.name.len()
));
}
}
for (index, card) in sideboard.iter().enumerate() {
if card.name.len() > MAX_CHOICE_LEN {
return Err(format!(
"SubmitSideboard.sideboard[{index}].name is {} bytes; at most {MAX_CHOICE_LEN} allowed",
card.name.len()
));
}
}| fn guard_deck_card_count_payload(field: &str, card: &DeckCardCount) -> Result<(), String> { | ||
| bound_string(&format!("{field}.name"), &card.name) | ||
| } |
There was a problem hiding this comment.
Unused Helper Function
Since we are inlining the sideboard card name validation to avoid allocating strings in a loop, this helper function is no longer needed and can be removed to keep the codebase clean.
🤖 Architecture Review (automated)Verdict: ✅ Approve Seam: PASS — Wire-size validation correctly lives in
FindingsNo blocking issues found. Additional correctness checks (all pass):
|
|
@matthewevans Could you plz review my PR? |
|
Pushed a maintainer follow-up in |
|
Also merged current |
matthewevans
left a comment
There was a problem hiding this comment.
Approved after maintainer architecture review and focused verification. Correct seam: server-core transport-boundary guard before native WebSocket dispatch/clone-heavy engine handling. Idiomatic at seam: exhaustive GameAction/DebugAction/CounterType matching with shared bounded string/list/streaming JSON helpers; no game logic added. Regression/perf: rejects oversized client-controlled lists/strings/keyword ASTs before reducer work; streaming JSON avoids allocating a full serialized payload for AST bounds. Tests discriminate the previously open gaps: reverting the debug token counter or keyword AST guards makes the new tests fail. Local verification: cargo fmt --all; git diff --check; server-core game_action_payload_guard integration test (13 passed); dispatch pre-handler guard test (1 passed).
Summary
GameActionwire validation against oversized nested client-controlled payloads.SubmitSideboardcard names, debug generic counter names, and debug keyword AST payloads.ClientMessage::Actionpayloads are rejected before handler/engine work.server-coretests,server-core/phase-servercompilation, diff hygiene, andphase-server --helpruntime smoke execution.Related Issue
Closes: #1892
Change Type
Real Behavior Proof
Before this change, some nested
GameActionpayload fields were not fully bounded even though outer vectors were checked.Now these payloads are rejected:
SubmitSideboard.main[0].namelonger thanMAX_CHOICE_LENDebug.ModifyCounters.counter_type.Genericlonger thanMAX_CHOICE_LENDebug.GrantKeyword.keywordserialized payload larger thanMAX_DEBUG_AST_JSON_LENClientMessage::Actionbefore dispatch to handler workValidation commands passed:
Test result proof:
Checklist
ClientMessage::Action