Make telefuncs default safe?

[redbar0n]

Reading https://telefunc.com/shield I am concerned that someone inevitably will forget to include a throw new Abort() or a shield() command, and thus expose functions that are dangerous.

How about making telefunc default safe / restrictive, and then only opening up based on an explicit whitelisting approach, instead of a blacklisting approach?

So instead of shield() you could have unshield() or expose(), and similar for Abort. All functions could throw new Abort() abort by default, unless the context is set explicitly.

So instead of:

  if( !context.user?.isAdmin ) {
    throw new Abort()
  }
  const result = await database.runSQL(query)
  return result

It could be something like:

  if( context.user?.isAdmin ) {
    const result = await database.runSQL(query)
    return result
  } else {
    throw new Abort() // could be made default, so this else-condition would not be needed
  }

[brillout]

I am concerned that someone inevitably will forget to include a throw new Abort() or a shield() command, and thus expose functions that are dangerous.

I agree that's a problem. It's actually a problem with RPC in general.

Or something like this:

export async function runSQL(query) {
  const { user } = getContext()
  if (user.isAdmin) {
    allow()
  }
  const result = await database.runSQL(query)
  return result
}

But the problem with that is that allow() cannot be called after await:

export async function someTelefunction() {
  await somePromise;
  // This won't work
  allow()
}

In JavaScript, async call stacks cannot be traced.

[redbar0n]

Yeah, that could work. I think that allow() is a little too undescriptive ("allow what?"). Maybe allowRPC or something, instead. Also, I feel that it is a bit imperative. Generally, I would prefer expressions over statements. So maybe something similar to a hook-style interface?

[brillout]

Generally, I would prefer expressions over statements. So maybe something similar to a hook-style interface?

What do you mean?

[redbar0n]

See expressions over statements, and statements suck.

I see that getContext() already looks a bit like hooks, so nvm that. What you have currently is similar to how NextAuth is called, so maybe its wise not to deviate too much from conventions.

[brillout]

If you have an example of how it could alternatively look like, I'm all hears.

I can't find a satisfactory solution for this so far.

[redbar0n]

Hm.. I can't seem to think of a good solution either. How do others handle auth server-side?

How would allowRPC() work inside, anyways? Set a global? I can't really see how it could prevent the next lines from being executed...

Maybe this is a good time to update the doc on shield()?

What about simply:

export async function runSQL(query) {
  const { user } = getContext()
  if (user.isAdmin) {
    const result = await database.runSQL(query)
  }
  return null // but should probably throw or return an error
}

Yet the problem of remembering to do that isAdmin check remains. The only alternative I can think about right now is to wrap the whole database operation in some function that needs to take in a true boolean to actually do anything. But that might be too invasive and hide too much of the control-flow and direct DB access that the developer needs...

[brillout]

import { permission, getContext } from 'telefunc'

permission(runSQL, query => {
  const { user } = getContext()
  if (!user.isAdmin) {
    return false
  }

  // We can also access `query` here
  if (query.toLowerCase().includes('delete')) {
    return false
  }

  return true
})
export async function runSQL(query) {
  const result = await database.runSQL(query)
  return result
}

Also:

const callTelefunc = createTelefuncCaller({
  // All telefunctions are required to have a `permission()` defined.
  safeMode: true
})

Small teams of seniors would set safeMode: false, while large companies would set safeMode: true.

Thoughts?

[redbar0n]

I'm a bit hesitant to the idea of requiring a separate function (to the telefunction) where permissions are defined. I fear it will be lost, or they will go out of sync, as code is changed. It might also be because I generally don't like things that seem similar in principle to "decorators". Maybe because it makes the control-flow not that obvious.

How about my last suggestion. Would that work?

[brillout]

You mean the following?

export async function runSQL(query) {
  const { user } = getContext()
  if (user.isAdmin) {
    const result = await database.runSQL(query)
  }
  return null // but should probably throw or return an error
}

Not sure but I feel like it's maybe also prone to mistakes? E.g. the user could forget the user.isAdmin if-condition.

In general, while Telefunc is indeed more prone to safety mistakes, I think it's not that bad.

That said, I'm more than open to develop a strictMode to harden security, but I fail to see a "perfect" solution, yet.

[redbar0n]

You mean the following?

Yeah.

Not sure but I feel like it's maybe also prone to mistakes? E.g. the user could forget the user.isAdmin if-condition.

Yeah, that's an issue.

Hm. Ideally one shouldn't need to write authorization / permission checks for every function that is called. Maybe the context getContext() could be checked by telefunc before the user's function is called? In some kind of beforeEach() function, similar to how testing setup is done.

[brillout]

Seems to me that something like beforeEach() would actually be quite similar to my idea of permission().

Note that when safeMode: true then each telefunction would be required to have a permission() function defined.

Anyways, we have time to figure that out, it's important but not urgent. In the meantime, I think it's reasonable to assume Telefunc users to be careful.

Let me know if you find some eureka moment.

[redbar0n]

Seems to me that something like beforeEach() would actually be quite similar to my idea of permission().

Yeah, being able to define such a permission() function once and for all would be nice.

Anyways, we have time to figure that out, it's important but not urgent. In the meantime, I think it's reasonable to assume Telefunc users to be careful.

I agree. The best way forward would probably become apparent with use, where one would discover what would be most convenient.

[brillout]

@redbar0n https://telefunc.com/permissions#getcontext-wrappers — thoughts?

[redbar0n]

I love the idea of getContext wrappers!

// components/Comment.telefunc.js
// Environment: Node.js

import { getUser } from '../auth/getUser'
import { shield } from 'telefunc'

shield(onCommentDelete, [shield.type.number])
export async function onCommentDelete(id) {
  const { user } = getUser({ permission: 'admin' }) // user is unused?
  const comment = await Comment.findOne({ id })
  await comment.delete()
}

I like how clean it looks. But it looks like user is not used..

I am also wondering if the contents of auth/getUser.ts could be generalized.

[brillout]

// Environment: Node.js server

import { telefuncConfig } from 'telefunc'
// Enforce all telefunctions to have a permission function
telefuncConfig.permissionFunction = true

// components/Comment.telefunc.js
// Environment: Node.js server

import { permission } from '../auth/permission'
import { shield } from 'telefunc'

// Only admins are allowed to delete comments
shield(onCommentDelete, [shield.type.number], permission('admin'))
export async function onCommentDelete(id) {
  const comment = await Comment.findOne({ id })
  await comment.delete()
}

// auth/permission.js
// Environment: Node.js server

export { permission }

import { getContext } from 'telefunc'

function permission(permissionName) {
  return () => {
    if (permissionName === 'public') {
      return true
    }

    if (permissionName === 'admin') {
      const { user } = getContext()
      return user?.isAdmin
    }

    // ...
  }
}

This means that our telefunctions are guaranteed to be secure (as long as our permission() function is correct).

Thoughts?

[redbar0n]

Looks good! I especially like the enforcement, because that would make it default safe and turn it into a whitelisting approach. Maybe rename permissionFunction to enforcePermissions so it is a bit more descriptive.

[brillout]

shield() is not automatically generated when using TypeScript.

This means we cannot use shield() to set the permission function. But we can use a hook instead; closing in favor of https://github.com/vikejs/telefunc/issues/27.

[brillout]

is now automatically*