EIPs icon indicating copy to clipboard operation
EIPs copied to clipboard

Add EIP-5593: Restrict Web3 Provider Object API Injection

Open kdenhartog opened this issue 3 years ago • 10 comments
trafficstars

When opening a pull request to submit a new EIP, please use the suggested template: https://github.com/ethereum/EIPs/blob/master/eip-template.md

We have a GitHub bot that automatically merges some PRs. It will merge yours immediately if certain criteria are met:

  • The PR edits only existing draft PRs.
  • The build passes.
  • Your GitHub username or email address is listed in the 'author' header of all affected PRs, inside .
  • If matching on email address, the email address is the one publicly listed on your GitHub profile.

cc @lightclient @axic @SamWilsn @MicahZoltu this is my first time submitting an EIP so if there's anything additional that needs to be done here please let me know.

kdenhartog avatar Sep 04 '22 22:09 kdenhartog

A critical exception has occurred: Message: pr 5593 is already merged; quitting (cc @alita-moore, @mryalamanchi)

eth-bot avatar Sep 04 '22 22:09 eth-bot

I just want to chime in to thank @kdenhartog for their excellent work here and that we will follow up on the PR to the MetaMask extension to get it merged.

rekmarks avatar Sep 06 '22 02:09 rekmarks

Thanks @rekmarks credit definitely goes to @diracdeltas and @bbondy for the idea and implementation. I got the easy job of turning the docs into an EIP. 😄

kdenhartog avatar Sep 06 '22 02:09 kdenhartog

@kdenhartog mind adding these files to the directory I just created? https://github.com/kdenhartog/EIPs/pull/1

Pandapip1 avatar Sep 06 '22 18:09 Pandapip1

@kdenhartog mind adding these files to the directory I just created? kdenhartog#1

Thank you for the PR to consolidate this into PDFs. I'll leave them in for now, but leave the links unchanged at the moment. If we decide that linking isn't going to be allowed in the #5597 discussion then we can update everything accordingly. That work for you?

My preference would be to link to these documents at this point because the TR directory at W3C has a very rigorous process for updating it and along with that they've built in good processes for how specs are edited with errata and new versions. The chromium links need to be updated to a better link as well. If we go towards the PDF route we end up having to maintain these PDFs rather than being able to lean on W3C's processes (or other SDOs like WHATWG) which handle these aspects well. Especially since some of these things like the permissions API are actively being worked on at the moment and require that I normatively reference them for an additional normative MAY I'm working on adding right now.

As an example, here's a spec for XML which was published in 2008, so going on 14 years now as a stable URL. It includes notes about links being updated and errata which are not available with the PDF approach. In my opinion using links for the W3C spec is actually an advantage here. If we were going to link to Chromium docs, I'd be more open to it, but even in that case we can lean on commits

kdenhartog avatar Sep 07 '22 07:09 kdenhartog

If we decide that linking isn't going to be allowed in the https://github.com/ethereum/EIPs/issues/5597 discussion then we can update everything accordingly. That work for you?

It would be my preference for you to just link to the PDFs anyway - it's also likely to get your EIP merged faster.

Pandapip1 avatar Sep 07 '22 16:09 Pandapip1

Currently, the policy on links is very restrictive - and for a pretty good reason. I'm leaning toward allowing some external specifications (as long as they have a PDF backup), but this would have to be an EIP-1 policy.

Pandapip1 avatar Sep 07 '22 16:09 Pandapip1

@SamWilsn found a way to bypass the relative links linting of the bot 🙃

image

kdenhartog avatar Sep 23 '22 04:09 kdenhartog

I've updated this to use markdown reference links and have reduced every link to either W3C or an in repo link (other than the eth magicians link at the header). I'm not certain @SamWilsn and @Pandapip1 are acceptable to this though, but I've not been tracking the latest EIP editors discussions that have been considering this.

An alternative option for this is for me to create a Citations PDF in the assets folder to address this linking concern as an alternative method to not need to directly link from the EIP.

kdenhartog avatar Oct 19 '22 02:10 kdenhartog

@Pandapip1 I'm betting my changes to the regex to address HTML links not being caught by the eipw-lint lib is what's causing these. This is assuming those changes have been deployed to the GH action. @SamWilsn it might be worth reverting those changes out for the time being.

kdenhartog avatar Oct 19 '22 19:10 kdenhartog

Small editorial changes made and also removed the optional test cases

kdenhartog avatar Oct 25 '22 21:10 kdenhartog

Opened https://github.com/kdenhartog/EIPs/pull/2 with my suggestions.

SamWilsn avatar Oct 28 '22 17:10 SamWilsn

@SamWilsn I've reviewed https://github.com/kdenhartog/EIPs/pull/2 and updated to try and and make it work as much as possible. FWIW, I just realized there's no way to link to a reference implementation right now either so I'm probably going to have to drop this section to merge it too.

kdenhartog avatar Nov 03 '22 01:11 kdenhartog

Once again the EIPW bot does not reflect language in the EIP-1 document. It's not a requirement for the sections to exactly match the template according to EIP-1. See here for details: https://eips.ethereum.org/EIPS/eip-1#eip-formats-and-templates. No where does it state that the EIP MUST match the template. Additionally, it's throwing errors on the recommended template for licensing.

If the bot is going to be the ultimate arbiter of truth then there's no point in having EIP-1 in my opinion and there should be more than @SamWilsn implementing and reviewing changes to the bot if so. Right now governance of this repo is being unilaterally managed by the EIP Walidator bot, not EIP-1.

kdenhartog avatar Nov 03 '22 01:11 kdenhartog

I'm sorry you've been having an unpleasant time with the bot and the process in general. I'm happy to try and help get your PR merged as much as I am able.

Once again the EIPW bot does not reflect language in the EIP-1 document. It's not a requirement for the sections to exactly match the template according to EIP-1. See here for details: https://eips.ethereum.org/EIPS/eip-1#eip-formats-and-templates. No where does it state that the EIP MUST match the template.

eipw enforces the generally agreed upon consensus of editors, and EIP-1 lays out the hard rules of the process. Where EIP-1 does allow leeway eipw probably doesn't. That's somewhat intentional. For the majority of cases, we expect that the rules in eipw will be followed. In the remaining exceptional cases, EIP editors can override eipw.

You'll note that EIP-1 says "Each EIP should have the following parts:", and lists the sections you should include. eipw enforces those rules but if an editor decides to allow it, you could theoretically have other sections. In practice, it's very hard to convince editors to override those rules.

Additionally, it's throwing errors on the recommended template for licensing.

Yes, that's a bug: https://github.com/ethereum/eipw/issues/51

Sorry about that! It looks like there's an http://localhost without backticks on line 78.

If the bot is going to be the ultimate arbiter of truth then there's no point in having EIP-1 in my opinion and there should be more than @SamWilsn implementing and reviewing changes to the bot if so. Right now governance of this repo is being unilaterally managed by the EIP Walidator bot, not EIP-1.

eipw is just a tool to catch the most common problems we see. The ultimate arbiter of truth is and will always be the rough consensus of EIP editors.

SamWilsn avatar Nov 08 '22 00:11 SamWilsn

Thank you Sam Wilson! And the gang, I apologize for my novice understanding....I do sincerely thank you for helping me, I will implement the things you know as I clearly do not..my phone was completely hacked and erased 2 days ago so I've been trying to recover all lost data....these advasaries are in every app program I have.....I usually have about 20 to 30 k trackers on me at all times....You guys are the best and thank you again for your gnosis. JCL

On Mon, Nov 7, 2022, 6:13 PM Sam Wilson @.***> wrote:

I'm sorry you've been having an unpleasant time with the bot and the process in general. I'm happy to try and help get your PR merged as much as I am able.

Once again the EIPW bot does not reflect language in the EIP-1 document. It's not a requirement for the sections to exactly match the template according to EIP-1. See here for details: https://eips.ethereum.org/EIPS/eip-1#eip-formats-and-templates. No where does it state that the EIP MUST match the template.

eipw enforces the generally agreed upon consensus of editors, and EIP-1 lays out the hard rules of the process. Where EIP-1 does allow leeway eipw probably doesn't. That's somewhat intentional. For the majority of cases, we expect that the rules in eipw will be followed. In the remaining exceptional cases, EIP editors can override eipw.

You'll note that EIP-1 says "Each EIP should have the following parts:", and lists the sections you should include. eipw enforces those rules but if an editor decides to allow it, you could theoretically have other sections. In practice, it's very hard to convince editors to override those rules.

Additionally, it's throwing errors on the recommended template for licensing.

Yes, that's a bug: ethereum/eipw#51 https://github.com/ethereum/eipw/issues/51

Sorry about that! It looks like there's an http://localhost without backticks on line 78.

If the bot is going to be the ultimate arbiter of truth then there's no point in having EIP-1 in my opinion and there should be more than @SamWilsn https://github.com/SamWilsn implementing and reviewing changes to the bot if so. Right now governance of this repo is being unilaterally managed by the EIP Walidator bot, not EIP-1.

eipw is just a tool to catch the most common problems we see. The ultimate arbiter of truth is and will always be the rough consensus of EIP editors.

— Reply to this email directly, view it on GitHub https://github.com/ethereum/EIPs/pull/5593#issuecomment-1306401400, or unsubscribe https://github.com/notifications/unsubscribe-auth/A3H6XKZ74L3JFOKUK322KHTWHGLKVANCNFSM6AAAAAAQEQKLRU . You are receiving this because you are subscribed to this thread.Message ID: @.***>

Cryptonics1 avatar Nov 08 '22 00:11 Cryptonics1

I appreciate the thoughtful and level-headed response as you always bring to the table @SamWilsn. I presume in this case the 4 ones about the license can be overridden. Is the one about the privacy considerations one that you're willing to do as well since it's a two hash header rather than 3? If not, I'll change it to a subsection of security for now as you've already made a huge effort on getting the links in place which was far more important to me. We can save the privacy considerations as a top level header for another day and merge this one.

kdenhartog avatar Nov 08 '22 05:11 kdenhartog

I presume in this case the 4 ones about the license can be overridden.

I think there are still external links? Three in the reference implementation, and the one on 78 I mentioned earlier.

Is the one about the privacy considerations one that you're willing to do as well since it's a two hash header rather than 3? If not, I'll change it to a subsection of security for now [...]

You should move it to be a subsection of security considerations.

SamWilsn avatar Nov 08 '22 07:11 SamWilsn

Thank you , You guys are top knotch!

On Tue, Nov 8, 2022, 1:18 AM Sam Wilson @.***> wrote:

I presume in this case the 4 ones about the license can be overridden.

I think there are still external links? Three in the reference implementation, and the one on 78 I mentioned earlier.

Is the one about the privacy considerations one that you're willing to do as well since it's a two hash header rather than 3? If not, I'll change it to a subsection of security for now [...]

You should move it to be a subsection of security considerations.

— Reply to this email directly, view it on GitHub https://github.com/ethereum/EIPs/pull/5593#issuecomment-1306741014, or unsubscribe https://github.com/notifications/unsubscribe-auth/A3H6XK26H4TLJM7U7VVTZMLWHH5DVANCNFSM6AAAAAAQEQKLRU . You are receiving this because you commented.Message ID: @.***>

Cryptonics1 avatar Nov 08 '22 07:11 Cryptonics1

I presume in this case the 4 ones about the license can be overridden.

I think there are still external links? Three in the reference implementation, and the one on 78 I mentioned earlier.

Is the one about the privacy considerations one that you're willing to do as well since it's a two hash header rather than 3? If not, I'll change it to a subsection of security for now [...]

You should move it to be a subsection of security considerations.

image

Ok, I'll try to go a few more rounds with this thing and see if I can figure it out. I see a few more links in this EIP (implementation ones for example), so I presume it's just flagging the wrong line then and I miss understood https://github.com/ethereum/eipw/issues/51.

kdenhartog avatar Nov 08 '22 10:11 kdenhartog

@Pandapip1 In the industry a lot of people confused security with privacy, I would suggest as a RFC/XIP type of body, we keep the distinction more obivous?

Shall we add a "Other considerations" to catch all? FYI @kdenhartog

xinbenlv avatar Nov 08 '22 15:11 xinbenlv

That's a good approach that allows the bot to remain more strict to the template while also having a good abstraction point that makes this useful beyond just privacy considerations.

kdenhartog avatar Nov 08 '22 18:11 kdenhartog