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

feat: add SecretString and SecretBytes datastructures #2966

Closed
wants to merge 9 commits into from
Closed

feat: add SecretString and SecretBytes datastructures #2966

wants to merge 9 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 8, 2024

This pull request implements SecretString and SecretBytes data structures to hide sensitive data in tracebacks, etc.

Closes #1312

@ghost ghost requested review from a team as code owners January 8, 2024 21:02
@provinzkraut
Copy link
Member

Thanks @sashahx!

I’ll give this a proper review soon but just a bite upfront:
The reason for having those was so that they could be de/encoded from/to their secret/raw value counterparts. So for them to be useful, they’ll have to be added to the default de/encoders.

@ghost
Copy link
Author

ghost commented Jan 9, 2024

Hey there!

No worries on the speedy review—I'll get cracking on adding that functionality you mentioned. I'll shoot you a message once it's good to go, so you can check it out when you have a moment

Copy link
Contributor

@cbscsm cbscsm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think object can replace Any. object means "any object", but Any means "no type constraint for it".

For example:

from typing import Any

def foo(a: Any, b: object):
    a[5]  # Type check pass
    b[5]  # Type check error(No __getitem__ is defined in object)
    if isinstance(b, list):  # object can be narrowed into list because object is the top type
        b[5]  # Type check pass

litestar/datastructures/secret_values.py Outdated Show resolved Hide resolved
litestar/datastructures/secret_values.py Outdated Show resolved Hide resolved
Copy link
Contributor

@peterschutt peterschutt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean and well structured code, thanks!

I've made a few suggestions in review. Additionally:

  • As mentioned by @provinzkraut, we need to have the default encoders/decoders be able to handle this type.
  • It would be nice to see some integration tests that show patterns of expected use, e.g., as a header type, and as a field on a data model, and validate its behavior with logging and inside exception rendering, etc.

I think this will be a valuable feature, thanks for doing the work!

litestar/datastructures/secret_values.py Outdated Show resolved Hide resolved
litestar/datastructures/secret_values.py Outdated Show resolved Hide resolved
litestar/datastructures/secret_values.py Outdated Show resolved Hide resolved
litestar/datastructures/secret_values.py Outdated Show resolved Hide resolved
litestar/datastructures/secret_values.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jan 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.25%. Comparing base (779726c) to head (36dd090).

❗ Current head 36dd090 differs from pull request most recent head b47e995. Consider uploading reports for the commit b47e995 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2966      +/-   ##
==========================================
+ Coverage   98.23%   98.25%   +0.02%     
==========================================
  Files         320      321       +1     
  Lines       14445    14508      +63     
  Branches     2298     2327      +29     
==========================================
+ Hits        14190    14255      +65     
+ Misses        113      111       -2     
  Partials      142      142              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

- Introduced `_SecretT` as a type variable bound to `str | bytes`
- Modified `SecretValue` to be a generic class parameterized by `_SecretT`
- Updated `__init__` to accept `_SecretT` type for `secret_value`
- Renamed `get_value` method to `get_secret`
- Renamed `_get_hidden_value` method to `_get_obscured_representation`
- Updated method implementations to use `_SecretT` type
- Modified `SecretString` and `SecretBytes` subclasses to utilize generics
- Implemented obscured representations for `SecretString` and `SecretBytes`
Copy link
Contributor

@peterschutt peterschutt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks @sashahx!

Copy link
Contributor

@peterschutt peterschutt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the issue of configuring the encoders/decoders to handle the type - do you need any assistance with this?

@ghost
Copy link
Author

ghost commented Jan 30, 2024

@peterschutt this week, I'll be tackling the default encoders/decoders. Before I begin, I'll research other data structures to get a better grasp of the topic. At the moment, I don't need assistance, but I might have questions later on

Once I've implemented the encoders/decoders, I'll shift focus to developing integration tests

@peterschutt
Copy link
Contributor

@peterschutt this week, I'll be tackling the default encoders/decoders. Before I begin, I'll research other data structures to get a better grasp of the topic. At the moment, I don't need assistance, but I might have questions later on

Once I've implemented the encoders/decoders, I'll shift focus to developing integration tests

Sounds great, thanks!

@peterschutt
Copy link
Contributor

Will also need to rebase this onto the develop branch.

@JacobCoffee
Copy link
Member

hi @sashahx , any updates here? I think the only remaining thing is the encoders/decoders ask.

@ghost
Copy link
Author

ghost commented Mar 11, 2024

hi @JacobCoffee! No updates yet as I've been very busy, but I plan to implement the feature this week, or at least ask some questions. Thank you for your patience

@JacobCoffee
Copy link
Member

All good, just checking! 😄

@peterschutt
Copy link
Contributor

Hey @sashahx - I cherry picked your commits into PR #3254 which is based off develop.

Closing this PR in favor of that one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancement: Add Secret
5 participants