Conversation
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix issue #306 where consecutive newlines in clear-signed PGP messages cause verification failures. The fix adds line separators to the output stream within the message processing loop, which were previously missing and causing empty lines to be lost.
Changes:
- Added conditional line separator writes in the clear-sign verification loop to preserve consecutive newlines
- Changed variable declarations from explicit types (
int,byte[]) to implicitvarfor consistency
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| PgpCore/PGP.VerifySync.cs | Added logic to write line separators in the clear-sign verification loop and changed variable types to var |
| PgpCore/PGP.VerifyAsync.cs | Same changes as sync version to maintain consistency between sync and async implementations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| line = lineOut.ToArray(); | ||
| await outStream.WriteAsync(line, 0, GetLengthWithoutSeparatorOrTrailingWhitespace(line)); | ||
| // Add missing new line | ||
| if (lookAhead != 1) |
There was a problem hiding this comment.
The condition lookAhead != 1 is incorrect. The value 1 represents ASCII character 1 (SOH - Start of Heading), not the end of stream. Based on the ReadInputLine implementation and consistent usage throughout the codebase, lookAhead returns -1 when the end of stream is reached. This condition should be lookAhead != -1 to add a line separator for all lines except the last one.
| if (lookAhead != 1) | |
| if (lookAhead != -1) |
| line = lineOut.ToArray(); | ||
| outStream.Write(line, 0, GetLengthWithoutSeparatorOrTrailingWhitespace(line)); | ||
| // Add missing new line | ||
| if (lookAhead != 1) |
There was a problem hiding this comment.
The fix for issue #306 (handling consecutive newlines in clear-signed messages) lacks test coverage. Consider adding tests that verify messages containing consecutive newlines such as "foo\nbar\n\n" or "foo\n\nbar" are correctly signed and verified, as these were the failing cases reported in the issue.
| if (lookAhead != 1) | |
| if (lookAhead != -1) |
| line = lineOut.ToArray(); | ||
| outStream.Write(line, 0, GetLengthWithoutSeparatorOrTrailingWhitespace(line)); | ||
| // Add missing new line | ||
| if (lookAhead != 1) |
There was a problem hiding this comment.
The condition lookAhead != 1 is incorrect. The value 1 represents ASCII character 1 (SOH - Start of Heading), not the end of stream. Based on the ReadInputLine implementation and consistent usage throughout the codebase, lookAhead returns -1 when the end of stream is reached. This condition should be lookAhead != -1 to add a line separator for all lines except the last one.
| if (lookAhead != 1) | |
| if (lookAhead != -1) |
|
#306