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

Define the [CrossOriginIsolated] extended attribute. #883

Merged
merged 9 commits into from
Sep 21, 2020

Conversation

mikewest
Copy link
Member

@mikewest mikewest commented May 11, 2020

WebIDL currently defines a [SecureContext] extended attribute that
governs whether or not a given construct is exposed within a given
context. This patch defines a similar [CrossOriginIsolated] attribute
to govern exposure based on cross-origin isolation.

This supports the broader Securer Contexts proposal
(https://github.com/mikewest/securer-contexts), which aims to guide
spec authors to combat threats we've started paying more attention to
over the last few years.

Closes #875.


Preview | Diff

@mikewest
Copy link
Member Author

Hey folks! This can't actually land as-is, given that "cross-origin isolated" is still floating around in a set of PRs against HTML, but I'm hopeful that y'all can give me some direction about the mechanics of defining the [CrossOriginIsolated] attribute in the meantime.

This patch does the simplest thing possible, by copy/pasting [SecureContext] and munging it a bit to define exposure restrictions based upon the "cross-origin isolated" concept rather than the "secure context" concept. This is possibly fine? I considered pulling out the common bits into an "exposure attribute" concept, which would make it simpler to define these two concepts, as well as an attribute related to XSS mitigation that I haven't written up in detail yet (what https://github.com/mikewest/securer-contexts currently calls [SecureContext=Injection]).

WDYT about the approach? Would it be worth going back to making "exposure attribute" a generic concept? Or is a bit of duplication better for clarity?

/cc @annevk @camillelamy @domenic

@domenic
Copy link
Member

domenic commented May 11, 2020

My take is that at least the "available only in cross-origin isolated contexts" / "available only in secure contexts" bits should be deduped into a single parametrized algorithm. (I noticed one [SecureContext] was not changed to [CrossOriginIsolated] in the former ^_^.) The duplication for the rest seems more clarity-enhancing, since it is kind of general "extended attribute boilerplate" that you expect to see for all of them.

That algorithm could live after the definition of "exposed", perhaps?

Another nit: extended attributes are alphabetized :).

@annevk
Copy link
Member

annevk commented May 12, 2020

I miss a statement that SecureContext and CrossOriginIsolated are mutually exclusive. I think like elsewhere, we should not allow duplicate requirements.

@mikewest
Copy link
Member Author

Hrm. I noted my preference for the opposite in #875 (comment). :)

But, since I kinda want to throw [SecureContext] away in any event, I can live with either. Would you prefer that we make [SecureContext, CrossOriginIsolated] an error, or is noting that "[CrossOriginIsolated] implies the [SecureContext] restrictions as a prerequisite" enough?

@annevk
Copy link
Member

annevk commented May 12, 2020

I think it should be an error.

@mikewest
Copy link
Member Author

I think it should be an error.

I can write something up that makes [SecureContext, CrossOriginIsolated] an error (e.g. "The [SecureContext] extended attribute must not appear on a construct that also specifies [CrossOriginIsolated]." Do you also want to do some sort of inheritance checking to ensure that it doesn't appear on e.g. a method inside an interface that defines CrossOriginIsolated? If so, are there any examples of that kind of check in the text that I could cargo-cult? :)

@annevk
Copy link
Member

annevk commented May 12, 2020

SecureContext has some of those checks specific to only SecureContext, e.g., "The [SecureContext] extended attribute must not be specified both on an interface member and its interface or partial interface." I guess you could make "The [SecureContext] extended attribute must not be specified on an interface member if its interface or partial interface has the [CrossOriginIsolated] extended attribute." from that. This is not my area of expertise though.

@saschanaz might have better ideas.

@saschanaz

This comment has been minimized.

@mikewest
Copy link
Member Author

Sounds like it needs two rules, one to prevent descendants to use [CrossOriginIsolated] when there is [SecureContext] and vise versa.

I think we'd have one rule for [CrossOriginIsolated] and another for [SecureContext] along the lines of what @annevk suggested.

I wonder we could rename [SecureContext] to [Context=Secure] and [CrossOriginIsolated] to [Context=CrossOriginIsolated] so that we can have only one check: whether descendants have [Context] or not.

This is more or less the spelling of the proposal in https://github.com/mikewest/securer-contexts#a-proposal ([SecureContext=(Transport,Isolation,Injection)]). That's a fine approach if it's what we end up wanting. I think I'd prefer distinct attributes, however, as that would allow us to invert the [SecureContext] model entirely (#876).

@mikewest
Copy link
Member Author

I think my suggestion still allows inversion: [Context=Insecure] or such.

That's fair. I think the inversion is clearer as distinct attributes, but it certainly works as [Context=UnsafelyExposedToNetworkAttackers] or something similar. I can certainly live with that spelling.

That said, let's go back to your suggestion that:

... we can have only one check: whether descendants have [Context] or not.

This works for secure transport and cross-origin isolation, as the latter implies the former. Injection mitigation is currently orthogonal, evaluating whether a CSP is present, not whether the context is securely delivered. Perhaps we'd say that any contextual exposure would require secure contexts?

@mikewest
Copy link
Member Author

@annevk: Any objections to something like [ExposureConditions=(type, type, type)]?

@EdgarChen
Copy link
Member

@annevk: Any objections to something like [ExposureConditions=(type, type, type)]?

I lean to this, but it probably should only allow one argument, given that there is no use case to have multiple arguments so far?

And maybe we could also consider having LegacyNone or such as the default exposure condition if there is no exposure condition specified explicitly. If we decide to move to #876 model in the future, we could then switch the default value to SecureContext.

@EdgarChen
Copy link
Member

... we can have only one check: whether descendants have [Context] or not.

This works for secure transport and cross-origin isolation, as the latter implies the former. Injection mitigation is currently orthogonal, evaluating whether a CSP is present, not whether the context is securely delivered. Perhaps we'd say that any contextual exposure would require secure contexts?

Another point is that do we allow a interface with SecureContext to contain a member with CrossOriginIsolated or a interface with CrossOriginIsolated inherits from another interface with SecureContext? It seems they are valid use cases to me.

@mikewest
Copy link
Member Author

I lean to this, but it probably should only allow one argument, given that there is no use case to have multiple arguments so far?

As hand-waved at above, I do think we'll end up with multiple arguments. I have concrete plans for at least CrossOriginIsolated and SomethingSomethingInjectionMitigation which have non-overlapping requirements. I'm happy to consider both as requiring secure transport, but the one doesn't otherwise imply the other.

And maybe we could also consider having LegacyNone or such as the default exposure condition if there is no exposure condition specified explicitly.

I'd be a bit more aggressive with the naming; UnsafelyExposedToNetworkAttackers wasn't really a joke. :)

Another point is that do we allow a interface with SecureContext to contain a member with CrossOriginIsolated or a interface with CrossOriginIsolated inherits from another interface with SecureContext? It seems they are valid use cases to me.

Yes. We would want to support this.

@saschanaz
Copy link
Member

Another point is that do we allow a interface with SecureContext to contain a member with CrossOriginIsolated or a interface with CrossOriginIsolated inherits from another interface with SecureContext? It seems they are valid use cases to me.

Yes. We would want to support this.

So we want them to be not mutually exclusive?

@EdgarChen
Copy link
Member

As hand-waved at above, I do think we'll end up with multiple arguments. I have concrete plans for at least CrossOriginIsolated and SomethingSomethingInjectionMitigation which have non-overlapping requirements. I'm happy to consider both as requiring secure transport, but the one doesn't otherwise imply the other.

I see, so if two conditions have non-overlapping requirements, then (Condition1, Condition2) could have two meanings, OR condition or AND condition. If we would want to support both combinations, we need a way to differentiate them.

@mikewest
Copy link
Member Author

I see, so if two conditions have non-overlapping requirements, then (Condition1, Condition2) could have two meanings, OR condition or AND condition. If we would want to support both combinations, we need a way to differentiate them.

I think AND is the only combination that makes sense.

@mikewest
Copy link
Member Author

Another point is that do we allow a interface with SecureContext to contain a member with CrossOriginIsolated or a interface with CrossOriginIsolated inherits from another interface with SecureContext? It seems they are valid use cases to me.

Yes. We would want to support this.

So we want them to be not mutually exclusive?

I want the following to be legal so that we can restrict new things added to old interfaces (I'm thinking of the Performance interface in particular, which isn't currently locked to secure contexts, but wouldn't it be nice if it was?).

[SecureContext]
interface VerySecureThing {
  Promise<float> secureNumber();
  [CrossOriginIsolated] Promise<boolean> verySecureBoolean(); 
}

@annevk does not want the following to be legal:

[SecureContext,CrossOriginIsolated]
interface extraSuperSecureThing {
  ...
};

[CrossOriginIsolated]
interface SecureThing {
  ...
  [SecureContext] bool verySecureBoolean();
};

interface RegularThing {
  ...
  [SecureContext,CrossOriginIsolated] bool verySecureBoolean();
};

So, SecureContext should not be specified when CrossOriginIsolated applies to a construct. The inverse is possible, as CrossOriginIsolated is more restrictive than SecureContext.

(Sorry this is confusing.)

@domenic
Copy link
Member

domenic commented Sep 15, 2020

This looks pretty close to landing, and https://github.com/WICG/performance-measure-memory/ would like to use it. I'm happy to carry this over the finish line, but can someone confirm for me what's left to do? My read is:

  • Update to cross-link to the HTML spec definition of "cross-origin isolated", now that it exists. Since it lives on an agent cluster I guess we'll need to indirect a bit; something like global -> relevant agent -> agent cluster -> cross-origin isolated.

  • Specify the restrictions @annevk prefers on doubling this up with [SecureContext]. In particular, I think it suffices to say something like "[SecureContext] must not be specified when a construct is conditionally exposed on [CrossOriginIsolated]; doing so would be redundant, since every environment which is cross-origin isolated is also a secure context".

@mikewest
Copy link
Member Author

I would love for you to pick this up, @domenic. I have not had time to work out the right way to address your second bullet above, which seems like the only complicated bit left. If simple prose assertions make other folks on the thread happy, I'm certainly fine with them. :)

mikewest and others added 5 commits September 16, 2020 12:18
WebIDL currently defines a `[SecureContext]` extended attribute that
governs whether or not a given construct is exposed within a given
context. This patch defines a similar `[CrossOriginIsolated]` attribute
to govern exposure based on cross-origin isolation.

This supports the broader Securer Contexts proposal
(https://github.com/mikewest/securer-contexts), which aims to guide
spec authors to combat threats we've started paying more attention to
over the last few years.

Closes whatwg#875.
@domenic
Copy link
Member

domenic commented Sep 16, 2020

OK, I think this is ready then. Instead of agent cluster's cross-origin isolated, I went with environment settings object's cross-origin isolated capability, since we want the exposure here to be controlled by allow="cross-origin-isolated".

@domenic domenic requested a review from annevk September 16, 2020 16:37
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.

Thanks this looks good. I rather not call things PowerfulFeature as that tends to create confusion.

Are the 4 newlines before a <h4> intended?

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Consider adding a [CrossOriginIsolated] extended attribute.
5 participants