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

ENS Address preservation when calling URLs #7740

Closed
wants to merge 1 commit into from
Closed

ENS Address preservation when calling URLs #7740

wants to merge 1 commit into from

Conversation

vasapower
Copy link

It could be extremely useful to have more ENS domains to point to the same IPFS/Swarm/every-other-distributed-file-system destination.
But how to find the original source call?
Inserting the original ens domain name as query param is the best method I found,
I hope you will appreciate the idea because me and my team really need it.

Thank you very much

@vasapower
Copy link
Author

vasapower commented Jan 9, 2020

Hello, @whymarrh, @tmashuang . Do you think this code change can be the best solution to obtain this result?

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

Interesting idea!

Has anybody else taken a similar approach? I'm wary of creating a de-facto standard for dapps served via ENS when one might already exist, or when we can avoid creating one altogether.

@@ -43,10 +43,11 @@ function setupEnsIpfsResolver ({ provider, getIpfsGateway }) {
const ipfsGateway = getIpfsGateway()
extension.tabs.update(tabId, { url: `loading.html` })
let url = `https://app.ens.domains/name/${name}`
const completeSearch = (search || '') + (!search || search.indexOf('?') === -1 ? '?' : '&') + 'ensd=' + name
Copy link
Member

Choose a reason for hiding this comment

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

In addition to checking for existing query params, it might be a good idea to ensure ensd itself isn't already set. If we include it twice, it might not be parsed the way we expect.

Not sure whether we'd want to leave alone a pre-existing ensd parameter or overwrite it though 🤔

Copy link
Author

@vasapower vasapower Jan 14, 2020

Choose a reason for hiding this comment

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

In query parameters, it is allowed to have multiple vars with the same name: it is one of the allowed methods to implicitly declare that the var is an array, as you can see here in the first example. https://medium.com/raml-api/arrays-in-query-params-33189628fa68

As this PR would just be a help to users who could potentially need it, we cannot imagine the scenario they are running: one of those could be that they need ensd as an array var.

So I would not bother to check that ensd is a unique parameter, that is the user's business, we are only concerned with passing it on.

What do you think?

Copy link
Author

@vasapower vasapower left a comment

Choose a reason for hiding this comment

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

Interesting idea!

Has anybody else taken a similar approach? I'm wary of creating a de-facto standard for dapps served via ENS when one might already exist, or when we can avoid creating one altogether.

Really glad you mentioned it! ✌

Unfortunately, I have not found anything that can help preserve the ENS address when querying an instrument such as a Chrome Extension like Metamask or similar.
The idea around this PR was, in fact, to lay the foundations for a de-facto standard, starting with Metamask (which is the main tool) and expanding it as much as possible.

If someone of you would like to make this happen I am willing to actively collaborate.
I really want to discuss it seriously because it's extremely useful for the growth of the Ethereum eco-system.

@Arachnid
Copy link

The ideal option would be to not redirect at all, preserving the host header. Since that's not presently possible, is it possible to include an http header with requests instead?

@Gudahtt
Copy link
Member

Gudahtt commented Jan 16, 2020

That would make sense for communicating to the server which ENS domain was used, but I suspect the goal in this case is to inform the dapp of the ENS address used. The dapp wouldn't have any information about what headers were sent to the server.

There may still be other options for informing the dapp. We could store it in localStorage (on the gateway origin, 'ipfs.dweb.link' by default), or we could set it in a cookie. I'm not enthusiastic about any of these options though.

@Arachnid
Copy link

Good point, I'd forgotten JS can't access HTTP headers for the request.

Storing it somewhere that's accessible to the dapp via JS seems better than adding it to a query string, to me.

@vasapower vasapower requested a review from Gudahtt January 16, 2020 07:18
@vasapower
Copy link
Author

@Gudahtt @Arachnid

Yeah I don't think that Cookie or local/session storage is a clean solution, it could be really a pain to keep the cache consisntent.

I followed this approach just for 3 reasons:

  • It's just to inform the destination page about the origin of the call for any reason, so it can use the queryParam as custom behavior instead of the default one. Even because the user can easily call the IPFS link inserting custom ensd directly form URL bar, bypassing ENS totally;
  • It's the same trick used by social networks to follow links interactions outside the platforms (e.g. Facebook adds ?ref=fbplbo&fbclid=xxx... to the post URL), so I followed this example;
  • It was really simple and non invasive to implement in current logic.

I think that the best implementation would be the one also used by GoDaddy's default homepages:

  1. Preserve the ENS link in URL bar (so no more extension.tabs.update(tabId, { url: ... }));

  2. Just render a template HTML page wich contains just the following code in the body:
    <html><body><iframe src="_ENS_REAL_URL_" style="width: 100%; height: 100%; border: none;"></body></html></iframe>;

  3. Before to render the template at point 2., do a string replace of the _ENS_REAL_URL_ with the IPFS/SMARM/Whatever link grabbed from the ENS engine;

  4. In the dApp logic page, if interested, the user will use the following code to retrieve the original ENS domain name: var url = (window.location != window.parent.location) ? document.referrer : document.location.href;

If you like it I can think to do another PR which works like this.

@vasapower
Copy link
Author

Hey @Gudahtt, any news about this PR?

Is there anything I could do?

Many thanks

@Arachnid
Copy link

Arachnid commented Feb 2, 2020

@marcovasapollo If you're able to preserve the ENS URL in the URL bar for an iframe page, can't you do it for the actual content?

RE using an iframe, this doesn't seem ideal because if a user navigates to a new page in the iframe, the URL will not update to reflect that.

@Gudahtt
Copy link
Member

Gudahtt commented Feb 14, 2020

Hey @marcovasapollo , sorry for the delay! I still have some reservations about this solution, and I'd like to try prototyping an alternative solution first - one that would preserve the ENS address in the address bar. I think it should be possible using the proxy API. The proxy API requires an additional manifest permission, which is a problem - we don't want to add more permissions. But we can consider adding it as a runtime permission - so it's off by default, but a user can opt-in if they want ENS resolution.

I should have time to try this in a week or so - I'm pretty busy until then. Feel free to give it a shot yourself in the meantime though, if you want to. If this is feasible with just the runtime proxy permission, I think this would be a better solution. If it would need additional permissions as well (such as webRequestBlocking), we might decide to accept the solution you've proposed instead.

@vasapower
Copy link
Author

@marcovasapollo If you're able to preserve the ENS URL in the URL bar for an iframe page, can't you do it for the actual content?

RE using an iframe, this doesn't seem ideal because if a user navigates to a new page in the iframe, the URL will not update to reflect that.

Hello @Arachnid. I made some experiments and you were right: managing stuff using iframes is really tricky and not applicable.
So we used a new solution you can find in the method window.onload of the page's code at this link: https://tossacointoyourwitcher.eth (use mainnet).

@vasapower
Copy link
Author

Hey @marcovasapollo , sorry for the delay! I still have some reservations about this solution, and I'd like to try prototyping an alternative solution first - one that would preserve the ENS address in the address bar. I think it should be possible using the proxy API. The proxy API requires additional manifest permission, which is a problem - we don't want to add more permissions. But we can consider adding it as a runtime permission - so it's off by default, but a user can opt-in if they want ENS resolution.

I should have time to try this in a week or so - I'm pretty busy until then. Feel free to give it a shot yourself in the meantime though, if you want to. If this is feasible with just the runtime proxy permission, I think this would be a better solution. If it would need additional permissions as well (such as webRequestBlocking), we might decide to accept the solution you've proposed instead.

Hey @Gudahtt, if I'm understanding you correctly, you are thinking on a solution like this:

  1. Add proxy as fixed permission in the manifest.json file (as the official permissions documentation says, it's not possible to use proxy in the optional_permissions field);
  2. Insert into Metamask settings panel the possibility for the user to enable/disable the preservation of the ENS link in the address bar (default is disabled);
  3. So the option at point 2. will trigger the business logic which uses the proxy API to start/stop the silent redirection;

An example of the business logic I have in mind is similar to this:

var config = {
    mode: "pac_script",
    pacScript: {
        data: "async function FindProxyForURL(url, host) {\n" +
            "  /*TODO: Insert here all the async logic\n" +
            "   * which is now present in webRequestDidFail\n" +
            "   * and attemptResolve methods into the actual ENS resolver file.*/\n" +
            "  return `PROXY ${resolvedENSName}`;\n" +
            "}"
    }
};
chrome.proxy.settings.set({value: config, scope: 'regular'}, function() {});

Need to investigate if the pacScript field can accept async functions because we need to call the blockchain to resolve the correct ENS redirection before to continue the call.

Otherwise, I think that the webRequestBlocking additional permission must be used to keep the request locked until the termination of the ENS resolution, hoping that it can be used in conjunction with the proxy API.

I will investigate for sure.
I am only concerned that this is a too specific solution and difficult to adopt by all other systems besides Metamask.

@Gudahtt
Copy link
Member

Gudahtt commented Feb 14, 2020

it's not possible to use proxy in the optional_permissions field

Whoops, that throws a wrench in my plan. Good find though - thanks for looking into this!

I'll bring this up with the rest of the team. We're pretty hesitant to add new permissions in general, so this might be a non-starter.

Insert into Metamask settings panel the possibility for the user to enable/disable the preservation of the ENS link in the address bar (default is disabled);

If we were going to support address preservation at all, we'd likely make that the only option. We might want to allow disabling ENS resolution altogether, but the current resolver seems strictly worse than one that would preserve the address.

Need to investigate if the pacScript field can accept async functions because we need to call the blockchain to resolve the correct ENS redirection before to continue the call.

Not necessarily - we could proxy anything with a .eth tld, regardless of whether it's a real address. We can check for a .eth tld synchronously.

@Gudahtt
Copy link
Member

Gudahtt commented Feb 14, 2020

I am only concerned that this is a too specific solution and difficult to adopt by all other systems besides Metamask.

Which other systems do you think would benefit from the query-parameter style solution you proposed? I know of a few others that preserve the address - I don't know of any offhand that work like ours.

@vasapower
Copy link
Author

@Gudahtt

Not necessarily - we could proxy anything with a .eth tld, regardless of whether it's a real address. We can check for a .eth tld synchronously.

This sounds cool even because the execution scope of the pacScript code would be sandboxed for sure (e.g. no possibility to do remote requests or other stuff).

Which other systems do you think would benefit from the query-parameter style solution you proposed? I know of a few others that preserve the address - I don't know of any offhand that work like ours.

I'm talking about all the other applications with ENS-browsing capabilities (mobile applications, other browsers).
If I write, for example, an HTML page that uses the calling ENS domain to dynamically customize its context, put it on IPFS, put the resulting hash in my ENS domain's resolver, it would be great for users to have the same user experience on every application, not only in Metamask.
So, if the ENS preservation logic is simple, general-purpose and easy to implement, every application would easily adopt it as a de-facto standard.

BTW can you tell me which are applications that already do ENS preservation? I don't know any. (Feel free to contact me on telegram @vasapower if you cannot write them here for any reason).

@Gudahtt
Copy link
Member

Gudahtt commented Feb 23, 2020

BTW can you tell me which are applications that already do ENS preservation? I don't know any. (Feel free to contact me on telegram @vasapower if you cannot write them here for any reason).

When I looked around for "ENS browsers", the first couple of examples I found both preserved ENS addresses: ENS Gateway, Opera. It seems to be the approach that is preferred by the ENS team itself as well, and it's clearly better for users.

At this point I'm inclined to not accept this PR, but instead pursue ENS address preservation. We have a few releases lined up that we'd like to get out first, but after that we might be able to ship an improved ENS resolver that preserves the ENS name in the address bar. It would require adding the proxy permission, which is a fairly disruptive for users and something we do not take lightly, but I think it would be worth it.

I'll see what the rest of the team thinks first though; we might accept this anyway in the meantime. Either way, thanks for implementing this and pushing the discussion forward!

@vasapower
Copy link
Author

BTW can you tell me which are applications that already do ENS preservation? I don't know any. (Feel free to contact me on telegram @vasapower if you cannot write them here for any reason).

When I looked around for "ENS browsers", the first couple of examples I found both preserved ENS addresses: ENS Gateway, Opera. It seems to be the approach that is preferred by the ENS team itself as well, and it's clearly better for users.

At this point I'm inclined to not accept this PR, but instead pursue ENS address preservation. We have a few releases lined up that we'd like to get out first, but after that we might be able to ship an improved ENS resolver that preserves the ENS name in the address bar. It would require adding the proxy permission, which is a fairly disruptive for users and something we do not take lightly, but I think it would be worth it.

I'll see what the rest of the team thinks first though; we might accept this anyway in the meantime. Either way, thanks for implementing this and pushing the discussion forward!

Glad to be of inspiration!

I already tried the Opera Beta Browser (regular does not have ENS support) and it still not work, but maybe that is the correct way. ENS GW is actually in beta too (and their website does not work, maybe abandoned project?) so I think that your idea to use queryParam in the meantime can be a good placeholder.

@AlicanC
Copy link

AlicanC commented Jul 13, 2021

I've had a different take on this where we redirect to myname-eth.ipns.gateway instead of adding a search param. Please see #11520, reviews are welcome.

@github-actions
Copy link
Contributor

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.
@vasapower

@danfinlay
Copy link
Contributor

One drawback of this approach is that all ENS related names share a common localStorage and cookie namespace.

I think that security issue makes me considerably prefer approaches like are discussed in #9353, and I'm not sure this change is worth the benefit before a more complete change like that.

I'm going to close this for now, because we do plan to implement one of those approaches, and so I don't think it makes sense to continue leaving this one open.

@danfinlay danfinlay closed this Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants