CXH-2458 Support brace matching on config string - #153
Conversation
…losing brace
splitDB2DSN decided whether a '{' opened a quoted ODBC value by checking
whether any '}' existed anywhere later in the string, with no concept of
brace pairing. An earlier, unterminated '{' could steal the closing '}'
of a later, legitimately-braced value, swallowing the key in between and
causing DSNDatabase to silently return an empty database name.
Replace the lookahead with LIFO stack-based brace matching (matchBraces)
so each '{' pairs with the '}' that actually closes it; an unmatched
brace stays literal instead of consuming unrelated content.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
matchBraces fixed the case of an unterminated '{' stealing a later
value's own closing '}', but a matched brace pair can still be wrong
when the "closing" '}' isn't part of any real quoted value: an earlier
unterminated '{' can pair with a stray '}' at the end of a later, plain
KEYWORD=value field (e.g. "PWD={oops;DATABASE=TESTDB}"), swallowing that
field and causing DSNDatabase to silently return "".
Detect this by checking whether a matched brace span's interior looks
like it contains a later "KEYWORD=" field. When it does, reject the DSN
with ErrAmbiguousDSN instead of guessing: DSNs carry credentials, so
fail loud rather than silently drop a field. ParseNativeDSN now returns
an error; callers in database.go propagate it instead of discarding it.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Connector PR Review: CXH-2458 Support brace matching on config stringBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryThe new commit gates Security IssuesNone found. The ambiguity error now reports only keyword names ( Correctness IssuesNone found. Suggestions
Prompt for AI agents |
- Never interpolate DSN values into ErrAmbiguousDSN; report only the
owning and swallowed keyword names, since values may be credentials.
- Only treat a brace-quoted span as ambiguous when the field name it
appears to swallow is a reserved DSN keyword, so a password merely
containing ";word=" text is no longer rejected.
- Make matchBraces value-start aware so a literal '{' inside an
already-open value isn't mistaken for a new opener; ODBC values
don't nest, and this was truncating values like "a{b;c" to "{a{b".
- Add regression tests for all three, and restore coverage for the
non-erroring brace-aware split path lost in an earlier fix.
- Trim comments across dsn.go and dsn_test.go to one or two sentences.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Requires a new review
matchBraces set atValueStart on every '=', including one appearing
inside an already-open braced value. A non-keyword field there (e.g.
"x=" in "DATABASE={db;x={y}") could push a second candidate opener
that then stole the real closing brace, truncating the value.
Only push a '{' when the identifier preceding its '=' is a reserved
DSN keyword, so an '=' inside an open value can no longer be mistaken
for a new field's opener. The original ticket's fix (two reserved
keywords racing for one close) is unaffected.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
mateoHernandez123
left a comment
mateoHernandez123
left a comment
There was a problem hiding this comment.
Approving — the original CXH-2458 swallow (an earlier unterminated { stealing a later value's }), the credential leak in ErrAmbiguousDSN, and the non-reserved nested { case are all covered, and CI is green.
One leftover, not blocking this merge: a nested ={ whose word is a reserved keyword still pushes a second opener and silently truncates. I left a suggestion on the table test that pins the non-reserved sibling. Happy to take that as a follow-up.
| {name: "literal brace inside a braced value does not truncate it", dsn: "HOSTNAME=h;DATABASE={a{b;c};UID=u", want: "a{b;c"}, | ||
| // A non-reserved keyword's '=' inside an already-open value must not open a new | ||
| // candidate brace, or it steals the real closing brace and truncates the value. | ||
| {name: "non-reserved keyword inside a braced value does not steal its closing brace", dsn: "HOSTNAME=h;DATABASE={db;x={y};UID=u", want: "db;x={y"}, |
There was a problem hiding this comment.
Suggestion: this pins the non-reserved half (x={y}) of the nested-= fix, which is the case Javier addressed. The reserved sibling is still open on HEAD 632856d: HOSTNAME=h;DATABASE={db;UID={u};PORT=1 returns database="{db" with err=nil instead of db;UID={u} (ODBC first-} / pre-PR) or ErrAmbiguousDSN.
matchBraces still pushes when the word before ={ is in reservedDSNKeywords, even with a span already on the stack, so UID's { steals DATABASE's } and the inner ; splits. Because the outer { never pairs, swallowedReservedField never runs either — silent mis-parse, including via nativeDB2DSN.
Worth a sibling row here, e.g. DATABASE={db;UID={u};UID=u (expect db;UID={u, or ErrAmbiguousDSN if rejection is the call). The matching parser change is skip the push while len(stack) > 0, which is also what the comment on matchBraces already claims ("ODBC values don't nest"). Not blocking the ticket; the original steal and the x={y} half are fixed.
There was a problem hiding this comment.
Great! We can handle this as a follow up item.
Thank you so much Tute!
3 participants
Description
Useful links: