Skip to content

Commit

Permalink
Disallow mutating the internals of TextInput
Browse files Browse the repository at this point in the history 
The TextInput::assert_ok_selection() method is meant to ensure that we
are not getting into a state where a selection refers to a location in
the control's contents which doesn't exist.

However, before this change we could have a situation where the
internals of the TextInput are changed by another part of the code,
without using its public API. This could lead to us having an invalid
selection.

I did manage to trigger such a situation (see the test added in this
commit) although it is quite contrived. There may be others that I
didn't think of, and it's also possible that future changes could
introduce new cases. (Including ones which trigger panics, if indexing
is used on the assumption that the selection indices are always valid.)

The current HTML specification doesn't explicitly say that
selectionStart/End must remain within the length of the content, but
that does seems to be the consensus reached in a discussion of this:

whatwg/html#2424

The test case I've added here is currently undefined in the spec which
is why I've added it in tests/wpt/mozilla.
  • Loading branch information
@jonleighton
jonleighton committed Feb 16, 2018
1 parent 7e31ae3 commit 32f7812
Show file tree
Hide file tree
Showing 7 changed files with 255 additions and 198 deletions.
107 changes: 50 additions & 57 deletions components/script/dom/htmlinputelement.rs
View file
Expand Up @@ -48,7 +48,6 @@ use script_traits::ScriptToConstellationChan;
use servo_atoms::Atom;
use std::borrow::ToOwned;
use std::cell::Cell;
use std::mem;
use std::ops::Range;
use style::attr::AttrValue;
use style::element_state::ElementState;
Expand Down Expand Up @@ -990,81 +989,64 @@ impl HTMLInputElement {
}

// https://html.spec.whatwg.org/multipage/#value-sanitization-algorithm
fn sanitize_value(&self) {
fn sanitize_value(&self, value: &mut DOMString) {
match self.input_type() {
InputType::Text | InputType::Search | InputType::Tel | InputType::Password => {
self.textinput.borrow_mut().single_line_content_mut().strip_newlines();
value.strip_newlines();
}
InputType::Url => {
let mut textinput = self.textinput.borrow_mut();
let content = textinput.single_line_content_mut();
content.strip_newlines();
content.strip_leading_and_trailing_ascii_whitespace();
value.strip_newlines();
value.strip_leading_and_trailing_ascii_whitespace();
}
InputType::Date => {
let mut textinput = self.textinput.borrow_mut();
if !textinput.single_line_content().is_valid_date_string() {
textinput.single_line_content_mut().clear();
if !value.is_valid_date_string() {
value.clear();
}
}
InputType::Month => {
let mut textinput = self.textinput.borrow_mut();
if !textinput.single_line_content().is_valid_month_string() {
textinput.single_line_content_mut().clear();
if !value.is_valid_month_string() {
value.clear();
}
}
InputType::Week => {
let mut textinput = self.textinput.borrow_mut();
if !textinput.single_line_content().is_valid_week_string() {
textinput.single_line_content_mut().clear();
if !value.is_valid_week_string() {
value.clear();
}
}
InputType::Color => {
let mut textinput = self.textinput.borrow_mut();

let is_valid = {
let content = textinput.single_line_content();
let mut chars = content.chars();
if content.len() == 7 && chars.next() == Some('#') {
let mut chars = value.chars();
if value.len() == 7 && chars.next() == Some('#') {
chars.all(|c| c.is_digit(16))
} else {
false
}
};

if is_valid {
let content = textinput.single_line_content_mut();
content.make_ascii_lowercase();
value.make_ascii_lowercase();
} else {
textinput.set_content("#000000".into(), true);
*value = "#000000".into();
}
}
InputType::Time => {
let mut textinput = self.textinput.borrow_mut();

if !textinput.single_line_content().is_valid_time_string() {
textinput.single_line_content_mut().clear();
if !value.is_valid_time_string() {
value.clear();
}
}
InputType::DatetimeLocal => {
let mut textinput = self.textinput.borrow_mut();
if textinput.single_line_content_mut()
.convert_valid_normalized_local_date_and_time_string().is_err() {
textinput.single_line_content_mut().clear();
if value.convert_valid_normalized_local_date_and_time_string().is_err() {
value.clear();
}
}
InputType::Number => {
let mut textinput = self.textinput.borrow_mut();
if !textinput.single_line_content().is_valid_floating_point_number_string() {
textinput.single_line_content_mut().clear();
if !value.is_valid_floating_point_number_string() {
value.clear();
}
}
// https://html.spec.whatwg.org/multipage/#range-state-(type=range):value-sanitization-algorithm
InputType::Range => {
self.textinput
.borrow_mut()
.single_line_content_mut()
.set_best_representation_of_the_floating_point_number();
value.set_best_representation_of_the_floating_point_number();
}
_ => ()
}
Expand All @@ -1075,20 +1057,23 @@ impl HTMLInputElement {
TextControlSelection::new(&self, &self.textinput)
}

fn update_text_contents(&self, value: DOMString, update_text_cursor: bool) -> ErrorResult {
fn update_text_contents(&self, mut value: DOMString, update_text_cursor: bool) -> ErrorResult {
match self.value_mode() {
ValueMode::Value => {
// Steps 1-2.
let old_value = mem::replace(self.textinput.borrow_mut().single_line_content_mut(), value);
// Step 3.
self.value_dirty.set(true);

// Step 4.
if update_text_cursor {
self.sanitize_value();
}
// Step 5.
if *self.textinput.borrow().single_line_content() != old_value {
self.textinput.borrow_mut().clear_selection_to_limit(Direction::Forward, update_text_cursor);
self.sanitize_value(&mut value);

let mut textinput = self.textinput.borrow_mut();

if *textinput.single_line_content() != value {
// Steps 1-2
textinput.set_content(value, update_text_cursor);

// Step 5.
textinput.clear_selection_to_limit(Direction::Forward, update_text_cursor);
}
}
ValueMode::Default |
Expand Down Expand Up @@ -1215,11 +1200,14 @@ impl VirtualMethods for HTMLInputElement {
}

// Step 6
self.sanitize_value();
let mut textinput = self.textinput.borrow_mut();
let mut value = textinput.single_line_content().clone();
self.sanitize_value(&mut value);
textinput.set_content(value, true);

// Steps 7-9
if !previously_selectable && self.selection_api_applies() {
self.textinput.borrow_mut().clear_selection_to_limit(Direction::Backward, true);
textinput.clear_selection_to_limit(Direction::Backward, true);
}
},
AttributeMutation::Removed => {
Expand All @@ -1240,9 +1228,10 @@ impl VirtualMethods for HTMLInputElement {
},
&local_name!("value") if !self.value_dirty.get() => {
let value = mutation.new_value(attr).map(|value| (**value).to_owned());
self.textinput.borrow_mut().set_content(
value.map_or(DOMString::new(), DOMString::from), true);
self.sanitize_value();
let mut value = value.map_or(DOMString::new(), DOMString::from);

self.sanitize_value(&mut value);
self.textinput.borrow_mut().set_content(value, true);
self.update_placeholder_shown_state();
},
&local_name!("name") if self.input_type() == InputType::Radio => {
Expand All @@ -1252,10 +1241,12 @@ impl VirtualMethods for HTMLInputElement {
&local_name!("maxlength") => {
match *attr.value() {
AttrValue::Int(_, value) => {
let mut textinput = self.textinput.borrow_mut();

if value < 0 {
self.textinput.borrow_mut().max_length = None
textinput.set_max_length(None);
} else {
self.textinput.borrow_mut().max_length = Some(value as usize)
textinput.set_max_length(Some(value as usize))
}
},
_ => panic!("Expected an AttrValue::Int"),
Expand All @@ -1264,10 +1255,12 @@ impl VirtualMethods for HTMLInputElement {
&local_name!("minlength") => {
match *attr.value() {
AttrValue::Int(_, value) => {
let mut textinput = self.textinput.borrow_mut();

if value < 0 {
self.textinput.borrow_mut().min_length = None
textinput.set_min_length(None);
} else {
self.textinput.borrow_mut().min_length = Some(value as usize)
textinput.set_min_length(Some(value as usize))
}
},
_ => panic!("Expected an AttrValue::Int"),
Expand Down
3 changes: 0 additions & 3 deletions components/script/dom/htmltextareaelement.rs
View file
Expand Up @@ -323,7 +323,6 @@ impl HTMLTextAreaElement {

// Step 1
let old_value = textinput.get_content();
let old_selection = textinput.selection_origin;

// Step 2
textinput.set_content(value, update_text_cursor);
Expand All @@ -334,8 +333,6 @@ impl HTMLTextAreaElement {
if old_value != textinput.get_content() {
// Step 4
textinput.clear_selection_to_limit(Direction::Forward, update_text_cursor);
} else {
textinput.selection_origin = old_selection;
}

self.upcast::<Node>().dirty(NodeDamage::OtherNodeDamage);
Expand Down
2 changes: 1 addition & 1 deletion components/script/dom/textcontrol.rs
View file
Expand Up @@ -257,7 +257,7 @@ impl<'a, E: TextControlElement> TextControlSelection<'a, E> {
}

fn direction(&self) -> SelectionDirection {
self.textinput.borrow().selection_direction
self.textinput.borrow().selection_direction()
}

// https://html.spec.whatwg.org/multipage/#set-the-selection-range
Expand Down
75 changes: 55 additions & 20 deletions components/script/textinput.rs
View file
Expand Up @@ -56,6 +56,18 @@ pub struct TextPoint {
pub index: usize,
}

impl TextPoint {
/// Returns a TextPoint constrained to be a valid location within lines
fn constrain_to(&self, lines: &[DOMString]) -> TextPoint {
let line = min(self.line, lines.len() - 1);

TextPoint {
line,
index: min(self.index, lines[line].len()),
}
}
}

#[derive(Clone, Copy, PartialEq)]
pub struct SelectionState {
start: TextPoint,
Expand All @@ -68,21 +80,26 @@ pub struct SelectionState {
pub struct TextInput<T: ClipboardProvider> {
/// Current text input content, split across lines without trailing '\n'
lines: Vec<DOMString>,

/// Current cursor input point
pub edit_point: TextPoint,
edit_point: TextPoint,

/// The current selection goes from the selection_origin until the edit_point. Note that the
/// selection_origin may be after the edit_point, in the case of a backward selection.
pub selection_origin: Option<TextPoint>,
selection_origin: Option<TextPoint>,
selection_direction: SelectionDirection,

/// Is this a multiline input?
multiline: bool,

#[ignore_malloc_size_of = "Can't easily measure this generic type"]
clipboard_provider: T,

/// The maximum number of UTF-16 code units this text input is allowed to hold.
///
/// <https://html.spec.whatwg.org/multipage/#attr-fe-maxlength>
pub max_length: Option<usize>,
pub min_length: Option<usize>,
pub selection_direction: SelectionDirection,
max_length: Option<usize>,
min_length: Option<usize>,
}

/// Resulting action to be taken by the owner of a text input that is handling an event.
Expand Down Expand Up @@ -175,6 +192,32 @@ impl<T: ClipboardProvider> TextInput<T> {
i
}

pub fn edit_point(&self) -> TextPoint {
self.edit_point
}

pub fn selection_origin(&self) -> Option<TextPoint> {
self.selection_origin
}

/// The selection origin, or the edit point if there is no selection. Note that the selection
/// origin may be after the edit point, in the case of a backward selection.
pub fn selection_origin_or_edit_point(&self) -> TextPoint {
self.selection_origin.unwrap_or(self.edit_point)
}

pub fn selection_direction(&self) -> SelectionDirection {
self.selection_direction
}

pub fn set_max_length(&mut self, length: Option<usize>) {
self.max_length = length;
}

pub fn set_min_length(&mut self, length: Option<usize>) {
self.min_length = length;
}

/// Remove a character at the current editing point
pub fn delete_char(&mut self, dir: Direction) {
if self.selection_origin.is_none() || self.selection_origin == Some(self.edit_point) {
Expand All @@ -196,12 +239,6 @@ impl<T: ClipboardProvider> TextInput<T> {
self.replace_selection(DOMString::from(s.into()));
}

/// The selection origin, or the edit point if there is no selection. Note that the selection
/// origin may be after the edit point, in the case of a backward selection.
pub fn selection_origin_or_edit_point(&self) -> TextPoint {
self.selection_origin.unwrap_or(self.edit_point)
}

/// The start of the selection (or the edit point, if there is no selection). Always less than
/// or equal to selection_end(), regardless of the selection direction.
pub fn selection_start(&self) -> TextPoint {
Expand Down Expand Up @@ -832,12 +869,6 @@ impl<T: ClipboardProvider> TextInput<T> {
&self.lines[0]
}

/// Get a mutable reference to the contents of a single-line text input. Panics if self is a multiline input.
pub fn single_line_content_mut(&mut self) -> &mut DOMString {
assert!(!self.multiline);
&mut self.lines[0]
}

/// Set the current contents of the text input. If this is control supports multiple lines,
/// any \n encountered will be stripped and force a new logical line.
pub fn set_content(&mut self, content: DOMString, update_text_cursor: bool) {
Expand All @@ -850,11 +881,15 @@ impl<T: ClipboardProvider> TextInput<T> {
} else {
vec!(content)
};

if update_text_cursor {
self.edit_point.line = min(self.edit_point.line, self.lines.len() - 1);
self.edit_point.index = min(self.edit_point.index, self.current_line_length());
self.edit_point = self.edit_point.constrain_to(&self.lines);
}
self.selection_origin = None;

if let Some(origin) = self.selection_origin {
self.selection_origin = Some(origin.constrain_to(&self.lines));
}

self.assert_ok_selection();
}

Expand Down

0 comments on commit 32f7812

Please sign in to comment.