Page MenuHomePhabricator

Newsletter extension should have validation
Closed, ResolvedPublic

Description

Split from https://phabricator.wikimedia.org/T110181

This might refer to the API but this should be checked everywhere

Details

Related Gerrit Patches:
mediawiki/extensions/Newsletter : masterAdd NewsletterValidator and changes to Special:CreateNewsletter
mediawiki/extensions/Newsletter : masterValidating parameter passed to database from API
mediawiki/extensions/Newsletter : masterAdd minimum length for description field.

Event Timeline

Addshore created this task.Aug 27 2015, 7:40 AM
Addshore raised the priority of this task from to Medium.
Addshore updated the task description. (Show Details)
Addshore added a subscriber: Addshore.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 27 2015, 7:40 AM
Qgil lowered the priority of this task from Medium to Low.Aug 28 2015, 11:31 AM
Qgil added a subscriber: Qgil.
Addshore removed a subscriber: Addshore.Sep 7 2015, 10:57 AM
Tinaj1234 raised the priority of this task from Low to High.Nov 12 2015, 7:22 AM
Tinaj1234 set Security to None.
Qgil assigned this task to Tinaj1234.Nov 12 2015, 11:53 AM
Qgil added a comment.Nov 26 2015, 12:33 PM

<tinajohnson> This refers to input validation ^ mainly ?
<Glaisher> yes
<Glaisher> check all the input points and make sure that people are not able to enter anything that doesn't make sense
<tinajohnson> and requires live valiation ?
<Glaisher> what do you mean by live validation?
<tinajohnson> hm, the kind you see in bootstrap form.. you get a tick icon right away after the text is entered
<Glaisher> oh, you mean ajax
<tinajohnson> yup
<Glaisher> No, this is about actually validating real input on the forms
<Glaisher> but I guess we could ajax validation for the forms later
<Glaisher> but that's not a must
<tinajohnson> the kind in https://junior.inctf.in/register/user/
<tinajohnson> okay, noted
<Glaisher> For example, in the the form which lets you add publishers, make you sure you can add only real users
<Glaisher> not an IP or a user that doesn't exist
<tinajohnson> okay
<qgil> I guess we need to document every form in the description of that task, and assue that we are applying the right validation?
<qgil> or types of input
<tinajohnson> right, that would be good
<Glaisher> we could do the [X] check thing
<Glaisher> List all the input points
<tinajohnson> okay, great!
<qgil> In fact...
<qgil> Isn't http://newsletter-test.wmflabs.org/wiki/Special:CreateNewsletter all the input we have?
<qgil> 3 fields
<qgil> ah no
<Glaisher> no, there's some other forms as well
<Glaisher> and the API
<qgil> announce newsletter
<tinajohnson> yeah, add publishers
<Glaisher> You could go through all of them and make sure someone wouldn't be able to add random stuff there
<qgil> but do checkboxes and buttons need validation?
<tinajohnson> just text input boxes, right ?
<Glaisher> I think HTMLForm does validation for checkboxes and dropdowns
<Glaisher> (not sure)
<tinajohnson> checking..

I think it would be better to decide what all kinds of inputs are allowed for the form fields that needs validation. Two pages have forms and thus require validation, Special:ManageNewsletter and Special:CreateNewsletter.

Special:ManageNewsletter

  • Publisher name - Does not take IP addresses or invalid usernames as of now.

Special:CreateNewsletter

  • Name of newsletter - we allow numbers, so right now IP addresses are possible. Is that okay ?
  • Descripton - Should have a minimum length of 20 maybe ? (can prevent one word descriptions)
  • Title of Main page - Only takes those pages which exist. Anything else to be done here ?

And in the announce newsletter section we have,

  • Summary of this issue - Would require the same validation as description of newsletter.
  • Page title of issue - only takes valid existing pages.

Change 266049 had a related patch set uploaded (by Tinaj1234):
Add minimum length for description field

https://gerrit.wikimedia.org/r/266049

Change 266049 merged by jenkins-bot:
Add minimum length for description field.

https://gerrit.wikimedia.org/r/266049

Change 267666 had a related patch set uploaded (by Tinaj1234):
Validating parameter passed to database from API

https://gerrit.wikimedia.org/r/267666

Change 267666 merged by jenkins-bot:
Validating parameter passed to database from API

https://gerrit.wikimedia.org/r/267666

anything more to be done on this patch @Glaisher, @Addshore, @Tinaj1234 ? We still have this open, and blocking deployment !

Anything else needed will be caught by the security review, this can likely be closed.

Tinaj1234 added a comment.EditedFeb 13 2016, 3:45 PM

Anything else needed will be caught by the security review, this can likely be closed.

@Qgil, About time we requested for a security review ?

I'm not happy with the validation on Special:CreateNewsletter yet. I'll try to work on something...

Change 270496 had a related patch set uploaded (by Glaisher):
Add NewsletterValidator and changes to Special:CreateNewsletter

https://gerrit.wikimedia.org/r/270496

Change 270496 merged by jenkins-bot:
Add NewsletterValidator and changes to Special:CreateNewsletter

https://gerrit.wikimedia.org/r/270496

Qgil added a comment.Feb 18 2016, 11:39 AM

fwiw

  • Newsletter name field should probably do more input validation (a single space is allowed?)

@Glaisher, can this one be marked resolved ?

fwiw

  • Newsletter name field should probably do more input validation (a single space is allowed?)

Now shows the error message:

Required input was not entered. Please try again.
Tinaj1234 closed this task as Resolved.Mar 10 2016, 12:11 PM

Closing this now. Tasks can be filed for further validation related bugs.

Qgil awarded a token.Mar 11 2016, 10:01 AM