The aim of CPluginClass::GetBrowserUrl looks wrong.

[sergz]

Trying to understand the aim of CPluginClass::GetBrowserUrl, there is only one usage of it in PluginClass.cpp, the code is

  case WM_LBUTTONUP:
  case WM_RBUTTONUP:
    {
      CString strURL = pClass->GetBrowserUrl();
      if (strURL != pClass->GetTab()->GetDocumentUrl())
      {
        pClass->GetTab()->SetDocumentUrl(strURL);
      }

When one clicks on our icon we check whether the browser url is equal to the actual current url and if it's not equal we set document url to the value of browser url. I could not reproduce it (not equal case) so far, if you know how to do it, let me know, please. Even more, I would say if one can reproduce it then here is a possibility for phishing attack. The idea looks clear, if the user sees the current browser url (which can be different from the url of the current page) we are going to use the browser url instead of actual url to avoid questions from users about different urls. But I would say that it's bad for the user, it's better to use the real url instead of some fake url of browser.

If the above is correct, we might should fix the bahaviour. After it there is only one reason to keep CPluginClass::GetBrowserUrl is to inform user about possible phishing attack.

[oleksandr]

This is done in case a user decides to navigate to some webpage and then that request is canceled for whatever reason. Then document URL would already have new URL, but actually the page currently navigated to would still remain with an old URL.
It is also possible, that the page will have multiple

I don't see anything wrong with this approach, but neither do I feel very strongly to stick with it. What definitely seems wrong, is that we shouldn't set SetDocumentUrl when user right clicks the icon.

Can you please elaborate on the possible phishing attack on this?

[eric@adblockplus.org]

document URL would already have new URL, but actually the page currently navigated to would still remain with an old URL.

If this is really possible, then it's a defect in the behavior of GetDocumentUrl(). It should always return the URL of the document that's rendered on the page, not some artifact of browsing history.

We're calling SetDocumentUrl() in OnNavigate(), and apparently that's too soon.