Opened 13 years ago

Closed 11 years ago

Last modified 11 years ago

#16302 closed Bug (fixed)

Ensure contrib (namely comments) is IPv6 capable

Reported by: Dan McGee Owned by: Sasha Romijn
Component: contrib.comments Version: dev
Severity: Normal Keywords: ipv6 sprint2013
Cc: eromijn@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This is a follow-on ticket now that #811 has been implemented. What's the design decision on updating django.contrib.comments.models to utilize the new IPv6-capable address field? It appears to be the only core Django application utilizing this field.

It assumes the value of REMOTE_ADDR can be coerced into the field, as you can see in this line from django/contrib/comments/views/comments.py:

comment.ip_address = request.META.get("REMOTE_ADDR", None)

This currently populates PostgreSQL-backed instances correctly if hooked up to an IPv6-capable webserver, but all other databases will fail or truncate (perhaps even silently!) any non-IPv4 value exceeding 15 characters, such as ::ffff:192.168.100.1. The prudent decision to me seems to be to modify the Comment model to use:

ip_address  = models.IPAddressField(_('IP address'), unpack_ipv4=True, blank=True, null=True)

A change like this will require manual database migration for all storage engines that use a char type for this; I'm not sure what your policy (if any) is on that. Either way the end result is no worse than what currently happens with truncation.

Attachments (3)

ipv6-comments.diff (777 bytes ) - added by Dan McGee 13 years ago.
notes-update-comments-ip-address.patch (3.7 KB ) - added by Dan McGee 12 years ago.
fix broken code found by changing field type, add release notes
notes-update-comments-ip-address-with-tests.patch (9.2 KB ) - added by Dan McGee 12 years ago.
added tests with ipv6 address persistence

Download all attachments as: .zip

Change History (17)

by Dan McGee, 13 years ago

Attachment: ipv6-comments.diff added

comment:1 by Aymeric Augustin, 13 years ago

Needs documentation: set
Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

We need to handle backwards compatibility carefully here.

I haven't followed closely the implementation of GenericIPAddressField, but I suppose it's not represented like IPAddressField in the database, so your change may break existing users of contrib.comments when they upgrade to 1.4.

If backwards compatibility is broken, detailed instructions must be added to the release notes.

comment:2 by Sasha Romijn, 13 years ago

Cc: eromijn@… added
Keywords: ipv6 added

It would look like there is a backwards compatibility issue, but I don't believe there is, because the existing code is just as broken as the new code would be when using the old database field.

Regarding storage: both IPAddressField and GenericIPAddressField use either postgres' inet or a char(15)/char(39) for storing. So an IPv4 address stored by IPAddressField can be read by GenericIPAddressField.

Considering the current case:

  • Comment comes from an IPv4 address in the current code: no problems
  • Comment comes from an IPv6 address in the current code: silently truncated to 15 characters, or complete failure if the database does not do silent truncation (there is no model validation for IPAddressField, so it is not detected as invalid).

In other words: the comment framework is broken when a client comments from an IPv6 address.

Considering this, there is no change in behavior if we use a GenericIPAddressField with a char(15) backing it. IPv4 just works, IPv6 is truncated, either silently or loudly. However, with the unpack_ipv4 parameter, IPv6-mapped IPv4 addresses would work even with a char(15) backend - whereas they break now with IPAddressField.

So, updating this even if the database field is not changed will not introduce any additional failures, and even avoid a failure that currently exists. Obviously the best for the user is to also update the database field appropriately. Therefore, changing the field is not breaking backwards compatibility.

Regarding the patch: I agree this needs tests, I think the field change is fine. REMOTE_ADDR is exactly one of the cases I had in mind when I wrote the unpack_ipv4 parameter.

comment:3 by Jannis Leidel, 13 years ago

Yeah, definitely one of those dogfood items. +1

comment:4 by Jacob, 12 years ago

milestone: 1.4

Milestone 1.4 deleted

by Dan McGee, 12 years ago

fix broken code found by changing field type, add release notes

comment:5 by Dan McGee, 12 years ago

I'm going to throw the magic "data loss" words out there to see if we can get this ticket some attention. :) There is definite data loss for anyone running a Django site over IPv6 using the comments application and not using PostgreSQL- the IP addresses will all be truncated.

Working on adding some tests, but I'm not sure what exactly you guys are looking for here. Given that sqlite just accepts whatever you throw at it, I'm not quite sure where you see tests lacking here and I want to get on the same page.

comment:6 by Alex Gaynor, 12 years ago

If there's actually dataloss, the test should come in the form of data being sent to the view, and not roundtripping correctly.

in reply to:  5 comment:7 by Carl Meyer, 12 years ago

Replying to toofishes:

Working on adding some tests, but I'm not sure what exactly you guys are looking for here. Given that sqlite just accepts whatever you throw at it, I'm not quite sure where you see tests lacking here and I want to get on the same page.

If the current problem only exists on backends that are neither Postgres nor SQLite (because SQLite doesn't enforce max-length, and Postgres has the native inet type), then it's fine to add a test which only fails (given current code) on database backends that exhibit the problem. We do run the tests with various database backends, not just SQLite. The test should also have a comment or docstring clarifying under what circumstances it would fail.

Thanks for the report, and your work on this patch!

comment:8 by Dan McGee, 12 years ago

So would a test change that precedes the field switch be acceptable?

If you run the patch with new tests I just submitted and then run the tests, one of the two new tests fail:

======================================================================
FAIL: testCreateValidCommentIPv6Unpack (regressiontests.comment_tests.tests.comment_view_tests.CommentViewTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File ".../django/tests/regressiontests/comment_tests/tests/comment_view_tests.py", line 134, in testCreateValidCommentIPv6Unpack
    self.assertEqual(c.ip_address, address[7:])
AssertionError: u'::ffff:18.52.18.52' != '18.52.18.52'

----------------------------------------------------------------------
Ran 93 tests in 2.719s

FAILED (failures=1)

However, the one you would expect to fail, testCreateValidCommentIPv6, does not, I believe because sqlite does not truncate values even if they are over a column limit. Here is that:

$ sqlite3 test.db
SQLite version 3.7.9 2011-11-01 00:52:41
Enter ".help" for instructions
Enter SQL statements terminated with a ";"
sqlite> create table ( testcol varchar(16) );
Error: near "(": syntax error
sqlite> create table testing ( testcol varchar(16) );
sqlite> insert into testing values ("foobar");
sqlite> insert into testing values ("this is way too long of a value for this column");
sqlite> select * from testing;
foobar
this is way too long of a value for this column

So I'm not sure how to add a positive test showing the existing behavior is broken, as it only breaks with certain DB backends.

I'm typing this and already saw the next comment, so here that is:

$ PYTHONPATH=../ python2 runtests.py --settings=test_mysql comment_tests
Creating test database for alias 'default'...
Got an error creating the test database: (1007, "Can't create database 'test_'; database exists")
Type 'yes' if you would like to try deleting the test database 'test_', or 'no' to cancel: yes
Destroying old test database 'default'...
............................................................../home/dmcgee/projects/django/django/db/backends/mysql/base.py:100: Warning: Data truncated for column 'ip_address' at row 1
  return self.cursor.execute(query, args)
FF.............................
======================================================================
FAIL: testCreateValidCommentIPv6 (regressiontests.comment_tests.tests.comment_view_tests.CommentViewTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/dmcgee/projects/django/tests/regressiontests/comment_tests/tests/comment_view_tests.py", line 122, in testCreateValidCommentIPv6
    self.assertEqual(c.ip_address, address)
AssertionError: u'2a02::223:6cff:' != '2a02::223:6cff:fe8a:2e8a'

======================================================================
FAIL: testCreateValidCommentIPv6Unpack (regressiontests.comment_tests.tests.comment_view_tests.CommentViewTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/dmcgee/projects/django/tests/regressiontests/comment_tests/tests/comment_view_tests.py", line 134, in testCreateValidCommentIPv6Unpack
    self.assertEqual(c.ip_address, address[7:])
AssertionError: u'::ffff:18.52.18' != '18.52.18.52'

----------------------------------------------------------------------
Ran 93 tests in 7.414s

FAILED (failures=2)
Destroying test database for alias 'default'...

So we have a definite breakage in MySQL, it just won't show up elsewhere. This is with the tests from the patch, but before s/IPAddressField/GenericIPAddressField/. After all changes are applied from the patch, all tests pass in MySQL (as well as the default SQLite).

Sorry for the extremely verbose response, but I think we're in good shape here now, outside of any code review or English grammar/phrasing comments.

comment:9 by Jannis Leidel, 12 years ago

Can you explain the changes made to django/db/models/fields/__init__.py line 1013?
Seems like you drop the verbose_name and name parameters of the GenericIPAddressField field, which it really shouldn't.

by Dan McGee, 12 years ago

added tests with ipv6 address persistence

comment:10 by Sasha Romijn, 11 years ago

Keywords: sprint2013 added
Owner: changed from nobody to Sasha Romijn
Status: newassigned

comment:11 by Sasha Romijn, 11 years ago

I have a made the https://github.com/django/django/pull/819 pull request which updates this field to GenericIPAddressField. I have carefully tested the change proposed here in SQLite, Oracle, MySQL and PostgreSQL. It requires a schema change, added to the 1.6 release notes, but without the schema change behaviour is unaffected so it does not break backwards compatibility.

This is the new and old behaviour when trying to save a comment submitted from an IPv6 address:

Database Current type New type Current situation With patch, without schema change With patch and schema change
SQLite TEXT TEXT Stored correctly, although field type is misleading Stored correctly Stored correctly
PostgreSQL INET INET Stored correctly, although field type is misleading Stored correctly Stored correctly
MySQL VARCHAR(15) VARCHAR(39) Silent truncation Silent truncation Stored correctly
Oracle VARCHAR2(15) VARCHAR2(39) Query fails Query fails Stored correctly

In other words, when applying the patch without the schema change, commenting from IPv6 can break, but no worse than it does now. Note that running the new regressiontests.comment_tests.tests.comment_view_tests.CommentViewTests.testCreateValidCommentIPv6 without applying the model field change will only trigger a failure on MySQL and Oracle.

comment:12 by Sasha Romijn, 11 years ago

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:13 by Erik Romijn <eromijn@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In ade992c61e15fbc83d87bfc688c0f844b6ef19fd:

Fixed #16302 -- Ensure contrib.comments is IPv6 capable

Changed the ip_address field for Comment to GenericIPAddressField. Added
instructions to the release notes on how to update the schema of existing
databases.

comment:14 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

In 7106a1e594dc28108e7e4f83af3ffb25289d3bb0:

Merge pull request #819 from erikr/master

Fixed #16302 -- Ensured contrib.comments is IPv6 capable.

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