comply with EIP-5593

[kdenhartog]

This limits the places where the provider object will be injected to comply with W3C secure contexts which can be found here https://www.w3.org/TR/secure-contexts/

Explanation

This limits the places where the provider object will be injected to comply with W3C secure contexts which can be found here https://www.w3.org/TR/secure-contexts/

More Information

See EIP-5593

Before

the provider object was able to be injected in non-secure contexts and now it's limited to fewer places where it can be injected.

After

Please be aware there is some limitations here that can break things, so please consider this or offer feedback of how we should address this in the ethereum magicians thread.

Manual Testing Steps

See EIP-5593 which defines where it can be used. I'll be authoring a test suite for it soon as well to make it easier to test compliance.

Pre-Merge Checklist

• PR template is filled out
• IF this PR fixes a bug, a test that would have caught the bug has been added
• PR is linked to the appropriate GitHub issue
• PR has been added to the appropriate release Milestone

+ If there are functional changes:

• Manual testing complete & passed
• "Extension QA Board" label has been applied

[github-actions]

CLA Signature Action:

Thank you for your submission, we really appreciate it. We ask that you read and sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just by adding a comment to this pull request with this exact sentence:

I have read the CLA Document and I hereby sign the CLA

By commenting with the above message you are agreeing to the terms of the CLA. Your account will be recorded as agreeing to our CLA so you don't need to sign it again for future contributions to this repository.

0 out of 1 committers have signed the CLA.
❌ @kdenhartog

[brad-decker]

@kdenhartog thanks for the contribution! In order for us to consider this we'll need you to sign the CLA (see the Github-actions bot comment for instructions). There will likely be an internal discussion on how we'll go about landing and rolling this out but I will keep you posted. From a code perspective looks good, and requires little review. The window.isSecureContext API is well supported. Thanks again.

[kdenhartog]

Sounds good, I'll work on getting the CLA sorted. I think it would be useful to have someone from your team write up your teams conclusions on that ETH magicians thread to encourage others to implement if your team decides to align with this proposal.

I'm working on getting more wallets aligned with this work so the more teams I can get to publicly support this, or provide feedback around the better it would be.

One thing that Brave does that I can't figure out how to support with extensions is the usage of the "allow" attribute to extend the Permissions API. This would be useful for a top level window to be able to allow a third party iframe to be considered a secure context because the top level window is granting it access. If anyone on your team can figure this out that would be awesome for enhancing the backwards comparability, but from my reading of the extension documents there's nothing possible at the moment.

[rekmarks]

I think we should probably add a setting as suggested in the EIP to toggle the behavior, just to keep developers relying on the current implementation happy. I suggest the following implementation:

• The setting should:
  • be a boolean.
  • live in the "Advanced" settings page.
    • This probably means that the state should live in the PreferencesController, but don't take my word for that.
  • have a specific name, maybe "Allow Insecure Provider Injection".
  • comply with EIP-5593 by default for all installs.
• We should also note this change in a "What's New?" notification and our documentation (this work can be done separately).
• The value of the setting will have to be communicated to the content script. This is potentially the most involved part, because IIRC we don't have any ergonomic abstractions in place for modifying the behavior of the content script based on application state.

Please consult @coreyjanssen re: the naming.

[kdenhartog]

Thanks for taking a look!

Should we update the EIP to align the toggle ability to do a complete disable then rather than just the localhost disable?

Ideally I'd like to see that not done because I suspect that DApps that are injecting 3P iframes are more likely to encourage their users to disable this check which is the biggest concern I'd have with doing that.

However, without the permissions API extensibility for extensions we probably have to do this for the time being until we can extend permissions API for extensions or find another way to allow 3P frames. Anything else like non-https I don't think we want to encourage at all since that would allow MITM attacks to tamper with transactions.

[brad-decker]

@kdenhartog curious about the logic here because I see where you indicate that the override is only for localhost, here:

https://github.com/ethereum/EIPs/pull/5593/files#diff-988509a7775edd935e28ec73219554c2c3a5713b53029e79b7188eebe93230d4R46

But curiously on this line https://github.com/ethereum/EIPs/pull/5593/files#diff-988509a7775edd935e28ec73219554c2c3a5713b53029e79b7188eebe93230d4R32 its stated explicitly that localhost IS considered a secure context. These lines seem to be in conflict with one another.. if localhost is considered a secure context, then why is an override necessary to make it be considered secure?

We should fully define what we expect and then modify this PR to reflect that, whether its a full override as per @rekmarks or if it acts to extend the definition of secure context to localhost.

Thanks!

[kdenhartog]

localhost is an odd origin within the secure context spec because it says that it MAY be secure, but is not guaranteed to be. In the testing that I've done with it chrome, firefox, and safari all seem to treat it as one even with a http:// scheme. However, since this appears to have some odd logic around it, I wanted to also specify how it should be worked around in the event that people may hit this edge depending on underlying implementation logic of the browsers for the window.isSecureContext API.

[github-actions]

This PR has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 14 days. Thank you for your contributions.

[kdenhartog]

I should be able to get the CLA signed no big deal, but I think we may want to hold off on this until we figure out how to update 5593 based on changes made in 6963.

[github-actions]

This PR has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 14 days. Thank you for your contributions.

[github-actions]

This PR was closed because there has been no follow up activity in the last 14 days. Thank you for your contributions.

[kdenhartog]

No concerns on this being closed for now. EIP-5593 has gone stagnant at this point and will need to be revitalized later. I'll re-open at that point.