Autofill: Send sensitive payment type predictions to the renderer [chromium/src : main]
Nan Lin (Gerrit)
Nan Lin added 1 comment![]( "Open in Gerrit")
Related details
- Stephen McGruer
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nan Lin (Gerrit)
Nan Lin added 1 comment![]( "Open in Gerrit")
- Chromium IPC Reviews
- Philip Rogers
- Stephen McGruer
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: ke...@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): ke...@chromium.org
Reviewer source(s):
ke...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Ken Buchanan
- Philip Rogers
- Stephen McGruer
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ken Buchanan (Gerrit)
Ken Buchanan added 1 comment![]( "Open in Gerrit")
// Indicates if the field contains sensitive payment data.This comment doesn't add any information.
More useful would be a reference to docs, specs or elsewhere in the code base that has more on the meaning of `sensitive payment data`.
Related details
- Nan Lin
- Philip Rogers
- 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. |
Nan Lin (Gerrit)
Nan Lin added 1 comment![]( "Open in Gerrit")
// Indicates if the field contains sensitive payment data.This comment doesn't add any information.
More useful would be a reference to docs, specs or elsewhere in the code base that has more on the meaning of `sensitive payment data`.
- Ken Buchanan
- Philip Rogers
- Stephen McGruer
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
void SetHasBeenSensitivePaymentField();A similar bit for password fields is slightly different (HTMLInputElement::has_been_password_field_, HTMLInputElement::MaybeSetHasBeenPasswordField, WebInputElement::MaybeSetHasBeenPasswordField). One difference is that there is an input type=password that is used to set has_been_password_field_, but otherwise, these features seem pretty similar. Are there reasons for being different (e.g. putting the bit on text control element rather than html input element, and using web_form_control_element rather than web_input_element)? If not, what do you think about copying how the password field bit works?
Related details
- Ken Buchanan
- Nan Lin
- 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. |
Nan Lin (Gerrit)
Nan Lin added 1 comment![]( "Open in Gerrit")
void SetHasBeenSensitivePaymentField();A similar bit for password fields is slightly different (HTMLInputElement::has_been_password_field_, HTMLInputElement::MaybeSetHasBeenPasswordField, WebInputElement::MaybeSetHasBeenPasswordField). One difference is that there is an input type=password that is used to set has_been_password_field_, but otherwise, these features seem pretty similar. Are there reasons for being different (e.g. putting the bit on text control element rather than html input element, and using web_form_control_element rather than web_input_element)? If not, what do you think about copying how the password field bit works?
IIUC type=password is only valid for the <input> element in HTML, that's why it's put in HtmlInputElement and WebInputElement. The sensitive payment data is determined by Autofill in the browser process. I chose TextControlElement and WebFormControlElement to handle both <input> and <textarea> elements uniformly.
Related details
- Ken Buchanan
- Philip Rogers
- Stephen McGruer
Ken Buchanan (Gerrit)
Ken Buchanan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ken Buchanan
| Code-Review | +1 |
1 comment
Related details
- Nan Lin
- Philip Rogers
- 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. |
Khushal Sagar (Gerrit)
Khushal Sagar added 2 comments![]( "Open in Gerrit")
Since the data was only available in the browser so far, the redaction is being done during APC proto generation [here](https://source.chromium.org/chromium/chromium/src/+/main:components/optimization_guide/content/browser/page_content_proto_util.cc;l=601;drc=bf712ec1a13783224debb691ba88ad5c15b93194). If the state can instead be sent to the renderer, we shouldn't use the browser-side code so all redaction can happen in the renderer. But I'm not sure how expensive it is to send these messages from browser -> renderer.
The alternative would be to send geometry for all form controls (which could be potentially sensitive) as a part of APC. That extraction is rare so it's ok but this will be more expensive when we track this geometry through the rendering pipeline (will happen every frame).
I think this is worth a design doc.
void SetHasBeenSensitivePaymentField();Nan LinA similar bit for password fields is slightly different (HTMLInputElement::has_been_password_field_, HTMLInputElement::MaybeSetHasBeenPasswordField, WebInputElement::MaybeSetHasBeenPasswordField). One difference is that there is an input type=password that is used to set has_been_password_field_, but otherwise, these features seem pretty similar. Are there reasons for being different (e.g. putting the bit on text control element rather than html input element, and using web_form_control_element rather than web_input_element)? If not, what do you think about copying how the password field bit works?
IIUC type=password is only valid for the <input> element in HTML, that's why it's put in HtmlInputElement and WebInputElement. The sensitive payment data is determined by Autofill in the browser process. I chose TextControlElement and WebFormControlElement to handle both <input> and <textarea> elements uniformly.
The browser-side code allows the autofill stack to set it for any form control element (link in the comment above). Based on that I'd say set it on `HTMLFormControlElement`? TextControlElement misses `select` elements for example. Those can be classified as sensitive payment fields as well right?
Nan Lin (Gerrit)
Nan Lin added 2 comments![]( "Open in Gerrit")
Since the data was only available in the browser so far, the redaction is being done during APC proto generation [here](https://source.chromium.org/chromium/chromium/src/+/main:components/optimization_guide/content/browser/page_content_proto_util.cc;l=601;drc=bf712ec1a13783224debb691ba88ad5c15b93194). If the state can instead be sent to the renderer, we shouldn't use the browser-side code so all redaction can happen in the renderer. But I'm not sure how expensive it is to send these messages from browser -> renderer.
The alternative would be to send geometry for all form controls (which could be potentially sensitive) as a part of APC. That extraction is rare so it's ok but this will be more expensive when we track this geometry through the rendering pipeline (will happen every frame).
I think this is worth a design doc.
I can write a one pager to compare the two approaches to redact APC. Just a note, we still need the data to be sent to the renderer in order to redact screenshot from the compositor.
void SetHasBeenSensitivePaymentField();Nan LinA similar bit for password fields is slightly different (HTMLInputElement::has_been_password_field_, HTMLInputElement::MaybeSetHasBeenPasswordField, WebInputElement::MaybeSetHasBeenPasswordField). One difference is that there is an input type=password that is used to set has_been_password_field_, but otherwise, these features seem pretty similar. Are there reasons for being different (e.g. putting the bit on text control element rather than html input element, and using web_form_control_element rather than web_input_element)? If not, what do you think about copying how the password field bit works?
Khushal SagarIIUC type=password is only valid for the <input> element in HTML, that's why it's put in HtmlInputElement and WebInputElement. The sensitive payment data is determined by Autofill in the browser process. I chose TextControlElement and WebFormControlElement to handle both <input> and <textarea> elements uniformly.
The browser-side code allows the autofill stack to set it for any form control element (link in the comment above). Based on that I'd say set it on `HTMLFormControlElement`? TextControlElement misses `select` elements for example. Those can be classified as sensitive payment fields as well right?
I followed https://crrev.com/c/7082971 which only redacts text control elements but not select elements. But it makes sense to set the flag on `HTMLFormControlElement` as this is not about content redaction. Updated accordingly, thanks.
Related details
- Ken Buchanan
- Khushal Sagar
- Philip Rogers
- 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. |
Philip Rogers (Gerrit)
Philip Rogers voted and added 2 comments![]( "Open in Gerrit")
Votes added by Philip Rogers
| Code-Review | +1 |
2 comments
void SetHasBeenSensitivePaymentField();Nan LinA similar bit for password fields is slightly different (HTMLInputElement::has_been_password_field_, HTMLInputElement::MaybeSetHasBeenPasswordField, WebInputElement::MaybeSetHasBeenPasswordField). One difference is that there is an input type=password that is used to set has_been_password_field_, but otherwise, these features seem pretty similar. Are there reasons for being different (e.g. putting the bit on text control element rather than html input element, and using web_form_control_element rather than web_input_element)? If not, what do you think about copying how the password field bit works?
Khushal SagarIIUC type=password is only valid for the <input> element in HTML, that's why it's put in HtmlInputElement and WebInputElement. The sensitive payment data is determined by Autofill in the browser process. I chose TextControlElement and WebFormControlElement to handle both <input> and <textarea> elements uniformly.
Nan LinThe browser-side code allows the autofill stack to set it for any form control element (link in the comment above). Based on that I'd say set it on `HTMLFormControlElement`? TextControlElement misses `select` elements for example. Those can be classified as sensitive payment fields as well right?
I followed https://crrev.com/c/7082971 which only redacts text control elements but not select elements. But it makes sense to set the flag on `HTMLFormControlElement` as this is not about content redaction. Updated accordingly, thanks.
- Ken Buchanan
- Khushal Sagar
- Nan Lin
- 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. |