Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@firebase/rules-unit-testing does not test storage "update" security rules (always evaluates "create" rule, even if file is being updated not created) #5079

Closed
lovelle-cardoso opened this issue Jun 30, 2021 · 15 comments

Comments

@lovelle-cardoso
Copy link
Contributor

[REQUIRED] Describe your environment

  • Operating System version: Windows 10 Home Version 20H2, Build 19042.1052
  • Browser version: N/A (using mocha and @firebase/rules-unit-testing testing library)
  • Firebase SDK version: @firebase/rules-unit-testing 1.3.8
  • Firebase Product: storage, storage emulator

[REQUIRED] Describe the problem

It is not currently possible to unit test "update" storage security rules using @firebase/rules-unit-testing. When you try to overwrite an existing file, the security rules always check the "create" rule instead of the "update" rule.


Steps to reproduce:

Step 1: Create a storage.rules file with rules that allow creating but not updating

rules_version = '2';
service firebase.storage {
  match /b/{bucket}/o {
    allow read:  if true;
    allow write:  if false;
    match /folder/{fileId} {
      allow create: if true;
    }
  }
}

Step 2: Write a unit test that checks that creates are allowed and a unit test that checks that updates are denied

import * as firebase from "@firebase/rules-unit-testing";
process.env.FIREBASE_STORAGE_EMULATOR_HOST = "localhost:9199";
(global as any).XMLHttpRequest = require("xhr2");

describe("storage update bug", () => {
  it("can create", async () => {
    const client = firebase.initializeTestApp({  storageBucket: "test-bucket" }).storage();
    // Create 5mb file
    const file = Buffer.allocUnsafe(5000000);
    await firebase.assertSucceeds(
      client.ref().child("folder/dummy").put(file).then()
    );
  });
  it("CANNOT update", async () => {
    const client = firebase.initializeTestApp({  storageBucket: "test-bucket" }).storage();
    // Create 5mb file
    const createFile = Buffer.allocUnsafe(5000000);
    await firebase.assertSucceeds(
      client.ref().child("folder/dummy").put(createFile).then()
    );
    // Update 5mb file (should fail, but does not fail!)
    const updateFile = Buffer.allocUnsafe(5000000);
    await firebase.assertFails(
      client.ref().child("folder/dummy").put(updateFile).then()
    );
  });
});

[REQUIRED] Expected behavior

Should throw a permission denied error. And "CANNOT update" test should pass.


[REQUIRED] Actual behavior

No permission denied error is thrown. The client is allowed to update the file despite the fact that the rules say they should not be allowed to update the file. And the "CANNOT update" test fails.

@lovelle-cardoso
Copy link
Contributor Author

lovelle-cardoso commented Jun 30, 2021

I also tried changing the rules to only allow writes if the resource is null, but no luck. The user can still overwrite a file that already exists, even though the security rules should be preventing that.

Here are the various rules I've tried. For all of these, overwriting files is still allowed even though it shouldn't be

rules_version = '2';
service firebase.storage {
  match /b/{bucket}/o {
   match /{path=**} {
      allow read:   if true;
      allow create: if true;
    }
  }
}
rules_version = '2';
service firebase.storage {
  match /b/{bucket}/o {
   match /{path=**} {
      allow read:   if true;
      allow create: if true;
      allow update: if false;
    }
  }
}
rules_version = '2';
service firebase.storage {
  match /b/{bucket}/o {
   match /{path=**} {
      allow write: if resource == null;
    }
  }
}
rules_version = '2';
service firebase.storage {
  match /b/{bucket}/o {
   match /{path=**} {
      allow write: if resource.size == 0;
    }
  }
}

@digimbyte
Copy link

Confirmed this is an issue outside of Unit-testing, this is an issue with production as well.

@digimbyte
Copy link

digimbyte commented Jun 30, 2021

minimal project - storage create only fails.zip
image

Here is a minified project, it pushes the data without auth, and on complete, it attempts it again.
to setup:
add your firebase config to the .env.local example file

Define Security Rules for Storage as above, example:

service firebase.storage {
  match /b/{bucket}/o {
    match /{allPaths=**} {
      allow read:   if true;
      allow create: if true;
      allow update: if false;
    }
  }
}

run npm install
run npm run dev
then Open the browser to localhost:3000
Open browser console Ctrl+Shift+J or F12

Per the documentation, it is implied a user can prevent overwrites by checking the update:

    // A write rule can be divided into create, update, and delete rules
      // Applies to writes to nonexistent files
      allow create: if <condition>;

      // Applies to writes to existing files
      allow update: if <condition>;

The documentation needs to be updated if this is false, otherwise there is a potential bug in Security Rules

@looptheloop88 looptheloop88 added the testing-sdk testing with emulator label Jun 30, 2021
@lovelle-cardoso
Copy link
Contributor Author

lovelle-cardoso commented Jun 30, 2021

All right so after doing extensive testing in both the unit testing library and in a production environment, here's the conclusion I've arrived at. There's a lot of misinformation on stackoverflow about this specific issue, and even google firebase employees have responded with with solutions that don't work in the current version of storage security rules:

https://stackoverflow.com/questions/38995011/firebase-3-storage-dont-allow-file-updating-overwriting
https://stackoverflow.com/questions/43392795/firebase-storage-rule-to-disallow-overwrite
https://stackoverflow.com/questions/39664404/firebase-storage-prevent-spam

So I'm going to state my findings here for the record:

Firebase storage security rules currently cannot differentiate between "uploading a new file" and "uploading a new file that overwrites an existing file". All file overwrites are evaluated using the "create" rule, NOT the "update" rule, and the resource value for a "write" rule is always null when you upload a file, even if there is already an existing file at that location.

Please note, that this behavior is VERY DIFFERENT than firestore security rules, where the create rule only applies if there isn't already a document existing at that path and in a write rule, the resource can be used to check if there is a already a document existing at that location.

In storage security rules, the "update" rule ONLY applies to write requests that just update a file's metadata, NOT to write requests that overwrite a file that exists at the same path.


Because of this limitation, it is not currently possible to write a storage security rule that prevents a user from overwriting existing files.

Please note that this limitation is not stated anywhere in firebase storage's documentation (neither in the storage security rules guide nor the storage security rules API reference).

And this limitation is present in production environments (not just the unit testing library like I originally thought).


So please exercise caution if you are relying on storage security rules to prevent users from overwriting files. That functionality is simply not possible in the current version of firebase storage security rules. You'll need to route your storage uploads through a cloud function or your own server to support that functionality.

If this limitation is actually just a bug, then hopefully it will be fixed soon. But if it is intentional, I'm hoping that firebase will update their documentation to be more clear about what they consider an file "update" and what they consider a file "creation". Because, it seems like several users (and even google employees) have been tripped up by the vagueness of the documentation in this specific area.

Hopefully this helps out anyone who's also confused by this behavior!

@looptheloop88
Copy link

Hi @lovelle-cardoso, apologies for the delay in response and thank you for the report. It seems that you've spent time and effort in investigating the issue. We really appreciate it.

I used the sample project that you've shared and was able to replicate the behavior. I agree with you that it's not clear how to trigger the "update" storage security rules. Also, writing to the same path in the bucket will not trigger it.

Let me discuss this matter with our engineers here and update this thread if I have any information to share.

@looptheloop88
Copy link

I also confirmed that the "update" rule is working fine using the rule simulator.

@looptheloop88
Copy link

I filed an internal bug (b/192561585) for this issue.

@lovelle-cardoso
Copy link
Contributor Author

lovelle-cardoso commented Jul 1, 2021

I also confirmed that the "update" rule is working fine using the rule simulator.

I also checked it out in the rules simulator. But it's still unclear what the "update" in the rules simulator is actually doing. Is it attempting to write to a path that already contains a file? Or is it (like I suspect) simply "updating" the metadata of an existing file.

Like I mentioned in my report: updating the metadata of an existing file does trigger the update rule (in the simulator, in production, and in the unit testing library). However, actually overwriting an existing file does not. So I suspect that the simulator may be just performing a metadata update but not actually attempting to overwrite an existing file.

I think this may all boil down to some confusion over what "update" truly means in the storage library. Some users seem to believe the "update" rule applies to file overwrites, but the actual implementation of the "update" rule seems to only apply to metadata updates, not file overwrites.

Further support for that theory is that the firebase cloud functions only support onCreate and onDelete triggers for storage changes. And onDelete is called even for file overwrites. (A file overwrite will trigger the onDelete function for the old file at that location, and a onCreate file for the new file you are overwriting it with).

Which leads me to believe that firebase storage simply treats file overwrites as a delete and create operation happening in quick succession, not its own special operation.

None of the documentation contradicts this persay. But considering how many users (including myself) have just assumed that the storage security update rule would work the same as the firestore security update rule, I believe its worth clarifying in the documentation what firebase storage considers an update and what it does not.

(Also in my specific use case, the security rules I need to enforce depend on being able to prevent users from overwriting files that already exist. So even if the functionality of the update rule stays as is, if some other mechanism could be provided that would allow us devs to write a rule to prevent file overwrites (like perhaps making sure the resource value is populated during overwrites so we can check for it) that would help out a bunch!)

@digimbyte
Copy link

digimbyte commented Jul 2, 2021

It's a fundamental concept to prevent file overwrites, the same way you would with Firestore and the create method.

Currently, the resource.size and other file data doesn't appear to be relative to the actual source, even != null validation on the file doesn't affect the create method.

@imbradyboy
Copy link

I can also confirm that there are no security rules to prevent a file from being updated/overridden. As per the previous comments I tried update, delete, resource.size == 0, resource == null, and as many other combinations of security rules that I could think of and nothing worked.

This is a vital piece of my app, so the support of this functionality in the near future is important.

@digimbyte
Copy link

According to GCP support, this is controlled with Versioning enabled:
https://cloud.google.com/storage/docs/object-versioning

as "update" only applies to the metadata of the object.
https://cloud.google.com/storage/docs/json_api/v1/objects/update

@lovelle-cardoso
Copy link
Contributor Author

lovelle-cardoso commented Jul 6, 2021

Thanks to @digimbyte I finally figured out how to get this thing working! Here's the process for anyone else who would also like to be able to prevent file overwrites using firebase storage security rules:

  1. Install the gsutil cli tool: https://cloud.google.com/storage/docs/gsutil_install
  2. Use this command to enable versioning on your bucket: gsutil versioning set on gs://your-project-id-here.appspot.com
  3. Change your security rules to allow create: if resource == null or allow write: if resource == null

And that's it! With versioning enabled, the resource value is now properly populated in storage security rules, just like it would be in firestore security rules. Though the firebase storage documentation really doesn't mention this anywhere, make sure you remember that the update rule only applies to file metadata updates, and not file overwrites. And the resource value will never be populated in the firebase storage security create rule unless you turn on versioning.

Since we're just using versioning as a workaround to fix storage security rules and make them work as you would expect, you probably only need to retain 1 version of each object. To do that, simply create a lifecycle rule to delete objects with +1 newer versions

  1. Go to https://console.cloud.google.com/storage/
  2. Click on your bucket
  3. Click the LIFECYCLE tab
  4. Click ADD A RULE
  5. Add a rule to delete an object if it has a newer version
    image

With all that in place, you can finally prevent file overwrites through firebase storage security rules!

However, I do think users would really appreciate if firebase updated their documentation to clarify that you won't be able to prevent file overwrites through storage security rules, unless you turn on object versioning for your bucket.

An even better solution would be if firebase changed their default storage bucket setup process so when you enable storage for firebase, firebase will create a bucket with object versioning already turned on and with a lifecycle rule to delete objects when a newer version exists. That would cause much less friction and confusion for developers who may not be very familiar with the gcs interface.

@lovelle-cardoso
Copy link
Contributor Author

Btw, the "turn on versioning" workaround will not help for unit testing, since there is no way to turn on object versioning in the storage emulator. So either this issue will need to be resolved by firebase in some other way that doesn't involve turning on versioning, or they'll need to add a way to configure versioning for the storage emulator. Just wanted to make that extra clear for anyone who's relying on the storage emulator for testing or local development.

@digimbyte
Copy link

digimbyte commented Jul 6, 2021 via email

@hsubox76
Copy link
Contributor

As part of an issue cleanup effort, we are closing old issues related to versions before 9. The SDK had a large rewrite for version 9 to introduce the modular API, and issues opened under previous versions may no longer apply or may manifest in a different way. If you are still experiencing the same issue with the latest version of the Firebase JS SDK, please open a new issue with as much information as you can provide about how you are currently experiencing it. Thank you!

@firebase firebase locked and limited conversation to collaborators Aug 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants