- 
                Notifications
    You must be signed in to change notification settings 
- Fork 92
feat: Add support for credential manager options; add Secrets SDK Win32 persistence flag #2630
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
| throw new ImperativeError({msg: `Namespace ${namespace} does not exist`}); | ||
| } | ||
|  | ||
| (namespaceObj as Record<string, SettingValue>)[key] = value; | 
Check warning
Code scanning / CodeQL
Prototype-polluting assignment Medium
library input
          
            
              
                
              
            
            Show autofix suggestion
            Hide autofix suggestion
          
      Copilot Autofix
AI 4 days ago
To fix the problem, we should prevent storing any properties on namespaceObj whose keys could introduce prototype pollution—specifically "__proto__", "constructor", or "prototype". The simplest way to do this, and to minimize functional changes, is to add an explicit check that rejects such keys before performing the assignment. If such a key is detected, we should throw an error (e.g., using ImperativeError) and refuse the operation. This preserves all existing logic and validation, only adding a guard clause before the assignment in set.
Alternatively (and arguably even more robustly), we could replace the storage structure with a Map object. However, as this requires a potentially more extensive change (possibly impacting serialization and other logic), and the code is already interface-driven (ISettingsFile), the correct way with the smallest disruption is to simply prohibit dangerous keys.
All changes are to be made within the set method (lines 131-143) in packages/imperative/src/settings/src/AppSettings.ts.
No new imports are needed.
- 
    
    
    Copy modified lines R141-R144 
| @@ -138,6 +138,10 @@ | ||
| } | ||
| } | ||
|  | ||
| if (key === "__proto__" || key === "constructor" || key === "prototype") { | ||
| throw new ImperativeError({msg: `Key '${key}' is not allowed.`}); | ||
| } | ||
|  | ||
| (namespaceObj as Record<string, SettingValue>)[key] = value; | ||
| this.flush(); | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but this change would be considered a bug fix and not an enhancement. Given that there are already 2 enhancements in this PR, I feel that it's sensible to resolve this in a follow-up PR to keep PR changes minimal and relevant to original issue/request scope.
Signed-off-by: Trae Yelovich <[email protected]>
| Codecov Report❌ Patch coverage is  
 Additional details and impacted files@@            Coverage Diff             @@
##           master    #2630      +/-   ##
==========================================
+ Coverage   91.81%   91.84%   +0.02%     
==========================================
  Files         644      645       +1     
  Lines       19143    19208      +65     
  Branches     4124     4146      +22     
==========================================
+ Hits        17577    17641      +64     
- Misses       1564     1565       +1     
  Partials        2        2              ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
 | 
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
b7ed7ee    to
    58ca777      
    Compare
  
    Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
a05939b    to
    9b469b8      
    Compare
  
    Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
6079197    to
    26563bb      
    Compare
  
    | Regarding the remaining SonarCloud issue:  While I agree, I'm choosing to leave the member as-is as this code has existed in the codebase for some time. We can revisit in v4 if we feel that its important to address. | 
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
103f2bd    to
    af7581a      
    Compare
  
    Signed-off-by: Trae Yelovich <[email protected]>
      
        
              This comment was marked as outdated.
        
        
      
    
  This comment was marked as outdated.
| 📅 Suggested merge-by date: 11/11/2025 | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I identified something that might be a typo in a comment, which is harmless, but might be misleading.
The results in the log are as follows:
[2025/10/28 17:20:41.970] [TRACE] [DefaultCredentialManager.js:96] [DefaultCredentialManager] Persistence level received (win32): session
[2025/10/28 17:17:15.879] [TRACE] [AppSettings.js:58] {
  overrides: { CredentialManager: '@zowe/cli' },
  credentialManagerOptions: { persist: 'session' }
}
I think that the log records are consistent with the code changes. The test instructions in the PR might be slightly wrong.
| } | ||
|  | ||
| await CredentialManagerFactory.initialize({ | ||
| // Load credential manager options from team config if available | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor detail: Unless I badly misunderstand, the comment about loading from "team config" is a typo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Gene - I initially jotted this down as a to-do for this PR, but after some discussions offline & in standup we felt it was best to save that for v4. I updated the comment in 3ca7d34 to reflect that 👍
I also updated the PR description to more accurately reflect the message seen in the log.
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
| 
 | 


What It Does
credentialManagerOptionsat the root-level of the object in.zowe/settings/imperative.jsonto specify options to a credential manager.persistproperty in thecredentialManagerOptionsobject, credentials are stored using the given persistence value. Incorrect persistence values are ignored and "enterprise" (CRED_PERSIST_ENTERPRISE, or0x3) is the default.How to Test
npm install && npm run build.zowe/settings/imperative.jsonfile to contain the new credential manager option:{ "overrides": { "CredentialManager": "@zowe/cli" }, "credentialManagerOptions": { "persist": "session" } }ZOWE_IMPERATIVE_LOG_LEVEL=TRACE zowe --help.zowe/logs/imperative.logand see the following:zowe config secureand enter in your secure valuesWhere the persistence shows "Logon session" with the example above.
Review Checklist
I certify that I have: