Re: Add support for IDNA 2008

[Carlton Gibson]

Hi Julien. 

I didn't get a canonical answer from the security team yet, but it may be 
that we can make the idna an optional dependency quite easily. I already 
have it installed in my dev environment, for instance, coming from selenium 
and requests. 

>From the package docs: https://pypi.org/project/idna/

   You may use the codec encoding and decoding methods using 
the idna.codec module:
   >>> import idna.codec 
   >>> print('домен.испытание'.encode('idna')) 
   b'xn--d1acufc.xn--80akhbyknj4f'

So "use if installed" (catching the ImportError if not) would look 
feasible. (The usage in the punycode helper is just `domain.encode("idna")` 
which matches this example already.)

Would you fancy looking a PR around that? 

We'd need *some* tests for both the installed and not-installed cases, 
ideally showing the difference. I didn't immediately have success with your 
https://fuss.standcore.com/ example: 

% python
Python 3.10.6 (v3.10.6:9c7b4bd164, Aug  1 2022, 17:13:48) [Clang 13.0.0 
(clang-1300.0.29.30)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> print('https://fuß.standcore.com/'.encode('idna'))
b'https://fuss.standcore.com/'
>>> import idna.codec
>>> print('https://fuß.standcore.com/'.encode('idna'))
b'https://fuss.standcore.com/'  # Was expecting 
https://xn--fu-hia.standcore.com/ from discussion 🤔
>>> import idna
>>> idna.encode('https://fuß.standcore.com/')
Traceback (most recent call last):
  File "", line 1, in 
  File 
"/Users/carlton/Envs/django/lib/python3.10/site-packages/idna/core.py", 
line 357, in encode
s = alabel(label)
  File 
"/Users/carlton/Envs/django/lib/python3.10/site-packages/idna/core.py", 
line 269, in alabel
check_label(label)
  File 
"/Users/carlton/Envs/django/lib/python3.10/site-packages/idna/core.py", 
line 250, in check_label
raise InvalidCodepoint('Codepoint {} at position {} of {} not 
allowed'.format(_unot(cp_value), pos+1, repr(label)))
idna.core.InvalidCodepoint: Codepoint U+003A at position 6 of 
'https://fuß' not allowed

Possibly there's some objection to such a change, but I'm struggling to 
imagine it short of concrete cases... 

Thanks! 

Kind Regards,

Carlton



On Wednesday, 7 September 2022 at 08:18:13 UTC+2 Carlton Gibson wrote:

> Hey Julien. 
>
> Thanks, OK... 📖
>
> The Python docs have it 
> 
> : 
>
> > If you need the IDNA 2008 standard from *RFC 5891* 
>  and *RFC 5895* 
> , use the third-party idna 
> module .
>
> So the question is do we **need** the newer standard?
>
> I will have a read of the various resources here, and I'll also ask the 
> Django Security Team if they have any thoughts. 
>
> Kind Regards,
>
> Carlton
>
>
> On Tue, 6 Sept 2022 at 23:03, 'Julien Bernard' via Django developers 
> (Contributions to Django itself)  wrote:
>
>> Hi Carlton,
>>
>> IDNA 2008 made some changes in the valid or invalid IDNs and some 
>> differences in the ways some characters are transformed in Punycode 
>> compared to IDNA 2003 for multiple reasons.
>> A difference that is often used as an example is the german 'ß' 
>> character. In IDNA 2003 it is transformed into 'ss' while it is converted 
>> into Punycode in IDNA 2008.
>> It means that, depending on the standard that is implemented, you may 
>> reach totally different domains with the same IDN, which may lead to 
>> security issues.
>> For example, the URL https://fuß.standcore.com/ 
>>  would be https://fuss.standcore.com/ with 
>> IDNA 2003 and https://xn--fu-hia.standcore.com/ with IDNA 2008.
>> This is only a very brief insight, for further quick readings, 
>> https://www.unicode.org/faq/idn.html is quite informative too.
>>
>> Best regards,
>> Julien
>>
>> Le mardi 6 septembre 2022 à 14:39:49 UTC-4, carlton...@gmail.com a 
>> écrit :
>>
>>> Hey Julian. 
>>>
>>> What's maybe missing is some concrete cases. "This conversion should be 
>>> made IDNA 2008 compliant." — OK, but what does that buy us? 
>>>
>>> Maybe the idna package is OK... It's widely depended on already — I got 
>>> it for free yesterday installing httpx in a project — and packaging isn't 
>>> what it was... — but **if** we take on an extra dependency it needs to be 
>>> for a clear gain. 
>>>
>>> Likely (still) a proof-of-concept at least showing what's added (as a 
>>> separate package? 🤔)  is the easiest way forward? 
>>>
>>> Others may yet agree that this is something needed. 
>>>
>>> Kind Regards,
>>>
>>> Carlton
>>>
>>> On Tue, 6 Sept 2022 at 20:23, 'Julien Bernard' via Django developers 
>>> (Contributions to Django itself)  wrote:
>>>
 Thanks Carlton.

 This makes total sense to keep things simple and avoid bringing another 
 dependency in the context of validatio

Re: UniqueConstraint validation error message conditional vs non-conditional

[Amol Rashinkar]

Then what I do..?

On Mon, 12 Sep, 2022, 8:36 pm David Sanders, 
wrote:

> Hi Simon,
>
> > This should already work for constraints over a single field but not on
> the ones with multiple fields[0] which is covered by the suite[1] but it
> doesn't look like UniqueConstraint.validate is providing code="unique"
> which might be the source of the issue you are encountering?
>
> Yep that's correct. The code="unique" is only ever set from
> Model.unique_error_message()
> 
> which, as you're already aware, is only called if there's no condition in
> UniqueConstraint.validate()
> .
> Updating one of the tests confirms this (see below).
>
> Given that we'd like to avoid wording complications with conditions… looks
> like we could simply add code="unique" in UniqueConstraint.validate() as
> per:
>
> index 49c7c91de9..61ca15c7c4 100644
> --- a/django/db/models/constraints.py
> +++ b/django/db/models/constraints.py
> @@ -364,6 +364,8 @@ class UniqueConstraint(BaseConstraint):
>  if (self.condition &
> Exists(queryset.filter(self.condition))).check(
>  against, using=using
>  ):
> -raise
> ValidationError(self.get_violation_error_message())
> +raise ValidationError(
> +message=self.get_violation_error_message(),
> code="unique"
> +)
>  except FieldError:
>  pass
>
> Which then fixes the patched test:
>
> index d4054dfd77..d433555f9f 100644
> --- a/tests/constraints/tests.py
> +++ b/tests/constraints/tests.py
> @@ -569,8 +569,9 @@ class UniqueConstraintTests(TestCase):
>  name=obj1.name, color="blue"
>  ).validate_constraints()
>  msg = "Constraint “name_without_color_uniq” is violated."
> -with self.assertRaisesMessage(ValidationError, msg):
> +with self.assertRaisesMessage(ValidationError, msg) as cm:
>  UniqueConstraintConditionProduct(name=obj2.name
> ).validate_constraints()
> +self.assertEqual(cm.exception.message_dict, {"name": [msg]})
>
>  def test_validate(self):
>  constraint = UniqueConstraintProduct._meta.constraints[0]
>
> (pasting test failure)
>
> ==
> FAIL: test_model_validation_with_condition
> (constraints.tests.UniqueConstraintTests)
> Partial unique constraints are not ignored by
> --
> Traceback (most recent call last):
> ...
> AssertionError: {'__all__': ['Constraint “name_without_color_uniq” is
> violated.']} != {'name': ['Constraint “name_without_color_uniq” is
> violated.']}
> - {'__all__': ['Constraint “name_without_color_uniq” is violated.']}
> ?   ^^ 
>
> + {'name': ['Constraint “name_without_color_uniq” is violated.']}
> ?   ^ ^^
>
>
> If this is acceptable I could open a ticket / start a PR?
>
> --
> David
>
> On Mon, 12 Sept 2022 at 23:26, charettes  wrote:
>
>> Hello David
>>
>> > Would it be possible to group the message by field in the same way as
>> standard unique?
>>
>> This should already work for constraints over a single field but not on
>> the ones with multiple fields[0] which is covered by the suite[1] but it
>> doesn't look like UniqueConstraint.validate is providing code="unique"
>> which might be the source of the issue you are encountering?
>>
>> [0]
>> https://github.com/django/django/blob/07ebef566f751e172e266165071081c7614e2d33/django/db/models/base.py#L1443-L1447
>> [1] https://djangoci.com/job/django-coverage/HTML_20Coverage_20Report/
>>
>>
>> Le dimanche 11 septembre 2022 à 22:02:35 UTC-4, shang.xia...@gmail.com a
>> écrit :
>>
>>> Hi Simon,
>>>
>>> Cheers for the explanation.
>>>
>>> I'm ok with the error message being the "constraint is violated" generic
>>> message as I agree with what you're saying.
>>>
>>> Would it be possible to group the message by field in the same way as
>>> standard unique?
>>>
>>> ie, would this be an idea?:
>>>
>>> django.core.exceptions.ValidationError: {'test': ['Constraint “unique”
>>> is violated.']}
>>>
>>> This way the customised error message would be preserved.
>>>
>>> This also leads to another question I had about whether it might be an
>>> idea to allow overriding error messages for constraints in forms using the
>>> error_messages dict… but I'll leave that for another post :)
>>>
>>> Regards,
>>> David
>>>
>>> On Mon, 12 Sept 2022 at 11:53, charettes  wrote:
>>>
 Hello David,

 This is expected because Django doesn't have a way to express the
 constraint in words to present to the user when a condition, which could be
 complex, is provided.

 When no conditions are defined the metadata is easy to in

Re: UniqueConstraint validation error message conditional vs non-conditional

[charettes]

Hello David,

The proposed patch and test adjustments make sense to me, please open an 
associated ticket and PR.

Thanks for digging this through and working on a solution!

I think that an argument could be made for the `if e.code == "unique" and 
len(constraint.fields) == 1` branch in `Model.validate_constraints` to be 
entirely encapsulated in `UniqueConstraint.validate` (which knows about 
len(self.fields)) but focusing on making sure the proper code is attributed 
to the ValidationError instance is definitely an improvement.

Cheers,
Simon

Le mardi 13 septembre 2022 à 09:40:59 UTC-4, amolras...@gmail.com a écrit :

> Then what I do..? 
>
> On Mon, 12 Sep, 2022, 8:36 pm David Sanders,  
> wrote:
>
>> Hi Simon,
>>
>> > This should already work for constraints over a single field but not on 
>> the ones with multiple fields[0] which is covered by the suite[1] but it 
>> doesn't look like UniqueConstraint.validate is providing code="unique" 
>> which might be the source of the issue you are encountering?
>>
>> Yep that's correct. The code="unique" is only ever set from 
>> Model.unique_error_message() 
>> 
>>  
>> which, as you're already aware, is only called if there's no condition in 
>> UniqueConstraint.validate() 
>> .
>>  
>> Updating one of the tests confirms this (see below).
>>
>> Given that we'd like to avoid wording complications with conditions… 
>> looks like we could simply add code="unique" in UniqueConstraint.validate() 
>> as per:
>>
>> index 49c7c91de9..61ca15c7c4 100644
>> --- a/django/db/models/constraints.py
>> +++ b/django/db/models/constraints.py
>> @@ -364,6 +364,8 @@ class UniqueConstraint(BaseConstraint):
>>  if (self.condition & 
>> Exists(queryset.filter(self.condition))).check(
>>  against, using=using
>>  ):
>> -raise 
>> ValidationError(self.get_violation_error_message())
>> +raise ValidationError(
>> +message=self.get_violation_error_message(), 
>> code="unique"
>> +)
>>  except FieldError:
>>  pass
>>
>> Which then fixes the patched test:
>>
>> index d4054dfd77..d433555f9f 100644
>> --- a/tests/constraints/tests.py
>> +++ b/tests/constraints/tests.py
>> @@ -569,8 +569,9 @@ class UniqueConstraintTests(TestCase):
>>  name=obj1.name, color="blue"
>>  ).validate_constraints()
>>  msg = "Constraint “name_without_color_uniq” is violated."
>> -with self.assertRaisesMessage(ValidationError, msg):
>> +with self.assertRaisesMessage(ValidationError, msg) as cm:
>>  UniqueConstraintConditionProduct(name=obj2.name
>> ).validate_constraints()
>> +self.assertEqual(cm.exception.message_dict, {"name": [msg]})
>>
>>  def test_validate(self):
>>  constraint = UniqueConstraintProduct._meta.constraints[0]
>>
>> (pasting test failure)
>>
>> ==
>> FAIL: test_model_validation_with_condition 
>> (constraints.tests.UniqueConstraintTests)
>> Partial unique constraints are not ignored by
>> --
>> Traceback (most recent call last):
>> ...
>> AssertionError: {'__all__': ['Constraint “name_without_color_uniq” is 
>> violated.']} != {'name': ['Constraint “name_without_color_uniq” is 
>> violated.']}
>> - {'__all__': ['Constraint “name_without_color_uniq” is violated.']}
>> ?   ^^ 
>>
>> + {'name': ['Constraint “name_without_color_uniq” is violated.']}
>> ?   ^ ^^
>>
>>
>> If this is acceptable I could open a ticket / start a PR?
>>
>> --
>> David
>>
>> On Mon, 12 Sept 2022 at 23:26, charettes  wrote:
>>
>>> Hello David
>>>
>>> > Would it be possible to group the message by field in the same way as 
>>> standard unique?
>>>
>>> This should already work for constraints over a single field but not on 
>>> the ones with multiple fields[0] which is covered by the suite[1] but it 
>>> doesn't look like UniqueConstraint.validate is providing code="unique" 
>>> which might be the source of the issue you are encountering?
>>>
>>> [0] 
>>> https://github.com/django/django/blob/07ebef566f751e172e266165071081c7614e2d33/django/db/models/base.py#L1443-L1447
>>> [1] https://djangoci.com/job/django-coverage/HTML_20Coverage_20Report/
>>>
>>>
>>> Le dimanche 11 septembre 2022 à 22:02:35 UTC-4, shang.xia...@gmail.com 
>>> a écrit :
>>>
 Hi Simon,

 Cheers for the explanation.

 I'm ok with the error message being the "constraint is violated" 
 generic message as I agree with what you're saying.

 Would it be possible to group the message by field in the same way as 
 standard unique?
>