Skip to content

Avoid reviewer privilege escalation vulnerability#25101

Open
chrstinalin wants to merge 1 commit into
mozilla:masterfrom
chrstinalin:2033254-rev-owner
Open

Avoid reviewer privilege escalation vulnerability#25101
chrstinalin wants to merge 1 commit into
mozilla:masterfrom
chrstinalin:2033254-rev-owner

Conversation

@chrstinalin

@chrstinalin chrstinalin commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Fixes bug 2033254.

  • Add #ISSUENUM at the top of your PR to an existing open issue in the mozilla/addons repository.
  • Successfully verified the change locally.
  • The change is covered by automated tests, or otherwise indicated why doing so is unnecessary/impossible.

@chrstinalin chrstinalin marked this pull request as draft July 2, 2026 17:52
@chrstinalin chrstinalin force-pushed the 2033254-rev-owner branch 4 times, most recently from 849cd94 to 992852e Compare July 3, 2026 12:43
@chrstinalin chrstinalin marked this pull request as ready for review July 3, 2026 13:01
@chrstinalin chrstinalin requested review from a team and diox and removed request for a team July 3, 2026 13:01
Comment thread src/olympia/api/permissions.py Outdated
Comment thread src/olympia/api/permissions.py Outdated
Comment on lines +203 to +205
can_access_because_admin = acl.action_allowed_for(
request.user, amo.permissions.ADDONS_EDIT
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In a similar vein to my other comment: let's explicitly require GroupPermission(amo.permissions.ADDONS_EDIT) in the list of permissions in views that need to allow that instead of making that permission class harder to follow.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The original class allowed admins to use unsafe methods, should I interpret this as similarly limiting admins as well then?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, users with ADDONS_EDIT are currently allowed to use unsafe methods everywhere when dealing with addons. (We might want to remove that in the future, very few folks have that permission and they very rarely need to make modifications anyway - but let's keep it for now)

Comment thread src/olympia/api/permissions.py Outdated
Comment on lines +240 to +241
can_access_because_admin = acl.action_allowed_for(
request.user, amo.permissions.ADDONS_EDIT

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same as above

@chrstinalin chrstinalin force-pushed the 2033254-rev-owner branch 2 times, most recently from bdbb9e7 to 84bc0c3 Compare July 3, 2026 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants