Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce Feature policy : document-write #172

Closed
wants to merge 2 commits into from

Conversation

ehsan-karamad
Copy link
Contributor

This document servers as an explainer of an experimental feature which was most recently committed to chromium codebase. The state of the proposal is not complete as there is still a critical open questions regarding the correct implementation and behavior for cross and same-origin contents.

All suggestions and ideas are most welcomed!

This document servers as an explainer of an experimental feature which was most recently committed to chromium [codebase](https://chromium.googlesource.com/chromium/src/+/d8e49fe9a8374c236010d75874c082e9701543fb). The state of the proposal is not complete as there is still a critical open questions regarding the correct implementation and behavior for cross and same-origin contents.

All suggestions and ideas are most welcomed!
@annevk
Copy link
Member

annevk commented Jun 5, 2018

I think it would be much clearer if we called this "document-write".

@clelland
Copy link
Collaborator

clelland commented Jun 5, 2018

Agreed -- the initial spec had a feature called 'docwrite', for even more terseness. Do you think document-write captures the idea that this also restricts document.open and document.close as well?


## Objective
The API around document stream modification ([dynamic mark-up insertion](https://www.w3.org/TR/2011/WD-html5-author-20110705/apis-in-html-documents.html#dynamic-markup-insertion)), i.e., `document.write`, `document.writeln`, `document.open` and `document.close`
are anti-pattern, parser-blocking javascritp API. The objective of the proposed feature is to limit the usage of this API in websites.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sp: JavaScript

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks!


## Solution

When the feature is disabled for a specific origin in a frame, any occurance of such API will be ignored and might
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sp: occurrence (or better yet, "usage")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks!

```
<iframe src="https://example.com" allow="document-stream-insertion http://www.google.com"></iframe>
```
would limit the usage of stream insertion API to only "google.com" origins using HTTPS protocol.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite accurate: This limits it to just www.google.com, not any *.google.com origin. (and you've used HTTP, rather than HTTPS)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I believe I confused site instance with origin.

1: The _most_ correct behavior is not known yet. For now, Chrome's implementation behind the flag simply throws exception
on API usage (for both same and cross origin). But this is not the final implementation.

2: An embedded website might rely on important `<script>` added to the document using `document.write`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should note that this is already not guaranteed to work in Chrome, because of WICG/interventions#17 -- so hopefully sites are already discouraged from implementing critical functionality via document.write of script content.

The difference with this proposal is that the embedder now has some control of whether or not the write will succeed, rather than just the user agent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Added a few lines here.

@annevk
Copy link
Member

annevk commented Jun 5, 2018

I think it's close enough (don't forget document.writeln()!). I suspect the variant of document.open() that's actually window.open() won't be blocked by this? This'll require careful integration with HTML.

Addressing comments, fixing typos, and more importantly renaming the feature from `document-stream-insertion` to `document-write`.
@ehsan-karamad ehsan-karamad changed the title Introduce Feature policy : document-stream-insertion Introduce Feature policy : document-write Jun 5, 2018
@ehsan-karamad
Copy link
Contributor Author

Just to confirm and clarify, this feature does not block the variant of document.open with 3+ args which would actually do window.open (link).

@ojanvafai
Copy link
Collaborator

You can already do the opening behavior with window.open. Is there a specific reason to continue to allow it with document.open? I think it's pretty subtle and confusing to have the API partially function and would be better for us to keep it simple that all uses of document.open fail.

@annevk
Copy link
Member

annevk commented Jun 7, 2018

@ojanvafai blocking both requires two checks (one for each algorithm), as they're different methods do to IDL overloading. Therefore it might also be a tad confusing, but it seems fine to block both to me.

@ehsan-karamad
Copy link
Contributor Author

Both variants of document.open() (returning window v/s document) are explained in the "dynamic markup insertion" section of the sepc, therefore, so far as the promise of the feature goes, we could block both. That said our current implementation in chrome (behind flag) does not block the variant returning window.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 11, 2018
It was proposed here w3c/webappsec-permissions-policy#172
that "document-write" is a better name for this feature. This CL makes
the naming change correspondingly.

Bug: 841605
Change-Id: Id0a928fd3f6669eb3700736ff5770d4717d2b414
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 13, 2018
It was proposed here w3c/webappsec-permissions-policy#172
that "document-write" is a better name for this feature. This CL makes
the naming change correspondingly.

Bug: 841605
Change-Id: Id0a928fd3f6669eb3700736ff5770d4717d2b414
aarongable pushed a commit to chromium/chromium that referenced this pull request Jun 13, 2018
It was proposed here w3c/webappsec-permissions-policy#172
that "document-write" is a better name for this feature. This CL makes
the naming change correspondingly.

Bug: 841605
Change-Id: Id0a928fd3f6669eb3700736ff5770d4717d2b414
Reviewed-on: https://chromium-review.googlesource.com/1095328
Commit-Queue: Ehsan Karamad <ekaramad@chromium.org>
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566829}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 13, 2018
It was proposed here w3c/webappsec-permissions-policy#172
that "document-write" is a better name for this feature. This CL makes
the naming change correspondingly.

Bug: 841605
Change-Id: Id0a928fd3f6669eb3700736ff5770d4717d2b414
Reviewed-on: https://chromium-review.googlesource.com/1095328
Commit-Queue: Ehsan Karamad <ekaramad@chromium.org>
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566829}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 13, 2018
It was proposed here w3c/webappsec-permissions-policy#172
that "document-write" is a better name for this feature. This CL makes
the naming change correspondingly.

Bug: 841605
Change-Id: Id0a928fd3f6669eb3700736ff5770d4717d2b414
Reviewed-on: https://chromium-review.googlesource.com/1095328
Commit-Queue: Ehsan Karamad <ekaramad@chromium.org>
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566829}
@clelland
Copy link
Collaborator

Merged into master as ca24bf4

@clelland clelland closed this Jun 27, 2018
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jul 11, 2018
…nsertion, a=testonly

Automatic update from web-platform-testsRename Feature Policy: document-stream-insertion

It was proposed here w3c/webappsec-permissions-policy#172
that "document-write" is a better name for this feature. This CL makes
the naming change correspondingly.

Bug: 841605
Change-Id: Id0a928fd3f6669eb3700736ff5770d4717d2b414
Reviewed-on: https://chromium-review.googlesource.com/1095328
Commit-Queue: Ehsan Karamad <ekaramad@chromium.org>
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566829}

--

wpt-commits: a0272053ab1712be6bf6b967ce75797b351001fc
wpt-pr: 11463


--HG--
rename : testing/web-platform/tests/feature-policy/experimental-features/resources/document-stream-insertion.html => testing/web-platform/tests/feature-policy/experimental-features/resources/document-write.html
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 3, 2019
…nsertion, a=testonly

Automatic update from web-platform-testsRename Feature Policy: document-stream-insertion

It was proposed here w3c/webappsec-permissions-policy#172
that "document-write" is a better name for this feature. This CL makes
the naming change correspondingly.

Bug: 841605
Change-Id: Id0a928fd3f6669eb3700736ff5770d4717d2b414
Reviewed-on: https://chromium-review.googlesource.com/1095328
Commit-Queue: Ehsan Karamad <ekaramadchromium.org>
Reviewed-by: Ian Clelland <iclellandchromium.org>
Reviewed-by: Kent Tamura <tkentchromium.org>
Reviewed-by: Nasko Oskov <naskochromium.org>
Cr-Commit-Position: refs/heads/master{#566829}

--

wpt-commits: a0272053ab1712be6bf6b967ce75797b351001fc
wpt-pr: 11463

UltraBlame original commit: 40f42f669a0b67e18bbc3beb3ac706bfb6cbb53a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 3, 2019
…nsertion, a=testonly

Automatic update from web-platform-testsRename Feature Policy: document-stream-insertion

It was proposed here w3c/webappsec-permissions-policy#172
that "document-write" is a better name for this feature. This CL makes
the naming change correspondingly.

Bug: 841605
Change-Id: Id0a928fd3f6669eb3700736ff5770d4717d2b414
Reviewed-on: https://chromium-review.googlesource.com/1095328
Commit-Queue: Ehsan Karamad <ekaramadchromium.org>
Reviewed-by: Ian Clelland <iclellandchromium.org>
Reviewed-by: Kent Tamura <tkentchromium.org>
Reviewed-by: Nasko Oskov <naskochromium.org>
Cr-Commit-Position: refs/heads/master{#566829}

--

wpt-commits: a0272053ab1712be6bf6b967ce75797b351001fc
wpt-pr: 11463

UltraBlame original commit: 40f42f669a0b67e18bbc3beb3ac706bfb6cbb53a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 3, 2019
…nsertion, a=testonly

Automatic update from web-platform-testsRename Feature Policy: document-stream-insertion

It was proposed here w3c/webappsec-permissions-policy#172
that "document-write" is a better name for this feature. This CL makes
the naming change correspondingly.

Bug: 841605
Change-Id: Id0a928fd3f6669eb3700736ff5770d4717d2b414
Reviewed-on: https://chromium-review.googlesource.com/1095328
Commit-Queue: Ehsan Karamad <ekaramadchromium.org>
Reviewed-by: Ian Clelland <iclellandchromium.org>
Reviewed-by: Kent Tamura <tkentchromium.org>
Reviewed-by: Nasko Oskov <naskochromium.org>
Cr-Commit-Position: refs/heads/master{#566829}

--

wpt-commits: a0272053ab1712be6bf6b967ce75797b351001fc
wpt-pr: 11463

UltraBlame original commit: 40f42f669a0b67e18bbc3beb3ac706bfb6cbb53a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants