Skip to content

issue #1347 Detect wrong armor checksum - #1371

Merged
DenBond7 merged 7 commits into
masterfrom
ip-1347-bad-checksum
Aug 8, 2021
Merged

issue #1347 Detect wrong armor checksum#1371
DenBond7 merged 7 commits into
masterfrom
ip-1347-bad-checksum

Conversation

@IvanPizhenko

Copy link
Copy Markdown
Contributor

This PR introduces detection of the armor CRC error with help of new version of PGPainless.

close#1347


Tests(delete all except exactly one):

  • Tests added or updated

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities

DenBond7
DenBond7 previously approved these changes Jul 28, 2021
} catch (ex: Exception) {
if (
ex is IOException &&
ex.message?.contains("crc check failed in armored message") == true

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does PGPainless return exception as IOException? it would be better to have a predefined exception here. For now, it looks a little ugly

@DenBond7

Copy link
Copy Markdown
Collaborator

@IvanPizhenko it seems JUnit tests were broken. Please look at them.

@IvanPizhenko

Copy link
Copy Markdown
ContributorAuthor

@DenBond7 thanks for noting this, will have a look.

@IvanPizhenko

Copy link
Copy Markdown
ContributorAuthor

@DenBond7 that's new issue arised as result of changes in the PGPainless which we asked for, see more details in this comment #1347 (comment)

@tomholub

tomholub commented Jul 28, 2021

Copy link
Copy Markdown
Collaborator

The fact our tests discovered this means they are pretty good - well done with the tests on this repo.

@IvanPizhenko

Copy link
Copy Markdown
ContributorAuthor

@DenBond7 I think I've got it working, please recheck.

@DenBond7DenBond7 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix use usage

@DenBond7
DenBond7 requested review from DenBond7 and removed request for seisvelasAugust 6, 2021 06:41
DenBond7
DenBond7 previously approved these changes Aug 6, 2021
@IvanPizhenko

IvanPizhenko commented Aug 6, 2021

Copy link
Copy Markdown
ContributorAuthor

@DenBond7 I can't get "Instrumentation tests(with email server)" pass in CI. Rerun few times with no luck. This is not first time that it happens. Any ideas how to solve/improve this?

@DenBond7

Copy link
Copy Markdown
Collaborator

@DenBond7 I can't get "Instrumentation tests(with email server)" pass in CI. Rerun few times with no luck. This is not first time that it happens. Any ideas how to solve/improve this?

Please mark failed tests as @NotReadyForCI. I will look at them later.

…phraseEmailOptionSingleFingerprint() as NotReadyForCI
@IvanPizhenko

Copy link
Copy Markdown
ContributorAuthor

Did it. Hoping this will help.

@DenBond7
DenBond7 merged commit 776c5d7 into masterAug 8, 2021
@DenBond7
DenBond7 deleted the ip-1347-bad-checksum branch August 8, 2021 10:02
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

security: PGPainless ignoring bad checksum?

3 participants

@IvanPizhenko@DenBond7@tomholub