Skip to content

fix(conf): treat numbers with a suffix followed by more characters as strings - #8263

Open
koriyoshi2041 wants to merge 2 commits into
nats-io:mainfrom
koriyoshi2041:fix/conf-lexer-number-suffix-string
Open

fix(conf): treat numbers with a suffix followed by more characters as strings#8263
koriyoshi2041 wants to merge 2 commits into
nats-io:mainfrom
koriyoshi2041:fix/conf-lexer-number-suffix-string

Conversation

@koriyoshi2041

Copy link
Copy Markdown

The config lexer mis-parses unquoted values that start with a number, then a size/exponent suffix, then more characters.

lexConvenientNumber emitted an integer token as soon as it saw a digit after a suffix (k, m, g, t, p, e and their uppercase forms). So 0K1abc lexed as the integer 0K and 5e7bcd as 5e7, leaving the trailing characters behind. The parser then failed with errors like:

Expected a top-level value to end with a new line, comment or EOF, but got '1' instead.

This most often shows up with unquoted passwords that happen to begin with a number and one of those suffix letters (e.g. password: 4e2abc).

A digit, or any other non-terminator, after the suffix means the token isn't a convenient number, so it should be lexed as a string. The fix drops the unicode.IsDigit(r) clause so an integer is only emitted on an actual value terminator (newline, EOF, map end, value/array terminator, whitespace); anything else falls through to the existing string state. This matches how 1Ghz is already handled.

Added lexer-level cases to TestConvenientIntegerValues and a parser-level TestUnquotedStringStartingWithNumberAndSuffix covering 0K1abc, 8m4dwr, 5e7bcd-abc10 and 4e2abc, both at top level and inside an authorization block.

Fixes #5186
Fixes #5189
Fixes #6891

lexConvenientNumber emitted an itemInteger whenever a digit followed a
size suffix (k, m, g, t, p, e and their uppercase variants). That made
values such as "0K1abc" or "5e7bcd" lex as the partial integer "0K" /
"5e7" with the remaining characters left dangling, so the parser then
errored with messages like:

    Expected a top-level value to end with a new line, comment or EOF,
    but got '1' instead.

A digit (or any other non-terminator) after a suffix means the token
is not a convenient number, so it should be lexed as a string instead.
This commonly bit unquoted passwords that happened to start with a
number and a suffix character.

Only emit itemInteger on a real value terminator (newline, EOF, map
end, value/array terminators or whitespace); otherwise fall through to
the string state.

Fixes nats-io#5186
Fixes nats-io#5189
Fixes nats-io#6891

Signed-off-by: kigland <shuaizhicheng336@gmail.com>
@koriyoshi2041
koriyoshi2041 requested a review from a team as a code owner June 4, 2026 11:16
@neilalexander

Copy link
Copy Markdown
Member

@claude @codex review

@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Jun 5, 2026

Copy link
Copy Markdown

Claude finished @neilalexander's task in 3m 52s —— View job


Code Review

  • Gather context
  • Read changed files and diff
  • Review fix correctness in conf/lex.go
  • Review test coverage in conf/lex_test.go and conf/parse_test.go

Summary

The fix is correct and minimal. The root cause was a single misplaced clause in lexConvenientNumber (conf/lex.go:1106–1122), and the test coverage is solid. A few observations below.


Fix correctness — conf/lex.go

Before (buggy line, -2):

if isNL(r) || r == eof || r == mapEnd || r == optValTerm || r == mapValTerm || isWhitespace(r) || unicode.IsDigit(r) {
    lx.emit(itemInteger)
    return lx.pop()
}

After (fixed, +4):

if isNL(r) || r == eof || r == mapEnd || r == optValTerm || r == mapValTerm || isWhitespace(r) {
    lx.emit(itemInteger)
    return lx.pop()
}
// Anything else following the suffix (including a digit, e.g. "0K1abc"
// or "5e7bcd") means this isn't a convenient number after all, so treat
// the whole token as a string.
lx.stringStateFn = lexString
return lexString

The || unicode.IsDigit(r) clause was treating any digit after a size suffix (k, K, m, M, g, G, e, E, …) as a token terminator, causing:

  • The partial prefix (e.g. 0K, 5e7) to be emitted as an itemInteger
  • The remaining characters to be left dangling in the input, producing a parse error

Removing that clause makes digits fall through to the lexString path, where lx.start still points to the beginning of the whole value — so the entire token (0K1abc, 5e7bcd) is emitted as a string, consistent with how 1Ghz was already handled.

String accumulation is correct: lx.start is set at the value start in lexValue/lexNumberOrDateOrStringOrIPStart. When lexConvenientNumber backs up and falls through to lexString, lx.input[lx.start:lx.pos] accumulates the full token. ✅

Consistency with lexNegNumber: That function also calls lexConvenientNumber for negative suffixed values (e.g. -1m). With the fix, -1k2abc would correctly fall through to string as well. ✅


Test coverage

conf/lex_test.go adds three cases to TestConvenientIntegerValues (lines 344–373):

conf/parse_test.go adds TestUnquotedStringStartingWithNumberAndSuffix (lines 180–194) covering:

  • Top-level: 0K1abc, 8m4dwr, 5e7bcd-abc10, 4e2abc
  • Inside a map/authorization block: password: 4e2abc

Minor gaps (not blockers)

  1. Array context not tested. There's no case for foo = [4e2abc] or foo = [0K1abc, 1k]. These should work correctly — lexString terminates on arrayValTerm (,) and arrayEnd (]) — but an array-context regression test would provide confidence.

  2. Negative suffix+extra chars. -1k2abc is not exercised. It would also become a string via lexNegNumber → lexConvenientNumber → lexString, but a test would confirm it.

  3. Pre-existing gap (out of scope): lexConvenientNumber does not include arrayEnd (]) in its integer-emit terminator list, while lexString does. This means a bare [1k] routes through lexString and is emitted as itemString "1k" rather than itemInteger "1k". This is a pre-existing issue, not introduced by this PR.


Overall assessment

The fix is correct, well-scoped, and addresses real user pain (unquoted passwords like 4e2abc breaking configs). The updated comment in lexConvenientNumber clearly explains the invariant. The test cases cover the exact failure modes from the linked issues.

Verdict: LGTM — the two optional test additions above would make the suite more robust but aren't required to accept this.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. More of your lovely PRs please.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Signed-off-by: kigland <shuaizhicheng336@gmail.com>
@koriyoshi2041

Copy link
Copy Markdown
Author

Added two small regression checks for the remaining contexts: a negative suffixed token (-1k2abc) and an array value list ([4e2abc, 0K1abc]). go test ./conf passes locally.

@koriyoshi2041

Copy link
Copy Markdown
Author

CI triage: the only red check I see is Test Other Packages, failing in test/route_discovery_test.go at TestStressChainedSolicitWorks with Expecting 3 routes, got 2.

That looks unrelated to this conf lexer change; this PR only touches the parser path and go test ./conf passed locally after the last update. I tried to rerun just the failed job, but GitHub rejected it because I do not have repository admin permission.

@github-actions github-actions Bot added the stale This issue has had no activity in a while label Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale This issue has had no activity in a while

Projects

None yet

2 participants