google/webcrypto.dart

Add .toString() methods on all private classes

Closed this issue · 7 comments

It's sad when print(HmacSecretKey.generateKey(Hash.sha256)) shows Instance of '_HmacSecretKey'.

Perhaps we should override the toString() method on all the private classes.

The user doesn't need to know about _HmacSecretKey, since it's a private class.
And we could probably display something more useful for debugging purposes.

I haven't figured this out yet, but we could do something like:

  • HmacSecretKey(hash: Hash.sha256, key: ***)
  • HmacSecretKey(Hash.sha256)
  • Instance of 'HmacSecretKey with Hash.sha256'
  • Instance of 'HmacSecretKey'
  • HmacSecretKey with sha256
  • or ???

It could be useful if print was helpful when debugging stuff :D

On the flip side, the HmacSecretKey doesn't have a hash property, so if you have an instance of HmacSecretKey and you want to know what hash it uses, the only thing you can do is:

  • (a) Search your code base, or,
  • (b) Use HmacSecretKey.toString().contains('sha256'), if we added a fancy variant of toString().

I imagine that we'd prefer to avoid using doing (b). Not necessarily be adding a hash property they don't need, but by forcing them to do (a) 🤣

I think the point I'm trying to make is that, if we add to much information in toString(), then maybe people will use to inspect an object at runtime and rely on the behavior of toString().
And I don't think we want people to rely on the behavior of toString().

So maybe it's safest to just do:
macSecretKey.generateKey(Hash.sha256).toString() == "Instance of 'HmacSecretKey'"

It's boring, it's simple, but it doesn't leak any information that could lure a user into misusing toString().
I could ofcourse be convinced otherwise.

Hey Jonas! Can you provide more info to solve? Is it in specific section of code?

Agree on the part - "user doesn't need to know about _HmacSecretKey"

imo providing the key with the hash function would be a pretty bad idea (option 1) over concerns of inspection at runtime by malicious actors.

I am yet to find credible evidence of cases where the knowledge of the hash function alone, by Eve could be an issue. Even if we double down on option 2, the output that the user will see is Instance of 'HmacSecretKey' with hash: Instance of '_Sha256'

I would like to avoid this for two reasons:

  1. The information is redundant
  2. It would require implementing an override for `_Sha256' as well. (which ig is the goal of the issue, but point 1 still holds)

I agree on the solution Instance of 'HmacSecretKey'. Its not the most informative one but is the best option that doesnt leak much information.

Hey Jonas! Can you provide more info to solve? Is it in specific section of code?

Hey @devenderbutani21 this is a WIP, can you please select other open issues, would be happy to assist you in one. Thanks a lot.

For context of this issue please check the following gist - https://gist.github.com/HamdaanAliQuatil/bd32945af3091ff7ba72071386ff4de7

@HamdaanAliQuatil Should I work on #105 this issue?

@HamdaanAliQuatil Should I work on #105 this issue?

Yes, that'd be great. Tysm for contributing 🚀

@devenderbutani21 Feel free to ping me @jonasfj for reviews, I'm happy to help merge PRs.

I'm jonasfj88 on discord, see dart_commmunity, @HamdaanAliQuatil and I are hanging out in a thread in the #packages channel.

imo providing the key with the hash function would be a pretty bad idea (option 1) over concerns of inspection at runtime by malicious actors.

The hash function isn't exactly secret, and we anyone who can do runtime inspection has probably won. Like that's exactly what we're trying to protect against. More like we attempt to protect against mistakes.