Skip to content

Fix: Account update blocked when avatar/cover file doesn't exist (#439) #440

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
polevaultweb wants to merge 1 commit into from
Closed

Fix: Account update blocked when avatar/cover file doesn't exist (#439) #440

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions includes/forms/trait-wpum-account.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ protected function update_account_values( $user, $values, $partial_form = false
$currently_uploaded_file = $current_uploaded_avatar ? esc_url_raw( $current_uploaded_avatar ) : false;
$existing_avatar_file_path = get_user_meta( $updated_user_id, '_current_user_avatar_path', true );

if ( $existing_avatar_file_path && strpos( realpath( $existing_avatar_file_path ), $upload_dir ) !== 0 ) {
if ( $existing_avatar_file_path && file_exists( $existing_avatar_file_path ) && strpos( realpath( $existing_avatar_file_path ), $upload_dir ) !== 0 ) {
throw new Exception( esc_html__( 'Path error with existing avatar', 'wp-user-manager' ) );
}

Expand Down Expand Up @@ -103,7 +103,7 @@ protected function update_account_values( $user, $values, $partial_form = false
$currently_uploaded_cover = $current_uploaded_cover ? esc_url_raw( $current_uploaded_cover ) : false;
$existing_cover_file_path = get_user_meta( $updated_user_id, '_user_cover_path', true );

if ( $existing_cover_file_path && strpos( realpath( $existing_cover_file_path ), $upload_dir ) !== 0 ) {
if ( $existing_cover_file_path && file_exists( $existing_cover_file_path ) && strpos( realpath( $existing_cover_file_path ), $upload_dir ) !== 0 ) {
throw new Exception( esc_html__( 'Path error with existing cover', 'wp-user-manager' ) );
}

Expand Down
170 changes: 170 additions & 0 deletions tests/wpunit/Account/Issue439Test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,170 @@
<?php
/**
* Tests for issue #439: Account update fails when avatar path doesn't exist.
*
* @see https://github.com/WPUserManager/wp-user-manager/issues/439
*/

require_once dirname( __DIR__ ) . '/WPUMTestCase.php';

class Issue439Test extends WPUMTestCase {

/**
* @test
* @group regression
*/
public function it_should_allow_account_update_when_avatar_path_is_missing() {
// Arrange: Create a test user.
$user_id = $this->factory()->user->create( array(
'user_login' => 'testuser439',
'user_email' => 'test439@example.com',
) );

// Simulate an avatar path stored in user meta but the file doesn't exist.
$non_existent_path = '/tmp/wpum-uploads/non-existent-avatar.jpg';
update_user_meta( $user_id, '_current_user_avatar_path', $non_existent_path );

// Enable custom avatars option.
update_option( 'wpum_custom_avatars', true );

// Get the user.
$user = get_user_by( 'id', $user_id );

// Mock the trait method by creating a minimal test class.
require_once WPUM_PLUGIN_DIR . 'includes/forms/trait-wpum-account.php';

$account_updater = new class() {
use WPUM_Form_Account;

public function test_update( $user, $values ) {
return $this->update_account_values( $user, $values );
}
};

// Act: Try to update the account with the non-existent avatar path.
$values = array(
'account' => array(
'user_firstname' => 'Updated',
),
);

// Simulate the POST data that would come from the form.
$_POST['current_user_avatar'] = '';

try {
$result = $account_updater->test_update( $user, $values );

// Assert: The update should succeed (no exception thrown).
$this->assertIsInt( $result, 'Account update should return user ID when avatar path is missing.' );
$this->assertEquals( $user_id, $result, 'Returned user ID should match the original user ID.' );

// Verify the user data was actually updated.
$updated_user = get_user_by( 'id', $user_id );
$this->assertEquals( 'Updated', $updated_user->first_name, 'First name should be updated.' );

} catch ( Exception $e ) {
// If we get here, the bug still exists.
$this->fail( 'Account update should not throw exception when avatar path is missing. Got: ' . $e->getMessage() );
}
}

/**
* @test
* @group regression
*/
public function it_should_allow_account_update_when_cover_path_is_missing() {
// Arrange: Create a test user.
$user_id = $this->factory()->user->create( array(
'user_login' => 'testuser439cover',
'user_email' => 'test439cover@example.com',
) );

// Simulate a cover path stored in user meta but the file doesn't exist.
$non_existent_path = '/tmp/wpum-uploads/non-existent-cover.jpg';
update_user_meta( $user_id, '_user_cover_path', $non_existent_path );

// Get the user.
$user = get_user_by( 'id', $user_id );

// Mock the trait method.
require_once WPUM_PLUGIN_DIR . 'includes/forms/trait-wpum-account.php';

$account_updater = new class() {
use WPUM_Form_Account;

public function test_update( $user, $values ) {
return $this->update_account_values( $user, $values );
}
};

// Act: Try to update the account with the non-existent cover path.
$values = array(
'account' => array(
'user_lastname' => 'UpdatedLast',
),
);

// Simulate no cover in POST.
$_POST['current_user_cover'] = '';

try {
$result = $account_updater->test_update( $user, $values );

// Assert: The update should succeed.
$this->assertIsInt( $result, 'Account update should return user ID when cover path is missing.' );
$this->assertEquals( $user_id, $result, 'Returned user ID should match the original user ID.' );

// Verify the user data was actually updated.
$updated_user = get_user_by( 'id', $user_id );
$this->assertEquals( 'UpdatedLast', $updated_user->last_name, 'Last name should be updated.' );

} catch ( Exception $e ) {
$this->fail( 'Account update should not throw exception when cover path is missing. Got: ' . $e->getMessage() );
}
}

/**
* @test
* @group security
*/
public function it_should_block_directory_traversal_when_avatar_path_exists() {
// Arrange: Create a test user.
$user_id = $this->factory()->user->create( array(
'user_login' => 'testuser439security',
'user_email' => 'test439security@example.com',
) );

// Enable custom avatars.
update_option( 'wpum_custom_avatars', true );

// Create a malicious path that exists but is outside upload dir.
$malicious_path = WPUM_PLUGIN_DIR . 'wp-user-manager.php';
update_user_meta( $user_id, '_current_user_avatar_path', $malicious_path );

$user = get_user_by( 'id', $user_id );

require_once WPUM_PLUGIN_DIR . 'includes/forms/trait-wpum-account.php';

$account_updater = new class() {
use WPUM_Form_Account;

public function test_update( $user, $values ) {
return $this->update_account_values( $user, $values );
}
};

$values = array(
'account' => array(
'user_firstname' => 'Hacker',
),
);

$_POST['current_user_avatar'] = '';

// Act & Assert: Should throw exception because the path exists but is outside upload dir.
$this->expectException( Exception::class );
$this->expectExceptionMessage( 'Path error with existing avatar' );

$account_updater->test_update( $user, $values );
}
}
Loading