fix(auth): handle null APPDATA environment variable in GoogleAuthUtils.getWellKnownCredentialsFile#12913
fix(auth): handle null APPDATA environment variable in GoogleAuthUtils.getWellKnownCredentialsFile#12913anishesg wants to merge 2 commits intogoogleapis:mainfrom
Conversation
…s.getWellKnownCredentialsFile
In `GoogleAuthUtils.getWellKnownCredentialsFile()`, on Windows systems where `CLOUDSDK_CONFIG` is unset, the code calls `provider.getEnv("APPDATA")` and passes the result directly to `new File(String)`. When `APPDATA` is not present in the environment, `getEnv()` returns `null`, and `new File(null)` immediately throws a raw `NullPointerException` with no useful context.
Signed-off-by: anish k <ak8686@princeton.edu>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds validation for the APPDATA environment variable on Windows in GoogleAuthUtils.java, ensuring an UncheckedIOException is thrown if it is missing, and includes a supporting unit test. Feedback suggests further improving this check by also handling empty or blank strings to prevent invalid path construction.
…trings, add tests Signed-off-by: anish k <ak8686@princeton.edu>
|
Friendly ping — all tests pass, just waiting on the multi-approver requirement. Let me know if there's anything else needed on this one. |
lqiu96
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
| } else if (provider.getOsName().indexOf("windows") >= 0) { | ||
| File appDataPath = new File(provider.getEnv("APPDATA")); | ||
| String appDataEnv = provider.getEnv("APPDATA"); | ||
| if (appDataEnv == null || appDataEnv.trim().isEmpty()) { |
There was a problem hiding this comment.
nit: Guava has Strings.isNullOrEmpty for a convenient API
There was a problem hiding this comment.
I think this check would reject if the env var was set to a string like " ", which is probably more robust than just checking for Null or Empty. If we decide to keep it this way, we might want to update it in here too, so that our checks for APPDATA are consistent.
There was a problem hiding this comment.
From a quick search, I don't think " " is a valid in Windows and probably not realistic (IIUC APPDATA is used throughout Windows and should be valid). Since the other location already uses Guava, let's just make it consistent and use Guava as well (both impls resolve the main NPE possibility and are fine with me).
| if (appDataEnv == null || appDataEnv.trim().isEmpty()) { | ||
| throw new UncheckedIOException( | ||
| new FileNotFoundException( | ||
| "APPDATA environment variable is not set or empty; cannot locate the well-known" |
There was a problem hiding this comment.
Can you update the message to be something like ... is not set or is empty for clarity?
4 participants
In
GoogleAuthUtils.getWellKnownCredentialsFile(), on Windows systems whereCLOUDSDK_CONFIGis unset, the code callsprovider.getEnv("APPDATA")and passes the result directly tonew File(String). WhenAPPDATAis not present in the environment,getEnv()returnsnull, andnew File(null)immediately throws a rawNullPointerExceptionwith no useful context.The fix adds an explicit null check after retrieving the
APPDATAvalue. When it is null, the method now throws anUncheckedIOExceptionwrapping aFileNotFoundExceptionwith a descriptive message indicating thatAPPDATAis not set, rather than propagating an opaqueNullPointerException. This is consistent with the library's overall approach of surfacing meaningful errors when credential paths cannot be resolved.A regression test in
GoogleAuthUtilsTestverifies that the new exception type and message are produced whenAPPDATAis absent on a simulated Windows environment.Fixes #12565