[FIXED] Validation in jetstream and KV #1613
Conversation
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
kv.go
Outdated
| validKeyRe = regexp.MustCompile(`\A[-/_=\.a-zA-Z0-9]+\z`) | ||
| validBucketRe = regexp.MustCompile(`\A[a-zA-Z0-9_-]+\z`) | ||
| validKeyRe = regexp.MustCompile(`\A[-/_=\.a-zA-Z0-9]+\z`) | ||
| validSearchKeyRe = regexp.MustCompile(`^[^ >]*[>]?$`) |
There was a problem hiding this comment.
this would allow matching keys that arent valid keys like x!y
There was a problem hiding this comment.
You're right, I'll just base it on validKeyRe + allow wildcards
There was a problem hiding this comment.
Changed to ^[-/_=\.a-zA-Z0-9*]*[>]?$
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
| if dur == "" { | ||
| return fmt.Errorf("%w: '%s'", ErrInvalidConsumerName, "name is required") | ||
| } | ||
| if strings.ContainsAny(dur, ">*. /\\") { |
There was a problem hiding this comment.
This now is aligned with CLI and .js clients, but I'm afraid implementing this change could break some users.
Any thoughts @ripienaar ?
There was a problem hiding this comment.
Yeah, perhaps the only one the server wouldnt catch really is the space right?
There was a problem hiding this comment.
actually no - space would break the protocol there so that wouldnt have worked, I think its fine but we do need to call it out carefully
There was a problem hiding this comment.
we definately need to add space and wildcards, but / and \ are not that obvious IMO.
There was a problem hiding this comment.
/ and \ are already rejected by the server with API error: code=400 err_code=10128 description=Stream name can not contain path separators (same for consumer names), so the only change here would be client-side validation instead of server-side. But I don't think we absolutely need it since this at least does not break protocol when put in subject.
There was a problem hiding this comment.
qq, when creating a consumer do we check for whether trying to create an filter subject like foo.>. or foo.>.*?
There was a problem hiding this comment.
We don't. I assume safe validations (100% non-breaking) would be checking for > and . placements right? Plus spaces in subject. Anything else?
There was a problem hiding this comment.
yes ending with . need to be client side since it becomes a malformed subject so the server cannot match the JS API, foo.>.* looks like the server does handle
There was a problem hiding this comment.
I added validating filter subject
| @@ -344,8 +344,9 @@ const ( | |||
|
|
|||
| // Regex for valid keys and buckets. | |||
| var ( | |||
| validBucketRe = regexp.MustCompile(`\A[a-zA-Z0-9_-]+\z`) | |||
| validKeyRe = regexp.MustCompile(`\A[-/_=\.a-zA-Z0-9]+\z`) | |||
| validBucketRe = regexp.MustCompile(`^[a-zA-Z0-9_-]+$`) | |||
There was a problem hiding this comment.
I would test thse regex expressions (or better - a functin that valide those items).
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
piotrpio
force-pushed
the
fix-validation
branch
from
ccf606d to
ff5437f
Compare
|
@ripienaar you're fine with the current state of this PR after recent changes? |
|
@Jarema i think so yes |
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
Signed-off-by: Piotr Piotrowski piotr@synadia.com