Locale JS alter breaks on remote JS files

This was added in http://cgit.drupalcode.org/drupal/diff/?id=733f68f17f0f227d0f970bc412177..., looks like the interaction with JS alter was not considered.

Makes sense IMHO.

I tried writing a test for this, but since common_test.libraries.yml defines the external files as type: external, this test passes. So from what I can tell, that is perhaps an undocumented workaround?

I tried writing a test for this, but since common_test.libraries.yml defines the external files as type: external, this test passes. So from what I can tell, that is perhaps an undocumented workaround?

Rather than an undocumented workaround, isn't it actually the expected way for external files to be loaded?

In other words, it's not immediately obvious to me what the core bug is here; locale_js_alter only calls locale_js_translate()=>_locale_parse_js_file() for JavaScript of type 'file', not for type 'external'.

Doesn't that mean this problem only occurs if there is calling code out there that incorrectly uses 'file' for an external JavaScript file, when it should have used 'external' instead?

I also marked #1814980: file_get_contents is used on remote URIs in locale.inc as a duplicate.

Hello all, I said on https://www.drupal.org/node/1814980 :

@David_Rothstein

This can be a security risk :

Imagine a user has a role in the backend that is able to feed javascript file/uri to add_js trough wathever module.

He can just request resources that wouldn't be touchable otherwise because it bypass the proxy.

I agree it requires some pre-configuration that might not be common to all users, but it's still possible to :

- Either get content from network resources that you shouldn't
- Trigger actions on a network resource that you shouldn't

Even if tiny, this is a risk, people usually use proxies in their app to avoid it using direct connections to external AND internal resources

Regards.

Just read :

In other words, it's not immediately obvious to me what the core bug is here; locale_js_alter only calls locale_js_translate()=>_locale_parse_js_file() for JavaScript of type 'file', not for type 'external'.

------------------------

This makes total sense to me now, yes, and indeed we had only one bad sheep ( using add_js() wrong , without specifying external )

Not sure then if a simple check could be added to avoid it from happening as I feel some contrib modules might be bad sheeps in this regards as well.

Maybe the methods needing filtering are add_js and add_css in this case !

This can be a security risk :

Imagine a user has a role in the backend that is able to feed javascript file/uri to add_js trough wathever module.

He can just request resources that wouldn't be touchable otherwise because it bypass the proxy.

I guess I see your point, but if an untrusted user can add arbitrary JavaScript files to the site, they can already do a cross-site-scripting attack against any other user... it seems like that would tend to be a much bigger concern.

I see your point, but I think it depends of the perspective and the size of the webmaster/dev team.

I'll try to clarifiy the example :

In our environment, we tend to decouple those responsabilities to developers, while all the network and proxy constraint are done at the sysadmin level.

In my point of view ( sysadmin ), I cannot check 100 sites for bad usage when developers allow customers to include JS (we have QA for that, but QA process can go wrong, they can miss it, not think about it, or include a contrib modules wrongly using add_js / add_css).

I can however "make sure" that they won't touch the infrastructure as much as possible by providing a solid core that won't allow them to make direct network queries (we forbid anything not HTTP and force dev to use drupal_http_request).

Again, I agree it's blurry, but I would say it can improve (but also break) a lot of contrib module security, I also admit it requires a lot of conditions to be exploitable.

( if it can help, here's a commit to (temporarly) fix the issue on our side and get notified of bad practices )

https://github.com/ec-europa/platform-dev/pull/414/commits/981a961b7dc00...

This is how the patch from #10 would look on Drupal 7. @gboddin, can you check if this would suit your needs? It does not include the logging you currently have in your proposed patch, but this will not be acceptable in a core patch anyway. I have no tested this by the way. It's just a straight port of the patch.

About the security risk, what I think you mean is this: you are managing 100+ websites which are allowing developers and site builders to include locally sourced javascript files in their websites. These local javascript files have been checked for potential security issues by the QA team. External files are considered a security risk since they have not been validated by the security team and can contain potential attack vectors. Is that a good description?

If this is the case then I agree this is a security issue from your devops perspective, but it would not affect a typical Drupal website since which has full control over the JS files they include since they come from a trusted source: the developer of the website. So from Drupal's point of view this is not a security issue. It depends how much you trust your developers :)

@pfrenssen , Actually I'm thinking more about contrib modules.

But as David_Rothstein said, it's the module not using drupal_add_js properly.

I opened https://www.drupal.org/node/2697611 and proposed a patch for drupal_add_js method.

Patch relloled, just updated for last version.

When we moved to libraries did we circumvent this issue or is it still possible to declare an external JavaScript library with type file?

Yes, it is still an issue with libraries. See #2735717: Stream wrapper reference in JS library causes error in _locale_parse_js_file()

Rerolled patch against 9.4X

Is this a duplicate of #2735717: Stream wrapper reference in JS library causes error in _locale_parse_js_file()? Can you test this issue with the patch from over there applied?

Following up if anyone was able to verify #36 if so we can close this out and move over credit.

Yes, this is a duplicate. Closing