Opened 14 months ago

Closed 9 months ago

#22234 closed Bug (fixed)

Special characters in database password are not escaped correctly on Windows platforms

Reported by: djangoproject.com@… Owned by: nobody
Component: Core (Management commands) Version: master
Severity: Normal Keywords:
Cc: anubhav9042@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

https://github.com/django/django/blob/master/django/db/backends/mysql/client.py#L38

On Windows platforms, the database client command line is composed as a string, but no escaping whatsoever is performed on the parameters. Database configuration can contain special characters (especially the password), which would require escaping.

Real-life case: http://stackoverflow.com/questions/22280126/sign-in-database-password-in-django

Change History (6)

comment:1 Changed 14 months ago by djangoproject.com@…

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Version changed from 1.6 to master

comment:2 Changed 14 months ago by bmispelon

  • Component changed from Database layer (models, ORM) to Core (Management commands)
  • Triage Stage changed from Unreviewed to Accepted

Hi,

It seems that the different codepath on windows was added as a result of #10357.

Looking at the discussion (especially comment number 3 [1]), it seems that using subprocess was considered at the time but it wasn't used because it was incompatible with Python 2.3 which was supported at the time.

The patch looks good and cleans things up nicely but I don't have access to a windows box to make sure it actually works.
I'm also concerned about a potential for subtle differences between os.execp and subprocess.call. The documentation of os.execp [2] is a bit hard to read and as a result, I can't really tell exactly what differences there are between the two.

Still, I'll mark this ticket as accepted, if only on the basis that using os.system(" ".join(args)) is clearly wrong.

Thanks.

[1] https://code.djangoproject.com/ticket/10357#comment:3
[2] http://docs.python.org/2/library/os.html?highlight=os.execvp#os.execvp

comment:3 follow-up: Changed 14 months ago by lanzz

(I'm the reporter of this bug, but forgot to log in when I submitted it)

From the subprocess.call docs:

The full function signature is the same as that of the Popen constructor - this functions passes all supplied arguments directly through to that interface.

From the subprocess.Popen docs:

Execute a child program in a new process. On Unix, the class uses os.execvp()-like behavior to execute the child program. On Windows, the class uses the Windows CreateProcess() function.

Sounds to me subprocess.call and os.execvp are explicitly meant to be compatible on Unix, and on Windows os.execvp isn't an option anyway.

Last edited 14 months ago by lanzz (previous) (diff)

comment:4 in reply to: ↑ 3 Changed 14 months ago by anubhav9042

  • Cc anubhav9042@… added

If only you could add a test to your PR, we can test the new implementation.

comment:5 Changed 14 months ago by lanzz

I would, but I'm not really sure how to approach testing this change. The fact that there are zero existing tests for the DatabaseClient implementations does not help either — it seems historical changes to backends.*.client.DatabaseClient have never included a testcase.

comment:6 Changed 9 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In bf5382c6e549d78da9f7029cd86686459b52eaed:

Fixed #22234 -- Replaced OS-specific code with subprocess.call() in dbshell.

This fixes escaping of special characters on Windows.

Note: See TracTickets for help on using tickets.
Back to Top