4

In my server app I want to return a "forbidden" value when the user has no permissions for the endpoint.

To this end I create a rejected promise for reuse:

export const forbidden = Promise.reject(new Error('FORBIDDEN'))

and then elsewhere in the app:

import {forbidden} from './utils'

...

    resolve: (root, {post}, {db, isCollab}) => {
        if (!isCollab) return forbidden
        return db.posts.set(post)
    },

However, when I start my app I get the warning

(node:71620) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: FORBIDDEN
(node:71620) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

How can I tell Node that this Promise is fine to be unhandled?

w00t
  • 17,944
  • 8
  • 54
  • 62
  • possible duplicate: https://stackoverflow.com/questions/45771024/how-to-properly-throw-an-error-if-promise-is-rejected-unhandledpromiserejectio – Raghav Garg Aug 26 '17 at 12:13
  • @RaghavGarg not quite, what I want is the reverse :) – w00t Aug 26 '17 at 12:15
  • Well, it's sort of not fine. Have you read the deprecation warning? I suggest you handle the error correctly – James Aug 26 '17 at 12:19
  • @James there is no error to handle, this is a pre-created rejection, to be returned when the user has no access. If I handle the error, it's not an error any more. – w00t Aug 26 '17 at 12:30
  • But are you handling it in any part of your app yet? I mean when the user has no permission? – Mekicha Aug 26 '17 at 12:38
  • @Mekicha of course, it is caught and transformed in the access layer before responding to the client. I use a library which expects promise rejections for this. – w00t Aug 26 '17 at 12:39
  • Please show an example of how your currently using this – James Aug 26 '17 at 12:41
  • You can try to suppress the error by doing this: `export const forbidden = Promise.reject(new Error('FORBIDDEN')).catch(() => {});` – Mekicha Aug 26 '17 at 12:42
  • 1
    @Mekicha then it is no longer a rejected promise… – w00t Aug 26 '17 at 12:45
  • It is. Because you are passing an empty function to `catch` – Mekicha Aug 26 '17 at 12:46
  • 1
    @James I added example use. @Mekicha the empty catch just converts it into a promise for `undefined`, no? – w00t Aug 26 '17 at 12:51

3 Answers3

2

I create a rejected promise for reuse

Well don't, it might be a lot easier to just create a function for reuse:

export function forbidden() { return Promise.reject(new Error('FORBIDDEN')); }

That will also get you an appropriate stack trace for the error every time you call it.

How can I tell Node that this Promise is fine to be unhandled?

Just handle it by doing nothing:

export const forbidden = Promise.reject(new Error('FORBIDDEN'));
forbidden.catch(err => { /* ignore */ }); // mark error as handled

(and don't forget to include the comment about the purpose of this seemingly no-op statement).

Bergi
  • 630,263
  • 148
  • 957
  • 1,375
  • Ooh that looks like it might work! Not at my computer right now but will check asap. – w00t Aug 26 '17 at 14:13
  • @w00t this was basically what I was getting at by "handle the error correctly". An unhandled rejection is effectively an unhandled exception. – James Aug 27 '17 at 16:05
  • @james yes but I didn't think to do the handling separately. It works :) – w00t Aug 27 '17 at 17:18
  • BTW I didn't want to create a stack trace for every forbidden error because I don't want to leak details about my app. So I prefer to create the rejection only once. – w00t Aug 30 '17 at 08:44
  • "doing nothing" is changing 'forbidden' to a resolved promise with value 'undefined'. It's doing something. – Craig Hicks Dec 04 '20 at 05:40
  • 1
    @CraigHicks `forbidden` stays rejected. It's the return value of `.catch()` that gets fulfilled with `undefined`. And while `catch` does something, it's callback doesn't. – Bergi Dec 04 '20 at 08:55
  • I see you have updated - but in the mean time I noticed that the OP's usage was not actually to throw, but to return `forbidden`. However, that doesn't work in the case of a synchronous function. I have subbmitted an edit to your answer with that update. – Craig Hicks Dec 04 '20 at 20:05
  • @CraigHicks Please post your own answer if you want to suggest a different solution. Not sure why you deleted it. – Bergi Dec 04 '20 at 20:45
  • @CraigHicks Also I disagree both with your assumption about the motivation of the OP and with your stance that returning rejected promises is an antipattern, so it would not be a good fit in my answer. – Bergi Dec 04 '20 at 20:50
  • It is a good idea to not assume anything about motivation, but rather describe the range of possibilities given the incomplete description of usage. Additionally, the term "anti-pattern" doesn't really add anything that couldn't be transmitted by noting that it is an unusual pattern, that returning a rejected promise has different behavior when returned from sync vs async functions, and considering ease of code maintenance. – Craig Hicks Dec 04 '20 at 22:10
  • 1
    @CraigHicks The term "anti-pattern" has a [precise meaning](https://en.wikipedia.org/wiki/Anti-pattern) of "ineffective pattern that you should avoid". If that's not what you meant, don't use the term, and just say that you think it's unusual. – Bergi Dec 04 '20 at 22:23
2

I wouldn't recommend using the return statement to provide an Error - this is ignoring the exact intention of throw!

Simply use:

if (!isCollab) throw new Error('FORBIDDEN');

If you don't want a stack trace there's no need to over-engineer - simply do:

if (!isCollab) throw 'FORBIDDEN';

If you need a message property to exist you can simply use:

if (!isCollab) throw { message: 'FORBIDDEN' };

(Note: I recommend against throwing anything other than an instance of Error! You'll regret it later when things break and you need to debug)

Gershom Maes
  • 7,358
  • 2
  • 35
  • 55
  • 1
    Yes, as a maintenance programmer that is what I would to see. The OP's concern is about stack leakage. I think @Bergi assumes OP doesn't want the client (another js program using the OP's runtime lib) to see the stack trace. From an I/F point of view just defining `export const forbidden=Symbol('FORBIDDEN')` and using it would solve that. Whether or not there is even a valid general concern about accidentally leaking stack info with rogue exceptions ---- I accept that may be a "bridge too far". – Craig Hicks Dec 04 '20 at 23:40
  • Leaking the stack is a strange concern to have, and I highly doubt the security benefits will outweigh the impact to maintainability. But if for some reason that's a high priority, `throw 'FORBIDDEN';` ticks all the boxes. – Gershom Maes Dec 10 '20 at 22:13
  • - The problem with that is the widespread expectation that catch(e){log(e.message);} is a valid operation. System exceptions are all `Error` instances, `Error` seems to be a part of js language specs, it is native in every js implementation, and they all have a message field. Throwing a string from a library would result in library user causing exceptions by trying to print e.message. – Craig Hicks Dec 11 '20 at 02:55
  • 1
    If there are expectations of the error's structure I recommend `throw { message: 'FORBIDDEN' };` – Gershom Maes Dec 11 '20 at 04:09
1
  • OP's usage is not completely described, but the OP's comment "BTW I didn't want to create a stack trace for every forbidden error because I don't want to leak details about my app. So I prefer to create the rejection only once." leads me to believe that at least part of the OP's motivation is to prevent info leakage from unhandled rejections of forbidden.

  • Returning a rejected (but defused) promise behaves differently in a sync vs. an async function. In the former the promise is returned verbatim. In the latter it is rewrapped in a promised and automatically rethrown (equivalent to throwing from inside the function). Whichever use was intended, it makes the program harder to understand.(Wrapping the Promise to be returned in an object or array would solve that problem).

Difference in behavior between sync and async funcs when returning forbidden :

async function test(){
  try {
    let a = await (async()=>{return forbidden;})();  
  } catch(e){console.log(e.message);} // outputs: 'FORBIDDEN'
  try {
    let a = (()=>{return forbidden;})();
    // error undetected  
  } catch(e){console.log(e.message);} // never reaches here !!!
  console.log("something else"); // outputs: something else
  let a=(()=>{return forbidden;})();  // UHR + '#<Promise>' + no addr
  console.log("something else"); // outputs: something else
  await (async()=>{return forbidden;})();  // UHR + '#<Promise>' + no addr leak}
}
test();
  • Regardless of the the OP's usuage, leakage of program info from unhandled-rejections is a valid concern.

The below factory function makeError would provide a general solution, and it builds on the OP's original inspiration:

const verboten=new Error('verbotten');
const makeError = () => verboten;
async function test2(){
  try {
    await (async()=>{throw makeError();})();  
  } catch(e){console.log(e.message);} // outputs: 'verboten'
  // uncomment the following to trigger UHR (unhandled rejection)
  //await (async()=>{throw makeError();})();  // UHR + 'verboten' + no addr leak
}

Note that makeError returns the constant object verboten, rather than itself. (Yes, that is allowed, although it is rarely used.) So the stack trace is a fixed value, unrelated to the error location in the program, just like the original OP's forbidden.

That's fine for the release version, but a minor change to makeError could be made for a development version, where seeing the stack is useful:

const makeError = Error;
Craig Hicks
  • 2,199
  • 20
  • 35
  • "*OP's motivation appears to be preventing internal stack info leakage*" - no, absolutely not. They want to create "*a "forbidden" value*" they can return from arbitrary functions to signify that "*the user has no permissions for the endpoint*". That's all. – Bergi Dec 04 '20 at 22:25
  • You seem to say that `throw`ing an error is better than returning a rejected promise, which is a suitable solution if you have to deal with synchronous functions as well (which is not always the case). But I don't see what this has to do with subclassing `Error`? – Bergi Dec 04 '20 at 22:29
  • @Bergi - No, that is not all. See OP's comment under your answer *"BTW I didn't want to create a stack trace for every forbidden error because I don't want to leak details about my app. So I prefer to create the rejection only once."*. Even if the OP had not stated that explicitly, it was hard not to assume otherwise. – Craig Hicks Dec 04 '20 at 22:37
  • @Bergi - Given the OP's **explicitly** stated goal of not leaking info from displayed stack trace after an unhandled rejection, it makes sense. I have used the OP's original inspiration to create an Error subclass that always returns a fixed unrelated stack trace. – Craig Hicks Dec 04 '20 at 22:41
  • I understood that to not leak the stack trace from the endpoint, where the wrapper handles the rejection and sends the error to the client. Nothing about leaking it from the unhandled rejection, which was unexpected after implementing their solution. – Bergi Dec 04 '20 at 22:44
  • "*I have used the OP's original inspiration to always return a fixed unrelated stack trace*" - ah, now I understand. There's no reason to use an `Error` subclass for that though, a simple factory function would have achieved the same. In development, you can just swap `const makeError = () => verboten;` against `const makeError = Error;`. – Bergi Dec 04 '20 at 22:50
  • Presumably the client is a js program using the OP's module, same process, same thread. (Nobody would JSONify the stack trace). Now the client is has a rejected promise, and it is a potential bomb if value is transformed through an await in their own code. Do they know that? Sounds bad to me. `const forbidden = Symbol('forbidden')` is the way to go, isn't it? – Craig Hicks Dec 04 '20 at 23:05
  • 1
    *"a simple factory function would have achieved the same"* - I agree that is better. Also, as my code stands, `((new MyError()) instanceof MyError)` evaluates to false, which is completely counter intuitive. So even if a subclass were desired (a common pattern), another level is required. – Craig Hicks Dec 04 '20 at 23:22