Firebase Security Rules gate your user's access to and enforce validations for Firestore, Firebase Storage, and the Realtime Database. It's important to code review the Security Rules, just like you code review the application code. Because these rules are written in a domain-specific language, that puts some people in the position of code reviewing something they don't feel like they understand.
If you're finding yourself in that position, don't worry! This post will walk through how to approach reviewing and giving good feedback on Security Rules. The examples will be from Firestore Security Rules, and are mostly applicable to Storage as well. If you're reviewing Realtime Database Security Rules, although these principles apply, the rules use a different language, so the examples will be different.
When you're looking at Security Rules, check first for any top level rules that apply to everything. A document can match multiple rules, and if any rule grants access, access is granted. In the example below, there's a specific rule that only grants authors access to post documents, but the global rule lets anyone on the internet read or write to any place in your database:
post
rules_version = '2'; service cloud.firestore { match /databases/{database}/documents { match /{document=**} { allow read, write: if true; } match /posts/{post} { allow read, write: if request.resource.data.authorUID == request.auth.uid ; } } }
It's important to look for the global match statement match /{document=**} throughout the entire rules file, not just at the top. If there are thorough rules but also a global match statement, universal access will still be granted through the global match statement.
match /{document=**}
There are a very small number of valid use cases for global match statements. For example, granting access to admin users:
rules_version = '2'; service cloud.firestore { match /databases/{database}/documents { match /{document=**} { allow read, write: if isAdmin(); } } }
If you find a global rule that is read, write: if true, you can stop your review and ask them to fix it. If you find any other condition on a global rule, make sure it makes sense for your application.
read, write: if true
The next thing to do is to look at the data that you're storing. What is Personally Identifiable Information (PII) that should only be accessed by that particular user?
Security Rules apply to entire documents, not just fields. You should be able to look at each kind of document that you have and describe which kind of users should be able to read it, create it, update it, and delete it.
If you find documents where it's fine for most of the document to be read by anyone, but a few fields need to remain private, break those fields into their own document; the easiest way to do this is usually to create a subcollection off the existing document.
Now check the Security Rules for each of the documents that you identified as containing PII. The best practice for PII is that it is keyed by the user's ID, and only that user is allowed access:
match /secrets/{uid} { allow create, update: if request.auth.uid == uid; }
This pattern only works if there's a max of one document in the collection for each user. For cases of multiple documents per user, you could create subcollections of a user document:
match /users/{uid}/secrets/{secret} { allow create, update: if request.auth.uid == uid; }
Or you could include the id of the user who should have access as an attribute, and restrict access to that user:
match /secrets/{secret} { allow create, update: if request.auth.uid == request.resource.data.uid; }
Ideally, you would check that each document is as locked down as possible, but if you have limited time, the most important thing to check is that PII lives in separate documents and those PII documents are only accessible to that user.
Just like application changes should come with test changes, so should Security Rules. Security Rules tests run against the Firebase Emulator Suite, and can be included in your CI setup.
If you're not yet testing Security Rules, check out this documentation to get started, this video for an overview of testing, and this blog post and video on adding your tests to CI.
If the Security Rules are fully tested, there should be a lot of tests. For each kind of document, there are usually four different kinds of access: read, create, update, and delete. (These are just the most common permissions to grant; create, update, and delete can be rolled up into write, and read can be broken into get and list.) For each permission, there should be a test of the happy path, granting permission, and a test for each situation that should deny access.
read
create
update
delete
write
get
list
Say I have one rule:
// firestore.rules allow update: if // User is the author resource.data.authorUID == request.auth.uid && // `authorUID` and `createdAt` are unchanged request.resource.data.diff(request.resource.data).unchangedKeys().hasAll([ "authorUID", "createdAt" ]) && // Title must be < 50 characters long request.resource.data.title.size < 50;
To completely test this, I would write tests around granting access and each way that access could be denied:
// test.js const firebase = require("@firebase/rules-unit-testing"); const dbAuthorAuth = firebase.initializeTestApp({ projectId: TEST_FIREBASE_PROJECT_ID, auth: { uid: "author", email: "alice@example.com" } }).firestore(); const dbOtherAuth = firebase.initializeTestApp({ projectId: TEST_FIREBASE_PROJECT_ID, auth: { uid: "other", email: "otto@example.com" } }).firestore(); describe("blog posts", () => { before(async () => { dbAuthorAuth.doc("drafts/12345").set({ authorUID: "author", createdAt: Date.now(), title: "Make an apple", content: "TODO!" }); }); it("can be updated by author if immutable fields are unchanged", async () => { await firebase.assertSucceeds(dbAuthorAuth.doc("drafts/12345").update({ title: "Make an app", content: "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat." })); }); it("cannot be updated by anyone other than the author", async () => { await firebase.assertFails(dbOtherAuth.doc("drafts/12345").update({ title: "Make an app", content: "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat." })); }); it("cannot be updated by author if the author ID is changed", async () => { await firebase.assertFails(authorDb.doc("drafts/12345").update({ authorUID: "New Person" title: "Make an app", content: "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat." })); }); it("cannot be updated by author if the created date is changed", async () => { await firebase.assertFails(authorDb.doc("drafts/12345").update({ createdAt: Date.now(), title: "Make an app", content: "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat." })); }); it("cannot be updated if the title is over 50 characters", async () => { await firebase.assertFails(authorDb.doc("drafts/12345").update({ title: "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.", content: "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat." })); });
As the reviewer, look through their test cases, and make sure they have tested each kind of document. If they've written good test descriptions, that's the easiest place to understand what the Security Rules are doing. Similarly, if they've written code comments in their rules, that can also be a good entry point to understanding the Security Rules.
Subcollections don't inherit the rules from the parent document, so make sure they're specifically covered in the Security Rules. Look at the subcollections you're storing in Firestore; if all documents should have the same access that a parent document has, then that parent document should use the glob syntax: match /post/{id=**}.
match /post/{id=**}
What's more common is that a subcollection has been broken out into a subcollection because a different person needs to read it, or someone else is allowed to write to it. In that case, check the Security Rules to make sure it has its own match statement. Nesting is only a stylistic difference; whether you nest your match statements or not, make sure this is a match statement for the documents in the subcollections:
// Nested match statements are fine match /post/{postID} { allow read: if ... match /comments/{commentID} { allow read: if ... } } // Unnested match statements are fine match /post/{postID} { allow read: if ... } match /comments/{commentID} { allow read: if ... }
If you're reviewing Realtime Database Security Rules, child nodes inherit from parent nodes, the opposite of the behavior in Firestore.
Security Rules can enforce type and data validations for specific fields in addition to preventing unwanted access. If you know that some documents have required fields, immutable fields, fields that must be a timestamp, or a field that must contain data in a specific range, check that those are enforced in the Security Rules for those documents. For example, in this rule, I'm checking that the immutable fields of authorUID, publishedAt, and url aren't changed by an update, and that the required fields of content, title, and visible are still present:
authorUID
publishedAt
url
content
title
visible
allow update: if // Immutable fields are unchanged request.resource.data.diff(request.resource.data).unchangedKeys().hasAll([ "authorUID", "publishedAt", "url" ]) && // Required fields are present request.resource.data.keys().hasAll([ "content", "title", "visible" ]);
If you use the more granular permissions of create, update, and delete in place of write, make sure that validations apply to both create and update.
Because the Admin SDK authorizes using service account credentials, all requests from the Admin SDK, including Cloud Functions for Firebase, bypass Security Rules. To give a thorough security review for a Firebase app, it's also necessary to look at any other writes that are happening to your backend via the Admin SDK.
For example, if I have great Firestore Security Rules, but a Cloud Function exports data to a storage bucket that has no Security Rules, that would be a terrible way to treat my user's data. The flip side of this is that Cloud Functions can be used in situations where Security Rules don't work, for example, returning individual fields from documents. See this blog post for tips to use Cloud Functions in conjunction with Security Rules.
