P5.js: document.cookie should be catched when in environment where it's not a safe operation

Created on 19 Oct 2020 · 14Comments · Source: processing/p5.js

Most appropriate sub-area of p5.js?

[ ] Accessibility (Web Accessibility)
[ ] Color
[ ] Core/Environment/Rendering
[ ] Data
[ ] Dom
[ ] Events
[ ] Image
[ ] IO
[ ] Math
[ ] Typography
[ ] Utilities
[ ] WebGL
[x] Other (specify if possible)

Details about the bug:

p5.js version: 1.1.9 (but I guess all using document.cookie)
Web browser and version: all
Operating System: all
Steps to reproduce this:
Run p5.js in a sandboxed iframe not allowing same-origin.

Uncaught (in promise) DOMException: The operation is insecure. p5.js:19493 (access document.cookie)

Access to document.cookie is a dangerous operation and should be catched if not allowed, without stopping the whole process as it is not a required feature to have a working rendering.

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <title>Document</title>
</head>
<body>
    <iframe sandbox="allow-scripts"
    srcdoc="
  <html>
    <body>
      <script
        type='text/javascript'
        src='https://cdnjs.cloudflare.com/ajax/libs/p5.js/1.1.9/p5.js'
      ></script>
    </body>
  </html>"
</iframe>

</body>
</html>

bug

Source

dievardump

Most helpful comment

Since p5 doesn't provide any such toggle, I think we should be okay with having it check navigator.language each time

If everyone here agrees, I can make a small edit by around this weekend to stop i18next from accessing local storage or cookies. I don't think it should cause a problem

akshay-99 on 9 Nov 2020

👍3

All 14 comments

Welcome! 👋 Thanks for opening your first issue here! And to ensure the community is able to respond to your issue, be sure to follow the issue template if you haven't already.

welcome[bot] on 19 Oct 2020

As far as I know, p5.js itself doesn't set any cookies and there certainly isn't any reference to cookies in the source itself. Looking at the built library and where the error originates from I think it may be a polyfill added by Babel, maybe to get around lack of localStorage support. @outofambit @stalgiag Can you have a look at this as you are more familiar with Babel/localStorage respectively.

limzykenneth on 25 Oct 2020

👀1

From what I got, the cookies are read for the i18n (I noticed since then that the minified version of p5js works in a Sandboxed iframe for example, because there is not i18n loaded in the min).

dievardump on 25 Oct 2020

👍1

Digging a bit I believe it does indeed come from i18n and specifically i18next-browser-languageDetector. Perhaps the detector options can be tweaked manually to avoid this problem.

limzykenneth on 26 Oct 2020

👍3

Perhaps the detector options can be tweaked manually to avoid this problem.

Do you have some doc on where it is possible to tweak it when configurating p5.js? I've got a hard time finding api that is not processing related.

The sandboxed iframe I use is for any user to run any code, and sometimes add any dependency they want. So I need to add things like:

      // specific p5 because it's causing troubles.
      if (typeof p5 !== 'undefined') {
        p5.disableFriendlyErrors = true;
        // p5.is18nconfiguration??
      }

in the rendering iframe. Couldn't find a way to do this by just reading p5 code

dievardump on 28 Oct 2020

@limzykenneth thanks for tracking this down to i18next-browser-languageDetector! I originally added this dependency as part of the friendly error localization work.

I took a look at the configuration docs and we might be able to disable the cookie caching altogether (here) but I have to look a bit more to make sure that won't break anything. (We don't currently expose the internationalize config to the p5.js coders, and that could be an option, but i think this might be hard to configure after p5 initializes.)

ccing @akshay-99 and @stalgiag since they worked with some of this code over the summer in case they have thoughts

outofambit on 30 Oct 2020

👍1

@outofambit I think a good set of defaults should be enough and exposing the config to users is a bit overkill also considering it isn't included in the minified build.

limzykenneth on 3 Nov 2020

👍1

@outofambit @limzykenneth I think we should be able to safely disable cookie caching and only use localStorage. I didn't make any changes that would require cookies specifically.

akshay-99 on 3 Nov 2020

Just as information, sadly, the use of localStorage is as insecure as the use of document.cookie in a sandboxed iframe, and will throw an exception.

dievardump on 3 Nov 2020

👀2

Do we know what i18next-browser-languageDetector is using these browser storage for?

limzykenneth on 3 Nov 2020

@limzykenneth It uses it to cache the current language. To be honest, I don't know if we need to do that with p5.

I think i18next implements language caching so that it can be used alongside a user-controlled language toggle. One of the ways i18next-browser-languageDetector actually detects the language is through the navigator.language property. As far as I know, it is directly linked to the browser's settings.
When the user wishes to change the language on a website, i18next would cache this new preference in some storage so that in the future, it will use this instead of navigator.language.

Since p5 doesn't provide any such toggle, I think we should be okay with having it check navigator.language each time, assuming accessing it is allowed in a sandboxed iframe.

I don't know if such a change would break anything.

akshay-99 on 3 Nov 2020

👍1

navigator.language works well in a sandboxed iframe.

Sandboxed iframe are often used to run user submitted scripts.
document.cookie and localStorage are not allowed so those script won't be able to access data that could harm the current user (session cookies and so on). The navigator language poses no threat.

dievardump on 4 Nov 2020

👍1

Since p5 doesn't provide any such toggle, I think we should be okay with having it check navigator.language each time

If everyone here agrees, I can make a small edit by around this weekend to stop i18next from accessing local storage or cookies. I don't think it should cause a problem

akshay-99 on 9 Nov 2020

👍3

I think you can go ahead.

limzykenneth on 9 Nov 2020

Was this page helpful?

0 / 5 - 0 ratings