[PoC] - prevent leaking of secrets when running validate_dict #2027
Conversation
✅ Deploy Preview for dlt-hub-docs canceled.
|
sh-rp
force-pushed
the
fix/validate_dict_value_obfuscation
branch
from
db97219 to
840832c
Compare
|
|
||
| def obfuscate_string(s: str) -> str: | ||
| """Obfuscates string by replacing some or all characters with asterisks""" | ||
| if len(s) < 6: |
There was a problem hiding this comment.
I'd consider replacing this magic number with a constant and maybe use a single constant for both this place as well as here:
dlt/dlt/sources/rest_api/__init__.py
Line 449 in c0e4795
There was a problem hiding this comment.
actually I think we can get rid of this mask_secrets code entirely, no?
willi-mueller
left a comment
willi-mueller
left a comment
There was a problem hiding this comment.
This implementation is nice! I think it would allow us to delete TODO comments, such as here:
dlt/dlt/sources/rest_api/__init__.py
Line 434 in c0e4795
Just in case you have discussed this already, you could maybe update the discussion here with your results:
#1797
Anton and I argued for the method this PR proposes, Marcin was against it.
|
@willi-mueller what where the arguments against implementing it this way? |
burnash
left a comment
burnash
left a comment
There was a problem hiding this comment.
Looks good to me, two important from my point of view to consider:
- Allow the user to disable the obfuscation via a setting in runtime configuration
- Instead of partial masking of the secrets where we leave first and last character, do the total masking as @rudolfix initially suggested.
|
Status on this PR? The obfuscation feature could be valuable when passing the dlt config to LLMs via the MCP |
Description
This change will require the
DictValidationExceptionto have the full incoming validatable object. We extract all strings from this object and make sure that the error message does not have any of these in clear text.We could probably allow the user to set an env var to disable this obfuscator for development.
Originally the plan was to also do stuff to prevent leakage from the configs, but the
__str__method of configs currently is not implemented and just returns the default python object identifier. So I don't think there is anything to be done there.