Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#28201 closed Cleanup/optimization (fixed)

Make CharField form field prohibit null characters

Reported by: CM Lubinski Owned by: Alejandro Zamora Fonseca
Component: Forms Version: 1.11
Severity: Normal Keywords:
Cc: Vlada Macek Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Input with null characters cause Django to crash when saving data.

Reproduction recipe:

  1. Setup Django 1.11.1, Postgres (I don't think the version matters) using Python 3.5 (though this may apply to any 3.x).
  2. Create an admin account & login.
  3. Navigate to /admin/auth/group/add/
  4. Using the JavaScript console, execute
    $('#id_name').val("\x00something")
    
  5. Submit the form. See the error

Result will be similar to:

dev-api_1           | Internal Server Error: /admin/auth/group/add/
dev-api_1           | Traceback (most recent call last):
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/django/core/handlers/exception.py", line 41, in inner
dev-api_1           |     response = get_response(request)
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/django/core/handlers/base.py", line 187, in _get_response
dev-api_1           |     response = self.process_exception_by_middleware(e, request)
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/django/core/handlers/base.py", line 185, in _get_response
dev-api_1           |     response = wrapped_callback(request, *callback_args, **callback_kwargs)
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/django/contrib/admin/options.py", line 551, in wrapper
dev-api_1           |     return self.admin_site.admin_view(view)(*args, **kwargs)
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/django/utils/decorators.py", line 149, in _wrapped_view
dev-api_1           |     response = view_func(request, *args, **kwargs)
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/django/views/decorators/cache.py", line 57, in _wrapped_view_func
dev-api_1           |     response = view_func(request, *args, **kwargs)
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/django/contrib/admin/sites.py", line 224, in inner
dev-api_1           |     return view(request, *args, **kwargs)
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/django/contrib/admin/options.py", line 1508, in add_view
dev-api_1           |     return self.changeform_view(request, None, form_url, extra_context)
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/django/utils/decorators.py", line 67, in _wrapper
dev-api_1           |     return bound_func(*args, **kwargs)
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/django/utils/decorators.py", line 149, in _wrapped_view
dev-api_1           |     response = view_func(request, *args, **kwargs)
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/django/utils/decorators.py", line 63, in bound_func
dev-api_1           |     return func.__get__(self, type(self))(*args2, **kwargs2)
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/django/contrib/admin/options.py", line 1408, in changeform_view
dev-api_1           |     return self._changeform_view(request, object_id, form_url, extra_context)
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/django/contrib/admin/options.py", line 1440, in _changeform_view
dev-api_1           |     if form.is_valid():
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/django/forms/forms.py", line 183, in is_valid
dev-api_1           |     return self.is_bound and not self.errors
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/django/forms/forms.py", line 175, in errors
dev-api_1           |     self.full_clean()
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/django/forms/forms.py", line 386, in full_clean
dev-api_1           |     self._post_clean()
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/django/forms/models.py", line 402, in _post_clean
dev-api_1           |     self.validate_unique()
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/django/forms/models.py", line 411, in validate_unique
dev-api_1           |     self.instance.validate_unique(exclude=exclude)
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/django/db/models/base.py", line 1032, in validate_unique
dev-api_1           |     errors = self._perform_unique_checks(unique_checks)
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/django/db/models/base.py", line 1129, in _perform_unique_checks
dev-api_1           |     if qs.exists():
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/django/db/models/query.py", line 668, in exists
dev-api_1           |     return self.query.has_results(using=self.db)
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/django/db/models/sql/query.py", line 517, in has_results
dev-api_1           |     return compiler.has_results()
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/django/db/models/sql/compiler.py", line 845, in has_results
dev-api_1           |     return bool(self.execute_sql(SINGLE))
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/django/db/models/sql/compiler.py", line 886, in execute_sql
dev-api_1           |     raise original_exception
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/django/db/models/sql/compiler.py", line 876, in execute_sql
dev-api_1           |     cursor.execute(sql, params)
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/debug_toolbar/panels/sql/tracking.py", line 165, in execute
dev-api_1           |     return self._record(self.cursor.execute, sql, params)
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/debug_toolbar/panels/sql/tracking.py", line 107, in _record
dev-api_1           |     return method(sql, params)
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/django/db/backends/utils.py", line 80, in execute
dev-api_1           |     return super(CursorDebugWrapper, self).execute(sql, params)
dev-api_1           |   File "/usr/src/app/.venv-dev/lib/python3.5/site-packages/django/db/backends/utils.py", line 65, in execute
dev-api_1           |     return self.cursor.execute(sql, params)
dev-api_1           | ValueError: A string literal cannot contain NUL (0x00) characters.

Change History (20)

comment:1 by Tim Graham, 7 years ago

Component: contrib.adminDatabase layer (models, ORM)
Severity: NormalRelease blocker
Summary: 500 when using a null character, Postgres + Python 3.5.3Saving a Char/TextField with psycopg2 2.7+ raises ValueError: A string literal cannot contain NUL (0x00) characters is unhandled
Triage Stage: UnreviewedAccepted

This exception was introduced in psycopg2 2.7+. With psycopg2 < 2.7, there is no exception and null bytes are silently truncated by PostgreSQL. Other databases that I tested (SQLite, MySQL, Oracle) allow saving null bytes. This creates possible cross-database compatibility problems when moving data from those databases to PostgreSQL, e.g.#28117. I've tentatively closed that ticket as a duplicate but will reopen it if the solution for this ticket doesn't address it.

I propose having CharField and TextField strip null bytes from the value either a) only on PostgreSQL or b) on all databases. I raised a django-developers thread to ask for feedback.

comment:2 by Biswajit Sahu, 7 years ago

Owner: changed from nobody to Biswajit Sahu
Status: newassigned

comment:3 by Tim Graham, 7 years ago

Owner: Biswajit Sahu removed
Status: assignednew

Please don't work on this issue until there's a consensus about the solution.

comment:4 by Tim Graham, 7 years ago

Component: Database layer (models, ORM)Forms
Severity: Release blockerNormal
Summary: Saving a Char/TextField with psycopg2 2.7+ raises ValueError: A string literal cannot contain NUL (0x00) characters is unhandledMake CharField/TextField prohibit null characters
Type: BugCleanup/optimization

The consensus on the mailing list is to make CharField and TextField prohibit null characters.

comment:5 by Rajesh Veeranki, 7 years ago

So the way to achieve this is by adding a default validator to CharField and TextField and raise a validation error. Am I right?

comment:6 by Tim Graham, 7 years ago

That seems reasonable.

comment:7 by Alejandro Zamora Fonseca, 7 years ago

Owner: set to Alejandro Zamora Fonseca
Status: newassigned

comment:8 by Alejandro Zamora Fonseca, 7 years ago

Added default null characters validation to CharField and TextField in
order to avoid raising errors on database layer. Added test for both
fields. Tested on PostgreSQL, MySQL, and SQLite backends.

comment:9 by Alejandro Zamora Fonseca, 7 years ago

Linked PR against master branch PR

Last edited 7 years ago by Claude Paroz (previous) (diff)

comment:10 by Claude Paroz, 7 years ago

Has patch: set
Patch needs improvement: set

comment:11 by Alejandro Zamora Fonseca, 7 years ago

Maybe only adding the null char validator to CharField would be enough?
Because TextField is converted to CharField when Model to Field translation occurs.

comment:12 by Tim Graham, 7 years ago

Summary: Make CharField/TextField prohibit null charactersMake CharField form field prohibit null characters

Correct, there's isn't a TextField form field.

comment:13 by Claude Paroz, 7 years ago

Patch needs improvement: unset

comment:14 by Tim Graham, 7 years ago

Patch needs improvement: set

comment:15 by Alejandro Zamora Fonseca, 7 years ago

I added documentation and unit tests for the validator.

comment:16 by Alejandro Zamora Fonseca, 7 years ago

Patch needs improvement: unset

comment:17 by Alejandro Zamora Fonseca, 7 years ago

Triage Stage: AcceptedReady for checkin

comment:18 by Tim Graham <timograham@…>, 7 years ago

Resolution: fixed
Status: assignedclosed

In 90d7b91:

Fixed #28201 -- Added ProhibitNullCharactersValidator and used it on CharField form field.

comment:19 by Vlada Macek, 6 years ago

Cc: Vlada Macek added

We just ran into this on 1.11 LTS just as the original author. May I ask whether it is usual to backport fixes like this?

Thank you.

comment:20 by Tim Graham, 6 years ago

Per our supported versions policy, 1.11 is only receiving data loss and security fixes.

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