Replacing document.all must not be allowed [chromium/src : master]
Bhagirathi Satpathy (Gerrit)
Please take a look. Thanks!
To view, visit change 823513. To unsubscribe, or for help writing mail filters, visit settings.
Kentaro Hara (Gerrit)
Does this match other browsers' behaviors?
Bhagirathi Satpathy (Gerrit)
Patch Set 6:
Does this match other browsers' behaviors?
Yes. I verified on Mozilla, same behavior is matched. I do not have latest safari to verify.
Thanks!
Bhagirathi Satpathy (Gerrit)
Bhagirathi Satpathy would like Philip Jägenstedt and Justin Schuh to review this change.
Replacing document.all must not be allowed
document.all must not be overwritten with something else as per spec.
https://html.spec.whatwg.org/
Bug: 794433
Change-Id: I40122d7107206fe2fb680fa221cf0ae003379ea9
---
M extensions/renderer/resources/platform_app.js
M third_party/WebKit/LayoutTests/external/wpt/html/dom/interfaces-expected.txt
M third_party/WebKit/LayoutTests/fast/dom/undetectable-document-all-expected.txt
M third_party/WebKit/LayoutTests/fast/dom/undetectable-document-all.html
M third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt
M third_party/WebKit/Source/core/dom/Document.idl
7 files changed, 7 insertions(+), 14 deletions(-)
Bhagirathi Satpathy (Gerrit)
Could you please review this patch. Thanks !
Kentaro Hara (Gerrit)
LGTM
Please wait for API owner's approval.
Patch set 6:Code-Review +1
Bhagirathi Satpathy (Gerrit)
Patch Set 6: Code-Review+1
LGTM
Please wait for API owner's approval.
Sure, Thank you !
Yuki Shiino (Gerrit)
AFAIK, document.all is NOT spec'ed. It's a non-standard API.
Could you elaborate what part of which spec is saying about document.all?
Also the link in the patch description doesn't make sense. Please update the link to be more specific.
Bhagirathi Satpathy (Gerrit)
Patch Set 6:
AFAIK, document.all is NOT spec'ed. It's a non-standard API.
Could you elaborate what part of which spec is saying about document.all?
Also the link in the patch description doesn't make sense. Please update the link to be more specific.
Can you please see below part of https://html.spec.whatwg.org/ where all is not 'Replaceable'. Also Mozilla behaves same behaviour.
partial interface Document {
[CEReactions] attribute [TreatNullAs=EmptyString] DOMString fgColor;
[CEReactions] attribute [TreatNullAs=EmptyString] DOMString linkColor;
[CEReactions] attribute [TreatNullAs=EmptyString] DOMString vlinkColor;
[CEReactions] attribute [TreatNullAs=EmptyString] DOMString alinkColor;
[CEReactions] attribute [TreatNullAs=EmptyString] DOMString bgColor;[SameObject] readonly attribute HTMLCollection anchors;
[SameObject] readonly attribute HTMLCollection applets;
void clear();
void captureEvents();
void releaseEvents();
[SameObject] readonly attribute HTMLAllCollection all;
};
Yuki Shiino (Gerrit)
Patch Set 6:
Patch Set 6:
AFAIK, document.all is NOT spec'ed. It's a non-standard API.
Could you elaborate what part of which spec is saying about document.all?
Also the link in the patch description doesn't make sense. Please update the link to be more specific.
Can you please see below part of https://html.spec.whatwg.org/ where all is not 'Replaceable'. Also Mozilla behaves same behaviour.
partial interface Document {
[CEReactions] attribute [TreatNullAs=EmptyString] DOMString fgColor;
[CEReactions] attribute [TreatNullAs=EmptyString] DOMString linkColor;
[CEReactions] attribute [TreatNullAs=EmptyString] DOMString vlinkColor;
[CEReactions] attribute [TreatNullAs=EmptyString] DOMString alinkColor;
[CEReactions] attribute [TreatNullAs=EmptyString] DOMString bgColor;[SameObject] readonly attribute HTMLCollection anchors;
[SameObject] readonly attribute HTMLCollection applets;void clear();
void captureEvents();
void releaseEvents();[SameObject] readonly attribute HTMLAllCollection all;
};
Ah, I see. I understand that it's spec'ed.
Anyway, could you elaborate it in the patch description and use more specific links in general?
Plus, in case of HTML, "multipage version"'s link is highly recommended because it's tough for everyone to open the one-page version.
https://html.spec.whatwg.org/multipage/obsolete.html#other-elements,-attributes-and-apis
or
https://html.spec.whatwg.org/C/obsolete.html#other-elements,-attributes-and-apis
for example.
I'll take a closer look later.
Bhagirathi Satpathy (Gerrit)
Bhagirathi Satpathy uploaded patch set #7 to this change.
document.all must not be replaced as per specification
document.all must not be overwritten with something else.
E.g., below javascript should not set 'document.all' to '1' instead
it should be always [object HTMLAllCollection].
<script>
document.all = 1;
</script>
Specification:
https://html.spec.whatwg.org/multipage/obsolete.html#other-elements,-attributes-and-apis
https://html.spec.whatwg.org/multipage/obsolete.html#dom-document-all
Bug: 794433
Change-Id: I40122d7107206fe2fb680fa221cf0ae003379ea9
---
M extensions/renderer/resources/platform_app.js
M third_party/WebKit/LayoutTests/external/wpt/html/dom/interfaces-expected.txt
M third_party/WebKit/LayoutTests/fast/dom/undetectable-document-all-expected.txt
M third_party/WebKit/LayoutTests/fast/dom/undetectable-document-all.html
M third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt
M third_party/WebKit/Source/core/dom/Document.idl
7 files changed, 7 insertions(+), 14 deletions(-)
To view, visit change 823513. To unsubscribe, or for help writing mail filters, visit settings.
Philip Jägenstedt (Gerrit)
Patch set 7:Code-Review +1
1 comment:
File third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt:
This change is cause for concern as document.all is ancient and even a corner case like this might have stuff depending on it. But, https://wpt.fyi/html/dom/interfaces.html to the rescue, Edge 15 already passed this test, giving some confidence that the whole won't break. Let's try it and see.
To view, visit change 823513. To unsubscribe, or for help writing mail filters, visit settings.
Yuki Shiino (Gerrit)
LGTM.
Thanks for updating the patch description. It looks much better. :)
File third_party/WebKit/Source/core/dom/Document.idl:
Patch Set #7, Line 161:
[MeasureAs=DocumentAll] readonly attribute HTMLAllCollection all;Could you add a TODO comment to make |all| [SameObject]?
To view, visit change 823513. To unsubscribe, or for help writing mail filters, visit settings.
Bhagirathi Satpathy (Gerrit)
Thanks for the review.
Hi Justin Schuh@, can you please review as owner of extensions/renderer/resources/platform_app.js.
2 comments:
File third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt:
This change is cause for concern as document. […]
Thank you !
File third_party/WebKit/Source/core/dom/Document.idl:
Patch Set #7, Line 161:
// TODO: |all| should be [SameObject].
Could you add a TODO comment to make |all| [SameObject]?
Done
To view, visit change 823513. To unsubscribe, or for help writing mail filters, visit settings.
Bhagirathi Satpathy (Gerrit)
Patch set 8:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Addressed review comments" https://chromium-review.googlesource.com/c/823513/8
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/823513/8
Bot data: {"action": "start", "triggered_at": "2017-12-15T10:42:43.0Z", "cq_cfg_revision": "b547f51ef97353cccb06eebcda71133909f61295", "revision": "d1de9eabe845a38e76cdcb2be89ba4166b4e0c2c"}
Commit Bot (Gerrit)
Try jobs failed on following builders:
chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/618763)
Bhagirathi Satpathy (Gerrit)
Hi John Abd-El-Malek@, can you please review as owner of extensions/renderer/resources/platform_app.js.
'document.all = undefined' is not required in platform_app.js because document.all is undefined by default and with this patch, it can not be overwritten.
John Abd-El-Malek (Gerrit)
Patch Set 8:
Hi John Abd-El-Malek@, can you please review as owner of extensions/renderer/resources/platform_app.js.
'document.all = undefined' is not required in platform_app.js because document.all is undefined by default and with this patch, it can not be overwritten.
Please pick an extensions/owner
Bhagirathi Satpathy (Gerrit)
Patch Set 8:
Patch Set 8:
Hi John Abd-El-Malek@, can you please review as owner of extensions/renderer/resources/platform_app.js.
'document.all = undefined' is not required in platform_app.js because document.all is undefined by default and with this patch, it can not be overwritten.Please pick an extensions/owner
Oh ok, thanks.
Bhagirathi Satpathy (Gerrit)
Hi Istiaque Ahmed@, can you please review as owner of extensions/renderer/resources/platform_app.js.
'document.all = undefined' is not required in platform_app.js because document.all is undefined by default and with this patch, it can not be overwritten.
Istiaque Ahmed (Gerrit)
rerouting to Devlin. Devlin, can you take a look?
Devlin (Gerrit)
1 comment:
File extensions/renderer/resources/platform_app.js:
I'm not sure I agree with this change. While this change is striving to meet the spec, I'm not sure it sits well with me that the result here is that we'd be opening up platform apps to using an API that's been deprecated for years that currently has no usage. It seems like we'd be causing ourselves a bigger problem.
Could we instead have document.all access throw an error in these contexts, or similar?
To view, visit change 823513. To unsubscribe, or for help writing mail filters, visit settings.
Bhagirathi Satpathy (Gerrit)
1 comment:
I'm not sure I agree with this change. […]
AFAIU here document properties like 'alinkColor', 'all', 'bgColor', 'fgColor', 'linkColor', 'vlinkColor' log 'not available' warnings to the console and return undefined when accessed because of disableGetters(). Also with this change, behavior would be same because disableGetters() is called for all above properties.
Since with this patch document.all can not be overwritten to match the spec (Mozilla, Edge behaves same). So, 'document.all = undefined;' would throw error "Cannot assign to read only property 'all' of object" which cause javascript to stop without executing disableGetters().
Also document.all is undefined by default here, so I removed this statement.
Please suggest if I understand incorrectly. Thanks!
To view, visit change 823513. To unsubscribe, or for help writing mail filters, visit settings.
Bhagirathi Satpathy (Gerrit)
1 comment:
AFAIU here document properties like 'alinkColor', 'all', 'bgColor', 'fgColor', 'linkColor', 'vlinkCo […]
one change in above description: document.all is not undefined but [object HTMLAllCollection].
Thanks!
To view, visit change 823513. To unsubscribe, or for help writing mail filters, visit settings.
Yuki Shiino (Gerrit)
1 comment:
one change in above description: document.all is not undefined but [object HTMLAllCollection]. […]
Just FYI,
If it's super important to disallow the apps to access document.all, technically it's possible to remove the property (or re-define the property).
delete Document.prototype.all // Delete the property.
// document.all => undefinedAnyway, as Bhagirathi wrote, |all| property is *shadowed* by |disableGetters|. It's just shadowed, so you can access it as:
Object.getOwnPropertyDescriptor(Document.prototype, 'all').get.call(document)
unless you've deleted |all| property like the above.
In other words, we can improve |disableGetters| and |disableMethods| so that they find the original definitions and replace them with whatever we want. IMHO, it's a minor improvement, though.
To view, visit change 823513. To unsubscribe, or for help writing mail filters, visit settings.
Devlin (Gerrit)
1 comment:
Just FYI, […]
Thanks for the extra information! A few thoughts.
Bhagirathi: I (think I) understand what the new behavior would be, but the end result is that it would be possible for platform apps to access document.all, which is something we want to avoid. Additionally, while I understand that this would (somewhat) violate the spec, there are plenty of times this is already the case for platform app contexts (which are *not* part of the open web). As such, I think there's more value in ensuring that platform apps cannot access document.all than satisfying the standard for a non-standard, non-web platform.
Yuki: I'm not a 100% sure I follow - while
Object.getOwnPropertyDescriptor(Document.prototype, 'all').get.call(document)
will return the value of document.all, since we set document.all to undefined above, it just returns undefined, right? So in today's world, it is impossible for platform apps to use the document.all API, is my understanding?
Additionally, will the "delete document.all" approach still work with this patch, or would that also violate the spec (since then document.all would not return HTMLAllCollection)? If the latter, is deleting document.all something we'd also remove support for?
---
At a higher level, my main priority is ensuring that we don't *increase* usage of a deprecated API by opening it up unnecessarily to platform apps. If using the `delete document.all` approach above works, I'd be fine with that. Otherwise, I'd want to find some other way to guard against this.
To view, visit change 823513. To unsubscribe, or for help writing mail filters, visit settings.
Bhagirathi Satpathy (Gerrit)
Patch Set 8:
(1 comment)
Thanks Devlin for the review.
As Yuki suggested, I verified 'delete Document.prototype.all' is making document.all as undefined with this patch.
Thanks!
Yuki Shiino (Gerrit)
1 comment:
Thanks for the extra information! A few thoughts. […]
Yes, Devlin, you're right.
With the current implementation without this patch, |document.all| is set to |undefined| as expected. So the above getOwnPropertyDescriptor hack returns |undefined|.
However, other properties such as 'alinkColor', 'bgColor', etc. are just hidden. So it's possible to access them with the same hack. Only |document.all| is set to |undefined| (this is a little different from the complete removal, though).
Let me summarize our situation.
a) As we're going to remove [Replaceable] from document.all's declaration in the IDL file, we can no longer set document.all to undefined.
b) However, we *can* remove the property. This is close to the current implementation (not completely equivalent, but it's true that the platform apps cannot access the original document.all).
c) Other properties such as 'alinkColor', 'bgColor', etc. are NOT removed nor set to undefined, regardless of this patch. We can improve disableGetters/disableMethods to re-define the properties. Then, the platform apps will no longer be able to access these properties of the original values.I'd suggest to remove Document.prototype.all in this patch so that the behavior will be close to the current implementation. We can update disableGetters/disableMethods in a follow-up patch as a separate issue.
I think that this approach meets the goal.
To view, visit change 823513. To unsubscribe, or for help writing mail filters, visit settings.
Bhagirathi Satpathy (Gerrit)
1 comment:
Yes, Devlin, you're right. […]
Modified platform_app.js as suggested. I need to rebase some files as they are changed now.
Please take a look.
Thanks!
To view, visit change 823513. To unsubscribe, or for help writing mail filters, visit settings.
Yuki Shiino (Gerrit)
LGTM on my side.
Devlin (Gerrit)
extensions lgtm
Patch set 9:Code-Review +1
1 comment:
Modified platform_app.js as suggested. I need to rebase some files as they are changed now. […]
This all sounds good to me! Thanks, folks!
Bhagirathi Satpathy (Gerrit)
Patch set 9:Commit-Queue +2
Commit Bot (Gerrit)
Commit Bot merged this change.
document.all must not be replaced as per specification
document.all must not be overwritten with something else.
E.g., below javascript should not set 'document.all' to '1' instead
it should be always [object HTMLAllCollection].
<script>
document.all = 1;
</script>
Specification:
https://html.spec.whatwg.org/multipage/obsolete.html#other-elements,-attributes-and-apis
https://html.spec.whatwg.org/multipage/obsolete.html#dom-document-all
Bug: 794433
Change-Id: I40122d7107206fe2fb680fa221cf0ae003379ea9
Reviewed-on: https://chromium-review.googlesource.com/823513
Reviewed-by: Devlin <rdevlin...@chromium.org>
Reviewed-by: Philip Jägenstedt <foo...@chromium.org>
Reviewed-by: Yuki Shiino <yukis...@chromium.org>
Reviewed-by: Kentaro Hara <har...@chromium.org>
Commit-Queue: Bhagirathi Satpathy <bhagir...@samsung.com>
Cr-Commit-Position: refs/heads/master@{#525058}
---
M extensions/renderer/resources/platform_app.js
M third_party/WebKit/LayoutTests/external/wpt/html/dom/interfaces-expected.txt
M third_party/WebKit/LayoutTests/fast/dom/undetectable-document-all-expected.txt
M third_party/WebKit/LayoutTests/fast/dom/undetectable-document-all.html
M third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt
M third_party/WebKit/Source/core/dom/Document.idl
7 files changed, 10 insertions(+), 14 deletions(-)