Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check filename has extension. Make more obvious. #801

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

listerr
Copy link
Contributor

@listerr listerr commented Apr 20, 2022

A follow up to: #686

Users still seem to be uploading files without extensions, by typing something in the "Name" field, not realising this is used as the actual file name.

Possibly because this is the first field, and it's before the file upload button.
If Name is not completed, the filename is completed automatically.

File then won't open when downloaded because it has no extension.

  • This fix changes "Name" to "File Name" to make it more obvious.
  • Validate the file name is "(something).extn"
  • Move upload file button before File Name so the user does this bit first. (Then will see the name is filled in for them.)

Can't see a way to make the help text visible by default (it's hidden unless the user clicks the help button.)

In addition to the above, I have:

  • ensured all relevant template output is escaped to avoid XSS attached with <?= $t->ee( $data ) ?> or equivalent.
  • ensured appropriate checks against user privilege / resources accessed
  • API calls (particular for add/edit/delete/toggle) are not implemented with GET and use CSRF tokens to avoid CSRF attacks

A follow up to: inex#686

Users still seem to be uploading files without extensions, by typing something in the "Name" field, not realising this is used as the actual file name.

Possibly because this is the first field, and it's before the file upload button. If name is not completed, the filename is completed automatically.

File then won't open when downloaded because it has no extension.

- This fix changes "Name" to "File Name" to make it more obvious.
- Validate the file name is "(something).extn"
- Move upload file button before File Name so the user does this bit first. (Then will see the name is filled in for them.)

Can't see a way to make the help text visible by default (it's hidden unless the user clicks the help button.)
@barryo
Copy link
Member

barryo commented Apr 24, 2022

Hey @listerr

Regarding the regex: match:"/^(?:^..*)\.\w{1,5}$/" - is the intention 'at least one char, a dot and an extension of 1-5 characters?

If so, this might be better: match:"/^\w[\w\-\.\(\)]+\.\w+$/" - i.e. make sure it starts with a "normal" character, anything 'sane' after that and followed by a plain extension (there are extensions >5 chars).

To accept, I'd need:

  • above regexp change
  • mirrored for normal (non-customer) docstore files for alignment
  • this checkForm() and this checkForm() updated

@barryo barryo self-requested a review April 24, 2022 11:26
@listerr
Copy link
Contributor Author

listerr commented Apr 25, 2022

Regarding the regex: match:"/^(?:^..*)\.\w{1,5}$/" - is the intention 'at least one char, a dot and an extension of 1-5 characters?

Yes, also wanted to specifically catch .pdf e.g. anything which seems to be just an extension with no filename.
Basically, "fine, as long as it isn't just an extension."

If so, this might be better: match:"/^\w[\w\-\.\(\)]+\.\w+$/" - i.e. make sure it starts with a "normal" character, anything 'sane' after that and followed by a plain extension (there are extensions >5 chars).

This would seem to exclude otherwise legal filenames:

"(Draft) Document.pdf"
"1.pdf"
"a.pdf"
"Signed Document.pdf"

I wish "Signed Document - Draft(2).pdf" or "1.mp4" wasn't a thing, but apparently is!

Maybe tweak it to accept spaces and in just one block?

match:"/^[\w\-\.\(\)\ ]+\.\w+$/"

Couldn't see any extensions > 5 chars in common use here, e.g. at most 5 to allow something like ".xhtml" - maybe you have an example you can think of?

I wanted to set some limit to prevent erroneous filenames like "signed.document"

Possibly a nicer approach might be a config option for the Document Store to specify allowed file extensions for the cust / general document stores. (Otherwise if this is unset, it'll accept everything but with the caveat "weird file names might be allowed")

Seems I stumbled into a bug with Former, after figuring out what Former is, how to add a validation.. then it didn't work!

formers/former#428

Not before hours of trying various things, much headbanging and google-foo!

It's marked as fixed here, but bug still apparent in the Former in IXP Manager.

To accept, I'd need:

  • above regexp change
  • mirrored for normal (non-customer) docstore files for alignment
  • this checkForm() and this checkForm() updated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants