Apply editor suggestions #2
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kdenhartog
reviewed
Oct 29, 2022
| - The origin MUST be a [potentially trustworthy origin][w3c-secure-context-trustworthy-origin] to have access to `window.ethereum`. This can be checked using `window.isSecureContext`, including inside iframes. | ||
| - Secure contexts include sites that are served from HTTPS but also HTTP `localhost`. | ||
| - The browser implementation MAY also support configured [potentially trustworthy origins] that would normally not be considered trustworthy if the user configures their browser to do so. See the [Development Environments section of Secure Contexts][w3c-secure-context-dev-env] for additional details. For example, in Chromium based browsers this is done via the `chrome://flags/#unsafely-treat-insecure-origin-as-secure` flag. | ||
| - To have access to `window.ethereum`, the origin MUST be a `potentially trustworthy origin` according to the user agent. This can be checked using `window.isSecureContext`, including inside iframes. |
There was a problem hiding this comment.
This is a bad normative statement. How is an implementer supposed to know what a "potentially trustworthy origin" is without a definition?
| @@ -39,12 +39,12 @@ The provider objects, e.g. `window.ethereum`, are expected to only inject the Et | |||
|
|
|||
| ## Rationale | |||
|
|
|||
| By limiting the capabilities of where the Ethereum Provider APIs are being injected we can reduce the surface area of where attacks can be executed.Given the sensitivity of data that's passed to the Ethereum Provider APIs some basic levels of authentication and confidentiality should be met in order to ensure that request data is not being intercepted or tampered with. While there has been attempts to [limit request access via the wallet](./eip-2255.md) interface itself there's not been limitations that have been set to where these Ethereum Provider APIs are expected to be or not be injected. Since the secure contexts web platform API is a well developed boundary that's been recommended by W3C and the fact that the Ethereum Provider APIs are extending the traditional web platform APIs, no other alternative solutions have been considered in order to extend current established prior art. | |||
| By limiting the capabilities of where the Ethereum Provider APIs are being injected we can reduce the surface area of where attacks can be executed. Given the sensitivity of data that's passed to the Ethereum Provider APIs some basic levels of authentication and confidentiality should be met in order to ensure that request data is not being intercepted or tampered with. While there have been attempts to limit request access via the wallet interface itself, there have not been limitations that have been set to where these Ethereum Provider APIs are expected to be or not be injected. Since the secure contexts web platform API is a well developed boundary that's been recommended by W3C and the fact that the Ethereum Provider APIs are extending the traditional web platform APIs, no other alternative solutions have been considered in order to extend current established prior art. | |||
There was a problem hiding this comment.
We can't link to other EIPs now?
| @@ -60,7 +60,7 @@ Wallet extensions SHOULD consider adding a "developer mode" toggle via a UX so t | |||
| - Top level `https://b.com` with `<iframe src="https://a.com">` with `<iframe src="https://b.com">` -> blocked (third-party iframe without sufficient privileges) | |||
| - Top level `https://a.com` with `<iframe src="https://sub.a.com">` -> blocked (third-party iframe without sufficient privileges) | |||
| - Top level `https://a.com` with `<iframe src="https://a.com" sandbox>` -> blocked (sandbox attribuute without "allow-same-origin") | |||
| - Top level `https://a.com` with `<iframe src="https://a.com" sandbox="allow-same-origin allow-scripts">` -> allowed (but note this case is discouraged in https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe#attr-sandbox because it’d allow the iframe to remove its own sandbox attribute) | |||
| - Top level `https://a.com` with `<iframe src="https://a.com" sandbox="allow-same-origin allow-scripts">` -> allowed (but note this case is discouraged because it'd allow the iframe to remove its own sandbox attribute) | |||
There was a problem hiding this comment.
How is an implementer expected to better understand why this is a bad idea? Is it just because this EIP declares it so or is it because this was a tradeoff made in the sandbox attribute design?
|
|
||
| ## Privacy Considerations | ||
| ### Privacy Considerations |
There was a problem hiding this comment.
AFAICT in EIP-1 it's acceptable to add additional categories and this is up to the author to determine. Why is the bot enforcing this specific template when it's not followed by all EIPs?
| - The browser implementation MAY also support configured [potentially trustworthy origins] that would normally not be considered trustworthy if the user configures their browser to do so. See the [Development Environments section of Secure Contexts][w3c-secure-context-dev-env] for additional details. For example, in Chromium based browsers this is done via the `chrome://flags/#unsafely-treat-insecure-origin-as-secure` flag. | ||
| - To have access to `window.ethereum`, the origin MUST be a `potentially trustworthy origin` according to the user agent. This can be checked using `window.isSecureContext`, including inside iframes. | ||
| - Secure contexts include sites that are served from HTTPS but also HTTP `localhost`. | ||
| - The user agent MAY also support configured "potentially trustworthy origins" that would normally not be considered trustworthy. For example, in Chromium based browsers this is done via the `chrome://flags/#unsafely-treat-insecure-origin-as-secure` flag. |
There was a problem hiding this comment.
This changes the meaning of this statement to leave it ambiguous. Who's configuring it this way? Is it done by default by the software developers or by the user?
kdenhartog
pushed a commit
that referenced
this pull request
Mar 15, 2023
* added EIP draft for private key encapsulation * minor updates to spec: intake function shall return the Ethereum address of the private key * added test vector #1 * minor formatting * minor edits * added test vector #2 and #3, added signature verification data to #1 * changed signature to against byte values * added test vector generator * renamed file to assigned EIP number * fixed file header * updated default value for salt * fixed offending links etc. * fixed typo Co-authored-by: xinbenlv <[email protected]> * updated based on review comments * replaced json formatting with none for better rendering * fixed grammar Co-authored-by: xinbenlv <[email protected]> * fixed grammar Co-authored-by: xinbenlv <[email protected]> * revision suggestions taken with gratitudes Co-authored-by: xinbenlv <[email protected]> * revision suggestions taken with gratitudes Co-authored-by: xinbenlv <[email protected]> * fixed grammar Co-authored-by: xinbenlv <[email protected]> * fixed grammar Co-authored-by: xinbenlv <[email protected]> * fixed grammar Co-authored-by: xinbenlv <[email protected]> * fixed grammar Co-authored-by: xinbenlv <[email protected]> * fixed grammar Co-authored-by: xinbenlv <[email protected]> * fixed grammar Co-authored-by: xinbenlv <[email protected]> * revision suggestions taken with gratitudes Co-authored-by: xinbenlv <[email protected]> * fixed grammar as suggested Co-authored-by: xinbenlv <[email protected]> * revision suggestions taken with gratitudes Co-authored-by: xinbenlv <[email protected]> * fixed grammar as suggested Co-authored-by: xinbenlv <[email protected]> * fixed grammar as suggested Co-authored-by: xinbenlv <[email protected]> * fixed grammar as suggested * fixed based on grammarly.com suggestions * Update EIPS/eip-6051.md Co-authored-by: Pandapip1 <[email protected]> * Update EIPS/eip-6051.md Co-authored-by: Pandapip1 <[email protected]> * Update EIPS/eip-6051.md Co-authored-by: Pandapip1 <[email protected]> * replacing bold fonts with links as suggested * fixed dead links * fixed markdown linter errors Co-authored-by: xinbenlv <[email protected]> Co-authored-by: Pandapip1 <[email protected]>
|
Closing this since I think we found a resolution for 5593 for now |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The changes necessary to pass
eipw(mostly removing external links, and changing section levels) need to be applied before merging your PR.The rest should just be minor grammar changes.