Merged
Conversation
|
Findings
Recommended tests to add
No direct security issue found in the changed code. I couldn’t run tests locally because |
ee05d33 to
7a95822
Compare
scottnemes
approved these changes
Mar 8, 2026
Previously the click parameter
flag_value="MYCLI_ASK_PASSWORD"
meant that when giving --password at the CLI without a value, the
internal value of the password variable was set to the literal string
"MYCLI_ASK_PASSWORD".
That meant that no password could be set to that literal string. Mycli
would take it as the sentinel value and re-prompt.
Sentinel values should be entirely out-of-band, so here we adopt a
value of -1 (type int) to indicate that --password was given with no
argument. Since the value is of type int, no user can have given it
as a string argument.
It is intended that as little as possible change here with regard to
functionality, though there might be a small change in how empty-string
passwords are interpreted when deciding to take a database argument
as a DSN, and another related possibility is given a comment.
Incidentally the --password helpdoc is updated to clarify "passing" vs
"entering", since "entering" would seem to refer to an interactive
prompt.
7a95822 to
f4f5995
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Previously the
clickparametermeant that when giving
--passwordat the CLI without a value, the internal value of the password variable was set to the literal string "MYCLI_ASK_PASSWORD".That meant that no password could be set to that literal string. Mycli would take it as the sentinel value and re-prompt.
Sentinel values should be entirely out-of-band, so here we adopt a value of
-1(typeint) to indicate that--passwordwas given with no argument. Since the value is of typeint, no user can have given it as a string argument.It is intended that as little as possible change here with regard to functionality, though there might be a small change in how empty-string passwords are interpreted when deciding to take a database argument as a DSN, and another related possibility is given a comment.
Incidentally the
--passwordhelpdoc is updated to clarify "passing" vs "entering", since "entering" would seem to refer to an interactive prompt.Checklist
changelog.mdfile.AUTHORSfile (or it's already there).