[WMDE-Fundraising] Validate field length
Closed, ResolvedPublic3 Story Points

Description

The old application truncates string values that are stored in the data blob to a maximum of 2048 characters: https://github.com/wmde/fundraising/blob/master/inc/spende.class.php#L601-L609

The new application will do something smarter: validate the maximum length of all form fields and reject fields that are too long.

The validation must be done for donations, memberships and comments.

Restricted Application added subscribers: Zppix, Aklapper. · View Herald TranscriptMay 31 2016, 10:19 AM

@WMDE-Fisch, @kai.nissen, @WMDE-leszek Any idea why this truncation was/is necessary?

To avoid "insanely long fields" of course!

It looks like a sanity check to prevent people from pasting in a whole book as a name etc. Also note that no matter what the method name might suggest, I believe the old code (including truncation) you linked to is not only about the stuff in the 'data' field but it truncates all other things as well. It might be needed to be done in the new code for all of other fields as well if the new app should behave like the old one in all the aspects (I haven't checked if the new code already prevents too long fields values for name, address, whatnot - if it does, please ignore it).

That thing were some extra "validation/normalization of the user input" step, I'd say this does not belong to FundraisingStore.

I think the FundraisingStore shouldn't be concerned about what data it stores. We would also never be able to store data that exceeds the limit if we ever want to.

Except for the comment field that limit is already "insanely long". Whatever input exceeds that limit (and actually a lower limit like 128 bytes) is likely to be something that is "deleted" by the fundraising team anyway. Assuming that, I don't think we should store that data at all, but care about this during validation (which would be yet another suggestion). An easy approach would be to limit the content of an input field using HTML attributes. (But as easy as this is to circumvent the limit by manipulating the DOM.)

gabriel-wmde renamed this task from [WMDE-Fundraising] Truncate long fields in `data` to [WMDE-Fundraising] Validate field length.Jun 28 2016, 12:10 PM
gabriel-wmde updated the task description. (Show Details)
gabriel-wmde changed the point value for this task from 2 to 3.

While it seems weird to limit things to 2048 characters in the persistence, I could imagine there being some limit as a safety net. Much like you escape a numeric string, even if you are already "sure" it can only be numeric at that point. Not saying we should do that, just that it does not seem inherently wrong to me/

Question for @JeroenDeDauw and @kai.nissen: How should the field lengths of tracking info (both donation and membership) be limited? Validating it in the donation/membership validator would be a violation of the SRP (validator only validates data related to the domain). At the moment I can imagine:

  1. Just throw an exception in the TrackingInfo constructor if it gets values that are too big. Since the tracking info is not strictly "user input" (but can be manipulated by users through the URL) I think the general "An error occured" page is fine.
  2. Adding a TrackingValidator class to the use cases.

I'd prefer the first option, what about you? Any other ideas on how do achieve this?

If we have validators that deal with the request, then it's clear they should also handle the tracking info, as that is part of the request.

I'm fine with the domain object checking the length. That'd be good to do in any case. The question is if we'd want to also do it with some validators, to have nicer error reporting, where I agree it is not really needed.

Current status:

  • The lengths of the input fields are validated, see this PR: https://github.com/wmde/FundraisingFrontend/pull/557
  • I'm working on a PR that validates length (or rather the upper bound ) of the Cookie-based impression counters and removes the tracking of "color", "skin" and "layout" (which are already unused in the old application).
gabriel-wmde closed this task as Resolved.Apr 7 2017, 8:15 AM
Restricted Application added a project: WMDE-Fun-Team. · View Herald TranscriptApr 7 2017, 8:15 AM