Firebase-js-sdk: Snapshot throws error after document is deleted

[pfiadDi]

[REQUIRED] Describe your environment

• Operating System version: mac 10.12.6
• Browser version: chrome 80
• Firebase SDK version: 7.14
• Firebase Product: Firestore Emulator

[REQUIRED] Describe the problem

• I listen realtime to a Firestore Snapshot
• When I delete one document I get an error from this snapshot because of a security rules violation
• In my rule, I check for a uid field which should be the same as the uid in the auth object.
• After deletion I get:

FirebaseError: Null value error. for 'get' @ L52

Which is this rule:

function isOwner() {
      return request.auth.uid == resource.data.uid;
}
allow read: if isOwner();

I assume uid doesn't exist in the deleted document (of course not) but how to avoid that?
If I remove this rule it works but - yeah that can't be the solution ;-)

Interestingly, the snapshot is updated correctly. So If I have three documents after deletion and before the error I only have two again.

Steps to reproduce:

snapshot code:

 fsDatabase.collection(collection).where("uid", "==", "userUid")
      .onSnapshot(function (querySnapshot) {
        if (querySnapshot.docs.length == 0) {
           console.log("no docs");
        } else {
          let docs = [];
          querySnapshot.forEach(function (doc) {
            docs.push({ "id": doc.id, "data": doc.data() });
          });
          console.log(docs);
        }
      }, function (err) {
        console.log(err);
      });

security rules:

    function isOwner() {
      return request.auth.uid == resource.data.uid;
    }
      allow read: if isOwner();

[pfiadDi]

Update: It seems it's an emulator issue - it works when I connect to my project and not to the Firestore emulator...

[schmidt-sebastian]

@pfiadDi Thanks for reporting this. I will defer to @yuchenshi to see if this is an issue in the Emulator.

[yuchenshi]

@pfiadDi Would you mind posting the full security rules or at least a minimal reproduction? I will need that to further dig into this.

[pfiadDi]

Sure np @yuchenshi

rules_version = '2';
service cloud.firestore {
  match /databases/{database}/documents {

    //match /{document=**} {
      //allow read, write: if true;
    //}

    //check if field exists
    function isDefined(field) {
     return field in request.resource.data
    }



    // Only use after checking if the field exists
    function val(field) {
      return request.resource.data[field];
    }


    function isString(field) {
        return string(val(field)) == val(field);
    }

    function isFloat(field) {
        return float(val(field)) == val(field);
    }

    function isAuthenticated() {
      return request.auth != null;
    }

    function isOwner() {
      return request.auth.uid == resource.data.uid;
    }

    function isValidAuthUid() {
      return request.auth.uid == request.resource.data.uid;
    }

    function isValidLink(field) {
      return isString(field) && val(field).matches("^https:\\/\\/www\\.domain\\.com\\/path\\/to\\/thing\\/.{1,}|^https:\\/\\/www\\.domain\\.com\\/sales\\/path\\/to.{1,}");
    }

    function isValidStatus(field) {
      return isString(field) && val(field) in ["Active","Draft","Paused","Ended"];
    }

    match /campaigns/{campaignId} {
      allow create: if isAuthenticated() && isValidAuthUid();
      allow delete: if isOwner();
      allow read: if isOwner();
      allow update: if isOwner() &&
           (!isDefined('name') || isString('name')) &&
           (!isDefined('searchResults') || isValidLink('searchResults') ) &&
           (!isDefined('invitationMsg') || val('invitationMsg').size() < 301) &&
           (!isDefined('msg1') || isString('msg1')) &&
           (!isDefined('msg2') || isString('msg2')) &&
           (!isDefined('msg3') || isString('msg3')) &&
           (!isDefined('msg4') || isString('msg4')) &&
           (!isDefined('msg5') || isString('msg5')) &&
           (!isDefined('msg1Delay') || val('msg1Delay') >= 0) &&
           (!isDefined('msg2Delay') || val('msg2Delay') >= 0) &&
           (!isDefined('msg3Delay') || val('msg3Delay') >= 0) &&
           (!isDefined('msg4Delay') || val('msg4Delay') >= 0) &&
           (!isDefined('msg5Delay') || val('msg5Delay') >= 0) &&
           (!isDefined('status') || isValidStatus('status')) &&
           (!isDefined('teamId') || isString('teamId'));
    }
  }
}

When I change my read rule to isAuthenticated() I don't have the problem so it really get's down to that the uid field is null.

[yuchenshi]

@pfiadDi Thanks! I'm just looking at the error message here that you've posted: FirebaseError: Null value error. for 'get' @ L52. However, L52 is not on a allow statement. Are you sure that's the latest error message?

Without a fully functional reproduction, it's really hard for me to debug on my side. One other thing you can help is to checkout the rules coverage report in the emulator. It shows you in details what expressions are evaluated and what values they evaluate to or what errors they raise. Could you please use that to drill down what exactly is throwing the null value error? You can also attach the coverage report HTML file by renaming it to .txt and upload to GitHub, and I'll take a closer look.

[pfiadDi]

Sorry @yuchenshi in the meantime I think I deleted a function so the line numbers are messed up - let me reproduce the bug with the rules file I posted here! One minute

[yuchenshi]

@pfiadDi Take your time -- just make sure to attach the rules coverage report or find out what exact expression is throwing the error.

[pfiadDi]

OK @yuchenshi I was able to reproduce and the error is thrown at L53 which is the allow read:

and here is the coverage report and this rule has the null value error:
Firestore Rule Coverage Report.txt

If you like I can add you to the repo to run the code but it's elm and you'll need parcel to run it.

[yuchenshi]

Thanks for the coverage report. I can confirm the error is indeed at request.data.uid where request.data is null. However, if the error says for 'get' @ L53, it must mean that the operations performed was a get operation at the exact document. Your code snippet, fsDatabase.collection(collection).where("uid", "==", "userUid").onSnapshot, would generate a list request instead.

I also tried to look at the raw JSON source of your report, and it just contains way too many requests for me to pinpoint the issue. I see a low of get requests plus two list requests, and it's unclear to me which ones are allowed v.s. denied. I'm tempted to say that the deny must be from some other parts of your code. Could you please double check your app to make sure it's not sending get requests for the document that does not exist any more?

I don't know elm and I cannot really help much here, but my advice would be to:

1. Separate get and list in your rules, like allow get: if isOwner(); and allow read: if isOwner(); instead. This way, you'll be able to see which ones succeed and which ones fail in the coverage report more clearly.
2. You can intentionally make get and list behave differently, to see what parts of your application succeeds / fails. For example, does allow get: if true silence the error? If so, the problem cannot be with collection.onSnapshot which uses list.
3. Try to disable / stub out the parts of the code that is not under test, so your rules coverage report contains only the request in question.
4. If your app works otherwise fine and you're just trying to remove the error, a temporary workaround like allow get: if resource.data == null || isOwner() can do. That allows anyone to send a get request to a non-existent doc and confirmed that it does not exist indeed.

To save both of us time, let's spare the elm repo and I'll only look further into a minimal, self-contained reproduction. Thanks for your understanding.

[pfiadDi]

Thank you very much that is all great feedback and I will try it out. There are a ton's of get's since I have an autosave form so with every input (also every new character in an input field) I save the data. So the snapshot listener is updated a lot. But yeah I'll try the things you said.

Thank you for this fast and extensive feedback :)

[pfiadDi]

So I can confirm it's not the snapshot. I separated it to list and get and yeah get is what throws the error. So perhaps on my delete command something get's requested before the deleting cmd is done or so...
Your work around (if request.data == null ...) doesn't work in this case I get this error:
Property data is undefined on object. for 'get' @ L54 (it'as l54 now since I added the list rules above..) I'll keep digging thank you for your help

[pfiadDi]

If I comment the snapshot out and don't call it I don't get the error - that makes all a little bit strange ;-)

[yuchenshi]

Sorry it's resource.data. typo.

[pfiadDi]

Yeah also with this workaround I get this error - only with get if true - I get rid of the error. I'll keep looking

[pfiadDi]

Hi, @yuchenshi I have now a working example of the error with pure javascript.
It's a little bit of a mess but I've thrown it together based on the functions of my app.
https://github.com/pfiadDi/firestore-snapshot-bug

I use parcelJS to pack it (since parcel builds the files into a folder called dist I set the public folder at firebase init to dist). So after you build it with parcel index2.html you can close the parcel dev server again and start the emulators:

• open the page
• login
• add data with a click on "Click to add data"
• delete one doc with a click on "This deletes doc1"

The error is caught in the snapshot function and logged in the console.

I hope it reproduces also on your end ;-)

[pfiadDi]

Oh and as you can see - I only use the onSnapshot function to get doc's - so no function to query a single document which could invoke the get rules error...

[pfiadDi]

And last thing :) - which is perhaps most important, I tested it with the file of the repo again on the real Firestore database, not on the emulator, and with the same rules. There is no error. So it really seems to be an issue with the emulator.

[yuchenshi]

@pfiadDi Sorry I was quite overwhelmed and did not get to you sooner. I think this correlates pretty well with https://github.com/firebase/firebase-tools/issues/2197 and it may be a real bug in the Firestore Emulator. I'm going to merge this into the issue above so that we can better track this, since the bug is in the Firestore Emulator instead of SDKs. But thanks a ton for the repro and I'll include that in the said bug as well.