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

Check for subject and queue name validity #279

Merged
merged 2 commits into from
Aug 26, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions NATS.Client/Conn.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3238,9 +3238,13 @@ private void disableSubChannelPooling()
internal AsyncSubscription subscribeAsync(string subject, string queue,
EventHandler<MsgHandlerEventArgs> handler)
{
if (string.IsNullOrWhiteSpace(subject))
if (!Subscription.IsValidSubject(subject))
{
throw new NATSBadSubscriptionException();
throw new NATSBadSubscriptionException("Invalid subject.");
}
if (queue != null && !Subscription.IsValidQueueGroupName(queue))
{
throw new NATSBadSubscriptionException("Invalid queue group name.");
}

AsyncSubscription s = null;
Expand Down Expand Up @@ -3272,9 +3276,13 @@ internal AsyncSubscription subscribeAsync(string subject, string queue,
// function that indicates interest in a subject.
private SyncSubscription subscribeSync(string subject, string queue)
{
if (string.IsNullOrWhiteSpace(subject))
if (!Subscription.IsValidSubject(subject))
{
throw new NATSBadSubscriptionException();
throw new NATSBadSubscriptionException("Invalid subject.");
}
if (queue != null && !Subscription.IsValidQueueGroupName(queue))
{
throw new NATSBadSubscriptionException("Invalid queue group name.");
}

SyncSubscription s = null;
Expand Down
3 changes: 2 additions & 1 deletion NATS.Client/Exceptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,8 @@ internal NATSMaxMessagesException() : base("Maximum number of messages have been
/// </summary>
public class NATSBadSubscriptionException : NATSException
{
internal NATSBadSubscriptionException() : base("Subcription is not valid.") { }
internal NATSBadSubscriptionException() : base("Subscription is not valid.") { }
internal NATSBadSubscriptionException(string s) : base("s") { }
}

/// <summary>
Expand Down
44 changes: 44 additions & 0 deletions NATS.Client/Subscription.cs
Original file line number Diff line number Diff line change
Expand Up @@ -648,6 +648,50 @@ public long Dropped
}
}

#region validation

static private readonly char[] invalidSubjectChars = { '\r', '\n', '\t', ' '};

private static bool IsValidTokenOrQName(string value)
{
return (!string.IsNullOrEmpty(value) && value.LastIndexOfAny(invalidSubjectChars) < 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know if LastIndexOfAny has received the perf improvements that IndexOfAny has as of .NET Core.

Suggested change
return (!string.IsNullOrEmpty(value) && value.LastIndexOfAny(invalidSubjectChars) < 0);
return (!string.IsNullOrEmpty(value) && value.IndexOfAny(invalidSubjectChars) < 0);

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, thanks!

}

/// <summary>
/// Checks if a subject is valid.
/// </summary>
/// <param name="subject">The subject to check</param>
/// <returns>true if valid, false otherwise.</returns>
static public bool IsValidSubject(string subject)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
static public bool IsValidSubject(string subject)
public static bool IsValidSubject(string subject)

Copy link
Member Author

Choose a reason for hiding this comment

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

+1

{
if (!IsValidTokenOrQName(subject))
{
return false;
}

string[] tokens = subject.Split('.');
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is ever on the hot path (e.g. Publish) we'll want to avoid Split.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add a comment.

foreach (string t in tokens)
{
if (t.Length == 0)
{
return false;
}
}
return true;
}

/// <summary>
/// Checks if the queue group name is valid.
/// </summary>
/// <param name="queueGroup"></param>
/// <returns>true is the queue group name is valid, false otherwise.</returns>
static public bool IsValidQueueGroupName(string queueGroup)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
static public bool IsValidQueueGroupName(string queueGroup)
public static bool IsValidQueueGroupName(string queueGroup)

Copy link
Member Author

Choose a reason for hiding this comment

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

+1

{
return IsValidTokenOrQName(queueGroup);
}

#endregion

} // Subscription

}
66 changes: 57 additions & 9 deletions NATSUnitTests/UnitTestSub.cs
Original file line number Diff line number Diff line change
Expand Up @@ -182,8 +182,8 @@ public void TestSlowSubscriber()
}
catch (Exception)
{
// ignore
}
// ignore
}

s.NextMessage();
});
Expand Down Expand Up @@ -282,12 +282,12 @@ public void TestAsyncErrHandler()
Assert.True(args.Subscription == s);
Assert.Contains("Slow", args.Error);

// release the subscriber
Monitor.Pulse(subLock);
// release the subscriber
Monitor.Pulse(subLock);
}

// release the test
lock (testLock) { Monitor.Pulse(testLock); }
// release the test
lock (testLock) { Monitor.Pulse(testLock); }
};

bool blockedOnSubscriber = false;
Expand Down Expand Up @@ -332,7 +332,7 @@ public void TestAsyncErrHandler()
[Fact]
public void TestAsyncSubscriberStarvation()
{
AutoResetEvent ev = new AutoResetEvent(false);
AutoResetEvent ev = new AutoResetEvent(false);

using (new NATSServer())
{
Expand Down Expand Up @@ -894,7 +894,7 @@ public void TestSubDelTaskCountReconnect()

var opts = utils.DefaultTestOptions;
opts.SubscriberDeliveryTaskCount = 2;
opts.DisconnectedEventHandler = (obj, args) => { disconnected = true;};
opts.DisconnectedEventHandler = (obj, args) => { disconnected = true; };
opts.ReconnectedEventHandler = (obj, args) => { reconnectEv.Set(); };

using (var server = new NATSServer())
Expand Down Expand Up @@ -992,6 +992,54 @@ public void TestSubDelTaskCountWithSyncSub()
}
}
}
}

static readonly string[] invalidSubjects = { "foo bar", "foo..bar", ".foo", "bar.baz.", "baz\t.foo" };
static readonly string[] invalidQNames = { "foo group", "group\t1", "g1\r\n2" };

[Fact]
public void TestSubscriptionValidationAPI()
{
Assert.True(Subscription.IsValidSubject("foo"));
Assert.True(Subscription.IsValidSubject("foo.bar"));

foreach (string s in invalidSubjects)
{
Assert.False(Subscription.IsValidSubject(s));
}

foreach (string s in invalidQNames)
{
Assert.False(Subscription.IsValidQueueGroupName(s));
}
}

[Fact]
public void TestInvalidSubjects()
{
EventHandler<MsgHandlerEventArgs> mh = (obj, args) => { /* NOOP */ };
using (new NATSServer())
{
var c = utils.DefaultTestConnection;

foreach (string s in invalidSubjects)
{
Assert.Throws<NATSBadSubscriptionException>(() => c.SubscribeSync(s));
Assert.Throws<NATSBadSubscriptionException>(() => c.SubscribeSync(s, "qgroup"));
Assert.Throws<NATSBadSubscriptionException>(() => c.SubscribeAsync(s));
Assert.Throws<NATSBadSubscriptionException>(() => c.SubscribeAsync(s, mh));
Assert.Throws<NATSBadSubscriptionException>(() => c.SubscribeAsync(s, "qgroup"));
Assert.Throws<NATSBadSubscriptionException>(() => c.SubscribeAsync(s, "qgroup", mh));
}

foreach (string s in invalidQNames)
{
Assert.Throws<NATSBadSubscriptionException>(() => c.SubscribeSync("subject", s));

Assert.Throws<NATSBadSubscriptionException>(() => c.SubscribeAsync("subject", s));
Assert.Throws<NATSBadSubscriptionException>(() => c.SubscribeAsync("subject", s, mh));
}
}
}
}
}