Implement frontend for screencast uploading

VERIFIED FIXED in 1.1

Status

VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: laura, Assigned: ecooper)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: sumo_only, URL)

Attachments

(3 attachments)

(Reporter)

Description

10 years ago
Use similar UI as the screenshot uploader, i.e. after uploading show a thumbnail that can be clicked on to insert the code snippet into the editor. 

We'd like to use jQuery for the upload progress bar.
(Reporter)

Updated

10 years ago
Depends on: 489801
(Assignee)

Updated

10 years ago
Depends on: 492697
(Assignee)

Updated

10 years ago
Depends on: 493895
(Assignee)

Updated

10 years ago
Depends on: 493946
(Assignee)

Updated

10 years ago
Depends on: 493953
(Assignee)

Comment 1

10 years ago
Created attachment 378600 [details] [diff] [review]
v1

This adds the screencasts to tiki-editpage.php and its templates. It also sets up a file (tiki-upload_screencasts_ajax.php) for ajax uploads.
Attachment #378600 - Flags: review?(laura)
(Assignee)

Updated

10 years ago
Attachment #378600 - Flags: review?(paul.craciunoiu)
Doing a drive-by review makes me worry about several security-related things regarding file uploading with how the patch does it currently. Alex just recently implemented a similar thing for a new marketing project, so maybe he can assist here with thoughts, ideas, and useful hints regarding best practices for handling file uploads and validating them.
Howdy, 

I can help review the patch, but I probably won't get to it for a couple days.

More generally, here are a few things to keep in mind regarding security when doing uploads...

* set up the web server to prevent any scripts from executing in the uploads directory.  For Apache, this is something like "SetHandler None"

* try to detect file types, but do NOT trust what the browser sends you, or the file extension.  PHP's Fileinfo package can be useful here

* Make sure the destination file name is clean, i.e. no directory traversal

* IT can run a virus scanner on the uploads dir

* It's good to randomize file names, so a malicious user doesn't know what the uploaded file is called on our servers.  In this case, it sounds like that might not be valid, because we'll be showing the user the uploaded file anyway

Outside of security...

* There are a bunch of PHP settings you'll need to check for uploading large files.  I can explain in more detail if you want, just ask.  A lot of these are explained here, http://us3.php.net/manual/en/features.file-upload.common-pitfalls.php

Reed, can you post that link you gave me here?  That was a pretty good overview of upload techniques
(In reply to comment #3)
> Reed, can you post that link you gave me here?  That was a pretty good overview
> of upload techniques

Sure, https://cert.belnet.be/content/web-server-security-best-practices#4

Also, keep this in mind:

"A common misconception among Apache administrators is that Apache will only process files ending in .php. This is not the complete truth. Apache will process files that contain .php, they don't necessarily have to end in .php. As a consequence backup or test files like index.php.bak or index.php.test are still processed. This means that the source code will not be disclosed but it can also only your users to upload their own scripts, for example myexploit.php.txt.

Your upload validation script should not only check if files end on .php, it should also check if the uploaded filename contains .php."
(Reporter)

Comment 5

10 years ago
(In reply to comment #4)
> 
> Your upload validation script should not only check if files end on .php, it
> should also check if the uploaded filename contains .php."

That particular issue doesn't apply here because we are renaming the file on upload (to an md5 hash).  So unless Apache is set up to try to execute everything as PHP then it's safe.
(Reporter)

Comment 6

10 years ago
In addition:
- We already do image uploads using a similar method, and when Tiki was first installed we had some issues as you know.  We have since altered the Apache config/PHP config to avoid those issues.  Video files are not different in this respect.
- all of these get approved by a human editor as part of the general SUMO process before they get unleashed on the general public, so that mitigates a lot of risk in terms of the end user.

We could try to do more detection on the file type.  This is always fraught, and I don't believe PHP's Fileinfo knows anything about ogg.
I tried it on OGG yesterday, and it seemed to work.  However, you already have a lot of review points, so I'm not sure file type validation is even worth it.
(Reporter)

Comment 8

10 years ago
Comment on attachment 378600 [details] [diff] [review]
v1

Once this is committed, let's get Reed to hammer on it all with Sentinel.
Attachment #378600 - Flags: review?(laura) → review+
(Assignee)

Updated

10 years ago
Attachment #378600 - Flags: review?(paul.craciunoiu) → review-
(Assignee)

Comment 9

10 years ago
Comment on attachment 378600 [details] [diff] [review]
v1

r26336/r26337

We won't be able to test this until after the remaining parts of the project are committed, which should be done shortly.
(Assignee)

Updated

10 years ago
Attachment #378600 - Flags: review-
(Assignee)

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 10

10 years ago
For the moment, I'm reopening this with what appears to be a webdav connection issue. I'll investigate tonight and this morning.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to comment #10)
> For the moment, I'm reopening this with what appears to be a webdav connection
> issue. I'll investigate tonight and this morning.

Any luck?
(Reporter)

Comment 12

10 years ago
Do you need help from IT to debug your network issues?
(Assignee)

Comment 13

10 years ago
I think code needs tweaked most likely. I'm troubleshooting now and should know what the deal is very soon. 

After my initial testing, I didn't have any time to dig in before being sent into the air. So, network issues was a preliminary hypothesis.
(Assignee)

Comment 14

10 years ago
(In reply to comment #13)
> I think code needs tweaked most likely. I'm troubleshooting now and should know
> what the deal is very soon. 
> 
> After my initial testing, I didn't have any time to dig in before being sent
> into the air. So, network issues was a preliminary hypothesis.

Some code tweaks need to be made, but this appears to be working properly (without them). Please, please hold off on testing on stage for a little bit until I change the config to use a staging directory on the webdav share.

I'll make a another note in the bug when it's ready to be played with.
(Assignee)

Comment 15

10 years ago
(In reply to comment #14)
> (In reply to comment #13)
> > I think code needs tweaked most likely. I'm troubleshooting now and should know
> > what the deal is very soon. 
> > 
> > After my initial testing, I didn't have any time to dig in before being sent
> > into the air. So, network issues was a preliminary hypothesis.
> 
> Some code tweaks need to be made, but this appears to be working properly
> (without them). Please, please hold off on testing on stage for a little bit
> until I change the config to use a staging directory on the webdav share.
> 
> I'll make a another note in the bug when it's ready to be played with.

This can be tested on staging now. A tweaks patch is coming to adjust some minor stuff shortly. Like I said, this can be tested in the meantime without it.

Current example of a screencast on a page: https://support-stage.mozilla.org/en-US/kb/*How+to+clear+Search+bar+history?bl=n
Nice!
(Assignee)

Comment 17

10 years ago
Created attachment 380499 [details] [diff] [review]
1.1.1

Some minor changes regarding perms (using the new ones) and making sure empty file names aren't returned.
Attachment #380499 - Flags: review?(paul.craciunoiu)
Attachment #380499 - Flags: review?(laura)
(Reporter)

Updated

10 years ago
Attachment #380499 - Flags: review?(laura) → review+
(Reporter)

Comment 18

10 years ago
Comment on attachment 380499 [details] [diff] [review]
1.1.1

There is no 1.1.1 :)  (or spoon).  So commit ASAP.
(Assignee)

Comment 19

10 years ago
r26597/r26598
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
In the source of the test page, I now see:

<p>Screencast ID wasn't specified or doesn't exist

I can't see the screencast on https://support-stage.mozilla.org/en-US/kb/*How+to+clear+Search+bar+history?bl=n any longer, using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090529 Shiretoko/3.5pre.

WebDAV issue?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 21

10 years ago
Hmm...Potentially a network hiccup, but I don't think so because the webdav share is polled only when something changes. Otherwise, the information on whether or not a video exists for a page is cached.

I edited the page, but didn't change anything (versions 19 and 20 are identical according to tiki) and now the video launcher is present again. Tres bizarre.

I guess that best thing we can do is keep playing with it to see if it reproduces itself.
(Assignee)

Updated

10 years ago
Attachment #380499 - Flags: review?(paul.craciunoiu)

Comment 23

10 years ago
in reply to comment #22 when I follow your link and login it works fine for me. Running as normal.
(Assignee)

Comment 24

10 years ago
Created attachment 380737 [details] [diff] [review]
Fix caching inconsistency 

So, it seems that the were two problems at play, both of which are fixed with this patch.

Stephen, until this patch is up, if a screencast says 'it doesn't exist' when you know it does, click 'edit page' and it will show up again.
Attachment #380737 - Flags: review?(paul.craciunoiu)
Attachment #380737 - Flags: review?(laura)
(Assignee)

Comment 25

10 years ago
(In reply to comment #23)
> in reply to comment #22 when I follow your link and login it works fine for me.
> Running as normal.

Just to verify comment 22, there is a caching and boolean check issue at play but a temporary (as in until someone else gets eyes on the patch) workaround is posted in comment 24.
(Reporter)

Updated

10 years ago
Attachment #380737 - Flags: review?(laura) → review+
(Assignee)

Updated

10 years ago
Attachment #380737 - Flags: review?(paul.craciunoiu)
(Assignee)

Comment 26

10 years ago
Comment on attachment 380737 [details] [diff] [review]
Fix caching inconsistency 

r26769/r26770

Intermittent failures shouldn't happen anymore.
(Assignee)

Comment 27

10 years ago
Another screencast has been successfully uploaded and added to https://support-stage.mozilla.org/en-US/kb/*How+to+customize+the+toolbar
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
Verified FIXED - frontend is indeed implemented, and we have been filing bugs and marking them as blocking bug 489803.
Status: RESOLVED → VERIFIED
Whiteboard: sumo_only
You need to log in before you can comment on or make changes to this bug.