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 SameOrigin and SameAgentCluster for [Serializable] #4940

Closed
wants to merge 3 commits into from

Conversation

annevk
Copy link
Member

@annevk annevk commented Sep 27, 2019

Useful for WebAssembly.Module and RTCCertificate. SharedArrayBuffer can reuse the underlying infrastructure.

Fixes #4939. Helps with #4920.


/structured-data.html ( diff )

@annevk annevk added topic: serialize and transfer do not merge yet Pull request must not be merged per rationale in comment labels Sep 27, 2019
annevk added a commit to WebAssembly/spec that referenced this pull request Sep 27, 2019
@annevk
Copy link
Member Author

annevk commented Sep 27, 2019

(I don't understand how both commits passed Travis whereas for the first commit PR Preview pointed out that there was an xref problem the second commit addresses.)

@annevk
Copy link
Member Author

annevk commented Sep 27, 2019

A better design might be allowing either SameOrigin or SameOriginAndAgentCluster as that's the end goal. I.e., we don't really want folks setting SameAgentCluster as proposed without also setting SameOrigin. That would require @dtapuska weighing in and allowing some more time for objections though, as it'd immediately affect SharedArrayBuffer.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Great stuff.

A better design might be allowing either SameOrigin or SameOriginAndAgentCluster as that's the end goal. I.e., we don't really want folks setting SameAgentCluster as proposed without also setting SameOrigin.

Right. Maybe we should land this with a warning and issue link, stating that we are actively investigating removing just-SameOrigin, and specs should strongly consider using only =(SameOrigin,SameAgentCluster) or =SameOrigin.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@annevk annevk removed the do not merge yet Pull request must not be merged per rationale in comment label Oct 23, 2019
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Another thing I realized we do not handle is inheritance. The spec text says

Platform objects can be serializable objects if they implement only interfaces decorated with the [Serializable] IDL extended attribute

kind of implying that you should put it on everything in the inheritance chain, but we don't handle that really. And if we were to handle it we'd need to handle it for the SameOrigin/SameAgentCluster things as well.

My proposal is that we:

  • Change the above sentence to talk about the primary interface.
  • Check the primary interface in the processing model (for SameOrigin/SameAgentCluster, and also we could fix the current text which says "the appropriate serialization steps" to say "value's primary interface's serialization steps").
  • Add a restriction that [Serializable] that it not be used on interfaces that inherit.
  • Add a note saying that we could allow that in the future so let us know if you have a use case.

Also a lot of this applies to [Transferable] too.

Because all of this has the feeling of uncovering tech debt, I'll do a pull request for most of it separately, which this can then be based on.

source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Useful for WebAssembly.Module and RTCCertificate. SharedArrayBuffer can reuse the underlying infrastructure.

Fixes #4939. Helps with #4920.
@annevk annevk force-pushed the annevk/serializable-identifiers branch from e2ebf7a to 9356e66 Compare October 25, 2019 09:49
@annevk annevk requested a review from domenic October 25, 2019 09:50
@annevk
Copy link
Member Author

annevk commented Oct 25, 2019

I wonder if @tabatkins thought about exporting syntax for this meanwhile, but we could also export this later.

@annevk
Copy link
Member Author

annevk commented Oct 25, 2019

So going through the WebRTC use case again it seems that SameOrigin might not be adequate.

In particular, partly due to document.domain / agent cluster key, replacing their setup with SameOrigin would mean two changes:

  1. It's now restricted to same-site + same-scheme, not same-origin.
  2. Messaging cross-origin creates a messageerror (this is likely compatible and perhaps still worth doing, but if 1 is a problem you cannot get rid of the [[Origin]] slot).

cc @jan-ivar

@domenic
Copy link
Member

domenic commented Oct 25, 2019

I'm confused, why does SameOrigin give same-site + same-scheme, not same-origin? Should we rename it?

@tabatkins
Copy link
Contributor

I wonder if @tabatkins thought about exporting syntax for this meanwhile, but we could also export this later.

I'm not sure I quite understand what's being discussed here. If you'd like something in Bikeshed, mind opening an issue on me explaining it in a little more detail?

@annevk
Copy link
Member Author

annevk commented Nov 1, 2019

@domenic for serialization and deserialization it is same-origin, but due to document.domain the object could still be shared with another origin. I attempted to explain the implications of this in w3c/webrtc-pc#2343. It might be that we'd have to remove the SameOrigin value if it's not adopted anywhere in the end (I still hope WebAssembly.Module will use it and SharedArrayBuffer will use the underlying model, but I'm no longer as sure that it's a good fit for RTCCertificate other than signaling intent).

@tabatkins I filed speced/bikeshed#1544.

@domenic
Copy link
Member

domenic commented Nov 4, 2019

I think we may want to add a note or example explaining that limitation in some detail. (Assuming this ends up being something we want to merge.) I'm happy to help draft that if you'd prefer. But for now I'll wait until we have a clearer signal that this would be used by other specs.

@annevk
Copy link
Member Author

annevk commented Nov 5, 2019

I've updated the note. Here's how I see this go down:

  1. SameAgentCluster is important and needed for SharedArrayBuffer (infrastructure only)/WebAssembly.Module. Need beyond that is unclear at this point.
  2. RTCCertificate would experiment with SameOrigin.
  3. SharedArrayBuffer (infrastructure only)/WebAssembly.Module ideally adopt SameOrigin, but this is not yet finalized.
  4. If 3 happens, we rename SameAgentCluster to SameAgentClusterAndOrigin (or equivalent) and disallow a list of identifiers.

Pushing for 2 and 3 might be harder if the infrastructure is not formalized, but perhaps workable if it's essentially guaranteed to land if the experiments work out.

@annevk
Copy link
Member Author

annevk commented Dec 5, 2019

I'm convinced this kind of fake restriction isn't worth it. Change agent cluster keying or bust.

@annevk annevk closed this Dec 5, 2019
@annevk annevk deleted the annevk/serializable-identifiers branch October 15, 2021 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Infrastructure to enforce same-origin and same-agent-cluster for serializable objects
3 participants