Hello everyone!
I've been working on my GSoC proposal for the plugin security project and wanted to share a draft here before the final submission. I have tried to cover all the points raised in the original RFC, including the open questions Laurent left around unreviewed plugins, the diff tooling, and the submission workflow
Proposal link: Proposal.docs Note: I have not included PoC link and would like to share personally only with Maintainers
Github Profile : varunkumar-22
I have been contributing actively to joplin and below are my PRs:
Merged PRs
#14504 :- Mobile:Rich Text Editor: Fix extra blank line above nested lists #14477:- Desktop: Fix UI freeze when closing plugin dialog with Escape key #14421:-All: Fixes #14335: Support include_deleted parameter for GET /folders endpoint #14461:-CLI: Fixes #13158: Fix null crash in e2ee decrypt command #14541:-Desktop: Fixes #14196: Fix file:// links with backslashes for Windows UNC paths #14580:-Mobile: Fix tapping rendered image scrolling to cursor position #14559:-CLI: Fix trailing spaces in ls -l output #14432:-Desktop: Fix "Copy dev mode command" producing an unquotable path on Windows
Open PRs: #14705:-Mobile Android: Fix Tab key ordering regression on note edit screen
Couple of questions (these are just some initial thoughts that I had reading the proposal, don't take anything said here as an actual requirement).
Why a separate approved-plugins.json rather than modifying manifests.json with the additional fields (as we do for recommended as well as storage of the commit and publish hashes)?
What does the status field in your approved-plugins.json file do? My understanding of the proposal (apologies if I missed something) was that the entry only gets added to that file once it has been reviewed and merged by the PR process? So why would this be anything other than approved?
Section 9 also shows a "Pending" label to show in "untrusted mode" within the Joplin plugin manager UI. I assume this is for new and updated plugins and not the legacy ones which will be tagged as "unreviewed". I'm not sure I understand how this "Pending" state is achieved as my (basic) understanding of the process is:
Developer updates plugin code + creates PR to the Joplin plugin repo
Maintainers review code and merge or reject. The "review"
On merge the CI runs and updates approved-plugins.json which will build the artifacts from the original repository
.jpl and manifest added to repo (assuming manifests.json is updated as well)
Plugin is now discoverable as "reviewed"
Lastly, the PR process to the repo seems to just be sending a commit hash and a URL for submission. I don't know how granular the review process wishes to be at this stage (i.e. just rejecting for actively malicious code or pointing out vulnerabilities that could be patched out) but that process has opportunity for human error. Is there potentially scope for performing the review on the same repo as the PR? i.e. the PR actually submits the repo code. That means the inbuilt GH review tools can be used which already can detect if parts of the code have been altered after an initial review or can ask for additional reviewer help if needed on specific lines. Although I'm aware this may involve alteration to the plugin template generator or to the repo/new repo branch etc. etc.
@DaeraxaThank You for reviewing! I have answered to each question, please check it out and suggest if any changes are required**
Question 1**
The reason I went with a separate file was to keep the review registry decoupled from the published manifest data, since they serve different purposes one is the trust anchor for the build pipeline, the other is what the joplin client reads to list plugins. But yeah you are right that extending manifests.json with the additional fields like the way _recommended and _publish_hash already work would be more consistent with the existing pattern. I am open to dropping the separate file entirely if that's the preferred direction
Question 2
The status field does real work in two cases: during migration, all existing plugins need to be added to approved-plugins.json as “unreviewed” so the build pipeline can still process them without falsely marking them as reviewed. It also handles incident response like if a previously approved plugin is found to have a security issue, you can flip the status to “suspended” to pull it from the build without deleting the entry entirely.
**Question 3
**
The "Pending" label was meant to represent plugins with an open PR that has not been merged yet but making the plugin manager aware of open PRs adds unnecessary complexity. I will simplify this to just two states reviewed and unreviewed and discuss with mentors whether pending is actually worth adding
**Question 4
**
It sounds interesting and my current approach keeps everything inside joplin/plugins so maintainers don't have to jump between external repos; the CI already generates the diff, so surfacing that directly in the PR and letting github's inline review tools handle line level discussion would be more powerful than what I have proposed. It would close the human error gap you are pointing at but the main tradeoff is that it might require changes to the plugin template generator and the repo structure which adds workflow complexity. I hadnot thought about it this way and it's worth discussing with mentors, i will be happy to explore
But it seems like out of scope for GSoC