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

replace location instead of navigating to new url #162

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rickytranmer
Copy link

Removes back button functionality when New Tab is opened.

@jimschubert
Copy link
Owner

Thanks for the PR, and sorry for the delay. I'm moving into my first home in a few weeks, and things have been crazy.


TLDR;

This is all a very lengthy explanation to ask… would it be possible to put the .replace change behind an option which clearly defines the option as removing New Tab Redirect from history upon redirection? I think such a change would address my concerns discussed below about changing this for all users, and would protect me from the wrath of Google for not making a background-only extension "not visible enough" to users. Read on to understand the concerns.


I didn't merge immediately because I wasn't 100% certain if location.replace() would maintain information about the current frame… and therefore give web pages access to extension internals or chrome APIs that the extension has access to. So, I checked Chrome's source code for Location.replace:

void Location::replace(LocalDOMWindow* current_window,
                       LocalDOMWindow* entered_window,
                       const USVStringOrTrustedURL& stringOrUrl,
                       ExceptionState& exception_state) {
  String url = GetStringFromTrustedURL(stringOrUrl, current_window->document(),
                                       exception_state);
  if (!exception_state.HadException()) {
    SetLocation(url, current_window, entered_window, &exception_state,
                SetLocationPolicy::kReplaceThisFrame);
  }
}

And compared it to that for Location.setHref:

void Location::setHref(LocalDOMWindow* current_window,
                       LocalDOMWindow* entered_window,
                       const USVStringOrTrustedURL& stringOrUrl,
                       ExceptionState& exception_state) {
  String url = GetStringFromTrustedURL(stringOrUrl, current_window->document(),
                                       exception_state);
  if (!exception_state.HadException()) {
    SetLocation(url, current_window, entered_window, &exception_state);
  }
}

The only difference is SetLocationPolicy::kReplaceThisFrame passed to SetLocation which just changes from WebFrameLoadType::kStandard to WebFrameLoadType::kReplaceCurrentItem. As you can see below, the navigation will work exactly the same way and the only apparent difference is the stack frames which define history:

  WebFrameLoadType frame_load_type = WebFrameLoadType::kStandard;
  if (set_location_policy == SetLocationPolicy::kReplaceThisFrame)
    frame_load_type = WebFrameLoadType::kReplaceCurrentItem;
  dom_window_->GetFrame()->ScheduleNavigation(*current_window->document(),
                                              completed_url, frame_load_type,
                                              UserGestureStatus::kNone);

Digging into the navigation scheduler, it looks like replace will schedule the request as a redirect rather than a standard navigation:

void NavigationScheduler::ScheduleFrameNavigation(
    Document* origin_document,
    const KURL& url,
    WebFrameLoadType frame_load_type) {
  if (!ShouldScheduleNavigation(url))
    return;

  if (MustReplaceCurrentItem(frame_))
    frame_load_type = WebFrameLoadType::kReplaceCurrentItem;

  base::TimeTicks input_timestamp = InputTimestamp();
  // If the URL we're going to navigate to is the same as the current one,
  // except for the fragment part, we don't need to schedule the location
  // change. We'll skip this optimization for cross-origin navigations to
  // minimize the navigator's ability to execute timing attacks.
  if (origin_document->GetSecurityOrigin()->CanAccess(
          frame_->GetDocument()->GetSecurityOrigin())) {
    if (url.HasFragmentIdentifier() &&
        EqualIgnoringFragmentIdentifier(frame_->GetDocument()->Url(), url)) {
      FrameLoadRequest request(origin_document, ResourceRequest(url), "_self");
      request.SetInputStartTime(input_timestamp);
      if (frame_load_type == WebFrameLoadType::kReplaceCurrentItem)
        request.SetClientRedirect(ClientRedirectPolicy::kClientRedirect);
      frame_->Loader().StartNavigation(request, frame_load_type);
      return;
    }
  }

  Schedule(ScheduledFrameNavigation::Create(origin_document, url,
                                            frame_load_type, input_timestamp));
}

This concerns me slightly for a few reasons:

  • with ~700k users, and no data gathering at all… I can't trust that the URLs which users have defined in the extension's options aren't laying in wait to exploit different techniques of client redirection in some way (specific to Chrome newtab override extensions)
  • redirection in the extension should be "as fast as possible". This would change every redirect to a client redirect and modify the history stack frame, potentially impacting the perceived speed of all users
  • removing New Tab Redirect from history removes the prominence and visibility that the extension was responsible for redirection.

A couple years ago, the extension was threatened to be taken down because it wasn't obvious enough to users that:

  1. The extension was installed
  2. Redirection occurred as a result of the extension.

Here's the text of that email:


Dear Developer,

Protecting users and user choice have been long-standing principles of Google's core philosophy. That is why three years ago, Chrome released an extension-based Settings Overrides API for Chrome on Windows that ensures all users have notice and control over any settings change that take place in their browser. Earlier this year, we implemented the same API for macOS.

Starting August 1, the only compliant way to programmatically change the startup page, homepage, search provider setting, or any other setting that has an API is via the appropriate API. If your extension affects changes to any of these functions, it must use the API.

Extensions that have been using the Webrequest API (or other mechanisms) to redirect search requests from the new tab page or the omnibox, must now use the search provider and/or new tab page API. After August 1st, extensions redirecting search requests from the new tab page or omnibox without using these APIs will be removed from the store.

Extensions that perform automatic redirections from a web page URL must display a prominent notice informing the user of the redirection. For example, if an extension redirects search queries from www.google.com (or any other search provider), the extension's notice must:

  • Be prominent: large, conspicuous, and close in proximity to the search box and on the search engine results page
  • Be timely: shown prior to the user executing the search
  • Be persistent: the prominent notice (including the name) cannot automatically disappear or be dismissable; it must show up every time, not just prior to the first redirect

Must show the name of the extension (and the brand of search engine, if different) and what will happen

Not look similar to a system or Chrome dialog box or user-interface element; for example, disclosures in yellow bars that look similar to Chrome's yellow ("butter") bars are not permitted.

Extensions must also comply with other Chrome Web Store policies and Google's Unwanted Software Policy.

Sincerely,

Chrome Web Store Team
You have received this mandatory service announcement to update you about important changes to your account.

© 2016 Google Inc., 1600 Amphitheatre Parkway, Mountain View, CA 94043, USA


So, as you can see modifying New Tab Redirect in this way would potentially violate the rules around timely and persistent behavior. As mentioned in the TLDR at the top of this comment, I think wrapping this functionality in an option would address all concerns. It does add an object hash lookup and if check but I think this would have minimal impact on a majority of users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants