Closed
Bug 489805
Opened 15 years ago
Closed 15 years ago
Implement frontend for screencast uploading
Categories
(support.mozilla.org :: General, defect)
support.mozilla.org
General
Tracking
(Not tracked)
VERIFIED
FIXED
1.1
People
(Reporter: laura, Assigned: ecooper)
References
()
Details
(Whiteboard: sumo_only)
Attachments
(3 files)
12.41 KB,
patch
|
laura
:
review+
|
Details | Diff | Splinter Review |
4.02 KB,
patch
|
laura
:
review+
|
Details | Diff | Splinter Review |
784 bytes,
patch
|
laura
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•15 years ago
|
||
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•15 years ago
|
Attachment #378600 -
Flags: review?(paul.craciunoiu)
Comment 2•15 years ago
|
||
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.
Comment 3•15 years ago
|
||
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
Comment 4•15 years ago
|
||
(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•15 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•15 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.
Comment 7•15 years ago
|
||
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•15 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•15 years ago
|
Attachment #378600 -
Flags: review?(paul.craciunoiu) → review-
Assignee | ||
Comment 9•15 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•15 years ago
|
Attachment #378600 -
Flags: review-
Assignee | ||
Updated•15 years ago
|
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 10•15 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•15 years ago
|
||
Do you need help from IT to debug your network issues?
Assignee | ||
Comment 13•15 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•15 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•15 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
Comment 16•15 years ago
|
||
Nice!
Assignee | ||
Comment 17•15 years ago
|
||
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•15 years ago
|
Attachment #380499 -
Flags: review?(laura) → review+
Reporter | ||
Comment 18•15 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•15 years ago
|
||
r26597/r26598
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
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•15 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•15 years ago
|
Attachment #380499 -
Flags: review?(paul.craciunoiu)
This broke again -- I can't see the screencast at https://support-stage.mozilla.org/en-US/kb/*How+to+clear+Search+bar+history?bl=n&login.
Comment 23•15 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•15 years ago
|
||
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•15 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•15 years ago
|
Attachment #380737 -
Flags: review?(laura) → review+
Assignee | ||
Updated•15 years ago
|
Attachment #380737 -
Flags: review?(paul.craciunoiu)
Assignee | ||
Comment 26•15 years ago
|
||
Comment on attachment 380737 [details] [diff] [review] Fix caching inconsistency r26769/r26770 Intermittent failures shouldn't happen anymore.
Assignee | ||
Comment 27•15 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
Closed: 15 years ago → 15 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
Updated•15 years ago
|
Whiteboard: sumo_only
You need to log in
before you can comment on or make changes to this bug.
Description
•