Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid message-id used in connection test #471

Closed
JvGinkel opened this issue Nov 10, 2017 · 9 comments

Comments

Projects
None yet
3 participants
@JvGinkel
Copy link

commented Nov 10, 2017

const char* response = connection.Request("ARTICLE <connection-test>\r\n");

The message-id used for the test is an invalid message-id. If you are testing against a strict usenet provider you will receive a response code 501 and not a 4xx code as you are testing against and now the test will fail because of this.

Solution would be to change the message-id to a valid one like <connection@test> but even then when someone create this message-id you will receive a 220 response code so the if-statement will still fail because of this as you want a response code starting with a 4.

In my opinion the best solution would be to test against a valid msg-id and accept the 220 and the 430 response code.

@hugbug

This comment has been minimized.

Copy link
Member

commented Nov 10, 2017

Why do you think it's invalid? RFC 3977 states only:

message-id is an arbitrary opaque string that merely needs to meet certain syntactic requirements

Without defining those "syntactic requirements".

@JvGinkel

This comment has been minimized.

Copy link
Author

commented Nov 10, 2017

See https://tools.ietf.org/html/rfc1036#section-2.1.5

The "Message-ID" line gives the message a unique identifier.  The
    Message-ID may not be reused during the lifetime of any previous
    message with the same Message-ID.  (It is recommended that no
    Message-ID be reused for at least two years.)  Message-ID's have the
    syntax:

                     <string not containing blank or ">">

    In order to conform to RFC-822, the Message-ID must have the format:

                          <unique@full_domain_name>

    where full_domain_name is the full name of the host at which the
    message entered the network, including a domain that host is in, and
    unique is any string of printing ASCII characters, not including "<"
    (left angle bracket), ">" (right angle bracket), or "@" (at sign).

So there must be a @ in the message-id

@JvGinkel JvGinkel changed the title Invalid message-id use in connection test Invalid message-id used in connection test Nov 10, 2017

@hugbug

This comment has been minimized.

Copy link
Member

commented Nov 10, 2017

Thanks for the reference. I'll do the change as you suggest.

@hugbug hugbug added the testing-bug label Nov 10, 2017

@hugbug hugbug added this to the v20 milestone Nov 10, 2017

hugbug added a commit that referenced this issue Nov 11, 2017

#471: more robust news server connection test
This fixes connection test errors with servers checking message id
format correctness.
@hugbug

This comment has been minimized.

Copy link
Member

commented Nov 11, 2017

Fixed.
I can't test this but I hope it will work correct now.

@hugbug hugbug closed this Nov 11, 2017

@Safihre

This comment has been minimized.

Copy link

commented Nov 11, 2017

Why request an article at all? SABnzbd doesn't do it and never had any complaints over the years?

@hugbug

This comment has been minimized.

Copy link
Member

commented Nov 11, 2017

We need to send some command to test credentials. Auth requests should be sent only when server requests authorization. Furthermore requesting articles is what nzbget does during download. Therefore that's a perfect test that everything works properly. If the test implementation isn't buggy of course ;)

@JvGinkel

This comment has been minimized.

Copy link
Author

commented Nov 12, 2017

Thanks for the fix! You can test with a free trial account of https://Usenet.farm or I can test it when you created a pre-release version with this fix in it.

@JvGinkel

This comment has been minimized.

Copy link
Author

commented Nov 12, 2017

Just tested 20.0-testing-r2159 and this problem is fixed!

@hugbug

This comment has been minimized.

Copy link
Member

commented Nov 12, 2017

Thanks for testing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.