Conversation
jo
force-pushed
the
perform-additional-origin-fixups
branch
2 times, most recently
from
c3e7f3d to
a862553
Compare
jo
force-pushed
the
perform-additional-origin-fixups
branch
from
a862553 to
d30191d
Compare
bendk
left a comment
bendk
left a comment
There was a problem hiding this comment.
Looks very nice to me. I'm picturing you going through some huge log of invalid URLs to get this code. I just had a couple naming/code organization suggestions, but nothing blocking.
|
|
||
| pub fn count_by_form_action_origin(&self, form_action_origin: &str) -> Result<i64> { | ||
| match LoginEntry::validate_and_fixup_origin(form_action_origin) { | ||
| match LoginEntry::validate_and_fixup_form_action_origin(form_action_origin) { |
There was a problem hiding this comment.
Maybe this should just be validate_form_action_origin at this point. I think all the fixup code has moved out of it.
There was a problem hiding this comment.
Yeah that's right. I added the term normalize to hint that this methods returns something useful. For the origin I think fixup includes normalization, so I think that's fine.
| } | ||
|
|
||
| #[cfg(feature = "perform_additional_origin_fixups")] | ||
| fn looks_like_bare_ipv4(s: &str) -> bool { |
There was a problem hiding this comment.
When I first saw this I thought it was operating on the entire URL, but I think it's just the origin. Maybe the names could be updated to make it more clear.
What about creating a module called something like origin_fixup? That way it's obvious what all the functions are operating over. Also, you'd could have one #[cfg] attr around the entire module. Either an inline module or a separate file could work.
jo
force-pushed
the
perform-additional-origin-fixups
branch
from
d30191d to
b76c917
Compare
The `perform_additional_origin_fixups` feature was unintentionally also repairing `form_action_origin` values. Split the generic `validate_and_fixup_origin` into two field-specific wrappers around a new shared `parse_and_normalize_origin` helper. Also documents the `keydb` and `perform_additional_origin_fixups` features in Cargo.toml.
jo
force-pushed
the
perform-additional-origin-fixups
branch
from
b76c917 to
047e1c9
Compare
2 participants
Add
perform_additional_origin_fixupsfeature flag that repairsoriginvalues which fail URL parsing (bare domains, FireFTP quirks, barehttps:/https://, etc.) into parseable URLs instead of rejecting them. Scoped to theoriginfield only;form_action_originkeeps the stricter behavior, but can be disabled viaignore_form_action_origin_validation_errors.Intended to be enabled on desktop during migration to salvage legacy/addon-generated entries.
Pull Request checklist
[ci full]to the PR title.