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

Add Headless API #639

Open
wants to merge 80 commits into
base: master
Choose a base branch
from
Open

Add Headless API #639

wants to merge 80 commits into from

Conversation

boatbomber
Copy link
Member

@boatbomber boatbomber commented Sep 25, 2022

@boatbomber boatbomber self-assigned this Sep 25, 2022
Copy link
Contributor

@sasial-dev sasial-dev left a comment

Choose a reason for hiding this comment

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

You might wanna change my impl. a bit but:

plugin/src/App/Notifications.lua Outdated Show resolved Hide resolved
@boatbomber boatbomber marked this pull request as ready for review January 5, 2023 00:38
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
plugin/src/HeadlessAPI.lua Outdated Show resolved Hide resolved
plugin/src/HeadlessAPI.lua Outdated Show resolved Hide resolved
@Dekkonot
Copy link
Member

Overall, I like the design of this but I am concerned about the potential for abuse it gives third parties. As an example, there is nothing stopping me from going rawset(require(game.Rojo), "API", {Notify = function() print("i am being malicious") end}).

This isn't a concern for most people, since I don't imagine most devs are just installing random plugins, but it's still a real problem that I want to avoid. Do you think there's be consequences to exposing the API as a userdata with a metatable instead of a table?

Copy link
Contributor

@nezuo nezuo left a comment

Choose a reason for hiding this comment

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

I'm curious why RequestAccess lets the users pick out which APIs they want to allow. I can't think of a case where you would want to give a plugin only some of the permissions it's asking for and for a plugin to only use some of the permissions it wants.

plugin/src/HeadlessAPI.lua Outdated Show resolved Hide resolved
plugin/src/HeadlessAPI.lua Outdated Show resolved Hide resolved
plugin/src/HeadlessAPI.lua Outdated Show resolved Hide resolved
plugin/src/HeadlessAPI.lua Outdated Show resolved Hide resolved
@boatbomber
Copy link
Member Author

Do you think there's be consequences to exposing the API as a userdata with a metatable instead of a table?

Will take a fair bit of rejiggering but that should work and close some potential attack vectors. Good call.

@boatbomber
Copy link
Member Author

I'm curious why RequestAccess lets the users pick out which APIs they want to allow. I can't think of a case where you would want to give a plugin only some of the permissions it's asking for and for a plugin to only use some of the permissions it wants.

Honestly, I can't remember my original reasoning so I'm gonna redo this to be all or nothing to create a simpler UX & DX. Thanks for bringing it up.

@boatbomber
Copy link
Member Author

Addressed all the feedback here and on the docs PR! 🚀

plugin/src/HeadlessAPI.lua Outdated Show resolved Hide resolved
plugin/src/HeadlessAPI.lua Outdated Show resolved Hide resolved
@Dekkonot
Copy link
Member

Reviewing the docs made me realize I have a more logistical comment for this PR: it exposes an ApiContext, which means we need to care about it a lot more than we current do. This applies to the Rust side of things too.

This isn't a problem, but it means we need to consider it in versioning and should probably also document both sides.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: plugin Relevant to the Roblox Studio plugin size: medium status: needs review This work is mostly done, but just needs work to integrate and merge it. type: enhancement Feature or improvement that should potentially happen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rojo Headless Plugin API Proposal
8 participants