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

Various updates to the document.domain section #5714

Merged
merged 1 commit into from
Jul 10, 2020

Conversation

domenic
Copy link
Member

@domenic domenic commented Jul 8, 2020

  • Expand the previous warning, and make it more prominent. Closes Discourage use of document.domain #5128.

  • Update the algorithm steps to modern Web IDL style.

  • Update the domintro by incorporating the note on what it does, and
    describing failure cases in more detail.

  • Move the helper algorithm below the getter/setter steps.


I'm not convinced that spelling out all the exception cases in the domintro is worth it. However, I thought noting that it fails for crossOriginIsolated (and soon, originIsolated) cases was important, and just talking about sandboxed iframes felt incomplete. Thoughts welcome.

I also welcome suggestions on ways to scare people off more convincingly. I may have over-focused on preserving the shared-hosting/different-ports example.

Finally, it might be worth mentioning that it doesn't work on subdomains of public suffixes? I dunno.

I'd like to get this merged relatively soon so that I can rebase origin isolation on top of it.


/origin.html ( diff )

* Expand the previous warning, and make it more prominent. Closes #5128.

* Update the algorithm steps to modern Web IDL style.

* Update the domintro by incorporating the note on what it does, and
  describing failure cases in more detail.

* Move the helper algorithm below the getter/setter steps.
@domenic domenic added removal/deprecation Removing or deprecating a feature clarification Standard could be clearer topic: origin labels Jul 8, 2020
@domenic domenic requested a review from annevk July 8, 2020 19:08
@domenic domenic mentioned this pull request Jul 8, 2020
3 tasks
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

I would remove the Permissions Policy mention for now.

It also struck me it's woefully insecure without HTTPS, but then anything is.

<p>In sandboxed <code>iframe</code>s, <code>Document</code>s with <span
data-x="concept-origin-opaque">opaque origins</span>, <code>Document</code>s without a <span
data-x="concept-document-bc">browsing context</span>, and when the "<code
data-x="document-domain-feature">document-domain</code>" feature is disabled, the setter will
Copy link
Member

Choose a reason for hiding this comment

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

"feature" here might get out-of-date quickly. Also not entirely sure we'd keep this as a Permissions Policy thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's better to keep it, as long as I keep a close eye on rebasing this on top of #5719 or updating #5719 if this merges first.

I agree it might not stay as a Permissions Policy, but at that point we'd make the change atomically.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess this doesn't need any changes due to the rename; it's still a "feature" (at least until https://github.com/w3c/webappsec-feature-policy/issues/369 gets settled).

data-x="dom-document-domain">document.domain</code> setter has been used.</p>

<p>Because of these security pitfalls, this feature is in the process of being removed from the
Web platform. (This is a long process that takes many years.)</p>
Copy link
Member

Choose a reason for hiding this comment

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

Lowercase web.

Copy link
Member Author

Choose a reason for hiding this comment

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

HTML uppercases Web (248 instances currently; probably some false positives). Including in other "in the process of being removed from the Web platform".

I'm happy to change this here though if you'd prefer to just kind of change the casing as we edit nearby areas, but that might get confusing...

Copy link
Member

Choose a reason for hiding this comment

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

I can do a PR.

@domenic domenic merged commit b868520 into master Jul 10, 2020
@domenic domenic deleted the editorial-document-domain branch July 10, 2020 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Standard could be clearer removal/deprecation Removing or deprecating a feature topic: origin
Development

Successfully merging this pull request may close these issues.

Discourage use of document.domain
2 participants