Validate stream name parameters in JSContext methods. #379

niklasfp · 2024-02-07T20:19:52Z

This pr adds validation of stream names in the JS context.

The validation checks for: ' ', and '.' if any of these characters are present in the stream name, an ArgumentException will be thrown. This at least gets us past the No API response received from the server error, but it still leaves us with the fact that we'll leave it up to the server to validate the rest of the invalid characters like specified in the docs

So in the situation where we have a stream name NOT containing ' ', or '.' but having one of the other illegal ones, e.g. '>', it will throw an exception like this:

NATS.Client.JetStream.NatsJSApiException: stream name is required and can not contain '.', '*', '>'`

In essence that means that some invalid stream names will throw ArgumentException(s), others (server validated) will throw NatsJSApiException.

Oh and I've added .DS_Storeto .gitignore, it's a folder attribute file that MacOS adds all over the place 😄

Fixes #357

niklasfp · 2024-02-07T20:21:25Z

@mtmk PTAL.

I would love som feedback on the ArgumentException(s) vs NatsJSApiException part 😄

mtmk · 2024-02-08T19:09:29Z

src/NATS.Client.JetStream/NatsJSContext.cs

+    internal static void ThrowIfInvalidStreamName([NotNull] string? name, [CallerArgumentExpression("name")] string? paramName = null)
+    {
+#if NET8_0_OR_GREATER
+        ArgumentNullException.ThrowIfNullOrEmpty(name);


looks like net6 and net8 are going to return different messages when the empty.

@mtmk for the empty exception, I took the string used in the framework, but you're right, running net with a localization pack, in that case the message would be localized in.net 8 but not in.net 6 because we have the string here. I'll see if I can come up with a better approach for that.

if the name is empty wouldn't they throw totally different exception messages? net8 isn't going to have the custom message set in net6, no?

The message used for .net 6 Is copied from .net 8 framework messages, this is what .net 8 does But I should probably change it to use ArrgumentException.ThrowIfNullOrEmpty for .net 8, it's the same static though 😄

As a side note, I just tried using characters like \x7 \x8 \x9 \r\n in different combinations when creating streams, and it gives all kinds of different behaviour , some ends with no response from server, others with the server creating the stream but silently removing the character, and in one case replacing it with an 'x' 😄 so we should probably consider making all the characters below '\1F' "illegal" as well.

I've aligned .net 6 and 8.

The message used for .net 6 Is copied from .net 8 framework messages, this is what .net 8 does But I should probably change it to use ArrgumentException.ThrowIfNullOrEmpty for .net 8, it's the same static though 😄

ah nice! didn't realise that

As a side note, I just tried using characters like \x7 \x8 \x9 \r\n in different combinations when creating streams, and it gives all kinds of different behaviour , some ends with no response from server, others with the server creating the stream but silently removing the character, and in one case replacing it with an 'x' 😄 so we should probably consider making all the characters below '\1F' "illegal" as well.

What about 'space'? Is the server replacing that with 'x'? Maybe we should stick to minimal checks as someone might be relying on the current behaviour somehow 😅

Edit: I just saw you comment below about server not responding. Which characters did you find to do that? I wonder if that'd be a server bug?

One example, create a stream ending with crlr: ABCDEF\r\n then you'll get a NATS.Client.JetStream.NatsJSApiNoResponseException: No API response received from the server

I'm not sure, but my gut feeling is that it's not a server bug.

btw, as mentioned in the issue, docs says:

Names cannot contain whitespace, ., *, >, path separators (forward or backwards slash), and non-printable characters.

https://docs.nats.io/nats-concepts/jetstream/streams#configuration

Yeah, should we check for them or just go for what we have now ?
I'm fine either way.

I'm leaning towards less validation (i.e. leave as it is) even though server not responding in some cases is not dev friendly, they should be very rare. cc @Jarema

mtmk · 2024-02-08T19:19:48Z

@mtmk PTAL.

I would love som feedback on the ArgumentException(s) vs NatsJSApiException part 😄

Yea it looks good I think (only had one minor comment). Do you think we shouldn't use ArgumentExceptions?

niklasfp · 2024-02-08T19:55:02Z

@mtmk PTAL.
I would love som feedback on the ArgumentException(s) vs NatsJSApiException part 😄

Yea it looks good I think (only had one minor comment). Do you think we shouldn't use ArgumentExceptions?

I meant the difference in exceptions that we will get, ' ' and '.' will throw argument exceptions because we validate them, any other invalid characters will be handled by the server, and will be a nats js api exception. Looking at the different client implememtations there's is no common way, java does a lot client side, go does not 😄

mtmk · 2024-02-08T20:31:06Z

@mtmk PTAL.
I would love som feedback on the ArgumentException(s) vs NatsJSApiException part 😄

Yea it looks good I think (only had one minor comment). Do you think we shouldn't use ArgumentExceptions?

I meant the difference in exceptions that we will get, ' ' and '.' will throw argument exceptions because we validate them, any other invalid characters will be handled by the server, and will be a nats js api exception. Looking at the different client implememtations there's is no common way, java does a lot client side, go does not 😄

We should probably go with Go 😃
https://github.com/nats-io/nats.go/blob/94eaa2ac8a7a3deb9dd8841e736a2c368b3f97a0/jetstream/jetstream.go#L772-L780

Server seems to check a few more
https://github.com/nats-io/nats-server/blob/e841c030db03fe31b695b4ec992abd1f242d83c4/server/jetstream.go#L2812-L2817

So, I guess if we stick to the same way of thinking (i.e. client only checks for most common errors) what you're checking (space and '.') covers the most common mistakes, I'd say.

niklasfp · 2024-02-08T20:48:59Z

@mtmk PTAL.
I would love som feedback on the ArgumentException(s) vs NatsJSApiException part 😄

Yea it looks good I think (only had one minor comment). Do you think we shouldn't use ArgumentExceptions?

I meant the difference in exceptions that we will get, ' ' and '.' will throw argument exceptions because we validate them, any other invalid characters will be handled by the server, and will be a nats js api exception. Looking at the different client implememtations there's is no common way, java does a lot client side, go does not 😄

We should probably go with Go 😃 https://github.com/nats-io/nats.go/blob/94eaa2ac8a7a3deb9dd8841e736a2c368b3f97a0/jetstream/jetstream.go#L772-L780

Server seems to check a few more https://github.com/nats-io/nats-server/blob/e841c030db03fe31b695b4ec992abd1f242d83c4/server/jetstream.go#L2812-L2817

So, I guess if we stick to the same way of thinking (i.e. client only checks for most common errors) what you're checking (space and '.') covers the most common mistakes, I'd say.

Yeah we could do that, but the client will end up with "no response from server" in some cases if we don't check, see the side note here: #379 (comment)

caleblloyd

LGTM

mtmk

LGTM thanks @niklasfp 💯 🥇

niklasfp added 8 commits February 7, 2024 19:30

Add MacOS .DS_Store

cdebd80

Add stream name parameter validation in NatsJSContext.Streams

3ad41a5

Fix test name

fb4b811

Add stream name parameter validation for consumer methods

1cfa7d9

Move validation methods

6f3dc30

Fix warning

49203e6

Add wilcard tests

ff39161

Remove checking for > and *

39dd523

Remove tests of * and > that were left behind

924bd3f

niklasfp marked this pull request as ready for review February 7, 2024 22:19

mtmk reviewed Feb 8, 2024

View reviewed changes

niklasfp added 2 commits February 8, 2024 22:00

Merge branch 'main' into 357-check-invalid-stream-names

4a93a80

Align .net6 and .net8 exception messages

916072a

niklasfp force-pushed the 357-check-invalid-stream-names branch from cb75ff1 to 916072a Compare February 8, 2024 21:02

mtmk added this to the 2.1.1 milestone Feb 15, 2024

caleblloyd approved these changes Feb 20, 2024

View reviewed changes

mtmk approved these changes Feb 20, 2024

View reviewed changes

mtmk merged commit f693efe into nats-io:main Feb 20, 2024
10 checks passed

niklasfp deleted the 357-check-invalid-stream-names branch February 20, 2024 14:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate stream name parameters in JSContext methods. #379

Validate stream name parameters in JSContext methods. #379

niklasfp commented Feb 7, 2024

niklasfp commented Feb 7, 2024

mtmk Feb 8, 2024

niklasfp Feb 8, 2024 •

edited

Loading

mtmk Feb 8, 2024

niklasfp Feb 8, 2024

niklasfp Feb 8, 2024

mtmk Feb 8, 2024 •

edited

Loading

niklasfp Feb 8, 2024

mtmk Feb 8, 2024

niklasfp Feb 8, 2024

mtmk Feb 9, 2024

mtmk commented Feb 8, 2024

niklasfp commented Feb 8, 2024

mtmk commented Feb 8, 2024

niklasfp commented Feb 8, 2024 •

edited

Loading

caleblloyd left a comment

mtmk left a comment

Validate stream name parameters in JSContext methods. #379

Validate stream name parameters in JSContext methods. #379

Conversation

niklasfp commented Feb 7, 2024

niklasfp commented Feb 7, 2024

Choose a reason for hiding this comment

niklasfp Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtmk Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtmk commented Feb 8, 2024

niklasfp commented Feb 8, 2024

mtmk commented Feb 8, 2024

niklasfp commented Feb 8, 2024 • edited Loading

caleblloyd left a comment

Choose a reason for hiding this comment

mtmk left a comment

Choose a reason for hiding this comment

niklasfp Feb 8, 2024 •

edited

Loading

mtmk Feb 8, 2024 •

edited

Loading

niklasfp commented Feb 8, 2024 •

edited

Loading