Skip to content

Fix FWS is not trimmed correctly#8

Merged
mbaechler merged 1 commit intoapache:masterfrom
Weiling-Liao:master
Jul 5, 2019
Merged

Fix FWS is not trimmed correctly#8
mbaechler merged 1 commit intoapache:masterfrom
Weiling-Liao:master

Conversation

@Weiling-Liao
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown
Member

@mbaechler mbaechler left a comment

Choose a reason for hiding this comment

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

Looks good to me (and tests are green when run locally)

@mbaechler mbaechler merged commit 008af18 into apache:master Jul 5, 2019
@mbaechler
Copy link
Copy Markdown
Member

Thank you for you contribution @Weiling-Liao

public void testInvalidFWSSyntax() {
try {
new TagValue("p=test \r\n\r\n ");
new TagValue("p=test \r\n\r\n");
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm not familiar with FWS. But does the assertion still make sense once the whitespace after the CRLF is removed ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

According to https://tools.ietf.org/html/rfc6376#section-2.8 , CRLF followed by at least one whitespace, so this new version without at least one whitespace after CRLF will be wrong FWS.

Copy link
Copy Markdown
Author

@Weiling-Liao Weiling-Liao Jul 5, 2019

Choose a reason for hiding this comment

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

And this is a invalid FWS test. Plz let me know if there is any concern. Thanks

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thank you

agrinchenko pushed a commit to agrinchenko/james-jdkim that referenced this pull request Mar 28, 2026
Fix FWS is not trimmed correctly
Sign up for free to 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.

5 participants