FR: Prevent something like "SQL Injection" out-of-the-box

[IchordeDionysos]

• Operating System version: Cloud Functions environment
• Firebase SDK version: 5.13.0
• Firebase Product: Firestore

Something similar to a SQL injection, would possible with the Firebase Admin SDK in a code like this:

app.post("/getDocument", async (req, res) => {
    const ref = firestore.collection("users").doc(userId);
    const snap = await ref.get();
    const doc = snap.data();
    res.status(200).send(JSON.stringify(doc));
});

A user could hack into the system by sending not only the sending the userId but also some additional path to a subdocument, like this: [[userId]]/secrectCollection/[[id]].

The code above would return this, even if the user isn't expected to be allowed to retrieve this document. I know I should have checked the Bearer and so on, but let's assume that I've done this, but including a middleware and so on.

There may be some cases where a developer won't expect to that there may be a path to a document and rather only would like to allow an id.

My suggestion would be to allow a path to a document or a collection only in the .collection() and.doc() functions of the interface FirebaseFirestore.Firestore and not on FirebaseFirestore.DocumentReference and FirebaseFirestore.CollectionReference

So this would be allowed:
VERSION 1:

admin.firestore().doc(`users/${userId}/secrectCollection/${someId}`)

And this will cause an error:
VERSION 2:

admin.firestore().collection("users").doc(`${userId}/secrectCollection/${someId}`)

I personally don't see any drawback on this, besides backwards compatility, as you could always use VERSION 1, which could be more complicated, but I think would make the Firestore in the Admin SDK more secure to prevent accidentially unexpected/unhandled paths.

What do you think?

[mbleigh]

In general, we'd recommend just connecting to the database directly if you're just reading a document. I'm sure this example is simplified/contrived, but Security Rules exist to definitively manage exactly this kind of issue.

Your proposal is an interesting one, but I think it ultimately creates a confusing API. The same method having different rules depending on when you call it is something that's likely to surprise developers.

Ultimately if you're using user-supplied data to call the Admin SDK, you should always sanitize and carefully validate the data*. This would be true even with your proposed change.

Thanks for the suggestion, though!

[IchordeDionysos]

Yeah, this example is massively simplified, so this is not a simple use-case like this.

Ahh and to the point of confusing API. Currently, there are different functions available depending on what type of object you are working on.
E.g.
Firestore: Both .collection() and .doc()
DocumentReference: Only .collection()
CollectionReference: Only .doc()

1. I probably thought because of this decision to only allow the opposite that you can ONLY get a direct child of the reference. I thought several times why isn't there a .doc() function on a DocumentReference since I discovered that I can also add a path here.

2. I think there would be a way to add this feature in a non-confusing way :D
   Just add two new functions: .childDoc() and .childCollection() or something with a similar name.

So with the 1) and 2) the API could look like this:
Firestore has all of these .collection(), .doc(), .childCollection(), .childDoc()
DocumentReference has .collection(), .doc(), .childCollection()
CollectionReference has .collection(), .doc(), .childDoc()

And to sanitization YES, we ARE doing this for everything, this is just another addition to the Firestore API as a second chance to prevent errors like this.

In SQL would never put in user-supplied data directly in a query you would always use parameterized queries.

[schmidt-sebastian]

With any kind of API, we would still ask you to sanitize your input. When you are using the Server SDKs, you have full admin access to Firestore, and hence any write that is based on user data needs to be guarded against any type of injection attack.

I will push a PR to disallow slashes in DocumentReference.doc() and CollectionReference.collection(), as this is already the case in the Firestore Mobile SDKs. As with these SDKS, the main accessors (Firestore.collection(), Firestore.doc()) will remain unchanged. This ensures parity across the SDK surfaces and addresses part of your concern.

We have explicitly decided to support slashes in Firestore.collection() and Firestore.doc() to allow for easy access to nested documents and subcollections. You should never pass unsanitized user input to these helpers.

Update: The Mobile SDKs do allow slashes in all accessors. To ensure API parity, I will leave the Server SDK surface as is.