Repository navigation
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+64
to
+71
| cmd.on("close", (code, signal) => { | ||
| logger( | ||
| code === 0 ? "warn" : "error", | ||
| `[${s.name}] exited with ${signal ? `signal ${signal}` : `code ${code}`}.${ | ||
| stderrTail ? ` Stderr: ${stderrTail.trim()}` : "" | ||
| }`, | ||
| "adjacent-servers" | ||
| ); |
Contributor
There was a problem hiding this comment.
🟥 Service database passwords exposed in exit logs
When a child reports POSTGRES_PASS=value or PGPASSWORD=value from a service .env, redactStderr leaves the password intact. process.env lacks service-only passwords, and the label matcher excludes these keys. The exit log persists the database credential.
Was this helpful? React with 👍 or 👎 to provide feedback.
…and diagnostics
Comment on lines
+71
to
+77
| // A partial first line may have lost the prefix identifying a secret. | ||
| let diagnostic = stderrTail; | ||
| if (stderrTruncated) { | ||
| const newline = stderrTail.indexOf("\n"); | ||
| diagnostic = newline === -1 ? "" : stderrTail.slice(newline + 1); | ||
| } | ||
| diagnostic = redactStderr(diagnostic.trim()).slice(-4096); |
Contributor
There was a problem hiding this comment.
🟥 Truncated stderr can expose secret fragments
When a credential spans lines, stderrTruncated discards only the first partial line. A remaining unlabeled fragment can survive redactStderr and enter the exit log.
Was this helpful? React with 👍 or 👎 to provide feedback.
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
1 participant
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.
Purpose
Closes #1138. MMGIS currently starts embedded adjacent processes even when its proxy targets separate Compose services. The proxy and local COG paths also change with Docker detection, while a missing service
.envterminates the embedded launcher without useful diagnostics.This change makes startup, routing, internal STAC requests and local COG paths follow
ADJACENT_SERVERS_MODE=embedded|externalindependently of container runtime detection.Proposed Changes
embedded(default): start enabled Python services and proxy to localhost.external: skip local processes and use the existing service names and ports. Both Compose samples explicitly select external mode..envfiles optional on Unix and Windows; retain inherited configuration and service-specific overrides. STAC/TiPG still requirePOSTGRES_*, and TiTiler-pgSTAC requiresPG*. These are deliberately not inferred from MMGIS'sDB_*, because the services normally use the separatemmgis-stacdatabase.PORT/HTTPSsettings are not imported into the dev server.../../Missions/prefix in the sample URL policy while rejecting extra traversal and incorrect prefixes.Upgrade note: Existing deployments with separate adjacent containers must set
ADJACENT_SERVERS_MODE=external. External services must resolve the existing service hostnames and mount mission files at/Missions. The new default is a deliberate behavior change; it is documented rather than silently retaining a runtime-dependent fallback.Validation
Automated regression coverage
On Node.js 22.20.0, matching
.nvmrc:1,471 passed, including 28 adjacent-service tests. New HTTP tests use real Express and http-proxy-middleware over ephemeral loopback ports. They verify routing under
ROOT_PATH, user/admin gates, expression validation, and acceptance/rejection by the sample COG policy. Regression failures were observed before the fixes, including the 403 for embedded Mission paths and credential fragments at the stderr buffer boundary.Actual Docker + TiTiler comparison
Tested original startup/proxy/launcher source from
9a04abc5and patched source in a minimal Express harness using the real Python service, rather than a mocked upstream. Docker 28.3.3, Linux arm64, Node 22.20.0, Python 3.12.15, TiTiler 2.2.0, TiTiler-pgSTAC 3.1.0, FastAPI 0.140.2, python-dotenv 1.2.2. Local raster: a 32×32 tiled GeoTIFF; patched requests used the actualsample.envURL policy..env/titiler/healthz/titiler/cog/infotitilercontainerAn additional real failure case removes Python from the embedded process PATH: MMGIS records exit code 127 and the
not foundstderr diagnostic. Temporary test containers and network are cleaned up.Configure and other checks
external. It retains default port 3000 even with backendPORT=8888in the root.env. Compilation completes with existing source warnings.pluginCliE2e.spec.jstestdisable non-required plugin succeeds(1 failed, 1,414 passed, 48 not run). The complete single-worker run above passes.Remaining validation
The Docker comparison tests the actual adjacent-service modules in a minimal application harness, not the full production MMGIS image or database-backed application E2E. Podman, Windows batch execution and actual STAC/TiPG/TiTiler-pgSTAC database startup have not been exercised. Full
npm testrequiresDB_USER_TESTandDB_PASS_TESTlocally.Upstream workflow runs currently report
action_requiredwith no jobs, so this PR does not claim upstream CI is green. Maintainer action is needed to get those checks running. The separate response URL rewriting issue #753 is outside this change.