Closed Bug 489805 Opened 15 years ago Closed 15 years ago

Implement frontend for screencast uploading

Categories

(support.mozilla.org :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: laura, Assigned: ecooper)

References

()

Details

(Whiteboard: sumo_only)

Attachments

(3 files)

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.
Depends on: 489801
Depends on: 492697
Depends on: 493895
Depends on: 493946
Depends on: 493953
Attached patch v1Splinter Review
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)
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."
(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.
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.
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+
Attachment #378600 - Flags: review?(paul.craciunoiu) → review-
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.
Attachment #378600 - Flags: review-
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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?
Do you need help from IT to debug your network issues?
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.
(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.
(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!
Attached patch 1.1.1Splinter Review
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)
Attachment #380499 - Flags: review?(laura) → review+
Comment on attachment 380499 [details] [diff] [review]
1.1.1

There is no 1.1.1 :)  (or spoon).  So commit ASAP.
r26597/r26598
Status: REOPENED → RESOLVED
Closed: 15 years ago15 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 → ---
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.
Attachment #380499 - Flags: review?(paul.craciunoiu)
in reply to comment #22 when I follow your link and login it works fine for me. Running as normal.
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)
(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.
Attachment #380737 - Flags: review?(laura) → review+
Attachment #380737 - Flags: review?(paul.craciunoiu)
Comment on attachment 380737 [details] [diff] [review]
Fix caching inconsistency 

r26769/r26770

Intermittent failures shouldn't happen anymore.
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
Closed: 15 years ago15 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.

Attachment

General

Created:
Updated:
Size: