mozilla / tls-canary

DEPRECATED - TLS regression scanner for Firefox
https://tlscanary.mozilla.org/
Mozilla Public License 2.0
18 stars 15 forks source link

Preparing for removal of NSS error messages #159

Closed cr closed 6 years ago

cr commented 6 years ago

NSS is about to move error message generation to frontend code. This patch is preparing for that change, defaulting to an empty error message string when reding the property from NSS info fails. See bug 1415279 for more info.

cr commented 6 years ago

158 is dealing with adapting the python side of things.

mwobensmith commented 6 years ago

I don't think this is correct. When sec_info.errorMessage goes away, the assignment in line 127 to info.raw_error will not throw an error. It will just return undefined. Right?

However, line 128 can throw, because attempts to call function split on value undefined aren't allowed.

In that case, when sec_info.errorMessage is gone, our current code will throw, and we will end up using undefined as short_error_message. I don't know how that is handled currently in the UI, whether it prints "undefined" or an empty string.

The change I made in #158 is actually something different. There are cases where we never go through this logic in JS, and the report generator tries to pull short_error_message from the log where it does not exist. That's why I added the check there.

cr commented 6 years ago

You are all so right. The new test should now properly handle undefined.

mwobensmith commented 6 years ago

@franziskuskiefer Do you know what the new API will look like for the error code? I assume it will be a property here, but whatever it ends up as, we might as well start writing code for it when we know what it is.

franziskuskiefer commented 6 years ago

The patch isn't through yet (hopefully later this week), but there won't be much more information. There'll be errorCodeString, which is the error code but as string (e.g. SEC_ERROR_UNKNOWN_ISSUER so that's what you display in erroron the website). That information together with the certificate (where I'll add the subject alternative names) should be enough to determine the error string. I just wonder if you actually need that error string. It's mostly so complicated because it tries to give the user useful information. But for canary I'd think the technical information would be enough?

cr commented 6 years ago

The patch isn't through yet (hopefully later this week), but there won't be much more information. There'll be errorCodeString, which is the error code but as string (e.g. SEC_ERROR_UNKNOWN_ISSUER so that's what you display in erroron the website).

I'll amend for that. Anything else? ;)

That information together with the certificate (where I'll add the subject alternative names) should be enough to determine the error string.

It's all passed on transparently, so the current code should be fine. We only require special handling of properties that don't make it through JSON serialization.

I just wonder if you actually need that error string. It's mostly so complicated because it tries to give the user useful information. But for canary I'd think the technical information would be enough?

In fact we only use it to extract what soon will be errorCodeString which is really all we need.

mwobensmith commented 6 years ago

I agree with @cr - errorCodeString is what we need, and we'll build the table of actual error messages ourselves. Thanks @franziskuskiefer !

cr commented 6 years ago

@mwobensmith, patch good to land?

mwobensmith commented 6 years ago

Yes, please land it. We'll write the error string lookup stuff once we have error codes.

cr commented 6 years ago

Error code lookup has been rewritten.