Skip to content

Commit

Permalink
chore: add the new policy.json review process documentation and config (
Browse files Browse the repository at this point in the history
#29383)

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

Introducing the document outlining policy review process.

The current change to CODEOWNERS doesn't remove the all devs group -
this makes it a soft-launch of the process where a specific approving
review is not yet mandatory.

We'll be releasing a training video early January.

After merging this, I'm hoping to reference the document in multiple
places to aid people in discovering and following the process.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29383?quickstart=1)

## **Related issues**

Fixes:  lack of attention to policy updates

## **Manual testing steps**

1. Follow the process and experience joy

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [x] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [x] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

---------

Co-authored-by: Mark Stacey <[email protected]>
  • Loading branch information
naugtur and Gudahtt authored Jan 8, 2025
1 parent 20b417c commit ae565bc
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 1 deletion.
2 changes: 1 addition & 1 deletion .github/CODEOWNERS
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
# audit these changes on their own, and leave their analysis in a comment.
# These codeowners will review this analysis, and review the policy changes in
# further detail if warranted.
lavamoat/ @MetaMask/extension-devs @MetaMask/supply-chain
lavamoat/ @MetaMask/extension-devs @MetaMask/policy-reviewers @MetaMask/supply-chain

# The offscreen.ts script file that is included in the offscreen document html
# file is responsible, at present, for loading the snaps execution environment
Expand Down
29 changes: 29 additions & 0 deletions docs/lavamoat-policy-review-process.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# LavaMoat Policy Review process in metamask-extension

When there's a need to change policy (because of new or updated packages that require a different set of capabilities), please follow these steps:

> In the initial soft-launch of the process the approval from policy reviewers is not mandatory, but it will be in the near future.
### Engineer on the dev team:
1. Notice the `Validate lavamoat policy*` PR status check fail because dependency updates or changes need a policy update.
2. (optional) Generate an updated policy and give it a cursory look in local development whenever you’re testing the change.
- If you're confident your update is complete, you can push it to the PR branch.
3. To generate a complete set of new policies, call `metamaskbot` for help:
- put `@metamaskbot update-policies` in a comment on the PR. When it produces changes, they need to be reviewed. The following steps assume update-policies produced changes.
- *Note the response from the bot points to instructions on how to review the policy for your convenience. https://lavamoat.github.io/guides/policy-diff/*
4. Analyze the diff of policy.json and use the understanding of the codebase and change being made to decide whether the capabilities added make sense in that context. Leave a comment listing any doubts you have with brief explanations or if everything is in order \- saying so and explaining why the most powerful capabilities are there (like access to child_process.exec in node or network access in the browser)
*Remember the Security Reviewer comes with more security expertise but less intimate knowledge of the codebase and the change you’ve built, so you are the most qualified to know whether the dependency needs the powers detected by LavaMoat or not.*
- You can use these questions to guide your analysis:
1. What new powers (globals and builtins) do you see? Why should the package be allowed to use these new powers? Explain if possible
2. What new packages do you see? Did you intend to introduce them? If you didn’t, which package did? (can you see them in `packages` field in policy of any other package that you updated or introduced?)
- The comment is mandatory even if you don’t understand the policy change, in which case you’re expected to state so (it’s ok to not understand)
- Note: this could be enforced by a job that is only passing if the comment was made
- Note: we’d introduce more tooling to summarize and analyze policy and post that as a comment on the PR
5. Mention `policy-reviewers` group in your comment.
policy-reviewers group includes security liaisons and their involvement is not limited to (but likely focused more around) their respective teams’ PRs.

### L1 Security Reviewer:
1. Look at the policy and the comment from the Developer. Approve the PR if they match and the policy change seems safe. Address questions the Developer had and discuss if the policy change doesn’t seem right.
2. If changes are hard to explain or seem dangerous, escalate to a review of the dependency and its powers by mentioning `supply-chain`
### (optional) L2 Security Reviewer:
1. Review the dependency in question and inform the PR reviewers whether it’s deemed malicious or safe.

0 comments on commit ae565bc

Please sign in to comment.