-
Notifications
You must be signed in to change notification settings - Fork 909
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
Comments
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
|
Confirmed this is an issue outside of Unit-testing, this is an issue with production as well. |
minimal project - storage create only fails.zip Here is a minified project, it pushes the data without auth, and on complete, it attempts it again. Define Security Rules for Storage as above, example:
run Per the documentation, it is implied a user can prevent overwrites by checking the update:
The documentation needs to be updated if this is false, otherwise there is a potential bug in Security Rules |
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 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! |
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. |
I also confirmed that the "update" rule is working fine using the rule simulator. |
I filed an internal bug (b/192561585) for this issue. |
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!) |
It's a fundamental concept to prevent file overwrites, the same way you would with Firestore and the create method. Currently, the |
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. |
According to GCP support, this is controlled with Versioning enabled: as "update" only applies to the metadata of the object. |
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:
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
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. |
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. |
You are correct, this is only a workaround since this should
be conditional based on Rules. This does not result in a perfect solution.
|
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! |
[REQUIRED] Describe your environment
[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
Step 2: Write a unit test that checks that creates are allowed and a unit test that checks that updates are denied
[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.
The text was updated successfully, but these errors were encountered: