search

polkadot-js / extension

Simple browser extension for managing Polkadot and Substrate network accounts in a browser. Allows the signing of extrinsics using these accounts. Also provides a simple interface for compliant extensions for dapps.
Apache License 2.0
973 stars 417 forks source link

wrong signRaw signature for polkadotjs due to u8aWrapBytes that forced adding <Bytes>...<Bytes> to payload before "sign" function #1357

Open S0c5 opened 5 months ago

S0c5 commented 5 months ago

As a developer when I want to sign a RAW hex payload, this payload must not be manipulated by adding any additional data like this wrapper, That why it is named signRaw.

Sorry, but WDF, it was a terrible idea to wrap this with tag in the signer, that manipulation must be done on the Dapp side, not on the signer side. the change was introduced by https://github.com/polkadot-js/extension/commit/e4ce268b1cad5e39e75a2195e3aa6d0344de7745

I'm crafting a raw substrate transaction, and the payload must be RAW and not manipulated given that it can not be verified for the blockchain node.

Solution

To avoid break changes to anyone using that signRaw with bytes, I recommend including an extra type called 'raw' in the SignRawPayload, then if the type is raw no manipulation must be done.

Tbaut commented 5 months ago

This is obviously not a bug,and if you read the description of the pr, you'll see where it comes from https://github.com/polkadot-js/extension/pull/743

I'm not saying it's great, but it is what it is.

S0c5 commented 5 months ago

I'd say it's a bug since it introduces an unwanted and undocumented behavior (I searched through the documentation and found no mention of that wrapping bytes feature), and today it doesn't follow the definition for "signRaw". Anyway, 😄 we must fix this, as there are plenty of use cases where we need to sign the payload as it is, more for low-level developers.

If the intention was to prevent evil actors from performing raw signatures, that must be in my opinion a warning in the UI that alerts to the user that a "low-level" operation is being performed.