Skip to content

Conversation

@ggreer
Copy link
Contributor

@ggreer ggreer commented Nov 20, 2025

Some people have weird characters in their usernames/passwords, and don't escape them in the environment variables. We have a username & password fields separate from the DSN that do get escaped, so use those in the examples.

Also add some user login names that show we're not vulnerable to SQL injection attacks.

Summary by CodeRabbit

  • Bug Fixes
    • Database connection configuration now handles credentials separately from the connection string, storing user and password in dedicated fields instead of embedding them in the DSN.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Nov 20, 2025

Important

Review skipped

Review was skipped due to path filters

⛔ Files ignored due to path filters (1)
  • .github/workflows/ci.yaml is excluded by none and included by none

CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including **/dist/** will override the default block on the dist directory, by removing the pattern from both the lists.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The changes extract database credentials from the DSN string into separate User and Password fields within the Config.Connect type. Tests were updated to verify credentials are properly stored in these dedicated fields while the DSN is constructed without embedded credentials.

Changes

Cohort / File(s) Summary
Credential field extraction
pkg/bsql/config.go
Added Connect.User and Connect.Password string fields to the Config type for storing database credentials separately from the DSN string
Test expectations update
pkg/bsql/config_test.go
Updated DSN test expectations to exclude embedded credentials; added assertions verifying Connect.User and Connect.Password fields are populated with credentials

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Areas requiring extra attention:

  • Verify that credentials are correctly parsed and extracted from the configuration source (environment variables, config files, etc.) into the new User and Password fields
  • Confirm DSN construction logic properly excludes credentials and handles the new separate fields
  • Check that no credentials are inadvertently logged, exposed in error messages, or included in debug output
  • Ensure backward compatibility if existing code relies on credentials being embedded in the DSN

Poem

🐰 Secrets tucked away so neat,
From the DSN they retreat,
User and Password find their place,
In separate fields, a safer space! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title mentions 'SQL injection test data' and 'username/password in examples', which aligns with the PR's objective of adjusting examples to use separate username and password fields and adding test data to demonstrate SQL injection protection.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
pkg/bsql/config_test.go (1)

34-36: DSN/user/password expectations correctly updated; consider explicit “no credentials in DSN” asserts

The new expectations for c.Connect.DSN, c.Connect.User, and c.Connect.Password correctly reflect separating credentials from the DSN, which matches the PR intent and improves safety around special characters in env vars.

As a small optional hardening/clarity tweak, you could also assert that the DSN never contains the credential placeholders:

 				require.Equal(t, "Wordpress Test", c.AppName)
-				require.Equal(t, "mysql://${DB_HOST}:${DB_PORT}/${DB_DATABASE}?charset=utf8mb4&parseTime=True&loc=Local", c.Connect.DSN)
-				require.Equal(t, "${DB_USER}", c.Connect.User)
-				require.Equal(t, "${DB_PASSWORD}", c.Connect.Password)
+				require.Equal(t, "mysql://${DB_HOST}:${DB_PORT}/${DB_DATABASE}?charset=utf8mb4&parseTime=True&loc=Local", c.Connect.DSN)
+				require.Equal(t, "${DB_USER}", c.Connect.User)
+				require.Equal(t, "${DB_PASSWORD}", c.Connect.Password)
+				require.NotContains(t, c.Connect.DSN, "${DB_USER}")
+				require.NotContains(t, c.Connect.DSN, "${DB_PASSWORD}")

This makes the “no credentials in DSN” contract explicit and guards against regressions. As per coding guidelines.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8307960 and d735d25.

⛔ Files ignored due to path filters (7)
  • examples/mysql-test.yml is excluded by none and included by none
  • examples/oracle-test.yml is excluded by none and included by none
  • examples/postgres-test.yml is excluded by none and included by none
  • examples/sap-hana-test.yml is excluded by none and included by none
  • examples/sqlserver-test.yml is excluded by none and included by none
  • examples/wordpress-test.yml is excluded by none and included by none
  • test/postgres-init.sql is excluded by none and included by none
📒 Files selected for processing (1)
  • pkg/bsql/config_test.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.go: Error handling: use fmt.Errorf with contextual messages; check specific errors with errors.Is
Organize imports: standard library first, then third-party, then project imports; alphabetize within each group
Naming: CamelCase for exported identifiers; camelCase for unexported; preserve acronyms like ID, URL, HTTP, API
Limit line length to a maximum of 200 characters
Comments for exported items must be complete sentences ending with periods
Do not use log.Fatal or log.Panic (ruleguard-enforced)

Files:

  • pkg/bsql/config_test.go
**/*_test.go

📄 CodeRabbit inference engine (CLAUDE.md)

Tests should be table-driven using testify/require; name tests TestStructName_methodName

Files:

  • pkg/bsql/config_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: go-test (ubuntu-latest)

… expose bugs in the account provisioning github workflow.
@pquerna pquerna merged commit 6b042da into main Nov 21, 2025
4 checks passed
@pquerna pquerna deleted the ggreer/sql-injection-test branch November 21, 2025 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants