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

Redact tokens/passwords from URLs in ConsDbClient HTTPError reports #120

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

Vebop
Copy link
Contributor

@Vebop Vebop commented Sep 4, 2024

Added clean_url() to requests.Session hooks for responses.

Added test to check initialization of ConsDbClient and to test that the url is sanitized according to our terms.

@Vebop Vebop marked this pull request as ready for review September 4, 2024 19:15
@bbrondel
Copy link

bbrondel commented Sep 4, 2024

I'd think you would want to redact the entire password, including the first two letters, and for that matter avoid revealing anything about the length of the password.

Copy link
Contributor

@mfisherlevine mfisherlevine left a comment

Choose a reason for hiding this comment

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

LGTM. I kind of agree with Brian about redacting the entire password, but two other points on that: 1) I can see how a couple of characters could be useful for debug info, and this isn't super security critical stuff, and the tokens are plenty long, but 2) the two that I've looked at both started gt- so I suspect you'd need more than 3 to actually reveal anything that's actually useful (if that's even desirable).

I think it's probably @ktlim's call though.

@mfisherlevine
Copy link
Contributor

Also, I'm not usually a stickler for asking people to squash commits, but given the middle commits include adding and removing a whole file which should never have been committed, and that this whole ticket feels fine as a single commit, I'd probably just squash the whole thing down to one or two commits.

Copy link
Collaborator

@ktlim ktlim left a comment

Choose a reason for hiding this comment

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

General approach looks great.

The response that could contain a URL with tokens
"""
url = urlparse(resp.url)
short_user = url.username[:2] if url.username is not None else "**"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's a bit more useful to have

short_user = f"{url.username[:2]}***" if url.username is not None else ""
short_pass = f":{url.password[:2]}***" if url.password is not None else ""
netloc = f"{short_user}{short_pass}@{url.hostname}"

That way, empty usernames and passwords are properly reflected and the password component is only included if it is actually present.

tests/test_consdbClient.py Outdated Show resolved Hide resolved

assert "v987wefVMPz" not in url
assert url == sanitized

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you really want coverage, you could also test the username/no password case, a password/no username case (although I don't know if that is actually legal), and a single-character username case (if someone uses 1 or 2 character passwords, that's their own problem...).

@Vebop
Copy link
Contributor Author

Vebop commented Sep 6, 2024

Updated the short_user and short_pass to be empty if none are provided;
Checked multiple test cases in a parametrized unit test;
Reorganized commits -- but don't we use a squash commit :)

@Vebop Vebop merged commit 0b3fd87 into main Sep 6, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants