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

Update "exporting_pcks" page to change our stance about mods and pcks #10355

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

Conversation

adamscott
Copy link
Member

As .pck files can have GDScript, and that there's no current way to limit the scope of the API of a .pck file, we should just change our stance in the "exporting_pcks" article to mention that we don't support this method for modding purposes anymore.

@adamscott adamscott added enhancement area:manual Issues and PRs related to the Manual/Tutorials section of the documentation topic:export cherrypick:4.3 labels Nov 30, 2024
@adamscott adamscott requested review from Calinou and hpvb November 30, 2024 17:12
@tetrapod00
Copy link
Contributor

tetrapod00 commented Nov 30, 2024

As a general comment on the structure of this PR: Usually we should change the docs to be correct in the present rather than leaving this kind of messy recommendation with caveats in a page, which mentions something we used to recommend and then says why we don't anymore.

I suppose in this case it does make sense to leave in a remnant of the old recommendation, since there are security concerns. There are slightly cleaner ways to go about that, I'll try to get around to a specific review for that, but others might want to discuss the PR overall first

@adamscott
Copy link
Member Author

adamscott commented Nov 30, 2024

As a general comment on the structure of this PR: Usually we should change the docs to be correct in the present rather than leaving this kind of messy recommendation with caveats in a page, which mentions something we used to recommend and then says why we don't anymore.

I suppose in this case it does make sense to leave in a remnant of the old recommendation, since there are security concerns. There are slightly cleaner ways to go about that, I'll try to get around to a specific review for that, but others might want to discuss the PR overall first

I can remove the remnants, but then, we need a blog post to clearly mention our change of stance. Maybe a blog post is needed either way.

I reread the article and I think the mentions are needed. As people will try to find out how to use PCKs as mods. We better explain why it's not a good idea instead of just removing all mentions about modding in the article. Because people will complain about "why it's not explained" or "where did it go?".

@tetrapod00
Copy link
Contributor

I think if the intent is to strongly signal that we no longer officially recommend using PCKs as mods (presumably at least partly in response to recent news?), a blog post is needed.

I reread the article and I think the mentions are needed.

That's okay with me, every rule has its exceptions and the reasons here make sense. In that case I do have suggestions for how to reword it to retain the information while being more idiomatic to the docs.

@adamscott
Copy link
Member Author

In that case I do have suggestions for how to reword it to retain the information while being more idiomatic to the docs.

Go ahead, @tetrapod00 ! I'm not a native English speaker, so I welcome suggestions.

@mhilbrunner
Copy link
Member

mhilbrunner commented Nov 30, 2024

I think its good to discuss this again in light of current events, but I disagree with this change. Security is always a necessary consideration, especially with implementing mods.

However, only download mods you trust is already a thing outside of Godot, same as for other executables. The majority of mods for games I'm familiar with are similarly able to execute arbitrary unsandboxed code.

Spend hundreds of hours to develop and maintain your own sandbox is not a useful suggestion to me. The people who have the technical chops or resources already know and would do that if necessary. For everyone else, suggesting they should spend their scarce resources specifically developing their own sandbox to implement modding when, say, Minecraft, Bethesda or most Unity game's mods get away without seems not that dev friendly.

Even with sandboxing, only download mods you trust remains important. We had asset importing/format vulnerabilities before, same for unpacking (as 7-Zip just had). So even if we had full sandboxing, one should not download any random file they find if they care about their machine and its local network.

Also, the Asset Lib and script addons need similar scary warnings too. Heck. a large part of Godot usage still seems to be small game jam and Itch games. Largely anonymous, untested things that run arbitrary code on your machine. People should be careful there too. Doesn't mean Godot itself is the issue, or that we should recommend avoiding those altogether. Just: judge things you download and run carefully, and consider there are risks involved. Keep backups, keep your eyes open, and maybe even consider separating your gaming rig from anything important if you really want to run your Skyrim 1000 mods install. (Yes, SKSE mods can execute arbitrary code too!)

I suggest instead we add a paragraph (if missing) to elaborate that people should think very carefully about supporting modding officially (including PCKs or any other ability to execute arbitrary code, same for network replication/object deserialization/implementing save files) and outline the hazards. There's a reason many games add a "no guarantees or warranty" disclaimer if mods are used. We should caution devs to similarly inform their users of possible risks. That doesn't mean allowing user mods is the end of the world.

Exporting packs and patches
===========================

.. warning::
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.. warning::
.. danger::

Since the docs use warning for so many things, we should use danger for anything that is a legal, security, or data loss risk. See #10252.


**We don't recommend anymore using PCK files to add modding functionality to your game.**

Please read :ref:`doc_exporting_pcks_about_mods_using_pck_files` for more information.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Please read :ref:`doc_exporting_pcks_about_mods_using_pck_files` for more information.
See :ref:`doc_exporting_pcks_about_mods_using_pck_files` for more information.

Standard phrasing, avoids imperative "read".

.. warning::
We changed our stance about exporting mods using PCK files for security reasons.

**We don't recommend anymore using PCK files to add modding functionality to your game.**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
**We don't recommend anymore using PCK files to add modding functionality to your game.**
**We no longer recommend using PCK files to add modding functionality to your game.**

While this seems ideal for modding purposes (and yes, it is), it comes with the downside of giving the keys to the castle to any bad actor.

While replacing shaders and models is quite benign, replacing scripts is problematic, as the GDScript scripting engine doesn't have actually
any concept of a `sandbox <https://en.wikipedia.org/wiki/Sandbox_(computer_security)>`__. Every mod containing GDScript files have access
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
any concept of a `sandbox <https://en.wikipedia.org/wiki/Sandbox_(computer_security)>`__. Every mod containing GDScript files have access
any concept of a `sandbox <https://en.wikipedia.org/wiki/Sandbox_(computer_security)>`__. Every mod containing GDScript files has access

any concept of a `sandbox <https://en.wikipedia.org/wiki/Sandbox_(computer_security)>`__. Every mod containing GDScript files have access
to the entire Godot Engine API, including making network calls and accessing the filesystem.

We do currently encourage game developers to create their own modding system for the time being. To be really secure, any modding system
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
We do currently encourage game developers to create their own modding system for the time being. To be really secure, any modding system
We do currently encourage game developers to create their own modding system. To be really secure, any modding system

No need to specify both "currently" and "for the time being".

We could also mention the criteria for no longer recommending this (presumably some implementation of sandboxing?), but I think that doing so is optional.

@@ -51,6 +60,23 @@ PCK files usually contain, but are not limited to:
The PCK files can even be an entirely different Godot project, which the
original game loads in at runtime.

.. _doc_exporting_pcks_about_mods_using_pck_files:

About mods using PCK files
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually headers are are either in the form "Using X for Y" or "Doing X with Y" or sometimes just "X", where X is the name of the feature. One of these suggestions is more idiomatic:

Suggested change
About mods using PCK files
Using PCK files for modding

or

Suggested change
About mods using PCK files
Using PCK files for mods

or

Suggested change
About mods using PCK files
Modding games with PCK files

or

Suggested change
About mods using PCK files
Supporting mods with PCK files

We should wait for other docs maintainers to comment before changing this. Note that the anchor and links to this section need to change, too.

@@ -51,6 +60,23 @@ PCK files usually contain, but are not limited to:
The PCK files can even be an entirely different Godot project, which the
original game loads in at runtime.

.. _doc_exporting_pcks_about_mods_using_pck_files:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.. UPDATE: Recommendation may change. If we ever implement sandboxing or another solution
.. change this section.

Recently we started using these kind of comments to keep track of information in the docs that is inherently likely to change.

@tetrapod00
Copy link
Contributor

tetrapod00 commented Nov 30, 2024

I don't have a strong opinion about this change overall (I really don't disagree with a lot of Max's points). I think that decision should probably be made with more than just the docs team as decisionmakers, though.

I think a single paragraph as Max suggests would also be a sufficient solution to the problem here.

If we do go through with these changes, they look alright to me after my suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:manual Issues and PRs related to the Manual/Tutorials section of the documentation cherrypick:4.3 discussion enhancement topic:export
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants