CXH-2458 Support brace matching on config string - #153
JavierCarnelli-ConductorOne wants to merge 2 commits into
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}
| if atValueStart { | ||
| if end, ok := pairs[i]; ok { | ||
| if bareFieldPattern.MatchString(dsn[i+1 : end]) { | ||
| return nil, fmt.Errorf("%w: %q", ErrAmbiguousDSN, dsn[i:end+1]) |
There was a problem hiding this comment.
🔴 Security: this error embeds the brace-quoted value verbatim, and the value that triggers this heuristic is almost always the credential field — PWD={oops;DATABASE=TESTDB} produces ambiguous DB2 DSN...: "{oops;DATABASE=TESTDB}". The error propagates out of ParseNativeDSN → nativeDB2DSN/convertToDB2DSN → database.Connect, where the SDK logs it, so a plaintext password (including one expanded from ${DB_PASSWORD} into a braced value) ends up in connector logs. Report the offending keyword name and/or byte offset instead of the value, e.g. fmt.Errorf("%w (keyword %q)", ErrAmbiguousDSN, keyword).
|
|
||
| // bareFieldPattern matches ";KEYWORD=" inside a brace-quoted span: a sign that the span has | ||
| // swallowed a separate KEYWORD=value field rather than deliberately quoting one value. | ||
| var bareFieldPattern = regexp.MustCompile(`;\s*[A-Za-z][A-Za-z0-9 _]*=`) |
There was a problem hiding this comment.
🟡 Suggestion: bareFieldPattern cannot distinguish "unterminated brace swallowed a later field" from "a value legitimately contains ;word=", so it rejects DSNs that are valid ODBC and worked before this PR. HOSTNAME=h;DATABASE=db;UID=u;PWD={pa;ss=word} (password pa;ss=word, brace-quoted exactly as quoteDB2Value would) now hard-fails with ErrAmbiguousDSN and the connector cannot connect at all. The keyword class also allows spaces ([A-Za-z0-9 _]*), widening it further. Consider only firing when there is an earlier unmatched { in the DSN (the actual swallow signal), and document the rejection plus the db2:// workaround in docs/db2.md.
| func matchBraces(s string) map[int]int { | ||
| pairs := make(map[int]int) | ||
| var stack []int | ||
| for i := 0; i < len(s); i++ { | ||
| switch s[i] { | ||
| case '{': | ||
| stack = append(stack, i) | ||
| case '}': | ||
| if n := len(stack); n > 0 { | ||
| open := stack[n-1] | ||
| stack = stack[:n-1] | ||
| pairs[open] = i | ||
| } | ||
| } | ||
| } | ||
| return pairs | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: LIFO matching treats { inside a brace-quoted value as a nested opener, but ODBC has no nesting — a { inside braces is literal and the first } closes the value (what the old scan did). So HOSTNAME=h;DATABASE={a{b;c};UID=u now pairs the inner { with the }, leaves DATABASE's { unmatched, splits on the inner ;, and DSNDatabase returns {a{b instead of a{b;c. Note quoteDB2Value deliberately brace-quotes values containing {, so this shape is one the code itself considers legal. Skipping openers that occur while already inside a matched span (or pairing each value-start { with the next }) would preserve the old semantics.
| func IsNativeDSN(dsn string) bool { | ||
| _, native := ParseNativeDSN(dsn) | ||
| return native | ||
| _, native, err := ParseNativeDSN(dsn) | ||
| return err == nil && native | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: the doc comment says callers "fail loudly downstream", but the one production caller — resolveConnectScheme in pkg/bsql/offline_validate.go:112 — falls through to url.Parse on false and reports connect: scheme missing from dsn for an ambiguous native DSN. The failure is loud but points the user at the wrong problem; consider having that caller use ParseNativeDSN directly so the ambiguity error surfaces during offline validation.
Connector PR Review: CXH-2458 Support brace matching on config stringBlocking Issues: 1 | Suggestions: 5 | Threads Resolved: 0 Review SummaryFull PR diff scanned for security and correctness: the DB2 native-DSN brace matcher ( Security Issues
Correctness IssuesNone found. Suggestions
Prompt for AI agents |
1 participant
Description
Useful links: