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

Fix leaked URLs with credentials in the output #1146

Merged
merged 3 commits into from May 21, 2020

Conversation

atugushev
Copy link
Member

@atugushev atugushev commented May 16, 2020

Fixes #1137.
Refs #811 and #1028.

Changelog-friendly one-liner: Fix leaked URLs with credentials in pip-compile's output.

Contributor checklist
  • Provided the tests for the changes.
  • Gave a clear one-line description in the PR (that the maintainers can add to CHANGELOG.md on release).
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@atugushev atugushev added bug fix logging Related to log or console output labels May 16, 2020
@atugushev atugushev added this to the 5.2.0 milestone May 16, 2020
@codecov
Copy link

codecov bot commented May 16, 2020

Codecov Report

Merging #1146 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1146   +/-   ##
=======================================
  Coverage   99.48%   99.49%           
=======================================
  Files          36       36           
  Lines        2736     2752   +16     
  Branches      324      326    +2     
=======================================
+ Hits         2722     2738   +16     
  Misses          8        8           
  Partials        6        6           
Impacted Files Coverage Δ
tests/test_utils.py 100.00% <ø> (ø)
piptools/exceptions.py 100.00% <100.00%> (ø)
piptools/scripts/compile.py 100.00% <100.00%> (ø)
piptools/utils.py 100.00% <100.00%> (ø)
tests/test_cli_compile.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5dd163a...02bfe85. Read the comment docs.

Copy link
Contributor

@AndydeCleyre AndydeCleyre left a comment

Choose a reason for hiding this comment

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

Nice work! Approved 👍

If you think it's appropriate, I would suggest renaming default_index_url in compile.py to better reflect that it's not the real default URL (redacted and used only for the help string).

@atugushev
Copy link
Member Author

@AndydeCleyre

If you think it's appropriate, I would suggest renaming default_index_url in compile.py to better reflect that it's not the real default URL (redacted and used only for the help string).

Yeah, naming is hard :) For example:

  • display_index_url
  • help_index_url
  • redacted_default_index_url
  • show_default_index_url
  • pipconf_index_url

What's your suggestion?

I'm also tempted to move some logic to a function to not waste global scope:

def _get_pip_default(option_name):
    install_command = create_command("install")
    pip_defaults = install_command.parser.get_default_values()
    return getattr(pip_defaults, option_name)

...

@click.option(
    "-i",
    "--index-url",
    help="Change index URL (defaults to {})".format(
        redact_auth_from_url(
            _get_pip_default("index_url")
        )
    ),
    envvar="PIP_INDEX_URL",
)

What do you think?

@AndydeCleyre
Copy link
Contributor

Moving that into the function looks good. Otherwise, I'd say redacted_default_index_url is the most straightforward choice.

@atugushev atugushev force-pushed the leak-index-url-password-1137 branch from 447cc55 to 5430aae Compare May 18, 2020 04:55
Move some logic to a function to not waste the global scope.

Co-Authored-By: Andy Kluger <andydecleyre@gmail.com>
@atugushev atugushev force-pushed the leak-index-url-password-1137 branch from 5430aae to 302d46b Compare May 18, 2020 04:57
@atugushev
Copy link
Member Author

@AndydeCleyre

Addressed in 302d46b.

@atugushev atugushev merged commit 4db0eb0 into jazzband:master May 21, 2020
@atugushev atugushev deleted the leak-index-url-password-1137 branch May 21, 2020 19:34
@atugushev
Copy link
Member Author

Thanks @AndydeCleyre for reviewing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logging Related to log or console output
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clear-text password leaked via index URL
2 participants