Employ user verification mechanisms from OpenPGP v5 #4946
Conversation
| // todo: `email-addresses` parser used by OpenPGP.js consider top-level domains to be valid for emails e.g. address@domain | ||
| // should we allow it too (or use `email-addresses` package when manipulating user ids in keys? | ||
| // For now, I'm explicitly `localhost` domain, which is perfectly legal for testing | ||
| return /^(([^<>()\[\]\\.,;:\s@"]+(\.[^<>()\[\]\\.,;:\s@"]+)*)|(".+"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}])|localhost|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))$/i.test( |
There was a problem hiding this comment.
I'm re-using Str.parseEmail method to re-extract email from userids verified by OpenPGP.js, as those verification methods only return userid string (and all valid userids are supposed to have email address embedded).
OpenPGP.js is using email-addresses NPM package to extract email address from the userid.
So several questions/issues arise:
- Should we use
email-addressespackage or our own version for the re-extraction? - If using own version, should we validate the email address?
email-addressesvalidation is more lenient, as it allows a top level domain only, e.g.address@domain. As I understand, this type of validation isn't convenient forcomposeboxes, should we have a separate validation option when extracting emails from userids, like `VALIDATE-LENIENT"?
I had to explicitly add localhost as a valid domain into the regex
There was a problem hiding this comment.
I think it's good as you've done it. It would indeed be odd to allow hello@yup as a valid email when composing. Similarly, having two ways to verify based on context is unnecessary complication. I think it doesn't necessarily need 100% alignment with OpenPGP.js. If there is an edge case when one check passes and the other doesn't, the result is still a failed action. Actually there's currently three places that validate email: this, OpenPGP.js, Gmail API. So it will never be unified. As long as nobody reports a problem, I'd leave it.
There was a problem hiding this comment.
As per above comment I'd remove the todo since I think it's ok as is
| result.set(`User id ${i}`, key.users[i].userID!.userID); | ||
| result.set(`User id ${i}`, users[i].valid ? users[i].userID : '* REVOKED, INVALID OR MISSING SIGNATURE *'); |
There was a problem hiding this comment.
Could say * REVOKED, INVALID OR MISSING SIGNATURE * ${users[i].userID} for easier debugging
| // todo: `email-addresses` parser used by OpenPGP.js consider top-level domains to be valid for emails e.g. address@domain | ||
| // should we allow it too (or use `email-addresses` package when manipulating user ids in keys? | ||
| // For now, I'm explicitly `localhost` domain, which is perfectly legal for testing | ||
| return /^(([^<>()\[\]\\.,;:\s@"]+(\.[^<>()\[\]\\.,;:\s@"]+)*)|(".+"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}])|localhost|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))$/i.test( |
There was a problem hiding this comment.
As per above comment I'd remove the todo since I think it's ok as is
…#4943) * build(deps-dev): bump @openpgp/web-stream-tools from 0.0.11 to 0.0.13 Bumps [@openpgp/web-stream-tools](https://github.com/openpgpjs/web-stream-tools) from 0.0.11 to 0.0.13. - [Release notes](https://github.com/openpgpjs/web-stream-tools/releases) - [Commits](openpgpjs/web-stream-tools@v0.0.11...v0.0.13) --- updated-dependencies: - dependency-name: "@openpgp/web-stream-tools" dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> * #4282 fix: duplicated contact search result (#4944) * fix: dupliated contact search result * feat: added ui test * fix: pr review * #4940 Update build script to not produce .bak files (#4945) * update build script * rename STREAMS_OUTDIR to STREAMS_FILES * #4844 feat: renew id token when expires (#4949) * feat: renew id token when expires * fix: pr reviews * Employ user verification mechanisms from OpenPGP v5 (#4946) * reuse getPrimeryUser method * verifying key users * allow localhost domain for email address * fix * fix and test * use verified users in key-import-ui * PR review fixes --------- Co-authored-by: Roman Shevchenko <[email protected]> * prevent pasting large input on secure compose (#4914) * prevent pasting large input on secure compose * Added type definition for SquireEditor.getRoot() * apply requested change * update * cleanup * update * consider selection * add warning modal * update * update * update --------- Co-authored-by: Roman Shevchenko <[email protected]> Co-authored-by: Roman Shevchenko <[email protected]> * fix --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Ioan Moldovan <[email protected]> Co-authored-by: Roma Sosnovsky <[email protected]> Co-authored-by: Roman <[email protected]> Co-authored-by: Roman Shevchenko <[email protected]> Co-authored-by: Mart G <[email protected]>
This PR employs user verification mechanisms from OpenPGP v5
On diagnostics page an unverified user id looks like this
As for Key parsing, all unverified user ids are omitted from the
Key.identitiesproperty.close #4588
Tests (delete all except exactly one):
To be filled by reviewers
I have reviewed that this PR... (tick whichever items you personally focused on during this review):