Bug 118898 - Make atob() throw an InvalidCharacterError on excess padding characters
Summary: Make atob() throw an InvalidCharacterError on excess padding characters
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: http://www.whatwg.org/specs/web-apps/...
Keywords: BlinkMergeCandidate, WebExposed
: 104358 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-07-19 05:14 PDT by Chris Dumez
Modified: 2015-05-11 02:11 PDT (History)
14 users (show)

See Also:


Attachments
Patch (11.99 KB, patch)
2013-07-19 05:22 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (15.42 KB, patch)
2013-07-22 01:08 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (928.37 KB, application/zip)
2013-07-22 03:00 PDT, Build Bot
no flags Details
Patch (8.49 KB, patch)
2013-07-22 05:26 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from APPLE-EWS-1 for win-future (310.45 KB, application/zip)
2013-07-27 05:26 PDT, Build Bot
no flags Details
Patch (8.98 KB, patch)
2013-08-09 00:29 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (8.77 KB, patch)
2013-08-09 03:36 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2013-07-19 05:14:51 PDT
According to the latest specification, window.atob() should throw an
InvalidCharacterError on excess padding characters:
http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#dom-windowbase64-atob (steps 4, 5 and 6)

This behavior is consistent with both Firefox, IE10 and since recently Blink.

Test suite:
http://www.w3c-test.org/html/tests/submission/AryehGregor/base64.html

Corresponding Blink revision:
https://src.chromium.org/viewvc/blink?revision=154538&view=revision
Comment 1 Chris Dumez 2013-07-19 05:22:11 PDT
Created attachment 207081 [details]
Patch
Comment 2 Benjamin Poulain 2013-07-19 14:47:44 PDT
Comment on attachment 207081 [details]
Patch

We don't need the pay the cost of this at runtime. You can have two functions, with and without padding, and make Base64PaddingValidationPolicy a template parameter.
Comment 3 Chris Dumez 2013-07-22 01:08:57 PDT
Created attachment 207229 [details]
Patch
Comment 4 Chris Dumez 2013-07-22 01:10:45 PDT
(In reply to comment #3)
> Created an attachment (id=207229) [details]
> Patch

Took Benjamin's feedback into consideration.

It is a bit unfortunate default function template parameters are only supported in C++11.
Comment 5 Build Bot 2013-07-22 02:59:59 PDT
Comment on attachment 207229 [details]
Patch

Attachment 207229 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1177092

New failing tests:
fast/loader/user-stylesheet-fast-path.html
Comment 6 Build Bot 2013-07-22 03:00:01 PDT
Created attachment 207234 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.3
Comment 7 Chris Dumez 2013-07-22 05:26:47 PDT
Created attachment 207240 [details]
Patch
Comment 8 Chris Dumez 2013-07-22 05:29:42 PDT
(In reply to comment #2)
> (From update of attachment 207081 [details])
> We don't need the pay the cost of this at runtime. You can have two functions, with and without padding, and make Base64PaddingValidationPolicy a template parameter.

I realized that padding validation only makes sense if we fail on invalid characters already (otherwise, there may be invalid characters / spaces at the end, after the padding or in the padding). Therefore, adding a new argument does not make much sense and it is better to add a new enumeration value such as "Base64FailOnInvalidCharacterOrExcessPadding".
Comment 9 Chris Dumez 2013-07-25 08:33:40 PDT
(In reply to comment #8)
> (In reply to comment #2)
> > (From update of attachment 207081 [details] [details])
> > We don't need the pay the cost of this at runtime. You can have two functions, with and without padding, and make Base64PaddingValidationPolicy a template parameter.
> 
> I realized that padding validation only makes sense if we fail on invalid characters already (otherwise, there may be invalid characters / spaces at the end, after the padding or in the padding). Therefore, adding a new argument does not make much sense and it is better to add a new enumeration value such as "Base64FailOnInvalidCharacterOrExcessPadding".

Benjamin, could you please take another look?
Comment 10 Build Bot 2013-07-27 05:26:00 PDT
Comment on attachment 207240 [details]
Patch

Attachment 207240 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1255430

New failing tests:
dom/xhtml/level1/core/documentinvalidcharacterexceptioncreateentref1.xhtml
dom/svg/level3/xpath/XPathEvaluator_evaluate_INVALID_EXPRESSION_ERR.svg
dom/html/level2/events/dispatchEvent04.html
dom/html/level2/html/HTMLSelectElement20.html
dom/svg/level3/xpath/XPathEvaluator_createExpression_INVALID_EXPRESSION_ERR.svg
dom/svg/level3/xpath/XPathEvaluator_createExpression_NAMESPACE_ERR_02.svg
dom/html/level1/core/documentinvalidcharacterexceptioncreateentref.html
dom/html/level2/core/hc_namednodemapinvalidtype1.html
dom/svg/level3/xpath/XPathEvaluator_evaluate_NAMESPACE_ERR.svg
dom/html/level2/events/dispatchEvent01.html
dom/xhtml/level1/core/hc_attrappendchild4.xhtml
dom/html/level2/events/dispatchEvent03.html
dom/html/level2/events/dispatchEvent02.html
dom/html/level2/core/createDocumentType04.html
dom/html/level1/core/documentinvalidcharacterexceptioncreateentref1.html
dom/html/level1/core/documentinvalidcharacterexceptioncreatepi1.html
dom/html/level2/events/dispatchEvent06.html
css1/basic/comments.html
dom/xhtml/level1/core/documentinvalidcharacterexceptioncreatepi1.xhtml
dom/html/level2/events/dispatchEvent07.html
dom/html/level2/core/setAttributeNS10.html
dom/html/level2/events/dispatchEvent05.html
dom/html/level1/core/hc_attrappendchild2.html
dom/html/level2/core/createAttributeNS06.html
dom/html/level1/core/hc_attrappendchild4.html
dom/xhtml/level1/core/documentinvalidcharacterexceptioncreatepi.xhtml
dom/svg/level3/xpath/XPathEvaluator_createExpression_NAMESPACE_ERR_01.svg
dom/xhtml/level1/core/documentinvalidcharacterexceptioncreateentref.xhtml
dom/xhtml/level1/core/hc_attrappendchild2.xhtml
dom/html/level1/core/documentinvalidcharacterexceptioncreatepi.html
Comment 11 Build Bot 2013-07-27 05:26:04 PDT
Created attachment 207583 [details]
Archive of layout-test-results from APPLE-EWS-1 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: APPLE-EWS-1  Port: win-future  Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit
Comment 12 Chris Dumez 2013-08-07 01:19:33 PDT
Any feedback on the latest patch?
Comment 13 Chris Dumez 2013-08-09 00:29:47 PDT
Created attachment 208400 [details]
Patch
Comment 14 Chris Dumez 2013-08-09 00:31:58 PDT
I updated the patch based on Benjamin's proposal on IRC.
Comment 15 Chris Dumez 2013-08-09 03:36:54 PDT
Created attachment 208413 [details]
Patch
Comment 16 WebKit Commit Bot 2013-08-09 11:53:21 PDT
Comment on attachment 208413 [details]
Patch

Clearing flags on attachment: 208413

Committed r153904: <http://trac.webkit.org/changeset/153904>
Comment 17 WebKit Commit Bot 2013-08-09 11:53:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Csaba Osztrogonác 2015-05-11 02:11:25 PDT
*** Bug 104358 has been marked as a duplicate of this bug. ***