Clear out form_action_origin and http_realm when deleting logins #4573
Assignees
Comments
bendk
added a commit
to bendk/application-services
that referenced
this issue
Oct 15, 2021
This is another deprecated function, although it was still in use by the
dupe checking code. So removing it required more effort.
There was a lot of overlap between `potential_dupes_ignoring_username()`
and `find_login_to_update()`. I factored out the common parts into the
new `get_by_entry_target()` function. Naming this one was hard, but
the idea is to search using `origin` plus either `form_action_origin` or
`http_realm`.
The new function has one difference from the old ones. It checks which
of `form_action_origin` or `http_realm` is set and uses that as the
query param, rather than throwing both in there. This means:
- For valid login entries where exactly 1 is set, it works exactly the
same, since NULL comparisons are always false.
- If neither or both are set, it returns an error rather than
running the query. I think in most cases that query would return 0
rows.
This affected one thing, which was the dupe-checking in the migration
function tests. I think this was maybe a test-only issue, since we
currently don't clear `form_action_origin` and `http_realm` when
deleting the login (mozilla#4573). I'm not sure what desktop does though.
In any case, I think not running the dupe check for deleted logins makes
the migration logic slightly safer.
bendk
added a commit
to bendk/application-services
that referenced
this issue
Oct 18, 2021
bendk
added a commit
to bendk/application-services
that referenced
this issue
Oct 19, 2021
bendk
added a commit
that referenced
this issue
Oct 19, 2021
Clear form_action_origin and http_realm fields when deleting (#4573)
bendk
added a commit
to bendk/application-services
that referenced
this issue
Oct 22, 2021
This is another deprecated function, although it was still in use by the
dupe checking code. So removing it required more effort.
There was a lot of overlap between `potential_dupes_ignoring_username()`
and `find_login_to_update()`. I factored out the common parts into the
new `get_by_entry_target()` function. Naming this one was hard, but
the idea is to search using `origin` plus either `form_action_origin` or
`http_realm`.
The new function has one difference from the old ones. It checks which
of `form_action_origin` or `http_realm` is set and uses that as the
query param, rather than throwing both in there. This means:
- For valid login entries where exactly 1 is set, it works exactly the
same, since NULL comparisons are always false.
- If neither or both are set, it returns an error rather than
running the query. I think in most cases that query would return 0
rows.
This affected one thing, which was the dupe-checking in the migration
function tests. I think this was maybe a test-only issue, since we
currently don't clear `form_action_origin` and `http_realm` when
deleting the login (mozilla#4573). I'm not sure what desktop does though.
In any case, I think not running the dupe check for deleted logins makes
the migration logic slightly safer.
bendk
added a commit
to bendk/application-services
that referenced
this issue
Oct 22, 2021
This is another deprecated function, although it was still in use by the
dupe checking code. So removing it required more effort.
There was a lot of overlap between `potential_dupes_ignoring_username()`
and `find_login_to_update()`. I factored out the common parts into the
new `get_by_entry_target()` function. Naming this one was hard, but
the idea is to search using `origin` plus either `form_action_origin` or
`http_realm`.
The new function has one difference from the old ones. It checks which
of `form_action_origin` or `http_realm` is set and uses that as the
query param, rather than throwing both in there. This means:
- For valid login entries where exactly 1 is set, it works exactly the
same, since NULL comparisons are always false.
- If neither or both are set, it returns an error rather than
running the query. I think in most cases that query would return 0
rows.
This affected one thing, which was the dupe-checking in the migration
function tests. I think this was maybe a test-only issue, since we
currently don't clear `form_action_origin` and `http_realm` when
deleting the login (mozilla#4573). I'm not sure what desktop does though.
In any case, I think not running the dupe check for deleted logins makes
the migration logic slightly safer.
|
Moved to bugzilla: https://bugzilla.mozilla.org/show_bug.cgi?id=1866509 Change performed by the Move to Bugzilla add-on. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
In the
LoginsDb.delete()method, we clear our all sensitive fields..Origin is considered one of the sensitive fields, I think that should mean that
form_action_originandhttp_realmshould also be sensitive, since both could be used to identify the site the login was saved for.┆Issue is synchronized with this Jira Task
┆Sprint End Date: 2021-10-29
The text was updated successfully, but these errors were encountered: