Docs: Only load trusted iframe

[r0hanSH]

Description

An XSS vulnerability was reported by me on huntr.dev platform. The vulnerability has been validated by @mrdoob
This PR is the patch for that vulnerability.

[mrdoob]

Thanks!

[r0hanSH]

Hi @mrdoob
Can you please validate that the fix has been merged on our huntr.dev report? so that we can disclose the report
https://huntr.dev/bounties/16901080-99b4-4fb5-8c5b-931bfbf33cba/

[mrdoob]

Done 👍

[lgarron]

I know it's better safe than sorry, but I'd like to ask what warranted marking this as a security vulnerability on npm?
Dealing with the alert was not a good experience for me.

This issue/fix affects only the index page of the docs, which is not at all in the published npm package and therefore can't consumed through npm at all. There are already a lot of issues with vulnerability alerts in the ecosystem "crying wolf", but this is the clearest case I've seen by far.

In this particular case, I upgraded from r135 to r137 figuring that it would probably be fine. However it seems that this introduces export definition that do not work for esbuild:

three.js/package.json

Line 14 in 2d80739

"./examples/jsm/*": "./examples/jsm/*",

The whole point of exports is to limit what is exported, which means it's not supposed to be possible to work around easily.
https://nodejs.org/api/packages.html#subpath-exports documents that the mapped value glob might need to end with .js to preserve semantics, and it's possible that more levels of globbing are needed. I wasn't really planning to spend time on that today, but it seems I have to if I want to avoid a vuln alert on my repo/package. 😕

I know I sound a bit ungrateful, but I think this is a situation where I think there was an easy alternative call.

[lgarron]

The whole point of exports is to limit what is exported, which means it's not supposed to be possible to work around easily.
https://nodejs.org/api/packages.html#subpath-exports documents that the mapped value glob might need to end with .js to preserve semantics, and it's possible that more levels of globbing are needed. I wasn't really planning to spend time on that today, but it seems I have to if I want to avoid a vuln alert on my repo/package. 😕

In case anyone also runs into this while upgrading, what worked for me was to explicitly add .js to the import. This is an anti-pattern in TypeScript, but it was relatively harmless in my case:

-import { BoxLineGeometry } from "three/examples/jsm/geometries/BoxLineGeometry";
+import { BoxLineGeometry } from "three/examples/jsm/geometries/BoxLineGeometry.js";

cubing/cubing.js@f1facf9

(I also want to note, I'm a big fan of exports! I'm glad to see three adopting it. But I would prefer to shake out compatibility bugs without my package being marked as vulnerable in the meantime.)

[mrdoob]

@lgarron

I know it's better safe than sorry, but I'd like to ask what warranted marking this as a security vulnerability on npm?

Can't agree more! Do you know how to fix it?

[lgarron]

@lgarron

I know it's better safe than sorry, but I'd like to ask what warranted marking this as a security vulnerability on npm?

Can't agree more! Do you know ow to fix it?

I work at GitHub, which npm is now a part of. Lemme try to ask someone!

[lgarron]

I work at GitHub, which npm is now a part of. Lemme try to ask someone!

So, we picked up the severity automatically from https://huntr.dev/bounties/16901080-99b4-4fb5-8c5b-931bfbf33cba/ — it sounds like the bet path forward is if you could revise it there.

I'm told maintainers can ask for revisions to severity on huntr.dev, and that they can push updates to the CVE that GitHub should automatically pick up.

Would you mind trying that?

[mrdoob]

I already wrote a message in that link 4 hours ago. What else can I do?

[lgarron]

I already wrote a message in that link 4 hours ago. What else can I do?

Ah, apologies, I didn't see that before. (I've never used huntr.dev before, and they really don't make it clear that there are hidden messages that will show if you log in.)

In this case it sounds like by far the best is for huntr.dev to update the CVE, which unfortunately we don't have control over.

(I'll also send a contact to your committer email in case you ever need to address issues with GitHub vuln alerts the future, although I'm not sure how much it would help in this particular case.)

[marcofugaro]

In this particular case, I upgraded from r135 to r137 figuring that it would probably be fine. However it seems that this introduces export definition that do not work for esbuild:

You have to add the .js extension now, which is exactly what you did 🙂

-import { BoxLineGeometry } from "three/examples/jsm/geometries/BoxLineGeometry";
+import { BoxLineGeometry } from "three/examples/jsm/geometries/BoxLineGeometry.js";

When the library has "type": "module", bundlers conform to the ES Module spec, which requires you to use the .js extension if you're importing a file.

Here is an example of webpack telling you to do so:

[alesandroortiz]

Hello, the GitHub security advisory at GHSA-7vvq-7r29-5vg3 seems to be misleading. The XSS was only on the Three.js website at https://threejs.org/docs/ correct?

If so, please contact GitHub to update their advisory and ensure the CVE at https://nvd.nist.gov/vuln/detail/CVE-2022-0177 also reflects the correct information.

For future reference, this is what the GHSA currently shows:

[mrdoob]

@alesandroortiz If you read the thread you'll see that @lgarron works at Github and is already taking care of it.

[r0hanSH]

Hi everyone,

The vulnerability only exists in docs section of threejs website. No vulnerability was reported in its npm package.
My role is to just find the vulnerability and fix it. nothing else.

The CVE publishing process and other miscellaneous tasks are performed by huntr.dev platform
I don't have edit access to the report now, otherwise I would have changed the severity and dumped the CVE.
Only admins at huntr.dev have access to modify the CVE and vulnerability report. @mrdoob please check the report link, the admin has contacted you.

[lgarron]

Following up here: the CVE was updated to zero vulnerability on hackr.dev.
(Thanks to Jamie Slome for following up.)

GitHub has picked up that change. This actually 404s GHSA-7vvq-7r29-5vg3 for the general public (see @alesandroortiz's screenshot above for historical reference). That should stop Dependabot alerts and banners on github.com!

[alesandroortiz]

@lgarron FYI the URL still loads for me, shows updated description indicating it was withdrawn but still has High severity label. Not sure if that stops alerts, etc.