Surface decrypt errors after recording persistent ids#15
Conversation
cassiostp
force-pushed
the
raise-decrypt-errors
branch
2 times, most recently
from
bc8a421 to
f0e3323
Compare
cassiostp
changed the title
Stop silently dropping decryption failures in the push receiver client. Known undecryptable messages still record their persistent id before reporting so reconnects do not redeliver the same packet. Surface all decrypt failures through the client error path on the next turn, which lets the parser advance before desktop app-level error handling reports the failure. Also carries forward the existing GCM semicolon lint fix needed for yarn lint on master. CLI-801
cassiostp
force-pushed
the
raise-decrypt-errors
branch
from
f0e3323 to
c91eee2
Compare
| }; | ||
|
|
||
| function isReportableDecryptionError(error) { | ||
| return DECRYPTION_ERRORS_TO_REPORT.some(message => |
There was a problem hiding this comment.
I would inline DECRYPTION_ERRORS_TO_REPORT here or define DECRYPTION_ERRORS_TO_REPORT right above isReportableDecryptionError. Right now reader has to jump back up to top of fine to find out what errors we're actually filtering on.
| if (isReportableDecryptionError(error)) { | ||
| this._persistentIds.push(object.persistentId); | ||
| } | ||
| this._emitError(error); |
There was a problem hiding this comment.
Wait actually don't we want to throw an error?
There was a problem hiding this comment.
I think we want to keep the throwing behaviour here
There was a problem hiding this comment.
I don't think we're listening for this error anywhere. So you would need to add handling to pass a listener in electron push receiver. Doesn't seem worth it since we're currently logging the raised errors.
There was a problem hiding this comment.
The idea of using the delayed emit was to not crash the push receiver client/parser when an error happens.
I thought it wouldn't be too much of a change to support it on the desktop side. I don't think we need to listen for it in the electron-push-receiver lib.
There was a problem hiding this comment.
crash the push receiver client/parser why would it crash anything? Its just raising an exception in the onmessage handler right now it doesn't seem like that would lead to a crash.
There was a problem hiding this comment.
I think I misunderstood how the pieces interact here then.
In Parser._onGotMessageBytes the _getNextMessage() function probably won't be reached for the cases in DECRYPTION_ERRORS_TO_REPORT if we throw, and that is a change in behavior. I'm not sure if it can recover on its own when the _getNextMessage() isn't called. That's what I was calling crash in that context.
I might be wrong there though. I'll try doing some testing to confirm what really happens.
There was a problem hiding this comment.
Ah I see what you're saying now. Sorry I was misunderstanding. I think you're likely right here but would double check
There was a problem hiding this comment.
I confirmed it stops getting new messages we throw at that point. That's why the old switch was just returning for the known cases.
One benefit of using this slightly hacky emit setup is that we would be able to log extra metadata into the bugsnag/honeycomb events.
The "best" approach would be to handle errors locally in the library, but that seems like too much work.
Maybe we should just always emit a non error event and deal with it on Desktop native side to avoid any unnecessary changes to the library.
There was a problem hiding this comment.
I agree emit makes sense here then. Good find!
There was a problem hiding this comment.
I think keeping your emit change and listening for the error emit should be pretty easy change. But defer to you
| } | ||
|
|
||
| _emitError(error) { | ||
| // Let Parser advance past this packet before consumers handle the error. |
There was a problem hiding this comment.
Don't understand this comment. Why not just emit immediately? EDIT: see my other comment lets just drop the error emit entirely.
2 participants
Summary
_onDataMessagefrom silently warning and returning for decryption failures that should be visible to consumers.persistentIdfor known undecryptable messages so reconnects do not redeliver the same packet.errorpath, deferred until the next turn so the parser can advance before app-level error handling reports the failure.yarn lintonmaster.Requested from Brian review on #13. I checked the desktop native path: a synchronous throw from message handling would reach desktop uncaught-error reporting, but it can interrupt parser advancement. This keeps the change inside
push-receiverand does not requireelectron-push-receiverto take on extra responsibility.Validation
yarn test test/client.test.js --runInBandyarn lint./node_modules/.bin/eslint test/client.test.js