Bug 202034 – Sanitize suggested filenames used for saving PDFs

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Percent-encode suggested filenames used for saving PDFs

[Tim Horton]

Created attachment 379212 [details]
Patch

[Tim Horton]

<rdar://problem/53183075>

[Alex Christensen]

Comment on attachment 379212 [details]
Patch

No unit test?

[Tim Horton]

Created attachment 379254 [details]
Patch

[Chris Dumez]

Comment on attachment 379254 [details]
Patch

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

> Source/WebKit/ChangeLog:3
> +        Percent-encode suggested filenames used for saving PDFs

Title is not accurate anymore. Maybe simply Sanitize instead of Percent-encode?

> Source/WebKit/ChangeLog:19
> +        Percent-encode suggested filenames to ensure that they comprise only one path component

ditto.

[Tim Horton]

(In reply to Alex Christensen from comment #3)
> Comment on attachment 379212 [details]
> Patch
> 
> No unit test?

No, it's not possible to get in this situation normally (since the filename comes *from* an already-encoded source). Only in the case of a compromised web content process. One could imagine adding a path to allow arbitrary text there for testing, but the added complexity seems not worth it (and possibly problematic).

[Tim Horton]

Good point about the title

[Tim Horton]

Created attachment 379255 [details]
Patch

[Chris Dumez]

Comment on attachment 379255 [details]
Patch

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

r=me with nits.

> Source/WebKit/ChangeLog:19
> +        Percent-encode suggested filenames to ensure that they comprise only one path component

You failed to fix this one.

> Source/WebKit/UIProcess/WebPageProxy.cpp:7830
> +    String encodedFilename = ResourceResponseBase::sanitizeSuggestedFilename(suggestedFilename);

s/encodedFilename/sanitizedFilename

> Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:501
> +    auto encodedFileName = ResourceResponseBase::sanitizeSuggestedFilename(suggestedFilename);

s/encodedFilename/sanitizedFilename

Seems like this could be after the data.isEmpty() check.

> Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:513
> +    if (!data.size()) {

data.isEmpty()

[Tim Horton]

Comment on attachment 379255 [details]
Patch

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

>> Source/WebKit/ChangeLog:19
>> +        Percent-encode suggested filenames to ensure that they comprise only one path component
> 
> You failed to fix this one.

:|

>> Source/WebKit/UIProcess/WebPageProxy.cpp:7830
>> +    String encodedFilename = ResourceResponseBase::sanitizeSuggestedFilename(suggestedFilename);
> 
> s/encodedFilename/sanitizedFilename

That's fair

>> Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:513
>> +    if (!data.size()) {
> 
> data.isEmpty()

What no I think we just delete this one.

[Tim Horton]

Created attachment 379256 [details]
Patch

[Chris Dumez]

Comment on attachment 379255 [details]
Patch

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

>>> Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:513
>>> +    if (!data.size()) {
>> 
>> data.isEmpty()
> 
> What no I think we just delete this one.

ahah lol, indeed.

[WebKit Commit Bot]

Comment on attachment 379256 [details]
Patch

Rejecting attachment 379256 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 379256, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=379256&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=202034&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Updating working directory
Processing patch 379256 from bug 202034.
Fetching: https://bugs.webkit.org/attachment.cgi?id=379256
Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	Source/WebKit/ChangeLog

ERROR from SVN:
Item is out of date: File '/trunk/Source/WebKit/ChangeLog' is out of date
W: 78ed1d0eefeeb178590e2b514f7a9dd117935b7a and refs/remotes/origin/master differ, using rebase:
:040000 040000 2c2c2cbcff4e8c8680f7a340f845ff88910e520f c981944f73a779f13da2347399f3e6ff5843f5b3 M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	Source/WebKit/ChangeLog

ERROR from SVN:
Item is out of date: File '/trunk/Source/WebKit/ChangeLog' is out of date
W: 78ed1d0eefeeb178590e2b514f7a9dd117935b7a and refs/remotes/origin/master differ, using rebase:
:040000 040000 2c2c2cbcff4e8c8680f7a340f845ff88910e520f c981944f73a779f13da2347399f3e6ff5843f5b3 M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
From https://git.webkit.org/git/WebKit
   c4dbc56aa50..0f4343f851f  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 250152 = c4dbc56aa502491b5b16d392212ab51999377119
r250153 = 7ca844508b2445d6e534cc031b3a6aa2a460766c
r250154 = 0f4343f851fe27cbdd5a7afafe9a0cfe935fadb3
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: https://webkit-queues.webkit.org/results/13054668

[Tim Horton]

What

[Chris Dumez]

(In reply to Tim Horton from comment #14)
> What

Try again :)

[Tim Horton]

Just gonna commit it manually

[Tim Horton]

https://trac.webkit.org/changeset/250157/webkit