[APC] Redact sensitive payment fields from screenshots using APC [chromium/src : main]
Nan Lin (Gerrit)
Nan Lin voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Chromium IPC Reviews
- Khushal Sagar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: ort...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): ort...@chromium.org
Reviewer source(s):
ort...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Giovanni Ortuno Urquidi
- Khushal Sagar
- Mike Wittman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Ortuno Urquidi (Gerrit)
Giovanni Ortuno Urquidi voted and added 1 comment![]( "Open in Gerrit")
Votes added by Giovanni Ortuno Urquidi
| Code-Review | +1 |
1 comment
Related details
- Khushal Sagar
- Mike Wittman
- Nan Lin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike Wittman (Gerrit)
Mike Wittman voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mike Wittman
| Code-Review | +1 |
1 comment
int accessibility_focused_node_id_ = 0;nit: no need to provide the `= 0` initializer since this is unconditionally initialized in the constructor
Related details
- Khushal Sagar
- Nan Lin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nan Lin (Gerrit)
Nan Lin added 2 comments![]( "Open in Gerrit")
nit: no need to provide the `= 0` initializer since this is unconditionally initialized in the constructor
Done
Related details
- Khushal Sagar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Khushal Sagar (Gerrit)
Khushal Sagar added 1 comment![]( "Open in Gerrit")
I want to see the conclusion on https://chromium-review.googlesource.com/c/chromium/src/+/7511630 before landing this. If it's possible to get autofill state in the renderer and do all APC redaction there, then we can make all redaction work like passwords.
Related details
- Nan Lin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aaron Leventhal (Gerrit)
Aaron Leventhal added 1 comment![]( "Open in Gerrit")
!(options_->include_sensitive_payments_for_redaction &&Optional nits:
1. Compound if statements are hard to read and reason about. Consider putting this condition into a helper so it's easier to read. Something like ShouldSkipSubtree() which uses the repeated early return pattern:
```
1 // comment
2 if (condition) {
3 return true|false;
4 }
```
2. It sounds like TrackPasswordRedactionIfNeeded() and some of the variables that go with it are no longer just about passwords. Do we need to rename to something like TrackSensitiveFieldRedactionIfNeeded()? There should also be a comment saying what it does above the declaration in the header file.
Related details
- Nan Lin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nan Lin (Gerrit)
Nan Lin added 2 comments![]( "Open in Gerrit")
!(options_->include_sensitive_payments_for_redaction &&Optional nits:
1. Compound if statements are hard to read and reason about. Consider putting this condition into a helper so it's easier to read. Something like ShouldSkipSubtree() which uses the repeated early return pattern:
```
1 // comment
2 if (condition) {
3 return true|false;
4 }
```2. It sounds like TrackPasswordRedactionIfNeeded() and some of the variables that go with it are no longer just about passwords. Do we need to rename to something like TrackSensitiveFieldRedactionIfNeeded()? There should also be a comment saying what it does above the declaration in the header file.
Compound if statements are hard to read and reason about. Consider putting this condition into a helper so it's easier to read.
Updated, thanks.
It sounds like TrackPasswordRedactionIfNeeded() and some of the variables that go with it are no longer just about passwords. Do we need to rename to something like TrackSensitiveFieldRedactionIfNeeded().
`TrackSensitiveFieldRedactionIfNeeded()` only tracks passwords here, the sensitive payment fields are redacted in the browser.
Related details
- Aaron Leventhal
- Giovanni Ortuno Urquidi
- Mike Wittman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aaron Leventhal (Gerrit)
Aaron Leventhal added 2 comments![]( "Open in Gerrit")
proto::REDACTION_DECISION_REDACTED_IS_SENSITIVE_PAYMENT_FIELD &&Should we log an error or something so that we can find out if there was a sensitive field where geometry was not provided? Otherwise we silently fail if Blink did not pass the geometry.
if (options_->include_sensitive_payments_for_redaction &&I think it probably matters what kind of form control. For example, I doubt checkboxes and radio buttons will be sensitive. We should look at all of the form control types. Obviously we want anything that's a textfield, and we might want <select> fields.
Related details
- Giovanni Ortuno Urquidi
- Mike Wittman
- Nan Lin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nan Lin (Gerrit)
Nan Lin added 3 comments![]( "Open in Gerrit")
proto::REDACTION_DECISION_REDACTED_IS_SENSITIVE_PAYMENT_FIELD &&Should we log an error or something so that we can find out if there was a sensitive field where geometry was not provided? Otherwise we silently fail if Blink did not pass the geometry.
Done
if (options_->include_sensitive_payments_for_redaction &&I think it probably matters what kind of form control. For example, I doubt checkboxes and radio buttons will be sensitive. We should look at all of the form control types. Obviously we want anything that's a textfield, and we might want <select> fields.
There was a similar discussion on what elements to consider for sensitive payments here: https://chromium-review.googlesource.com/c/chromium/src/+/7511630/comment/4c85fbe2_04052111/
Currently the browser-side code allows the autofill stack to set it for any form control element (see [here](https://source.chromium.org/chromium/chromium/src/+/main:components/optimization_guide/content/browser/page_content_proto_util.cc;l=601;drc=bf712ec1a13783224debb691ba88ad5c15b93194).
Therefore we decided to consider all form control elements, and I think we can follow this similarly here as well?
Related details
- Aaron Leventhal
- Giovanni Ortuno Urquidi
- Mike Wittman
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aaron Leventhal (Gerrit)
Aaron Leventhal added 1 comment![]( "Open in Gerrit")
if (options_->include_sensitive_payments_for_redaction &&Nan LinI think it probably matters what kind of form control. For example, I doubt checkboxes and radio buttons will be sensitive. We should look at all of the form control types. Obviously we want anything that's a textfield, and we might want <select> fields.
There was a similar discussion on what elements to consider for sensitive payments here: https://chromium-review.googlesource.com/c/chromium/src/+/7511630/comment/4c85fbe2_04052111/
Currently the browser-side code allows the autofill stack to set it for any form control element (see [here](https://source.chromium.org/chromium/chromium/src/+/main:components/optimization_guide/content/browser/page_content_proto_util.cc;l=601;drc=bf712ec1a13783224debb691ba88ad5c15b93194).
Therefore we decided to consider all form control elements, and I think we can follow this similarly here as well?
Since we're concerned about performance let's ask that team which fields can be detected as sensitive. I'm pretty sure a button is never sensitive, and lot of form controls are buttons. Also, with the logging you're adding for when we miss a rect, we'll find out if we ever under-populate the geometry anyway.
Related details
- Giovanni Ortuno Urquidi
- Mike Wittman
- Nan Lin
Nan Lin (Gerrit)
Nan Lin added 1 comment![]( "Open in Gerrit")
if (options_->include_sensitive_payments_for_redaction &&Nan LinI think it probably matters what kind of form control. For example, I doubt checkboxes and radio buttons will be sensitive. We should look at all of the form control types. Obviously we want anything that's a textfield, and we might want <select> fields.
Aaron LeventhalThere was a similar discussion on what elements to consider for sensitive payments here: https://chromium-review.googlesource.com/c/chromium/src/+/7511630/comment/4c85fbe2_04052111/
Currently the browser-side code allows the autofill stack to set it for any form control element (see [here](https://source.chromium.org/chromium/chromium/src/+/main:components/optimization_guide/content/browser/page_content_proto_util.cc;l=601;drc=bf712ec1a13783224debb691ba88ad5c15b93194).
Therefore we decided to consider all form control elements, and I think we can follow this similarly here as well?
Since we're concerned about performance let's ask that team which fields can be detected as sensitive. I'm pretty sure a button is never sensitive, and lot of form controls are buttons. Also, with the logging you're adding for when we miss a rect, we'll find out if we ever under-populate the geometry anyway.
@schw...@google.com @smcg...@chromium.org Could you please suggest what form control elements could be detected for sensitive payment fields? Definitely input, textarea, select, any other types?
Re the performance, I don't think it's a major concern for this approach as APC extraction is rare. But let's see whether we can limit this.
Related details
- Aaron Leventhal
- Christoph Schwering
- Giovanni Ortuno Urquidi
- Mike Wittman
- Stephen McGruer
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aaron Leventhal (Gerrit)
Aaron Leventhal added 1 comment![]( "Open in Gerrit")
if (options_->include_sensitive_payments_for_redaction &&Nan LinI think it probably matters what kind of form control. For example, I doubt checkboxes and radio buttons will be sensitive. We should look at all of the form control types. Obviously we want anything that's a textfield, and we might want <select> fields.
Aaron LeventhalThere was a similar discussion on what elements to consider for sensitive payments here: https://chromium-review.googlesource.com/c/chromium/src/+/7511630/comment/4c85fbe2_04052111/
Currently the browser-side code allows the autofill stack to set it for any form control element (see [here](https://source.chromium.org/chromium/chromium/src/+/main:components/optimization_guide/content/browser/page_content_proto_util.cc;l=601;drc=bf712ec1a13783224debb691ba88ad5c15b93194).
Therefore we decided to consider all form control elements, and I think we can follow this similarly here as well?
Nan LinSince we're concerned about performance let's ask that team which fields can be detected as sensitive. I'm pretty sure a button is never sensitive, and lot of form controls are buttons. Also, with the logging you're adding for when we miss a rect, we'll find out if we ever under-populate the geometry anyway.
@schw...@google.com @smcg...@chromium.org Could you please suggest what form control elements could be detected for sensitive payment fields? Definitely input, textarea, select, any other types?
Re the performance, I don't think it's a major concern for this approach as APC extraction is rare. But let's see whether we can limit this.
To be clear, the input type will matter.
I would say, any input that creates an editable field (e.g. input type=tel) but not things like input type=button.
Related details
- Christoph Schwering
- Giovanni Ortuno Urquidi
- Mike Wittman
- Nan Lin
- Stephen McGruer
Christoph Schwering (Gerrit)
Christoph Schwering added 1 comment![]( "Open in Gerrit")
<!DOCTYPE html>
<body>
<form action='/initial'>
<input style='position: fixed; top: 0; left: 0; width: 100px; height: 100px;' id='filled-cc-number' type='text' autocomplete='cc-number' value='4111111111111111'>
<input style='position: fixed; top: 110; left: 0; width: 100px; height: 100px;' id='filled-cc-exp' type='text' autocomplete='cc-exp' value='12/2030'>
</form>
</body>I'd inline this in the test or move it to some subdirectory because other tests use other credit card forms.
Related details
Nan Lin (Gerrit)
Nan Lin added 1 comment![]( "Open in Gerrit")
if (options_->include_sensitive_payments_for_redaction &&Nan LinI think it probably matters what kind of form control. For example, I doubt checkboxes and radio buttons will be sensitive. We should look at all of the form control types. Obviously we want anything that's a textfield, and we might want <select> fields.
Aaron LeventhalThere was a similar discussion on what elements to consider for sensitive payments here: https://chromium-review.googlesource.com/c/chromium/src/+/7511630/comment/4c85fbe2_04052111/
Currently the browser-side code allows the autofill stack to set it for any form control element (see [here](https://source.chromium.org/chromium/chromium/src/+/main:components/optimization_guide/content/browser/page_content_proto_util.cc;l=601;drc=bf712ec1a13783224debb691ba88ad5c15b93194).
Therefore we decided to consider all form control elements, and I think we can follow this similarly here as well?
Nan LinSince we're concerned about performance let's ask that team which fields can be detected as sensitive. I'm pretty sure a button is never sensitive, and lot of form controls are buttons. Also, with the logging you're adding for when we miss a rect, we'll find out if we ever under-populate the geometry anyway.
Aaron Leventhal@schw...@google.com @smcg...@chromium.org Could you please suggest what form control elements could be detected for sensitive payment fields? Definitely input, textarea, select, any other types?
Re the performance, I don't think it's a major concern for this approach as APC extraction is rare. But let's see whether we can limit this.
To be clear, the input type will matter.
I would say, any input that creates an editable field (e.g. input type=tel) but not things like input type=button.
For inputs, we can use [IsTextFieldInputType](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/input_type.h;l=167;drc=976a71ef2a509d4391d814b52b9f4700fdf35789;bpv=1;bpt=1). Would month input type be predicted as cc-exp-month? @schw...@google.com @smcg...@chromium.org?
For selects, only select-ones are possible right?
Related details
- Aaron Leventhal
- Giovanni Ortuno Urquidi
- Mike Wittman
- Stephen McGruer
Christoph Schwering (Gerrit)
Christoph Schwering added 1 comment![]( "Open in Gerrit")
if (options_->include_sensitive_payments_for_redaction &&Nan LinI think it probably matters what kind of form control. For example, I doubt checkboxes and radio buttons will be sensitive. We should look at all of the form control types. Obviously we want anything that's a textfield, and we might want <select> fields.
Aaron LeventhalThere was a similar discussion on what elements to consider for sensitive payments here: https://chromium-review.googlesource.com/c/chromium/src/+/7511630/comment/4c85fbe2_04052111/
Currently the browser-side code allows the autofill stack to set it for any form control element (see [here](https://source.chromium.org/chromium/chromium/src/+/main:components/optimization_guide/content/browser/page_content_proto_util.cc;l=601;drc=bf712ec1a13783224debb691ba88ad5c15b93194).
Therefore we decided to consider all form control elements, and I think we can follow this similarly here as well?
Nan LinSince we're concerned about performance let's ask that team which fields can be detected as sensitive. I'm pretty sure a button is never sensitive, and lot of form controls are buttons. Also, with the logging you're adding for when we miss a rect, we'll find out if we ever under-populate the geometry anyway.
Aaron Leventhal@schw...@google.com @smcg...@chromium.org Could you please suggest what form control elements could be detected for sensitive payment fields? Definitely input, textarea, select, any other types?
Re the performance, I don't think it's a major concern for this approach as APC extraction is rare. But let's see whether we can limit this.
Nan LinTo be clear, the input type will matter.
I would say, any input that creates an editable field (e.g. input type=tel) but not things like input type=button.
For inputs, we can use [IsTextFieldInputType](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/input_type.h;l=167;drc=976a71ef2a509d4391d814b52b9f4700fdf35789;bpv=1;bpt=1). Would month input type be predicted as cc-exp-month? @schw...@google.com @smcg...@chromium.org?
For selects, only select-ones are possible right?
Could you please suggest what form control elements could be detected for sensitive payment fields?
Autofill currently may fill these form control types: https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/content/renderer/form_autofill_util.cc;l=2300-2331;drc=a636e7dace1ec22bdd5eb4ab27ccbca9712020d3. Radio button and checkbox support will soon be removed but will likely be added back in a different form.
Autofill also supports `contenteditable`s to some extent, but that's not intended for credit card forms and it'd require a strange combination of special cases for them to be filled with CC data.
Would month input type be predicted as cc-exp-month?
Yes `<input type=month>` may be filled with the expiration date.
only select-ones are possible right?
- Aaron Leventhal
- Nan Lin
- Stephen McGruer