[PATCH v2] models: Validate Project.linkname does not contain forward slash

[Stephen Finucane]

On Tue, 2020-09-08 at 16:06 +0200, Thomas Bracht Laumann Jespersen wrote:
> I started by creating a project that contained a forward slash
> (importing patches from https://lists.sr.ht/~sircmpwn/sr.ht-dev/) and
> it fails to render the "projects" main page.
> 
> The specific error reads:
> 
>     NoReverseMatch at /
> 
>     Reverse for 'patch-list' with keyword arguments
>     '{'project_id': 'foo/bar'}' not found. 1 pattern(s) tried:
>     ['project/(?P<project_id>[^/]+)/list/$']
> 
> which appears to explicitly disallow forward slashes.
> 
> So I think it makes sense to validate that project linkname doesn't
> contain forward slahes.
> 
> Signed-off-by: Thomas Bracht Laumann Jespersen <t at laumann.xyz>

Thanks! This is one of those things you just assume people are doing so
you never bother enforcing it :) In hindsight, the 'Project.linkname'
field is supposed to be a slug - that is, restricted to letters,
numbers, underscores and hyphens. You'd use 'Project.name' if you
wanted more details. Does it make sense to enforce that, as opposed to
simply excluding slashes? If so, I suspect we should use the built-in
'validate_unicode_slug' validator [1]?

Thoughts/objections?

Stephen

[1] https://docs.djangoproject.com/en/3.1/ref/validators/#validate-unicode-slug

> ---
> 
> I simplified the check for '/' as suggested, and got the pre-commit hook to
> work. I changed the formatting in the migration to satisfy the max line length
> check.
> 
>  .../0044_add_project_linkname_validation.py   | 23 +++++++++++++++++++
>  patchwork/models.py                           | 10 +++++++-
>  2 files changed, 32 insertions(+), 1 deletion(-)
>  create mode 100644 patchwork/migrations/0044_add_project_linkname_validation.py
> 
> diff --git a/patchwork/migrations/0044_add_project_linkname_validation.py b/patchwork/migrations/0044_add_project_linkname_validation.py
> new file mode 100644
> index 0000000..8add430
> --- /dev/null
> +++ b/patchwork/migrations/0044_add_project_linkname_validation.py
> @@ -0,0 +1,23 @@
> +# Generated by Django 3.0.10 on 2020-09-06 22:47
> +
> +from django.db import migrations, models
> +import patchwork.models
> +
> +
> +class Migration(migrations.Migration):
> +
> +    dependencies = [
> +        ('patchwork', '0043_merge_patch_submission'),
> +    ]
> +
> +    operations = [
> +        migrations.AlterField(
> +            model_name='project',
> +            name='linkname',
> +            field=models.CharField(
> +                max_length=255,
> +                unique=True,
> +                validators=[patchwork.models.validate_project_linkname]
> +            ),
> +        ),
> +    ]
> diff --git a/patchwork/models.py b/patchwork/models.py
> index 77ab924..64d5a10 100644
> --- a/patchwork/models.py
> +++ b/patchwork/models.py
> @@ -31,6 +31,13 @@ def validate_regex_compiles(regex_string):
>          raise ValidationError('Invalid regular expression entered!')
>  
>  
> +def validate_project_linkname(linkname):
> +    if '/' in linkname:
> +        raise ValidationError(
> +            'Invalid project linkname: Value cannot contain forward slash (/)'
> +        )
> +
> +
>  class Person(models.Model):
>      # properties
>  
> @@ -56,7 +63,8 @@ class Person(models.Model):
>  class Project(models.Model):
>      # properties
>  
> -    linkname = models.CharField(max_length=255, unique=True)
> +    linkname = models.CharField(max_length=255, unique=True,
> +                                validators=[validate_project_linkname])
>      name = models.CharField(max_length=255, unique=True)
>      listid = models.CharField(max_length=255)
>      listemail = models.CharField(max_length=200)