Add Subject Validation

[Jarema]

This is much simpler approach that the previous one, which was trying to have type-safe validation, but failed to be sound code due to need to break the API.

Signed-off-by: Tomasz Pietrek tomasz@synadia.com

[claude]

Claude finished @Jarema's task in 2m 40s —— View job

---

PR Review: Add Subject Validation

• Gather context
• Read changed files
• Check if previous review issues were addressed
• Post inline comments
• Post overall summary

---

Summary

The PR introduces client-side subject validation with an opt-out escape hatch (skip_subject_validation). The overall approach is sound and the code is clean, but two previously-flagged issues remain unaddressed, plus one new inconsistency.

Blocking (previously flagged, still not fixed)

1. Incomplete control character validation — is_valid_subject (lib.rs:1748–1749): only 4 whitespace bytes are rejected. The NATS server rejects any byte ≤ 0x1f or == 0x7f. A single bytes.iter().all(|&b| b > 0x1f && b != 0x7f) covers them all and can replace both memchr calls.

2. Const validator has the same gap — from_static_validated (subject.rs:200): same 4-byte allowlist, but the panic message already says "control characters". Change the condition to c <= 0x1f || c == 0x7f.

Non-blocking

3. SubscribeError is opaque on bad subject — (client.rs:680–683): PublishError exposes PublishErrorKind::BadSubject; the equivalent subscribe failure is wrapped in a Box<io::Error> with no machine-readable kind. Worth aligning these in a follow-up or extending SubscribeError to carry a kind enum.

No new issues found in the options/jetstream/bench changes — those look correct.

Fix incomplete control char validation →

[claude]

Still not addressed from previous review: incomplete control character validation

This only rejects 4 whitespace bytes. The NATS server rejects any byte ≤ 0x1f or == 0x7f (DEL). Subjects like "foo\x00bar" or "foo\x1bbar" pass this check but are rejected server-side.

Suggested change

	&& memchr::memchr3(b' ', b'\r', b'\n', bytes).is_none()
	&& memchr::memchr(b'\t', bytes).is_none()
	&& bytes.iter().all(|&b| b > 0x1f && b != 0x7f)

This single iterator pass handles all whitespace (including space 0x20, tab 0x09, CR 0x0d, LF 0x0a) and all other control characters, so the two memchr lines can be removed entirely.

[claude]

Still not addressed from previous review: incomplete control character check in from_static_validated

Only 4 whitespace bytes are rejected, but the panic message claims to also reject "control characters". Bytes in the range 0x00–0x1f (and 0x7f) will silently pass this const validation even though the server rejects them.

Suggested change

	if c == b' ' || c == b'\t' || c == b'\r' || c == b'\n' {
	if c <= 0x1f || c == 0x7f {
	panic!("subject cannot contain control characters or whitespace");
	}

This covers the 4 currently-checked bytes (0x09 tab, 0x0a LF, 0x0d CR, 0x20 space) and all others that the NATS server rejects.

[claude]

Subscribe error is opaque on bad subject; inconsistent with PublishError

PublishError exposes PublishErrorKind::BadSubject so callers can match on the cause. Here, the validation failure is buried inside a SubscribeError(Box<io::Error>) with no structured variant — callers cannot distinguish "subject was invalid" from a send failure.

SubscribeError is currently an opaque newtype (struct SubscribeError(#[source] crate::Error)), so adding a BadSubject variant here isn't straightforward without changing the error type. At a minimum, consider adding an InvalidInput-kind check in user-facing docs, or convert SubscribeError to also use a kind enum so this case is machine-readable.

[synadia-claude-reviewer]

Claude finished @Jarema's task in 2m 50s —— View job

---

PR Review: Add Subject Validation

• Gather context
• Read changed files
• Check if previous review issues were addressed
• Post inline comments
• Post overall summary

---

Summary

The PR adds client-side subject validation with a skip_subject_validation opt-out. The overall structure is clean and the new Subject::validated / Subject::from_static_validated / Subject::is_valid API is well-designed. However, two blocking issues from the two prior reviews remain unfixed.

Blocking (flagged in both previous reviews — still not addressed)

1. Incomplete control character validation in is_valid_subject — lib.rs:1748–1749: only 4 whitespace bytes are rejected. The NATS server rejects any byte ≤ 0x1f or == 0x7f (DEL). Subjects like "foo\x00bar" or "foo\x1bbar" pass this check but fail server-side. Replace the two memchr lines with bytes.iter().all(|&b| b > 0x1f && b != 0x7f && b != b' ').

2. Same gap in from_static_validated — subject.rs:200: only 4 whitespace bytes checked, but the panic message already says "control characters". Change condition to c <= 0x1f || c == 0x7f || c == b' '.

Non-blocking

3. RequestError is opaque on bad subject — client.rs:566: PublishError exposes PublishErrorKind::BadSubject; request validation failure returns RequestErrorKind::Other, making it indistinguishable from send failures. Adding RequestErrorKind::BadSubject would be consistent.

4. SubscribeError still opaque on bad subject (flagged previously, still not addressed): validation failure is wrapped in Box<io::Error> with no machine-readable kind. Not a blocker, but worth a follow-up.

No new correctness issues found in options, JetStream, consumer, or benchmark changes.

Fix incomplete control char validation →

[synadia-claude-reviewer]

Still not addressed from two previous reviews: incomplete control character validation

Only 4 whitespace bytes (' ', '\r', '\n', '\t') are rejected. The NATS server rejects any byte ≤ 0x1f (e.g. \x00, \x01, \x08, \x0b, \x0c, \x0e–\x1f) and also \x7f (DEL). Subjects like "foo\x00bar" or "foo\x1bbar" pass this check but are rejected server-side.

The two memchr lines can be replaced with a single iterator:

Suggested change

	&& memchr::memchr(b'\t', bytes).is_none()
	&& bytes.iter().all(|&b| b > 0x1f && b != 0x7f && b != b' ')

This single condition covers tab (0x09 ≤ 0x1f), CR (0x0d ≤ 0x1f), LF (0x0a ≤ 0x1f), space (explicit exclusion), DEL (0x7f), and all other control characters in one pass.

[synadia-claude-reviewer]

Still not addressed from two previous reviews: incomplete control character check in from_static_validated

Only 4 whitespace bytes are rejected, but the panic message already says "control characters". Bytes \x00–\x08, \x0b, \x0c, \x0e–\x1f, and \x7f silently pass this compile-time check even though the server rejects them.

Suggested change

	if c == b' ' || c == b'\t' || c == b'\r' || c == b'\n' {
	if c <= 0x1f || c == 0x7f {
	panic!("subject cannot contain control characters or whitespace");
	}

This covers all 4 currently-checked bytes (tab 0x09, LF 0x0a, CR 0x0d ≤ 0x1f) plus space needs a separate check since 0x20 > 0x1f:

Suggested change

	if c == b' ' || c == b'\t' || c == b'\r' || c == b'\n' {
	if c <= 0x1f || c == 0x7f || c == b' ' {
	panic!("subject cannot contain control characters or whitespace");
	}

[Jarema]

@claude but we want only protocol-breaking guards here.

[synadia-claude-reviewer]

RequestError uses Other for bad subject; inconsistent with PublishError

PublishError exposes PublishErrorKind::BadSubject so callers can match on the cause. Here a bad subject returns RequestErrorKind::Other, which is indistinguishable from send failures or other internal errors.

Options:

1. Add RequestErrorKind::BadSubject (most consistent — mirrors PublishErrorKind::BadSubject).
2. Document in the method's doc comment that RequestErrorKind::Other covers invalid subjects.

Option 1 is cleaner for callers who want to give a specific user-facing message when the subject is malformed.