Skip to content

fix: Strip quotes from env vars #51

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Nov 5, 2024
Merged

fix: Strip quotes from env vars #51

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0e72808
fix: Strip quotes from env vars
SiddhantSadangi Oct 14, 2024
bf3b0ac
chore: Ignore line from mypy checks
SiddhantSadangi Oct 14, 2024
8eba7fd
chore: Ignore line from mypy checks
SiddhantSadangi Oct 14, 2024
a763054
chore: Ignoring mypy unused ignores error
SiddhantSadangi Oct 14, 2024
0ada996
Not stripping API_TOKEN
SiddhantSadangi Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,7 @@ authors = ["neptune.ai <contact@neptune.ai>"]
repository = "https://github.com/neptune-ai/neptune-client-scale"
readme = "README.md"
license = "Apache License 2.0"
packages = [
{ include = "neptune_scale", from = "src" },
]
packages = [{ include = "neptune_scale", from = "src" }]
include = []
classifiers = [
"Development Status :: 4 - Beta",
Expand Down Expand Up @@ -93,4 +91,4 @@ no_implicit_optional = "True"
check_untyped_defs = "True"
warn_return_any = "True"
show_error_codes = "True"
warn_unused_ignores = "True"
# warn_unused_ignores = "True"
11 changes: 8 additions & 3 deletions src/neptune_scale/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,13 +171,17 @@ def __init__(
raise ValueError("`max_queue_size` must be greater than 0.")

project = project or os.environ.get(PROJECT_ENV_NAME)
if project is None:
if project:
project = project.strip('"').strip("'")
else:
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("'")
Copy link
Contributor

@michalsosn michalsosn Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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:
image

Can we strip quotes just from the project name?

Copy link
Member Author

Choose a reason for hiding this comment

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

@michalsosn , @kgodlewski - can we have a decision here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, I haven't seen quotes being preserved anywhere besides the jupyter %env magic, but unfortunately it's too important to be ignored

Copy link
Contributor

@michalsosn michalsosn Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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:

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

else:
raise NeptuneApiTokenNotProvided()
assert api_token is not None # mypy
input_api_token: str = api_token
Expand Down Expand Up @@ -261,7 +265,8 @@ def __init__(
self._exit_func: Optional[Callable[[], None]] = atexit.register(self._close)

if platform.system() != "Windows":
signal.signal(signal.SIGCHLD, self._handle_signal)
# Ignoring the type because the signal module is not available on Windows
signal.signal(signal.SIGCHLD, self._handle_signal) # type: ignore[attr-defined]

if not resume:
self._create_run(
Expand Down
Loading