P5.js-web-editor: Add character limit to sketch name - Back end validation

[shinytang6]

Nature of issue?

• [x] Found a bug
• [ ] Existing feature enhancement
• [ ] New feature request

Details about the bug:

name a project with special characters and try to donwload.
some of them will cause problem, e.g. &,|...

[catarak]

this is two things, right? 1. sketch names need a length limit (i don't know what an appropriate character limit is) and 2. sketch names need to be... url encoded? escaped somehow? not sure.

[himanshuc3]

@shinytang6 Can you provide an example. I tried a couple of them including &, | and when downloading the sketch, & is named and, | - or .

[shinytang6]

@himanshuc3 What you listed is the problem:)

[himanshuc3]

But what's the problem if the name of the file changes. It downloaded without any errors.

[shinytang6]

it doesn't matter with the content, should download the project in what you name.

[GaurangTandon]

@catarak Is anyone working on this issue? If not, then I'd like to take it up :) I plan to follow the advice given on this post for generating file names.

I plan to either:

1. generate file names based on the platform the user is using when they download the sketch (so same sketch can have different file names when downloaded on Windows/Linux) (also fall back to safer Windows naming when OS is not Windows/Mac/Linux)
2. generate safe cross platform file names (i.e. like Windows) every time for consistency's sake.

Would you suggest anything else? I prefer the second option. I can also display a toast message every time a restricted character is trimmed out from a file name, so user would not be immediately surprised.

[catarak]

i suggest the latter. i don't think it makes sense to change the sketch titles as they are in the editor, other than giving them a length limit; rather, i think the sketch titles should be escaped only when they are downloaded and translated to the name of a zip file, as this is the only time the sketch title affects this.

[GaurangTandon]

So I was going through the codebase and noticed that the relevant method is this:

https://github.com/processing/p5.js-web-editor/blob/244af16b64cfa37f1fac0f54750e9a7c314269a3/client/modules/IDE/actions/project.js#L181-L184

Note that it just opens a new window pointing to a resource that is probably already in the server. Where can I look at to edit the zip files file name?

[catarak]

The zip file is generated on the fly, so when a browser window requests that url, it generates the zip file. The entry point, server-side, is here: https://github.com/processing/p5.js-web-editor/blob/master/server/controllers/project.controller.js#L332

[GaurangTandon]

So, the problem here is that the first part of the file name (before the timestamp) is generated using slugify, which is too restrictive in what it allows. I noticed that even Chinese characters, which are perfectly valid in file names, are completely removed. Try 文字 as the sketch name for example.

I'll send in a PR.

[catarak]

there are TWO issues here, not just one:

• [ ] sketch names need a maximum length
• [x] file names need to work with Mac/Windows/Linux/etc file systems.

[GaurangTandon]

Ok, I can work on the first one too. Two things:

1. What limit should be assigned? I would think that the least integer value that does not invalidate 99% of the existing sketch names should be fine. (1% for those occasional unreasonably long sketch names)
2. I can't find a place under the server/ directory where the limit should be checked. Searching for project.name brings up files related to server/migration and other unrelated places. Where exactly to add this length validation?

[catarak]

1. I looked at some other online code editors for limit lengths:
   
   
   
   • CodePen: no length limit
   
   
   • glitch.com: 51 characters
   
   
   • jsfiddle: 256 characters
   
   
   • Codesandbox: no length limit
   
   

256 seems like it's a good starting point? maybe smaller than that makes sense?

1. I think this check should be done both client and server side—on the server this would be done in models/project.js, and client side on the project input form.

[GaurangTandon]

On an average use case, I think names would be like Lab 25th January 2019 - Demonstrating recursive functions with colors that's about 60-70 characters. Thus, at max, even 128 chars seems more than enough for most cases. However, before setting out to impose a limit, I would first want to make sure what percentage of existing sketches would be impacted by this length limiting, and minimize the damage thereof.

[catarak]

let me do a query on the db...

UPDATE: there are 33 sketches with a name length greater than 128, and 15 with a name length greater than 256 (of 341,000 sketches). So, a very smaller number would be affected.

UPDATE2: because of these outlier name lengths, i can't create an index on the sketch names. so, that's another big reason to shorten the names.

[GaurangTandon]

Perfect, I'll try and get the length limit of 256 added into the code!

[catarak]

the OS issue was fixed, but adding a project name limit would also be good to add.

[siddhant1]

Will send a pr to close this

[shakeabi]

@siddhant1 I don't think a front-end check would cut it. You must also include a backend validation.

[josephmwells]

I'm going to try and tackle this.

Quick question, can we just use mongoose validators such as minlength and maxlength found here ? These will throw an error which can be caught and sent back to the client. (I haven't checked to see how validation errors are currently being handled.)

Also, by 'sketch name' are we referring to the project name or the file names, or both ?

[catarak]

@josephmwells using the validators makes sense to me! Also for this issue, it is referring to specifically the sketch title. It would be a separate issue/PR for the sketch files.