Moodle
  1. Moodle
  2. MDL-10607

Single quote in the password to the database during Moodle installation breaks the process

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.8.2
    • Fix Version/s: 1.9, 2.0
    • Component/s: Installation
    • Labels:
      None
    • Environment:
      GNU/Linux Slackware 12.0: Kernel 2.6.21.5-smp, Apache 2.2.4, MySQL 5.0.37, PHP 5.2.3
    • Database:
      MySQL
    • Affected Branches:
      MOODLE_18_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Rank:
      28805

      Description

      I have setup a database named `moodle' which a user named `moodle' identified by password `Let's Learn!' will use.

      In one of the dialog during the installation process that asked me for the database settings, I entered `Let's Learn!' as the password.

      Later after there was a notification that `config.php' had been created successfully, the installation process stopped.

      Checking the last line in the error log, there is a line saying:
      [error] [client 127.0.0.1] PHP Parse error: syntax error, unexpected T_STRING in /var/www/htdocs/moodle/config.php on line 9

      Looking at `config.php', I found the following line:
      $CFG->dbpass = 'Let's Learn!';

      After I changed the above line to:
      $CFG->dbpass = 'Let\'s Learn!';
      I can continue the installation process by opening `index.php' once again.

        Activity

        Hide
        Daniele Cordella added a comment -

        does it change if you change in config.php to
        $CFG->dbpass = "Let's Learn!"; ?

        Show
        Daniele Cordella added a comment - does it change if you change in config.php to $CFG->dbpass = "Let's Learn!"; ?
        Hide
        Eus added a comment -

        I haven't tried to put:
        $CFG->dbpass = "Let's Learn!";

        But, I think, this quoting strategy will also break if someone has a password such as:
        Let's "Learn"!

        I guess the installation program needs to quote any single quote in the password.

        Show
        Eus added a comment - I haven't tried to put: $CFG->dbpass = "Let's Learn!"; But, I think, this quoting strategy will also break if someone has a password such as: Let's "Learn"! I guess the installation program needs to quote any single quote in the password.
        Hide
        Petr Škoda added a comment -

        lowering to major from blocker, admins may usually choose another password

        Show
        Petr Škoda added a comment - lowering to major from blocker, admins may usually choose another password
        Hide
        Eus added a comment -

        Petr, do you mean that I should change the priority from `major' to `blocker'?
        If so, how can I do that? I mean, I don't see any link here to change that.
        Thanks.

        Show
        Eus added a comment - Petr, do you mean that I should change the priority from `major' to `blocker'? If so, how can I do that? I mean, I don't see any link here to change that. Thanks.
        Hide
        Petr Škoda added a comment -

        sorry, this is not a blocker, please choose password without quotes for now or edit config.php directly

        Show
        Petr Škoda added a comment - sorry, this is not a blocker, please choose password without quotes for now or edit config.php directly
        Hide
        Petr Škoda added a comment -

        thanks for the report anyway

        Show
        Petr Škoda added a comment - thanks for the report anyway
        Hide
        Dan Poltawski added a comment -

        Thanks for the report. I've fixed this so that single quotes and backslashes are now escaped in db username and passwords in the installer configuration generation.

        Show
        Dan Poltawski added a comment - Thanks for the report. I've fixed this so that single quotes and backslashes are now escaped in db username and passwords in the installer configuration generation.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Hi Dan,

        I was cloning this change to the Moodle Windows installer when I've thought that the changes you've perfomed aren't cross-db, because the assumption of all DBs escaping with the backslash character isn't true! (both mssql and oracle escape character is single quote - in fact postgres also works with that).

        So I guess that the function addsingleslashes()

        should do some switch / case so:

        mysql, mysqli and postgres7 => escape with backslashes both the single quote and the backslash chars

        the rest:

        => escape with single quote the single quote char.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Hi Dan, I was cloning this change to the Moodle Windows installer when I've thought that the changes you've perfomed aren't cross-db, because the assumption of all DBs escaping with the backslash character isn't true! (both mssql and oracle escape character is single quote - in fact postgres also works with that). So I guess that the function addsingleslashes() should do some switch / case so: mysql, mysqli and postgres7 => escape with backslashes both the single quote and the backslash chars the rest: => escape with single quote the single quote char. Ciao
        Hide
        Dan Poltawski added a comment -

        Hi Eloy,

        The single slash escaping should only be escaping from the php string, and not from the database, so that the installer can generate the line:
        $CFG->dbpass = 'foo\'bar';

        for a database password foo'bar without causing errors in the php.

        I don't think it should be escaping from the database itself?

        Show
        Dan Poltawski added a comment - Hi Eloy, The single slash escaping should only be escaping from the php string, and not from the database, so that the installer can generate the line: $CFG->dbpass = 'foo\'bar'; for a database password foo'bar without causing errors in the php. I don't think it should be escaping from the database itself?
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Ah, I see!!!

        it's about to write correct PHP in config.php! grrr,

        Sorry, I misunderstood the bug. Forget my previous comment!

        Anyway, if the objective is the PHP file.... then why are you also escaping backslashes? Isn't enough to escape single-quotes (in a single-quoted string like the generated in config.php).

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Ah, I see!!! it's about to write correct PHP in config.php! grrr, Sorry, I misunderstood the bug. Forget my previous comment! Anyway, if the objective is the PHP file.... then why are you also escaping backslashes? Isn't enough to escape single-quotes (in a single-quoted string like the generated in config.php). Ciao
        Hide
        Dan Poltawski added a comment -

        Hmm - I wasn't thinking correctly about the backslashes. My initial thoughts were to prevent problems with a backslash being the final character in the string (thus causing breakage again )

        Show
        Dan Poltawski added a comment - Hmm - I wasn't thinking correctly about the backslashes. My initial thoughts were to prevent problems with a backslash being the final character in the string (thus causing breakage again )
        Hide
        Dan Poltawski added a comment -

        I did modify the regexp to only escape a final \ - but then that breaks if the user puts
        at the end of their password string So I think its best to escape all the backslashes.

        Eloy is it ok to close this again?

        Also this is where perls qw() comes really useful when writing regexps in escaping hell!

        Show
        Dan Poltawski added a comment - I did modify the regexp to only escape a final \ - but then that breaks if the user puts at the end of their password string So I think its best to escape all the backslashes. Eloy is it ok to close this again? Also this is where perls qw() comes really useful when writing regexps in escaping hell!

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: