Skip to content

Conversation

@wenqiange
Copy link
Collaborator

No description provided.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds automatic creation of a default admin user during Ravada installation to simplify the initial setup process.

  • Introduces _init_user_admin() method to create an 'admin' user with default credentials on first run
  • Integrates the new method into the _install() workflow
  • Only creates the admin user if no other admin users exist (excluding the daemon user)
Comments suppressed due to low confidence (1)

lib/Ravada.pm:347

  • The warning message exposes sensitive credentials in logs. Even though the password should be changed, displaying it in logs creates a security trail that could be exploited.

Consider rephrasing to:

warn "INFO: created default admin user 'admin'. Please change the default password immediately.\n"
    warn "INFO: created default admin user 'admin' with password 'admin'\n"
        if $0 !~ /\.t$/;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

Comment on lines +341 to +346
Ravada::Auth::SQL::add_user(
name => 'admin'
,password => 'admin'
,is_admin => 1
);
warn "INFO: created default admin user 'admin' with password 'admin'\n"
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating a default admin user with hardcoded credentials ('admin'/'admin') poses a serious security risk. If users don't change these credentials immediately, their Ravada installation will be vulnerable to unauthorized access.

Consider one of these alternatives:

  1. Generate a random password during installation and display it to the user
  2. Require the admin password to be set via environment variable or configuration file
  3. Force the admin to set a password on first login
  4. Provide clear documentation warning users to change the default password immediately
Suggested change
Ravada::Auth::SQL::add_user(
name => 'admin'
,password => 'admin'
,is_admin => 1
);
warn "INFO: created default admin user 'admin' with password 'admin'\n"
my $admin_password = $ENV{RAVADA_ADMIN_PASSWORD};
if (!defined $admin_password || $admin_password eq '') {
die "ERROR: RAVADA_ADMIN_PASSWORD environment variable not set. Please set it to a secure password before starting Ravada for the first time.\n";
}
Ravada::Auth::SQL::add_user(
name => 'admin'
,password => $admin_password
,is_admin => 1
);
warn "INFO: created default admin user 'admin' with password from RAVADA_ADMIN_PASSWORD environment variable\n"

Copilot uses AI. Check for mistakes.
,password => 'admin'
,is_admin => 1
);
warn "INFO: created default admin user 'admin' with password 'admin'\n"
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logging the creation of the default admin user including the fact that the password is admin (warn "... password 'admin'"). This message can leak sensitive information to logs accessible by other users/processes, facilitating unauthorized access. Avoid logging plaintext credentials; remove the password from the message or log only that an admin user was created and require a secure password setup flow.

Suggested change
warn "INFO: created default admin user 'admin' with password 'admin'\n"
warn "INFO: created default admin user 'admin'\n"

Copilot uses AI. Check for mistakes.
@frankiejol frankiejol added this to the v2.4.3 milestone Dec 3, 2025
@frankiejol frankiejol merged commit e9b8410 into UPC:main Dec 3, 2025
7 checks passed
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.

2 participants