targaryen icon indicating copy to clipboard operation
targaryen copied to clipboard

Trailing backslash produces unreliable test results

Open lukepighetti opened this issue 6 years ago • 6 comments

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 avatar Sep 21 '18 13:09 lukepighetti

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 avatar Sep 21 '18 17:09 lukepighetti

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

lukepighetti avatar Sep 21 '18 17:09 lukepighetti

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 avatar Sep 21 '18 18:09 lukepighetti

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

lukepighetti avatar Sep 21 '18 18:09 lukepighetti

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 avatar Sep 21 '18 18:09 lukepighetti

@goldibex @mhuebert @pthrasher @FredrikL @dinoboff

lukepighetti avatar Sep 21 '18 18:09 lukepighetti