fix: Strip quotes from env vars #51
Conversation
src/neptune_scale/__init__.py
Outdated
| raise NeptuneProjectNotProvided() | ||
| assert project is not None # mypy | ||
| input_project: str = project | ||
|
|
||
| api_token = api_token or os.environ.get(API_TOKEN_ENV_NAME) | ||
| if api_token is None: | ||
| if api_token: | ||
| api_token = api_token.strip('"').strip("'") |
There was a problem hiding this comment.
Not sure about going down the path of correcting the value of a critical secret like token for the user.
They should be always able to just remove the extra characters on their side?
I think it'd be less invasive to emit a warning when the authorization fails (or a project is not found), saying that that we detected that the value is wrapped in ''/"" and it's likely causing the issue.
There was a problem hiding this comment.
We are not correcting the token, we are just trimming the leading/training quotes.
I think it improves UX, as some methods in Python require the env variables to be enclosed in quotes (like any other string). However, this behavior does not carry over if the user uses a different method of setting variables that does not require quotes.
The client should be agnostic to how the variable was set, as long as the value is correct.
There was a problem hiding this comment.
I agree with Michał, we should not correct user input. It's trivial to properly export an env variable. The proposition to say "hey, I can see you have quotes in your API token, how about correcting this?" is better. Otherwise there is no end in how badly malformed input we can handle.
There was a problem hiding this comment.
I noticed that we do not need to strip quotes from the API TOKEN as quotes don't seem to make a difference, but if I try to pass NEPTUNE_PROJECT with quotes, I get a NeptuneUnauthorizedError , which is definitely misleading:
Can we strip quotes just from the project name?
There was a problem hiding this comment.
@michalsosn , @kgodlewski - can we have a decision here?
There was a problem hiding this comment.
I see, I haven't seen quotes being preserved anywhere besides the jupyter %env magic, but unfortunately it's too important to be ignored
There was a problem hiding this comment.
the reason why API_TOKEN is accepted is because we're passing it through
base64.b64decode
which ignores most characters outside 0-9 a-z. You can even append a random string of ();:*& etc and it'll still work.
from base64.b64decode docs:
If validate is False (the default), characters that are neither in the
normal base-64 alphabet nor the alternative alphabet are discarded prior
to the padding check. If validate is True, these non-alphabet characters
in the input result in a binascii.Error.
For more information about the strict base64 check, see:
There was a problem hiding this comment.
Most users won't use neptune with notebooks, but we rely on notebooks for interactive tutorials. And this small change will enhance that experience and make life a bit easier for the few users who do
There was a problem hiding this comment.
since we're (maybe unintentionally) so lenient with the api token, refusing to trim the project name would not make too much sense
so I think the PR is good
Without quotes being stripped, Neptune would not recognize the API TOKEN and Project name, resulting in a
NeptuneUnauthorizedError