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

Hide password values from raw HTTP logs. #4066

Merged

Conversation

dikwickley
Copy link
Contributor

@dikwickley dikwickley commented Feb 27, 2024

This is a supposed fix for #3935

So as explained in the issue (#3935 (comment)) there is no straightforward way to do this.
But here is something that works.

So basically there is called Modifier keys in Selenium webdriver and there is one that we can use, Key.NULL
https://github.com/SeleniumHQ/selenium/blob/da62a402d0565dd2dda2ced71cf74965caa4391c/javascript/node/selenium-webdriver/lib/webdriver.js#L2644-L2646

Passing this key along without string DOES NOT affect the actual string that is sent to textbox, but comes as a
 or ('\uE000') special Unicode character in the final response after executing the command on selenium.

I check for this character in the data, and any data containing this is flagged to be redacted.

Other potential solution (garg3133)

We could potentially also achieve this by adding a static variable to HttpRequest class (lib/http/request.js) and setting/unsetting it just before and after running element.sendKeys() command for .setPassword() inside method-mappings.js file. Using this static variable, we could decide at the time of logging of request whether or not to redact the request data.

Instead of creating a separate static variable above, we could also call HttpRequest.updateGlobalSettings({redact: true}); just before and after element.sendKeys() command to set this.httpOpts.redact in the request.js file, and then use this option at the time of logging of request to decide whether or not to redact the request data.

Copy link

Status

  • ❌ No modified files found in the types directory.
    Please make sure to include types for any changes you have made. Thank you!.

Copy link
Member

@garg3133 garg3133 left a comment

Choose a reason for hiding this comment

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

This is actually a good idea.

A few thoughts:

  • If we just check for the presence of NULL key in the data, there will be many false positives (since NULL will be used by the people while using modifier keys). To get around this, I was earlier thinking of adding a pattern of keys to value if we want to redact it, but a better solution would be to add the NULL key at the starting of the value instead of at the end because normally users would never use a NULL key at the start. With that, just check if the value starts with a NULL key and if so, mark it for redaction.

lib/transport/selenium-webdriver/httpclient.js Outdated Show resolved Hide resolved
lib/transport/selenium-webdriver/httpclient.js Outdated Show resolved Hide resolved
lib/transport/selenium-webdriver/method-mappings.js Outdated Show resolved Hide resolved
@dikwickley dikwickley requested a review from garg3133 February 28, 2024 05:50
lib/transport/selenium-webdriver/method-mappings.js Outdated Show resolved Hide resolved
lib/transport/selenium-webdriver/method-mappings.js Outdated Show resolved Hide resolved
lib/http/request.js Outdated Show resolved Hide resolved
@dikwickley dikwickley requested a review from garg3133 February 29, 2024 06:57
Copy link
Member

@garg3133 garg3133 left a comment

Choose a reason for hiding this comment

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

This looks good now. Just a bunch of refactoring changes.

lib/transport/selenium-webdriver/method-mappings.js Outdated Show resolved Hide resolved
lib/http/request.js Show resolved Hide resolved
lib/http/request.js Outdated Show resolved Hide resolved
@dikwickley dikwickley requested a review from garg3133 February 29, 2024 14:39
Copy link
Member

@garg3133 garg3133 left a comment

Choose a reason for hiding this comment

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

Looks good!

One last thing, we should also add a test to make sure that the final generated report does not contain the actual password but *s in rawHttpOutput. We can get the rawHttpOutput by defining a reporter function in globals config and then accessing results.modules.<moduleName>.rawHttpOutput.

See the last test case of test/src/api/commands/element/testWaitForElementNotPresent.js file for reference.

@dikwickley dikwickley requested a review from garg3133 March 2, 2024 18:06
Copy link
Member

@garg3133 garg3133 left a comment

Choose a reason for hiding this comment

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

There is still some scope for improvements in the tests. But I think this will make you really good at writing Nightwatch tests in the future :)

Please feel free to ask if you don't understand the reasoning behind anything.

@dikwickley dikwickley requested a review from garg3133 March 6, 2024 17:25
@garg3133 garg3133 force-pushed the issue/3935-hiding-redacted-values branch from 0c1631c to 60eb36f Compare March 7, 2024 12:54
@garg3133
Copy link
Member

garg3133 commented Mar 7, 2024

I did a few refactors myself in the PR, but the major takeaway from the refactors is that while writing mocks we should also specify how many times we want the mock to run. By default, the mock will run for indefinite times, passing true as the second argument to .addMock() makes it run only once, and for more, we can specify the times parameter in the first argument of .addMock().

But thanks a lot for this PR, this looks really great now.

@dikwickley
Copy link
Contributor Author

Thanks @garg3133 for all the help 🙌

@garg3133 garg3133 changed the title Hidding redacted values in raw HTTP logs - html reporter Hide password values from raw HTTP logs. Mar 7, 2024
@garg3133 garg3133 merged commit a441ca4 into nightwatchjs:main Mar 7, 2024
17 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.

3 participants