Bug 76996 - [chromium] Fix ClipboardChromium::validateFilename to actually operate on extensions
Summary: [chromium] Fix ClipboardChromium::validateFilename to actually operate on ext...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Daniel Cheng
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-25 02:31 PST by Daniel Cheng
Modified: 2012-01-25 21:02 PST (History)
0 users

See Also:


Attachments
Patch (8.38 KB, patch)
2012-01-25 02:33 PST, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (13.07 KB, patch)
2012-01-25 13:03 PST, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (13.82 KB, patch)
2012-01-25 13:12 PST, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (19.92 KB, patch)
2012-01-25 14:18 PST, Daniel Cheng
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Cheng 2012-01-25 02:31:19 PST
[chromium] Fix ClipboardChromium::validateFilename to actually operate on extensions
Comment 1 Daniel Cheng 2012-01-25 02:33:59 PST
Created attachment 123905 [details]
Patch
Comment 2 Daniel Cheng 2012-01-25 02:39:32 PST
I left the test oops for now because I'm not sure what your opinion here is. Unless I'm missing something very clever, this isn't currently testable with layout tests. However... we could probably write a webkit unit test for it since it's a static helper; any idea on the preferred way to handle platform differences Separate ClipboardChromiumWinTest.cpp and ClipboardChromiumMacTest.cpp?
Comment 3 Tony Chang 2012-01-25 10:10:41 PST
Comment on attachment 123905 [details]
Patch

The code change looks fine.  Can you use eventSender.beginDragWithFiles to test?  I would try all file names on all platforms and check in different results for each platform.  If that doesn't work, a unittest is fine (probably with some #ifs in the cpp file).
Comment 4 Daniel Cheng 2012-01-25 13:03:45 PST
Created attachment 123994 [details]
Patch
Comment 5 Daniel Cheng 2012-01-25 13:04:27 PST
Comment on attachment 123994 [details]
Patch

Need to update ChangeLogs. Please ignore.
Comment 6 Daniel Cheng 2012-01-25 13:12:18 PST
Created attachment 123995 [details]
Patch
Comment 7 Tony Chang 2012-01-25 13:24:55 PST
Comment on attachment 123995 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123995&action=review

> Source/WebCore/platform/chromium/ClipboardChromiumLinux.cpp:36
> +void ClipboardChromium::validateFilename(String& name, String& extension)
>  {
>      notImplemented();

Aren't null and forward slash not allowed?

> Source/WebKit/chromium/WebKit.gypi:150
> +            ['OS!="linux"', {
> +                'webkit_unittest_files': [
> +                    'tests/ClipboardChromiumTest.cpp',

We should run this test on Linux too.
Comment 8 Daniel Cheng 2012-01-25 14:18:19 PST
Created attachment 124008 [details]
Patch
Comment 9 Daniel Cheng 2012-01-25 14:20:10 PST
(In reply to comment #7)
> (From update of attachment 123995 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123995&action=review
> 
> > Source/WebCore/platform/chromium/ClipboardChromiumLinux.cpp:36
> > +void ClipboardChromium::validateFilename(String& name, String& extension)
> >  {
> >      notImplemented();
> 
> Aren't null and forward slash not allowed?

I was opting to keep the same implementation for simplicity. Examining the FS limits though, it seems easier to just share the Linux/Mac code for this.

> 
> > Source/WebKit/chromium/WebKit.gypi:150
> > +            ['OS!="linux"', {
> > +                'webkit_unittest_files': [
> > +                    'tests/ClipboardChromiumTest.cpp',
> 
> We should run this test on Linux too.

I omitted it because we didn't have any implementation before. While I was here, I also adjusted the Win implementation to be more correct--paths are limited to MAX_PATH length with a null terminator, but individual path components are a max of 255 without null.
Comment 10 Tony Chang 2012-01-25 15:00:57 PST
Comment on attachment 124008 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124008&action=review

> Source/WebCore/platform/chromium/ClipboardChromiumPosix.cpp:40
> +    // HFS+ disallows '/' and Linux systems also disallow null. For sanity's sake we'll also
> +    // disallow control characters.

Why would we disallow control characters?  What if the user is trying to drag&drop a file with a control character in it?
Comment 11 Daniel Cheng 2012-01-25 15:06:21 PST
Comment on attachment 124008 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124008&action=review

>> Source/WebCore/platform/chromium/ClipboardChromiumPosix.cpp:40
>> +    // disallow control characters.
> 
> Why would we disallow control characters?  What if the user is trying to drag&drop a file with a control character in it?

This is only used when dragging images out of WebKit, IIRC. I don't know why we disallow, other than whoever wrote the code originally thought it'd be nice. I guess it's nice to have filenames without too many strange characters in it or causing your terminal to behave strangely after you 'ls'
Comment 12 Tony Chang 2012-01-25 16:42:04 PST
Comment on attachment 124008 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124008&action=review

>>> Source/WebCore/platform/chromium/ClipboardChromiumPosix.cpp:40
>>> +    // disallow control characters.
>> 
>> Why would we disallow control characters?  What if the user is trying to drag&drop a file with a control character in it?
> 
> This is only used when dragging images out of WebKit, IIRC. I don't know why we disallow, other than whoever wrote the code originally thought it'd be nice. I guess it's nice to have filenames without too many strange characters in it or causing your terminal to behave strangely after you 'ls'

Ah, I see.  I guess that's OK.
Comment 13 Daniel Cheng 2012-01-25 21:02:19 PST
Comment on attachment 124008 [details]
Patch

Clearing flags on attachment: 124008

Committed r105962: <http://trac.webkit.org/changeset/105962>
Comment 14 Daniel Cheng 2012-01-25 21:02:23 PST
All reviewed patches have been landed.  Closing bug.