Throwing HttpsError('ok') leads to internal exception in httpsCallable

[alex-roboto]

Yes. Doug Stevenson suggested I file a bug.

[REQUIRED] Describe your environment

• Operating System version: Windows 10
• Browser version: N/A (Using Node.js)
• Firebase SDK version: "firebase-admin": "^8.13.0",
• Firebase Product: Functions

[REQUIRED] Describe the problem

When a deployed, callable Firebase Function raises an HttpsError with error code "ok" the HttpsCallable on the caller receives an error with code "internal" and the message: "Response is missing data field."

Steps to reproduce:

1. Deploy a callable Firebase Function that raises httpsError("ok");
2. Call that function from a client using Node.js and HttpsCallable.
3. Notice HttpsCallable's catch() gets triggered, with error containing: {"message":"Response is missing data field.","code":"internal"}. The documentation for HttpsError implies that "ok" should set the status to 200 and set response.error.
4. Expected: HttpsCallable triggers the then() code path, with response.error containing the message given to httpsError.
5. Guess: My guess is that HttpsError sets status to 200 and sets response.error. However, httpsCallable assumes that all 200's must have response.data set, so an exception is raised on the client. Unfortunately, this makes httpsError("ok") useless as there's no way to access any more information or distinguish httpsError("ok") from any other unhandled exceptions.

Relevant Code:

//Firebase function
exports.UpdateLobby = functions.https.onCall(async (data, context) => {
    throw new functions.https.HttpsError('ok', 'test', 'test2');
}
//Node.js client code
var testFirebaseFunction = firebase.functions().httpsCallable("UpdateLobby");
return testFirebaseFunction().then(function(result) {
    console.log(JSON.stringify(result));
    return;
}).catch(function(error) {
    console.log(JSON.stringify(error, Object.getOwnPropertyNames(error)));
}

[google-oss-bot]

I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.

[Feiyang1]

We do treat error code ok as success, but expect either response.json.data or response.json.result to be defined. This seems to be a very special use case, which I can't seem to find any documentation for. Would you mind elaborating your use case? Why do you use HttpsError with ok code?

[alex-roboto]

@Feiyang1 Just for info, here's the documentation: https://firebase.google.com/docs/functions/callable-reference#request_body:

If the callable is invoked and returns an explicit error condition using the API provided for callable functions, then the request fails. The HTTP status code returned is based on the official mapping of error status to HTTP status, as defined in code.proto. The specific error code, message, and details returned are encoded in the response body as detailed below. This means that if the function returns an explicit error with status OK, then the response has status 200 OK, but the error field is set in the response.

I mainly filed the bug just because httpsError("ok") has documented behavior which HttpsCallable seems to not adhere to.

Our use case is not very vital as we have an easy workaround. We have a validation function at the top of every Firebase Function that validates our request data. If it finds an error, it throws an httpsError and the Firebase Function automatically terminates. In one special case, the request data will contain a value that asks the Firebase Function for it's version number. In that case, we want the Firebase Function to immediately terminate (like the error cases) but succeed, so we throw HttpError("ok", something, something2);

Here's an example:

exports.DoSomething = functions.https.onCall(async (data, context) => {
    Response.validateCallData(data, context); //If this throws HttpsError("ok") it will terminate and our client can handle a special success case.
    //Do whatever the function normally does
});

Then, the client can handle this success case in the success flow. However, the workarounds are pretty easy: 1) We can just throw some HttpsError other than "ok" and just handle the version number in the catch() error handling code path. This isn't perfect as semantically, the code reads as an error case. Or, we terminate the Firebase Function as usual:

exports.DoSomething = functions.https.onCall(async (data, context) => {
    if (!Response.validateCallData(data, context) { //This works but this code block is more complicated and repetitive.
       return { result: "success", version: version }
    }
    //Do whatever the function does
});

HttpsError("ok") is just a neat hack that works perfectly for us, since it acts to terminate the Firebase Function from inside a helper function. The only problem is that HttpsCallable trips on httpsError("ok") and converts it into a meaningless "internal" error.

[Feiyang1]

Thanks for explaining your use case! And thanks for the link to the reference doc. I saw this in the doc:

error - If this field is present, then the request is considered failed, regardless of the HTTP status code or whether data is also present.

It sounds like the client SDK should treat httpsError("ok") as an error instead of a success. I will discuss it with the team to see what is the right thing to do.

[hsubox76]

We can update the condition here to also check for the error that should have been parsed in the previous block: https://github.com/firebase/firebase-js-sdk/blob/8e9aac2eff5f6d5623c1412151b1c563b37ed8b7/packages/functions/src/error.ts#L161

[0x80]

I was throwing HttpsError with "ok" because I need the message to show up in the client. With v1 callable functions this was working as expected, but since I moved to v2, or maybe it was something else in the firebase SDK update, I am running into this problem.

I have now replaced instances of "ok" with "unknown" as a workaround, but it would be great to have this fixed 🙏

[dlarocque]

Since this has already been labelled as a bug, I'll remove the needs-attention label.

[0x80]

I think "ok" is confusing for an error type, but I guess it is not easy to just add another type... But if you can, I would suggest "internal_message" or something so indicate that's an internal error but you also want to pass along the message.