Repository navigation
Conversation
duanemay
marked this pull request as draft
September 19, 2025 22:47
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
duanemay
force-pushed
the
fix/test_props
branch
from
September 21, 2025 20:26
3819a7c to
5e1f3a0
Compare
fhanik
force-pushed
the
fix/test_props
branch
from
November 13, 2025 17:39
5e1f3a0 to
0d52aea
Compare
Contributor
|
rebased on develop branch |
…tion tests application-mysql.properties/application-postgresql.properties no longer ship database.username/password/url, since those are packaged into the WAR. bootRun, the embedded integrationTest auto-start, and the CI external-Tomcat integration tests (scripts/tomcat/integration_tests_tomcat.sh) all relied on those defaults to reach the root/changeme database that lib_db_helper.sh boots locally/in CI. Supply the same values via -D system properties instead, so those flows keep working without reintroducing the credentials into the shipped artifact.
Gradle's gradle.taskGraph.whenReady block only forwarded spring.profiles.active, testId, and zones.paths.enabled into forked Test-task JVMs; arbitrary -D flags passed on the command line (e.g. -Ddatabase.username) are never propagated to a forked test JVM automatically. Once application-mysql.properties/application- postgresql.properties stopped shipping database.username/password/url, any test that builds a DatabaseConfiguration context (e.g. ClientAdminBootstrapProdEncoderTest) under the mysql/postgresql profile got a null database URL and failed to create a DataSource. Forward the same root/changeme local-dev credentials used for bootRun/integrationTest/CI Tomcat into every Test task's active profile. Also apply the same fix to bootWarRun for consistency. Verified locally against a real Postgres: ClientAdminBootstrapProdEncoderTest now passes under -Dspring.profiles.active=postgresql, reproducing and fixing the CI db-integration-test/tomcat-integration-test failures on PR #3628.
The previous commit forwarded database.username/password/url into every Test task, but the plain `test` task includes tests (DatabasePropertiesTest, YamlServletProfileInitializerTest) that deliberately exercise their own per-profile/custom database property fixtures (e.g. hsqldb's "sa" default, a mysql URL pattern, a custom "donkey" YAML value). Since JVM system properties take precedence over everything else in Spring's Environment, the blanket override clobbered those fixtures and broke db-unit-test in CI. Restrict the override to the integrationTest task, which is the one that actually needs it (it connects to a real external mysql/postgresql with no per-test property source of its own). Verified locally: DatabasePropertiesTest and YamlServletProfileInitializerTest pass again under -Dspring.profiles.active=postgresql, the full :cloudfoundry-identity-server:test suite passes (4819/4820, the one failure is an unrelated collation/locale artifact of the local Postgres install), and ClientAdminBootstrapProdEncoderTest still passes under integrationTest.
…ncher The mysql JDBC URL contains an unescaped & (...useSSL=true&trustServerCertificate=true). The doFirst block builds a raw command string passed straight to `bash -c`, which parses it fresh (unlike the CATALINA_OPTS case in integration_tests_tomcat.sh, where the same value only ever appears via variable expansion, so control-operator lexing doesn't apply). An unquoted & there is lexed as a background-job operator, splitting the command mid-argument and silently truncating the java -jar invocation, which is why db-integration-test (mysql-8.0/8.4/9) kept failing to start UAA within 300s while the postgresql matrix (no & in its URL) passed. Single-quote each -D value.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved critical and moderate findings affect database startup, shell handling, and packaged profile behavior.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 3
Open (4)
What changed in this PR
Moves local MySQL/PostgreSQL test database defaults out of packaged resources while supplying them explicitly for local and CI launchers.
Changes:
- Adds database fixtures to test resources.
- Adds Gradle and Tomcat runtime database arguments.
- Leaves packaged profile files with driver configuration only.
| File | Change / final findings |
|---|---|
uaa/src/test/resources/application-postgresql.properties |
Adds PostgreSQL test defaults. |
uaa/src/test/resources/application-mysql.properties |
Adds MySQL test defaults. |
uaa/build.gradle.kts |
Configures boot and embedded integration startup. Critical (3 votes, line 294): apostrophes in overrides are not shell-escaped. Critical (1 vote, line 11): YAML-selected profiles may bypass injected properties. |
server/src/test/resources/application-postgresql.properties |
Adds PostgreSQL test defaults. |
server/src/test/resources/application-mysql.properties |
Adds MySQL test defaults. |
server/src/main/resources/application-postgresql.properties |
Removes packaged defaults. Moderate (1 vote): platform-specific settings change to defaults. Nit (2 votes): documentation must describe the required database URL. |
server/src/main/resources/application-mysql.properties |
Removes packaged defaults. Moderate (1 vote): database.caseinsensitive changes to its Java default. Nit (1 vote): documentation must describe the required database URL. |
scripts/tomcat/integration_tests_tomcat.sh |
Supplies database options to Tomcat. Critical (1 vote, lines 45 and 182): raw & may be interpreted as a shell separator. |
build.gradle.kts |
Configures integration-test and WAR-run database properties. Critical (1 vote, line 11): YAML-selected profiles may bypass injected properties. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| function local_database_catalina_opts() { | ||
| local profiles="$1" | ||
| if [[ ",${profiles}," == *,mysql,* ]]; then | ||
| echo "-Ddatabase.username=root -Ddatabase.password=changeme -Ddatabase.url=jdbc:mysql://127.0.0.1:3306/uaa?useSSL=true&trustServerCertificate=true" |
Comment on lines
+11
to
+12
| fun localDatabaseCredentialArgs(activeProfiles: String): Map<String, String> { | ||
| val profiles = activeProfiles.split(",").map { it.trim() } |
| // Single-quoted: this string is parsed fresh by `bash -c` below (not expanded from an | ||
| // already-set shell variable), so an unescaped `&` in the mysql JDBC URL would otherwise | ||
| // be lexed as a background-job operator and silently truncate the java invocation. | ||
| val databaseArgsList = localDatabaseCredentialArgs(springProfile).map { (key, value) -> "-D$key='$value'" } |
| database.caseinsensitive=false | ||
| database.default-url=jdbc:postgresql:uaa No newline at end of file | ||
| # This file is required by : org.cloudfoundry.identity.uaa.db.beans.DatabaseConfiguration | ||
| database.driverClassName=org.postgresql.Driver No newline at end of file |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
application-mysql.properties/application-postgresql.propertiesinserver/src/main/resourcesshipped hardcodeddatabase.username=root/database.password=changeme/a127.0.0.1/local JDBC URL — these look like local test/dev fixtures, not production defaults, and end up packaged into the WAR. This moves those values intosrc/test/resources(kept for the test suite, which needs them), leaving onlydatabase.driverClassNamein the packagedapplication-mysql.properties/application-postgresql.properties.This is not a fix for an exploitable default: UAA falls back to the
hsqldbprofile with zero configuration (YamlServletProfileInitializer.applySpringProfiles), so these values are only ever read if an operator explicitly activates themysql/postgresqlprofile. It's hygiene — don't ship plausible-looking credentials in the production artifact.Follow-up fix included
Removing these from
src/main/resourcesbroke three local/CI flows that relied on them being present in the packaged artifact when themysql/postgresqlprofile is active, with no other credentials supplied:./gradlew run/bootRunwith-Dspring.profiles.active=postgresqlormysql(documented inREADME.md)integrationTestauto-start (java -jarlaunch inuaa/build.gradle.kts)scripts/tomcat/integration_tests_tomcat.sh,.github/workflows/integration-tests.yml, mysql/postgresql matrix)All three now supply the same
root/changemelocal-dev credentials via explicit-Dsystem properties (still overridable) instead of relying on the packaged properties files. Verified by runningbootRun -Dspring.profiles.active=postgresqlagainst a real local Postgres — UAA started and Flyway created all tables as expected.Test plan
./gradlew :cloudfoundry-identity-uaa:bootRun -Dspring.profiles.active=postgresql -Ddatabase.url=jdbc:postgresql://localhost:5433/uaaagainst a local Postgres with aroot/changemesuperuser — UAA started, Flyway created all 16 tablesshellcheck scripts/tomcat/integration_tests_tomcat.sh— no new findings