| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
There was a problem hiding this comment.
This PR adds environment inspection capabilities to the start-proxy action to log information about proxy-related settings on the runner. The goal is to provide better observability for debugging potential proxy configuration conflicts that may affect the authentication proxy's operation.
Changes:
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file| File | Description |
|---|---|
| src/start-proxy/environment.ts | New module implementing environment inspection with functions to check proxy-related environment variables and Java configuration files |
| src/start-proxy/environment.test.ts | Comprehensive unit tests for the environment inspection module |
| src/start-proxy-action.ts | Integration of environment inspection with try-catch wrapper for best-effort execution |
| src/languages.ts | Addition of JavaEnvVars enum for consistent reference to Java environment variable names |
| src/init-action.ts | Refactored to use JavaEnvVars enum instead of string literals for JAVA_TOOL_OPTIONS |
| lib/*.js | Auto-generated JavaScript (not reviewed per coding guidelines) |
Sorry, something went wrong.
| function checkEnvVar(logger: Logger, name: string): boolean { | ||
| const value = process.env[name]; | ||
| if (isDefined(value)) { | ||
| logger.info(`Environment variable '${name}' is set to '${value}'.`); |
There was a problem hiding this comment.
This code logs the complete value of proxy-related environment variables (HTTP_PROXY, HTTPS_PROXY, ALL_PROXY) at INFO level. These environment variables commonly contain embedded credentials in the format http://username:password@proxy:port. Logging these values would expose sensitive credentials in workflow logs.
Consider implementing sanitization to redact credentials from proxy URLs before logging, or logging only the presence/absence of these variables without their values (similar to how the else branch logs at DEBUG level that variables are not set).
Sorry, something went wrong.
There was a problem hiding this comment.
I have added some protection against this in 99fcc7b by trying to parse the value as a URL in checkEnvVar and, if successful, clearing the username and password. Broadly, this should only be a concern for the members of ProxyEnvVars since, for the Java properties, there are separate http(s).proxyUser and http(s).proxyPassword which we don't try to find and therefore don't log.
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM with a very minor suggestion for yet another FF.
Sorry, something went wrong.
Note also that we run this after `getCredentials` which already instructs Actions to mask credentials that we know about in logs
| /** Invokes `java` to get it to show us the active configuration. */ | ||
| async function showJavaSettings(logger: Logger): Promise<void> { | ||
| try { | ||
| const java = await io.which("java", true); |
There was a problem hiding this comment.
I'm probably grasping at straws here. But what if it is not our Java version that is problematic? But some other Java version chosen at a different point in time. I suppose this is somewhat addressed by the fact that we log File contents explicitly in addition to this effective settings logging
Sorry, something went wrong.
There was a problem hiding this comment.
I think this is a valid question and one I have thought about. Other than what you have already said about us inspecting some of the configuration files, my thinking is that there are essentially two scenarios since this is default setup:
Based on that, I don't think this is unreasonable.
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
This PR modifies the start-proxy action to perform a best-effort inspection of some environment variables and language-specific settings which may affect the operation of the authentication proxy. Relevant values are logged so that they are visible in the workflow log.
Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Workflow types:
Products:
Environments:
How did/will you validate this change?
If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist