Fix password leak from JDBC URL#11568
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5be0f0b137
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| final int colonLoc = userInfo.indexOf(':'); | ||
| builder.user(colonLoc >= 0 ? userInfo.substring(0, colonLoc) : userInfo); |
There was a problem hiding this comment.
Preserve escaped colons in JDBC usernames
When the JDBC URL contains an escaped colon in the username (for example jdbc:postgresql://tenant%3Aalice@pg.host/db with no password), URI.getUserInfo() has already decoded %3A to :, so this split stores only tenant in db.user instead of the actual username tenant:alice. That is a regression from the previous behavior for valid URL-encoded userinfo; use the raw userinfo to detect only an unescaped password separator, then decode the retained username.
Useful? React with 👍 / 👎.
dougqh
left a comment
There was a problem hiding this comment.
Review from Claude (via claude.ai/code)
The fix itself is correct and security-sound — indexOf(':') on URI.getUserInfo() is the right approach since the spec guarantees userinfo is user:password and getUserInfo() already percent-decodes the string, so URL-encoded colons in passwords won't slip through. A few issues are worth addressing before merge (see inline comments).
| "scenario | url | type | host | user ", | ||
| "PostgreSQL userinfo with password | jdbc:postgresql://myuser:secret123@pg.host/mydb | postgresql | pg.host | myuser", | ||
| "PostgreSQL userinfo without password | jdbc:postgresql://myuser@pg.host/mydb | postgresql | pg.host | myuser" | ||
| }) |
There was a problem hiding this comment.
Claude: The scenario column appears in the table header but has no matching method parameter. Depending on how @TableTest maps columns to parameters this will either fail at runtime or silently drop the column. Either add String scenario as the first parameter (many table-test frameworks use it as the display label), or drop the scenario column from the header.
There was a problem hiding this comment.
The Scenario special column is used to display label, so it doesn't map to method param
| import static datadog.trace.bootstrap.instrumentation.jdbc.JDBCConnectionUrlParser.extractDBInfo; | ||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
|
|
||
| import org.tabletest.junit.TableTest; |
There was a problem hiding this comment.
Claude: Project conventions say no multi-line comment blocks (one short line max). This 8-line class Javadoc can be removed entirely — the test name and table rows are already self-documenting.
There was a problem hiding this comment.
Fixed thanks
| assertEquals(host, info.getHost()); | ||
| assertEquals(user, info.getUser()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Claude: The comprehensive parser tests live in dd-java-agent/instrumentation/jdbc/src/test/groovy/JDBCConnectionUrlParserTest.groovy and cover dozens of URL variants across every GENERIC_URL_LIKE scheme. These two new rows fit better there (after migrating that file from Groovy to Java per project conventions) rather than in a standalone class. Additionally, only PostgreSQL is tested here — GENERIC_URL_LIKE also handles SAP, Snowflake, Redshift, IRIS, and Sybase TDS, so at least one non-PostgreSQL user:password@host case (e.g. jdbc:sap://myuser:secret@sap.host:88/sapdb) would confirm the fix covers those schemes too.
There was a problem hiding this comment.
Added SAP test-case, thanks
| builder.user(user); | ||
| final String userInfo = uri.getUserInfo(); | ||
| if (userInfo != null) { | ||
| // getUserInfo() returns "user:password" — strip the password |
There was a problem hiding this comment.
Claude: The comment is justified (non-obvious platform behaviour worth explaining), but it could be shorter per project conventions — e.g. // userInfo is "user:password"; keep only the username.
dougqh
left a comment
There was a problem hiding this comment.
Looks good to me.
Claude had some feedback based on conventions - that I had it add.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
This comment has been minimized.
This comment has been minimized.
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in Use ⏳ Processing |
What Does This Do
Strips the password from JDBC URL credentials before storing the username in the db.user span tag.
jdbc:postgresql://myuser:secret123@pg.host/mydbBefore:
db.user = "myuser:secret123" ← password leaks into the tag
After:
db.user = "myuser"
Motivation
JDBC URLs can embed credentials directly in the URL (user:password@host). Java's URI.getUserInfo() returns the full string including the password.
Without sanitization, this password ends up in the db.user span tag sent to Datadog.
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issue/merge. You can also:/merge --commit-message "..."/merge -c/merge -f --reason "reason"; please use this judiciously, as some checks do not run at the PR-level (note: the PR still needs to be mergeable, this will only skip the pre-merge build)Jira ticket: [PROJ-IDENT]