Page 1 of 1

InternalError: Too much recursion

Posted: Sat Jun 05, 2021 4:58 am
by jdigital
Downloading a Tableau workbook from Tableau Public generally results in infinite recursion, see attached screenshot showing modifyWindowOpenMethod from patchWindow.js. This happens with 11.2.8 and 11.2.9rc1.

screenshot: https://imgur.com/a/TjMXuMF

Repro steps:
visit https://public.tableau.com/en-us/galler ... bage-patch
scroll down, click on one of the charts, such as the bar chart ("Frequency of Polymer Type")
at the bottom of the dashboard (the colored pane), there's a toolbar with icons.
click on the icon that looks like a download icon
in the popup list, select Tableau Workbook
in the popup dialog, click Download
generally this will produce the error condition seen in the screenshot

Firefox 78.11.0 ESR

Re: InternalError: Too much recursion

Posted: Sat Jun 05, 2021 10:08 pm
by Giorgio Maone
Investigating, thank you.

Re: InternalError: Too much recursion

Posted: Tue Jun 08, 2021 1:26 pm
by skriptimaahinen
Was wondering if this would turn out to be a problem...

Since the patches should not be detectable (in theory), there is no practical way to tell if a window has been already patched. Which means the patch needs to be applied every time the window is accessed through the methods that need patching. Too many calls and the patches form a chain that Firefox will consider as infinite loop and throws an error.

Solution: WeakSet of the patched windows so a window can be tested if it has already been patched.

Caveat: This would prevent calling patchWindow separately for webgl, prefetchCSSResources and media. What is needed is separate function to "register" the patches and another to apply them as one patch.

I'll be posting the code later when I have tested it a bit.

Re: InternalError: Too much recursion

Posted: Thu Jun 10, 2021 6:33 am
by skriptimaahinen
patchWindow.js

Code: Select all

"use strict";
/**
 * Injects code into page context in a cross-browser way, providing it
 * with tools to wrap/patch the DOM and the JavaScript environment
 * and propagating the changes to child windows created on the fly in order
 * to prevent the modifications to be cancelled by hostile code.
 *
 * @param function patchingCallback
 *        the (semi)privileged wrapping code to be injected.
 *        Warning: this is not to be considered a closure, since Chromium
 *        injection needs it to be reparsed out of context.
 *        Use the env argument to propagate parameters.
 *        It will be called as patchingCallback(unwrappedWindow, env).
 * @param object env
 *        a JSON-serializable object to made available to patchingCallback as
 *        its second argument*
 * @returns object port
 *        A Port object to be used to communicate with the privileged content
 *        script, by using port.postMessage(msg, [event]) and
 *        the port.onMessage(msg, event) user-defined callback.
 */

const patches = [];

function patchWindow(patchingCallback, env = {}) {
  let eventId = this && this.eventId || `windowPatchMessages:${uuid()}`;
  let { dispatchEvent, addEventListener } = window;

  function Port(from, to) {
    // we need a double dispatching dance and maintaining a stack of
    // return values / thrown errors because Chromium seals the detail object
    // (on Firefox we could just append further properties to it...)
    let retStack = [];

    function fire(e, detail, target = window) {
      dispatchEvent.call(target, new CustomEvent(`$eventId:$e`, {detail, composed: true}));
    }
    this.postMessage = function(msg, target = window) {
      retStack.push({});
      let detail = msg;
      fire(to, detail, target);
      let ret = retStack.pop();
      if (ret.error) throw ret.error;
      return ret.value;
    };
    addEventListener.call(window, `$eventId:$from`, event => {
      if (typeof this.onMessage === "function" && event.detail) {
        let ret = {};
        try {
          ret.value = this.onMessage(event.detail.msg, event);
        } catch (error) {
          ret.error = error;
        }
        fire(`return:$to`, ret);
      }
    }, true);
    addEventListener.call(window, `$eventId:return:$from`, event => {
      let detail = event;
      if (detail && retStack.length) {
       retStack[retStack.length -1] = detail;
      }
    }, true);
    this.onMessage = null;
  }
  let port = new Port("extension", "page");

  let nativeExport = this && this.exportFunction || typeof exportFunction == "function";
  if (!nativeExport) {
    // Chromium
    let exportFunction = (func, targetObject, {defineAs, original}) => {
      try {
        let  [propDef, getOrSet, propName] = defineAs && /^([gs]et)(?:\s+(\w+))$/.exec(defineAs) || [null, null, defineAs];
        let propDes = propName && Object.getOwnPropertyDescriptor(targetObject, propName);
        if (!original) original = propDef && propDes ? propDes[getOrSet] : targetObject[defineAs];

        let proxy = new Proxy(original, {
          apply(target, thisArg, args) {
            return func.apply(thisArg, args);
          }
        });
        if (propName) {
          if (!propDes) {
            targetObject[propName] = proxy;
          } else {
            if (getOrSet) {
              propDes[getOrSet] = proxy;
            } else {
              if ("value" in propDes) {
                propDes.value = proxy;
              } else {
                return exportFunction(() => proxy, targetObject, `get $propName`);
              }
            }
            Object.defineProperty(targetObject, propName, propDes);
          }
        }
        return proxy;
      } catch (e) {
        console.error(e, `setting $targetObject.${defineAs || original}`, func);
      }
      return null;
    };
    let cloneInto = (obj, targetObject) => {
      return obj; // dummy for assignment
    };
    let script = document.createElement("script");
    script.text = `
    (() => {
      let patchWindow = $patchWindow;
      let cloneInto = $cloneInto;
      let exportFunction = $exportFunction;
      let env = ${JSON.stringify(env)};
      let eventId = ${JSON.stringify(eventId)};
      env.port = new ($Port)("page", "extension");
      ({
        patchWindow,
        exportFunction,
        cloneInto,
        eventId,
      }).patchWindow($patchingCallback, env);
    })();
    `;
    document.documentElement.insertBefore(script, document.documentElement.firstChild);
    script.remove();
    return port;
  }
  env.port = new Port("page", "extension");
  
  patches.push([patchingCallback, env]);
  
  return port;
}

function applyPatches() {
  
  const patchedWindows = new WeakSet();
  
  function modifyWindow(win, modifyTarget) {
    try {
      win = win.wrappedJSObject || win;
      if (patchedWindows.has(win)) return;

      modifyTarget.forEach(([callback, env]) => callback(win, env));
      modifyWindowOpenMethod(win, modifyTarget);
      modifyFramingElements(win, modifyTarget);
      // we don't need to modify win.opener, read skriptimaahinen notes
      // at https://forums.informaction.com/viewtopic.php?p=103754#p103754
      patchedWindows.add(win);
    } catch (e) {
      if (e instanceof DOMException && e.name === "SecurityError") {
        // In case someone tries to access SOP restricted window.
        // We can just ignore this.
      } else throw e;
    }
  }

  function modifyWindowOpenMethod(win, modifyTarget) {
    let windowOpen = win.open;
    exportFunction(function(...args) {
      let newWin = windowOpen.call(this, ...args);
      if (newWin) modifyWindow(newWin, modifyTarget);
      return newWin;
    }, win, {defineAs: "open"});
  }

  function modifyFramingElements(win, modifyTarget) {
    for (let property of ["contentWindow", "contentDocument"]) {
      for (let iface of ["Frame", "IFrame", "Object"]) {
        let proto = win[`HTML$ifaceElement`].prototype;
        modifyContentProperties(proto, property, modifyTarget)
      }
    }
  }

  function modifyContentProperties(proto, property, modifyTarget) {
    let descriptor = Object.getOwnPropertyDescriptor(proto, property);
    let origGetter = descriptor.get;
    let replacementFn;

    if (property === "contentWindow") { replacementFn = function() {
      let win = origGetter.call(this);
      if (win) modifyWindow(win, modifyTarget);
      return win;
    }}
    if (property === "contentDocument") { replacementFn = function() {
      let document = origGetter.call(this);
      if (document && document.defaultView) modifyWindow(document.defaultView, modifyTarget);
      return document;
    }}

    descriptor.get = exportFunction(replacementFn, proto, {defineAs: `get $property`});
    Object.defineProperty(proto, property, descriptor);
  }

  modifyWindow(window, patches);
}
Quick tour of the changes:

patchWindow.js:

- Added const patches = []; at the beginning to store the patching callbacks and envs.

- patchWindow() is now used to "register" the callbacks (feel free to rename it more appropriately).

- Moved modifyWindow() and rest to the new applyPatches() and left patches.push([patchingCallback, env]); at the end of the patchWindow().

- modifyWindow() now takes an array of callbacks (and envs) and loops through them.

Needed a way to run applyPatches() after all the patchWindow() calls were done. This seems to do the trick:

Code: Select all

ns.on("after_capabilities", event => {
  applyPatches();
});
Which I positioned at the end of content.js but that should matter little. Feel free to put in more appropriate file.

Also need to actually fire the event after "capabilities" have fired, so...

staticNS.js: 113

Code: Select all

      this.fire("capabilities");
      this.fire("after_capabilities");
Might also want to encapsulate the patchWindow.js a bit so that the patches don't collide with anything.

Plus reminding that this viewtopic.php?f=10&t=26316#p103952 needs to be rolled back as it breaks things unless there is a compelling reason not to.

Re: InternalError: Too much recursion

Posted: Sun Jun 13, 2021 6:06 am
by Giorgio Maone
Thank you for examining this and trying to patch it!

I don't feel comfortable with the two-steps approach, though: I fully understand it rationale, but unless applying all the patches in one shot at the right time can be automated and decoupled from NoScript's internal event firing machinery (which I doubt can be done reliably) I'm afraid it can be a serious hit at NSCL's (re)usability.

I'm currently trying to adapt your approach so that the patches can be applied to the current window as soon as patchWindow is invoked, but collected for deferred one-shot injection in dependent windows.

Thanks again.

Re: InternalError: Too much recursion

Posted: Sun Jun 13, 2021 1:20 pm
by skriptimaahinen
You mean something like this?

Code: Select all

"use strict";
/**
 * Injects code into page context in a cross-browser way, providing it
 * with tools to wrap/patch the DOM and the JavaScript environment
 * and propagating the changes to child windows created on the fly in order
 * to prevent the modifications to be cancelled by hostile code.
 *
 * @param {function} patchingCallback
 *        the (semi)privileged wrapping code to be injected.
 *        Warning: this is not to be considered a closure, since Chromium
 *        injection needs it to be reparsed out of context.
 *        Use the env argument to propagate parameters.
 *        It will be called as patchingCallback(unwrappedWindow, env).
 * @param {object} env
 *        a JSON-serializable object to made available to patchingCallback as
 *        its second argument*
 * @returns {object} port
 *        A Port object to be used to communicate with the privileged content
 *        script, by using port.postMessage(msg, [event]) and
 *        the port.onMessage(msg, event) user-defined callback.
 */

function patchWindow(patchingCallback, env = {}) {
  let eventId = this && this.eventId || `windowPatchMessages:${uuid()}`;
  let { dispatchEvent, addEventListener } = window;

  function Port(from, to) {
    // we need a double dispatching dance and maintaining a stack of
    // return values / thrown errors because Chromium seals the detail object
    // (on Firefox we could just append further properties to it...)
    let retStack = [];

    function fire(e, detail, target = window) {
      dispatchEvent.call(target, new CustomEvent(`${eventId}:${e}`, {detail, composed: true}));
    }
    this.postMessage = function(msg, target = window) {
      retStack.push({});
      let detail = {msg};
      fire(to, detail, target);
      let ret = retStack.pop();
      if (ret.error) throw ret.error;
      return ret.value;
    };
    addEventListener.call(window, `${eventId}:${from}`, event => {
      if (typeof this.onMessage === "function" && event.detail) {
        let ret = {};
        try {
          ret.value = this.onMessage(event.detail.msg, event);
        } catch (error) {
          ret.error = error;
        }
        fire(`return:${to}`, ret);
      }
    }, true);
    addEventListener.call(window, `${eventId}:return:${from}`, event => {
      let {detail} = event;
      if (detail && retStack.length) {
       retStack[retStack.length -1] = detail;
      }
    }, true);
    this.onMessage = null;
  }
  let port = new Port("extension", "page");

  let nativeExport = this && this.exportFunction || typeof exportFunction == "function";
  if (!nativeExport) {
    // Chromium
    let exportFunction = (func, targetObject, {defineAs, original}) => {
      try {
        let  [propDef, getOrSet, propName] = defineAs && /^([gs]et)(?:\s+(\w+))$/.exec(defineAs) || [null, null, defineAs];
        let propDes = propName && Object.getOwnPropertyDescriptor(targetObject, propName);
        if (!original) original = propDef && propDes ? propDes[getOrSet] : targetObject[defineAs];

        let proxy = new Proxy(original, {
          apply(target, thisArg, args) {
            return func.apply(thisArg, args);
          }
        });
        if (propName) {
          if (!propDes) {
            targetObject[propName] = proxy;
          } else {
            if (getOrSet) {
              propDes[getOrSet] = proxy;
            } else {
              if ("value" in propDes) {
                propDes.value = proxy;
              } else {
                return exportFunction(() => proxy, targetObject, `get ${propName}`);
              }
            }
            Object.defineProperty(targetObject, propName, propDes);
          }
        }
        return proxy;
      } catch (e) {
        console.error(e, `setting ${targetObject}.${defineAs || original}`, func);
      }
      return null;
    };
    let cloneInto = (obj, targetObject) => {
      return obj; // dummy for assignment
    };
    let script = document.createElement("script");
    script.text = `
    (() => {
      let patchWindow = ${patchWindow};
      let cloneInto = ${cloneInto};
      let exportFunction = ${exportFunction};
      let env = ${JSON.stringify(env)};
      let eventId = ${JSON.stringify(eventId)};
      env.port = new (${Port})("page", "extension");
      ({
        patchWindow,
        exportFunction,
        cloneInto,
        eventId,
      }).patchWindow(${patchingCallback}, env);
    })();
    `;
    document.documentElement.insertBefore(script, document.documentElement.firstChild);
    script.remove();
    return port;
  }
  env.port = new Port("page", "extension");

  const patches = [];
  const patchedWindows = new WeakSet();

  function modifyWindow(win, modifyTarget) {
    try {
      win = win.wrappedJSObject || win;
      modifyTarget.forEach(([callback, env]) => callback(win, env));
      
      if (patchedWindows.has(win)) return;
      
      modifyWindowOpenMethod(win);
      modifyFramingElements(win);
      // we don't need to modify win.opener, read skriptimaahinen notes
      // at https://forums.informaction.com/viewtopic.php?p=103754#p103754
      patchedWindows.add(win);
    } catch (e) {
      if (e instanceof DOMException && e.name === "SecurityError") {
        // In case someone tries to access SOP restricted window.
        // We can just ignore this.
      } else throw e;
    }
  }

  function modifyWindowOpenMethod(win) {
    let windowOpen = win.open;
    exportFunction(function(...args) {
      let newWin = windowOpen.call(this, ...args);
      if (newWin) modifyWindow(newWin, patches);
      return newWin;
    }, win, {defineAs: "open"});
  }

  function modifyFramingElements(win) {
    for (let property of ["contentWindow", "contentDocument"]) {
      for (let iface of ["Frame", "IFrame", "Object"]) {
        let proto = win[`HTML${iface}Element`].prototype;
        modifyContentProperties(proto, property)
      }
    }
  }

  function modifyContentProperties(proto, property) {
    let descriptor = Object.getOwnPropertyDescriptor(proto, property);
    let origGetter = descriptor.get;
    let replacementFn;

    if (property === "contentWindow") { replacementFn = function() {
      let win = origGetter.call(this);
      if (win) modifyWindow(win, patches);
      return win;
    }}
    if (property === "contentDocument") { replacementFn = function() {
      let document = origGetter.call(this);
      if (document && document.defaultView) modifyWindow(document.defaultView, patches);
      return document;
    }}

    descriptor.get = exportFunction(replacementFn, proto, {defineAs: `get ${property}`});
    Object.defineProperty(proto, property, descriptor);
  }

  patches.push([patchingCallback, env]);
  modifyWindow(window, [[patchingCallback, env]]);
  return port;
}

(edit: fix broken code)

Yes, I think this should work too and it is more concise. Probably better approach. Need to test it a bit more though.

However, applyPatches is not dependent on NS events. It can be called whenever the library user has determined that all patches have been registered. Be it right after the patchWindow calls in the same function or as in NoScripts case, in a event that's fired right after the event where last of the patchWindow calls are.

Re: InternalError: Too much recursion

Posted: Sun Jun 13, 2021 6:39 pm
by Giorgio Maone
skriptimaahinen wrote: Sun Jun 13, 2021 1:20 pm You mean something like this?
Not sure why, but something really nasty happen to code when you post it here :(
Especially interpolated strings with variables inside get really messed up.
Could you please send me as a text file or a link pastebin somewhere on the web, if you really prefer not to create a proper PR on Github?
Thank you!

Re: InternalError: Too much recursion

Posted: Sun Jun 13, 2021 7:37 pm
by Giorgio Maone
BTW, I'm almost done with my version (which fixes a couple of issues with your latest iteration, e.g. only the first patchWindow() call gets to actually modify the window), so in this instance I'd probably prefer you to review my incoming commit. Thanks!

Re: InternalError: Too much recursion

Posted: Sun Jun 13, 2021 8:51 pm
by Giorgio Maone
Giorgio Maone wrote: Sun Jun 13, 2021 7:37 pm I'd probably prefer you to review my incoming commit. Thanks!
And here it is.
I've removed the patch accumulator because I couldn't figure out any way to make it work with the Chromium injection code path, which is unavoidably stateless. Of course this means that every patchWindow call creates to a new interception layer around window spawning related stuff, but this shouldn't be an issue (and is not the cause of the bug at hand, anyway).

Thanks again for any further insight.

Re: InternalError: Too much recursion

Posted: Tue Jun 15, 2021 6:46 am
by skriptimaahinen
Your version seems to work quite well. In hindsight, it was quite unnecessary to try to cram all the patches into one call. Preferring your approach.
Giorgio Maone wrote: Sun Jun 13, 2021 8:51 pm Of course this means that every patchWindow call creates to a new interception layer around window spawning related stuff, but this shouldn't be an issue (and is not the cause of the bug at hand, anyway)
Yes, should not be a problem. patchedWindows should make sure it doesn't blow out of hands anymore even if some script gets spammy accessing the windows.

Re: InternalError: Too much recursion

Posted: Tue Jun 15, 2021 6:48 am
by skriptimaahinen
Giorgio Maone wrote: Sun Jun 13, 2021 6:39 pm Not sure why, but something really nasty happen to code when you post it here :(
Especially interpolated strings with variables inside get really messed up.
Could you please send me as a text file or a link pastebin somewhere on the web, if you really prefer not to create a proper PR on Github?
Oh, so they seem to. Need to be vary of that. You would prefer submits through GitHub?

Re: InternalError: Too much recursion

Posted: Tue Jun 15, 2021 2:08 pm
by Giorgio Maone
skriptimaahinen wrote: Tue Jun 15, 2021 6:48 am Oh, so they seem to. Need to be vary of that. You would prefer submits through GitHub?
If it's not too much of an hassle for you, that would be great, thanks!

Re: InternalError: Too much recursion

Posted: Tue Jun 15, 2021 11:12 pm
by barbaz
skriptimaahinen wrote: Tue Jun 15, 2021 6:48 am Oh, so they seem to. Need to be vary of that.
skriptimaahinen, would you mind editing your post viewtopic.php?p=104124#p104124 and trying again to post that code there again in light of this? - viewtopic.php?f=14&t=26364

Thanks Giorgio for fixing that, much better now 8-)