FlowCrypt / flowcrypt-browser

FlowCrypt Browser extension for Chrome and Firefox
https://flowcrypt.com
Other
373 stars 46 forks source link

#5861 Upgrade openpgp to v6 #5866

Closed ioanmo226 closed 1 day ago

ioanmo226 commented 1 week ago

This PR upgraded puppeteer to v6

close #5861 // if this PR closes an issue


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):

ioanmo226 commented 6 days ago

For reference, I updated somePubKey because it was missing key flags required by the PR mentioned below. The current key throws an error when calling getEncryptionKey or a similar method.

Error: Could not find valid encryption key packet in key 0d5688ebf3102be7: None of the key flags is set: consider passing config.allowMissingKeyFlags

https://github.com/openpgpjs/openpgpjs/pull/1677

https://github.com/FlowCrypt/flowcrypt-browser/blob/d063d0d0380a4be87ea2059fe46cd8d72cdeb4e1/test/source/mock/attester/attester-key-constants.ts#L32

ioanmo226 commented 6 days ago

@sosnovsky FYI currently fixing failed tests and it's going well. But for decrypt - wrong message - checksum throws error test, Ascii armor integrity check failed error was expected. But after upgrading to openpgp v6, content is now displayed. I think current behavior is correct? Let me know your thought.

image

const expectedContent = 'Ascii armor integrity check failed';
  await BrowserRecipe.pgpBlockVerifyDecryptedContent(
    t,
    browser,
    threadId,
    {
      content: [expectedContent],
      error: 'decrypt error',
    },
    authHdr
  );
sosnovsky commented 5 days ago

Seems like current behavior is incorrect, as openpgpjs should throw error when encrypted message contains wrong checksum. I found old Tom's issue in openpgpjs repository - https://github.com/openpgpjs/openpgpjs/issues/924.

If in the v6 changing message checksum doesn't cause error, then can you please fill an issue for openpgpjs as it can be an unexpected behavior.

ioanmo226 commented 5 days ago

You are correct, @sosnovsky Thank you. Just filled an issue for it

ioanmo226 commented 5 days ago

https://github.com/openpgpjs/openpgpjs/issues/1810

ioanmo226 commented 5 days ago

@sosnovsky The OpenPGP team replied, confirming that the current v6 behavior is correct. RFC 4880 states that implementations MUST NOT reject an OpenPGP object if the CRC24 footer is present, missing, malformed, or disagrees with the computed CRC24 sum.

Should we update the current test to display the correct result and avoid throwing an error?

https://github.com/openpgpjs/openpgpjs/issues/1810#issuecomment-2490780175

sosnovsky commented 4 days ago

Got it, then let's stick with the newest spec and update test to expect decrypted message text.

By the way, is there any possibility to find if message has incorrect checksum? Then we at least be able to show some warning about checksum issue

ioanmo226 commented 4 days ago

Yeah, I think it's possible. First extract checksum from the message and then compute CRC24 and validate checksum.

ioanmo226 commented 4 days ago

Maybe need to make another issue and consider this feature in there?

sosnovsky commented 4 days ago

Sounds good 👍 Yes, let's file another issue for it