models: Validate Project.linkname does not contain forward slash

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@laumann.xyz>
---

I hard a hard time satisfying flake8, so I ended up disabling the pre-commit
hook. I also looked over the documentation for contributors and figure that if
a release note is required, just let me know then I'll add it.

 .../0044_add_project_linkname_validation.py   | 19 +++++++++++++++++++
 patchwork/models.py                           | 10 +++++++++-
 2 files changed, 28 insertions(+), 1 deletion(-)
 create mode 100644 patchwork/migrations/0044_add_project_linkname_validation.py

On 6/9/20 10:55 pm, 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@laumann.xyz>
> ---
> 
> I hard a hard time satisfying flake8, so I ended up disabling the pre-commit
> hook. I also looked over the documentation for contributors and figure that if
> a release note is required, just let me know then I'll add it.

TIL we have precommit hooks. I've been contributing to patchwork for a 
few years now...

In any case, it also causes the test suite to fail its flake8 check. 
Perhaps we need to exclude the migrations directory?

> 
>   .../0044_add_project_linkname_validation.py   | 19 +++++++++++++++++++
>   patchwork/models.py                           | 10 +++++++++-
>   2 files changed, 28 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..2fff7df
> --- /dev/null
> +++ b/patchwork/migrations/0044_add_project_linkname_validation.py
> @@ -0,0 +1,19 @@
> +# 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..9027219 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 re.fullmatch(r'[^/]+', linkname) is None:

This could just be "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)
>

> > I hard a hard time satisfying flake8, so I ended up disabling the pre-commit
> > hook. I also looked over the documentation for contributors and figure that if
> > a release note is required, just let me know then I'll add it.
> 
> TIL we have precommit hooks. I've been contributing to patchwork for a few
> years now...
> 
> In any case, it also causes the test suite to fail its flake8 check. Perhaps
> we need to exclude the migrations directory?

I can also just bite the bullet and get precommit working, but it does seem
aggressive to fail tests on auto-generated files.

> > diff --git a/patchwork/models.py b/patchwork/models.py
> > index 77ab924..9027219 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 re.fullmatch(r'[^/]+', linkname) is None:
> 
> This could just be "if '/' in linkname:"

Sure, that's way simpler, I'll submit another patch. Thanks for the feedback!

-- Thomas

On Tue, 2020-09-08 at 21:31 +1000, Andrew Donnellan wrote:
> On 6/9/20 10:55 pm, 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@laumann.xyz>
> > ---
> > 
> > I hard a hard time satisfying flake8, so I ended up disabling the pre-commit
> > hook. I also looked over the documentation for contributors and figure that if
> > a release note is required, just let me know then I'll add it.
> 
> TIL we have precommit hooks. I've been contributing to patchwork for a 
> few years now...
> 
> In any case, it also causes the test suite to fail its flake8 check. 
> Perhaps we need to exclude the migrations directory?

Running 'pre-commit run -a' before you commit will do the trick also.
The 'patchwork.migrations directory was previously excluded, but we re-
added it for validation recently enough as quite a few of the
migrations in there were hand-written and could do with the validation.

Stephen

> >   .../0044_add_project_linkname_validation.py   | 19 +++++++++++++++++++
> >   patchwork/models.py                           | 10 +++++++++-
> >   2 files changed, 28 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..2fff7df
> > --- /dev/null
> > +++ b/patchwork/migrations/0044_add_project_linkname_validation.py
> > @@ -0,0 +1,19 @@
> > +# 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..9027219 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 re.fullmatch(r'[^/]+', linkname) is None:
> 
> This could just be "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)
> >