goldibex / targaryen

Test Firebase security rules without connecting to Firebase.
ISC License
242 stars 36 forks source link

Trailing backslash produces unreliable test results #152

Open lukepighetti opened 6 years ago

lukepighetti commented 6 years ago

Hello,

I'm running into a problem with Targaryan where it says writes are being allowed (jest toAllowWrite) while updates are not (jest toAllowUpdate). The value is an object.

When I test in Rule Simulator in the firebase console it properly blocks the request. Targaryan allows it, but only if it's a "toAllowWrite" test

When should I use Write and when should I use Update?

Are there any known issues here?

Hoping for more clarity here.

lukepighetti commented 6 years ago

I think there is a bug here but I'm not sure.

I am able to set any type of value to any of these fields, as long as they all exist... and it lets me pass extra items that should be stopped by the "$other" rule

    "users": {
      "$uid": {
        ".validate": "newData.hasChildren(['name', 'avatar', 'user_status', 'email', 'fcm_token', 'time_created'])",
        "name": {
          ".validate": "((newData.isString() && newData.val().length <= 50) && newData.val().matches(/^(\\w)+$/))"
        },
        "avatar": {
          ".validate": "((newData.isString() && newData.val().matches(/^http/)) && newData.val().matches(/^(http).+(png|jpg|jpeg|gif)$/))"
        },
        "user_status": {
          ".validate": "(newData.isString() && ((newData.val() == 'new' || newData.val() == 'free') || newData.val() == 'premium'))"
        },
        "email": {
          ".validate": "(newData.isString() && newData.val().matches(/\\@/))"
        },
        "fcm_token": {
          ".validate": "newData.isString()"
        },
        "time_created": {
          ".validate": "newData.isNumber()"
        },
        "$other": {
          ".validate": "false"
        },
        ".read": "auth.uid == $uid",
        ".write": "auth.uid == $uid"
      }
    },

This is isAllowedWrite() to /users/2222/ by a user with uid 2222

{
      name: false,
      avatar: false,
      user_status: true,
      email: true,
      fcm_token: true,
      time_created: "this should fail"
    }

And it is allowing the write.

Here is the Bolt type


path /users/{uid} is User {
  read() { isCurrentUser(uid) }
  write() { isCurrentUser(uid) }
}

type User {
  name: UserName,
  avatar: ImageUrl,
  user_status: UserStatus,
  email: Email,
  fcm_token: String,

  time_created: Number,
}

type UserName extends String {
  /// usernames have to be medium length and be made only of word characters
  validate() { this.length <= 50 && this.test(/^(\w)+$/) }
}

type ImageUrl extends Url{
  validate() { this.test(/^(http).+(png|jpg|jpeg|gif)$/) }
}

type UserStatus extends String {
  validate() { this == "new" || this == "free" || this == "premium" }
}

type Email extends String{
  validate() { this.test(/\@/) }
}

isCurrentUser(uid) { auth.uid == uid }
lukepighetti commented 6 years ago

The amazing thing is I have another section of my rules that are very similar that work perfectly

lukepighetti commented 6 years ago

Ok, this is interesting.

path /users/{uid} {
    read() { isCurrentUser(uid) }
    write() { isCurrentUser(uid) }
}

This allows writes that it shouldn't.

path /users {

  path /{uid} is User {
    read() { isCurrentUser(uid) }
    write() { isCurrentUser(uid) }
  }
}

This blocks the writes appropriately.

lukepighetti commented 6 years ago

OK I have no idea. Everything is working as expected now and I cannot see where anything changed. :|

lukepighetti commented 6 years ago

OK. I found it.

expect(theNewUser).not.toSet("/users/2222", incorrectUserValue); works as expected expect(theNewUser).not.toSet("/users/2222/", incorrectUserValue); allows write that should be blocked

Apparently this trailing backslash is really jacking up the results?

Also, toSet is defined here

expect.extend({
  toRead: targaryen.toAllowRead,
  toSet: targaryen.toAllowWrite,
  toUpdate: targaryen.toAllowUpdate
});
lukepighetti commented 6 years ago

@goldibex @mhuebert @pthrasher @FredrikL @dinoboff