Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#16302 closed Bug (fixed)

Ensure contrib (namely comments) is IPv6 capable

Reported by: toofishes Owned by: erikr
Component: contrib.comments Version: master
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 toofishes 4 years ago.
notes-update-comments-ip-address.patch (3.7 KB) - added by toofishes 4 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 toofishes 4 years ago.
added tests with ipv6 address persistence

Download all attachments as: .zip

Change History (17)

Changed 4 years ago by toofishes

comment:1 Changed 4 years ago by aaugustin

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 4 years ago by erikr

  • 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 Changed 4 years ago by jezdez

Yeah, definitely one of those dogfood items. +1

comment:4 Changed 4 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

Changed 4 years ago by toofishes

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

comment:5 follow-up: Changed 4 years ago by toofishes

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 Changed 4 years ago by Alex

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

comment:7 in reply to: ↑ 5 Changed 4 years ago by carljm

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 Changed 4 years ago by toofishes

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 Changed 4 years ago by jezdez

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.

Changed 4 years ago by toofishes

added tests with ipv6 address persistence

comment:10 Changed 3 years ago by erikr

  • Keywords sprint2013 added
  • Owner changed from nobody to erikr
  • Status changed from new to assigned

comment:11 Changed 3 years ago by erikr

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 Changed 3 years ago by erikr

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:13 Changed 3 years ago by Erik Romijn <eromijn@…>

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

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 Changed 3 years ago by Aymeric Augustin <aymeric.augustin@…>

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