Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add .toString() methods on all private classes #104

Closed
jonasfj opened this issue May 6, 2024 · 7 comments · Fixed by #110
Closed

Add .toString() methods on all private classes #104

jonasfj opened this issue May 6, 2024 · 7 comments · Fixed by #110
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@jonasfj
Copy link
Member

jonasfj commented May 6, 2024

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.

@jonasfj jonasfj added enhancement New feature or request good first issue Good for newcomers labels May 6, 2024
@devenderbutani21
Copy link

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

@HamdaanAliQuatil
Copy link
Collaborator

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.

@HamdaanAliQuatil
Copy link
Collaborator

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

@devenderbutani21
Copy link

@HamdaanAliQuatil Should I work on #105 this issue?

@HamdaanAliQuatil
Copy link
Collaborator

@HamdaanAliQuatil Should I work on #105 this issue?

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

@jonasfj
Copy link
Member Author

jonasfj commented May 8, 2024

@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.

@jonasfj
Copy link
Member Author

jonasfj commented May 8, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants