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

feat: mask values typed into input[type=password] in a reporter #6191

Merged
merged 4 commits into from
May 12, 2021

Conversation

wentwrong
Copy link
Contributor

@wentwrong wentwrong commented Apr 30, 2021

Purpose

Values typed into input[type=password] should be replaced by a placeholder before sending action logs with these values to a reporter. Also the pressKey and the typeText actions have the confidential flag. If it set to 'true', then any typed text & pressed keys would be replaced by a placeholder in action logs.

Approach

Detect what is the target element (either specified in the typeText action or current active element for the pressKey action). If it's the input which type is 'password' or the confidential flag is set to true, then replace info with a placeholder.

References

Closes #6014

Pre-Merge TODO

  • Write tests for your proposed changes
  • Make sure that existing tests do not fail

@wentwrong wentwrong temporarily deployed to CI April 30, 2021 08:28 Inactive
@wentwrong wentwrong temporarily deployed to CI April 30, 2021 12:57 Inactive
@wentwrong wentwrong marked this pull request as ready for review May 3, 2021 09:58
src/client/driver/driver.js Outdated Show resolved Hide resolved
src/client/driver/driver.js Outdated Show resolved Hide resolved
src/reporter/command/interfaces.ts Outdated Show resolved Hide resolved
constructor (obj, validate) {
super();

this.confidential = void 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to put down the confidential property to the basic classes and set the false as a default value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

void 0 is necessary as a default value. It's needed to indicate that a user didn't set this flag explicitly.

I've tried to use enum for this flag, but it looks overcomplicated in my opinion (ts-hack to create 3-state boolean):

enum ConfidentialSetting {
    False,
    True,
    CheckRequired
}

Now by default, this option will be shown in the "options" property of the command:

{
    type:    TYPE.typeText,
    options: {
        confidential: 2 // or the "CheckRequired" string (not better)
    }
}

Whereas void 0 wouldn't be set at all. I think it's better to stick with the void 0 variant.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear what the void 0 value means in the context of the confidential option.
So, in my opinion, the enum ConfidentialSetting is a good variant.

@AndreyBelym what do you think about it?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO:
TLDR; I'm against a tri-state enum:

  1. In this enum, 2 of 3 values just replicate boolean options. For me, it's a sign of redundant complexity.

  2. I can't see any case when I would want to specify ConfidentialSetting.CheckRequired manually. Why should we give it a name, if a user won't interact with it?

  3. I can describe easily the boolean configuration:
    If true, we hide the input value
    If false, we show the input value
    By default, it's true when the target selector is input[type=password], false otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, about point 2: I see a case, if we implement some global-level options, you may want to reset this flag to default in some cases. But since undefined is a common way to reset something to default in JS everywhere, I don't see it as a problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. Let it be.

src/test-run/index.js Outdated Show resolved Hide resolved
src/utils/is-password-input.ts Outdated Show resolved Hide resolved
@wentwrong wentwrong temporarily deployed to CI May 6, 2021 15:51 Inactive
src/client/driver/driver.js Outdated Show resolved Hide resolved
constructor (obj, validate) {
super();

this.confidential = void 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear what the void 0 value means in the context of the confidential option.
So, in my opinion, the enum ConfidentialSetting is a good variant.

@AndreyBelym what do you think about it?

"include": [ "../@types", "./"],
"include": [
"../@types",
"../ts-defs-src",
Copy link
Contributor

Choose a reason for hiding this comment

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

"../ts-defs-src"
Why does it necessary? I as remember we have a separate Gulp step for the TypeScript definitions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it should test the compatibility between our end-user TS defs and our source-level defs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added the end user TS defs in order to reuse them in the source code and don't maintain two versions of the same TS defs.

For example NodeSnapshot was already defined in the end-user TS defs:

interface NodeSnapshot {

Also the CompilerOptions interface was defined too:
type CompilerOptions = {

constructor (obj, validate) {
super();

this.confidential = void 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO:
TLDR; I'm against a tri-state enum:

  1. In this enum, 2 of 3 values just replicate boolean options. For me, it's a sign of redundant complexity.

  2. I can't see any case when I would want to specify ConfidentialSetting.CheckRequired manually. Why should we give it a name, if a user won't interact with it?

  3. I can describe easily the boolean configuration:
    If true, we hide the input value
    If false, we show the input value
    By default, it's true when the target selector is input[type=password], false otherwise.

Copy link
Contributor

@miherlosev miherlosev left a comment

Choose a reason for hiding this comment

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

@wentwrong wentwrong temporarily deployed to CI May 12, 2021 11:50 Inactive
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.

Mask values typed into masked inputs when sending action logs to a reporter
3 participants