Skip to content

Conversation

MahdiBagheri71
Copy link
Contributor

Prerequisites

  • Laravel Reverb Server
  • PHP 8.1+
  • Composer

Description

This pull request introduces improved validation for WebSocket message payloads and enhances the handling of channel_data. These changes ensure better stability, prevent server crashes, and align with the latest Laravel best practices.

Key Changes

  1. Message Payload Validation: Comprehensive validation added for incoming WebSocket messages.
  2. Improved Channel Data Parsing: Safely handles channel_data JSON parsing to avoid crashes.
  3. Error Handling: Returns clear error responses for invalid payloads.

Current Behavior

Currently, the server accepts messages without proper validation. This can lead to:

  • Server crashes due to invalid data types
  • Unexpected behavior caused by malformed channel_data
  • Missing validation for required fields

Example of Problematic Payload:

{
    "event": [],  // Invalid: should be a string
    "data": {
        "channel": null,  // Invalid: should be a string
        "channel_data": "invalid-json",  // Invalid: should be valid JSON
        "auth": []  // Invalid: should be a string
    }
}

New Behavior

With this PR:

  1. All incoming message payloads are validated.
  2. Invalid messages receive proper error responses.
  3. channel_data is safely parsed and handled.

Validation Rules:

Validator::make($event, [
    'event' => ['required', 'string'],
    'data' => ['nullable', 'array'],
    'data.channel' => ['required', 'string'],
    'data.auth' => ['nullable', 'string'],
    'data.channel_data' => ['nullable', 'json'],
])->validate();

Improved channel_data Handling:

if (isset($message['data']['channel_data']) && is_string($message['data']['channel_data'])) {
    $message['data']['channel_data'] = json_decode($message['data']['channel_data'], true);
}

Test Coverage

Comprehensive tests have been added in tests/Unit/Protocols/Pusher/ServerTest.php to cover:

  • Validation for required fields
  • Handling of invalid channel_data
  • Error responses for malformed payloads

Example Test Case:

it('sends an error for invalid channel_data type', function () {
    $this->server->message(
        $connection = new FakeConnection,
        json_encode([
            'event' => 'pusher:subscribe',
            'data' => [
                'channel' => 'presence-test-channel',
                'auth' => '',
                'channel_data' => [],
            ],
        ])
    );

    $connection->assertReceived([
        'event' => 'pusher:error',
        'data' => json_encode([
            'code' => 4200,
            'message' => 'Invalid message format',
        ]),
    ]);
});

Benefits

  • ✅ Prevents server crashes from malformed messages
  • ✅ Provides clear and consistent error responses
  • ✅ Improves server stability and reliability
  • ✅ Maintains compatibility with the Pusher protocol
  • ✅ Zero breaking changes

Breaking Changes

None. Invalid messages that previously caused crashes will now receive proper error responses instead.


Related Issues

Fixes #298

Adds type checking for channel_data to prevent JSON decode errors when the data is not a string format, improving the robustness of message handling in the CLI logger.
Add comprehensive validation for incoming WebSocket messages using Laravel Validator to:
- Ensure correct message structure and data types
- Prevent malformed data from causing server errors
- Validate required fields (event, data.channel)
- Enforce JSON format for channel_data
- Improve overall server stability and security

This change helps prevent server crashes and unexpected behavior caused by invalid message formats.
Add comprehensive test coverage for message payload validation including:
- Event type validation
- Data structure validation
- Channel format validation
- Auth data type checking
- Channel data format verification

Each test case ensures proper error responses for invalid message formats,
maintaining consistency with Pusher protocol specifications.
@taylorotwell taylorotwell requested a review from joedixon January 19, 2025 17:38
@taylorotwell taylorotwell marked this pull request as draft January 19, 2025 17:38
@joedixon
Copy link
Collaborator

Thanks @MahdiBagheri71 - I think this looks pretty sensible, though looks like it introduces some failing tests. Have your changes caused a regression elsewhere?

@joedixon
Copy link
Collaborator

Thanks @MahdiBagheri71 - I think this looks pretty sensible, though looks like it introduces some failing tests. Have your changes caused a regression elsewhere?

I've carried out some testing on this pull request and the validation doesn't account for evetns such as client events and control messages which have a completely different format. We may have to move the validation to each event handler, but I want to think on this to see if there is a better solution.

@MahdiBagheri71
Copy link
Contributor Author

Thanks for reviewing it @joedixon
The tests I wrote were unit tests, and I checked that there were no issues. I wasn’t able to run the feature tests on my system, either before or after the changes!

I tried to cover all the possible cases in the functions I wrote and considered the input typescript of all the functions in my validation to avoid any errors and ensure the socket doesn’t go out of access.

@joedixon
Copy link
Collaborator

@MahdiBagheri71 sounds good, but there are some breaking changes so leave this with me for a bit and I'll see about addressing them.

@joedixon
Copy link
Collaborator

@MahdiBagheri71 can you give these updates a try with the tests you have configured in your video?

@MahdiBagheri71 MahdiBagheri71 marked this pull request as ready for review January 21, 2025 06:12
@MahdiBagheri71
Copy link
Contributor Author

I tested everything based on the new changes, and the issues I had were completely resolved.
Thank you @joedixon

@joedixon joedixon marked this pull request as draft January 21, 2025 11:34
@joedixon joedixon marked this pull request as ready for review January 21, 2025 13:41
@taylorotwell taylorotwell merged commit c55c36a into laravel:main Jan 21, 2025
11 checks passed
@MahdiBagheri71 MahdiBagheri71 deleted the feat/message-validation branch January 23, 2025 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect condition in CliLogger::message method

3 participants