Polykey icon indicating copy to clipboard operation
Polykey copied to clipboard

Review `toError` and `fromError` sensitivity filter, agent service sensitive information removal, PK specific error passthrough, and unknown errors

Open tegefaulkes opened this issue 2 years ago • 7 comments

Specification

The current implementation of toError and fromError is basic and minimally functional. It needs to be reviewed and expanded and tested to handle all kinds of errors.

We also need the sensitive functionality for the client service. The error stack needs to be stripped from serialised errors.

Additional context

  • Related: #560

Tasks

  1. Review and expand toError and fromError

tegefaulkes avatar Oct 10 '23 08:10 tegefaulkes

Is this being done with your recent changes to js-rpc? Is there a PR for this?

CMCDragonkai avatar Oct 11 '23 19:10 CMCDragonkai

Yeah, I need to fix it now. There isn't a PR for this yet.

tegefaulkes avatar Oct 11 '23 23:10 tegefaulkes

What still needs to be done here? I see alot of change for this RPC error thing but no spec as to what has been changed, or what the target is. How is RPC errors still receiving so much changes?

CMCDragonkai avatar Oct 12 '23 08:10 CMCDragonkai

The toError and fromError was expanded during the final push on friday. But I'm still not sure it's fully polished. We can consider this done, but the functions could still be improved.

tegefaulkes avatar Oct 15 '23 23:10 tegefaulkes

Need to prove all of these cases with tests.

  1. There needs to be tests that prove that the sensitive works and completely removes all sensitive information from errors passed back to the client of JS-RPC.
  2. There needs to be tests in PK that proves that sensitive information is removed from the errors in the agent service. (BUT NOT THE client service).
  3. Prove that all errors can be serialised and unserialised fromError to toError. This includes basic JS errors, and also Polykey-specific errors.
  4. Prove that unknown errors is appropriately captured on the client side of the RPC.

CMCDragonkai avatar Nov 05 '23 23:11 CMCDragonkai

It was found that the RPCServer was never using the replacer passed in from the constructor parameter. So no error stacks were ever being redacted. Furthermore, I think the filterSensitive utility function that returned a replacer, is not sufficient for our usecase, as any value with key of stack would be redacted. So stack would have to become a reserved keyword if the replacer returned from filterSensitive were used. I'm in the process of changing these

amydevs avatar Nov 10 '23 06:11 amydevs

Well in terms of filtering, we have to decide whether it is more explicit, in the sense of filtering a specific path in a structure, or filtering based on patterns. A hybrid form is a JSON-path like structure, where you can filter specific or pattern match.

An easier way is just a regex applied to the path somehow.

The most simplest for now is a specific key path, since we already know what kind of sensitive data we want to filter.

CMCDragonkai avatar Nov 10 '23 16:11 CMCDragonkai