fix: handle_event(next_round) missing early return causes double broadcast (#2418)#2707
Open
immortal71 wants to merge 1 commit intoOWASP:masterfrom
Open
fix: handle_event(next_round) missing early return causes double broadcast (#2418)#2707immortal71 wants to merge 1 commit intoOWASP:masterfrom
immortal71 wants to merge 1 commit intoOWASP:masterfrom
Conversation
…OWASP#2418) Elixir has no early returns; the final three lines (Game.find, Endpoint.broadcast, {:noreply, ...}) executed unconditionally because the outer if/else was not the last expression in the function. Fix: move Game.find + broadcast + noreply into the else branch so the outer if/else is the function's last expression and each branch's value is properly returned. - round_open? true + can_continue? true: closes round, schedules 100ms timer, returns immediately - no premature broadcast - round_open? true + can_continue? false: returns {:noreply, socket} unchanged - no spurious state change - round_open? false: advances rounds_played, optionally sets finished_at, broadcasts exactly once Fixes OWASP#2418
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a control-flow bug in the Player LiveView’s "next_round" handler where logic after an if/else was unintentionally executed for all branches, causing double broadcasts and premature state updates.
Changes:
- Moves
Game.find/1,Endpoint.broadcast/3, and the final{:noreply, ...}into theelsebranch so theif/elseis the function’s last expression. - Ensures the
"round_open?"branches return without performing an immediate broadcast when a delayed:proceed_to_next_roundis scheduled.
Contributor
Author
|
@sydseter is this good to go ?? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #2418
In copi.owasp.org/lib/copi_web/live/player_live/show.ex, the handle_event(next_round) function had a control-flow bug: Elixir has no early returns, so the outer if/else block was not the last expression in the function. This caused the final three lines (Game.find, Endpoint.broadcast, {:noreply, ...}) to always execute unconditionally, regardless of which branch ran.
Root Cause
`elixir
if round_open?(game) do
..
{:noreply, assign(socket, :game, game)} # value discarded - NOT an early return
else
..
end
These always ran -- even when waiting for the 100ms timer
{:ok, updated_game} = Game.find(game.id)
CopiWeb.Endpoint.broadcast(...)
{:noreply, assign(socket, :game, updated_game)}
`
Fix
Moved Game.find + Endpoint.broadcast + {:noreply, ...} into the else branch so the outer if/else is the last expression of the function. Each branch now returns the correct value