Conversation
| name: Audit Project | ||
|
|
||
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v3 | ||
|
|
||
| - name: Setup Node | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: '24' | ||
|
|
||
| - name: Lib - Install | ||
| run: npm i | ||
| working-directory: ./keeperapi | ||
| env: | ||
| NPM_TOKEN: '' | ||
|
|
||
| - name: Lib - Build | ||
| run: npm run build | ||
| working-directory: ./keeperapi | ||
| env: | ||
| NPM_TOKEN: '' | ||
|
|
||
| - name: Lib - Check Types | ||
| run: npm run types:ci | ||
| working-directory: ./keeperapi | ||
| env: | ||
| NPM_TOKEN: '' | ||
|
|
||
| - name: Lib - Run Unit Tests | ||
| run: npm run test | ||
| working-directory: ./keeperapi | ||
|
|
||
| - name: Examples (node) - Installation | ||
| run: npm run link-local | ||
| working-directory: ./examples/print-vault-node | ||
|
|
||
| - name: Examples (node) - Check Types | ||
| run: npm run types:ci | ||
| working-directory: ./examples/print-vault-node | ||
|
|
||
| - name: Examples (browser) - Installation | ||
| run: npm run link-local | ||
| working-directory: ./examples/print-vault-browser | ||
|
|
||
| - name: Examples (browser) - Check Types | ||
| run: npm run types:ci | ||
| working-directory: ./examples/print-vault-browser |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To fix the problem, explicitly declare restricted GITHUB_TOKEN permissions in the workflow. The safest and simplest approach here is to add a permissions: block at the workflow (top) level, which will apply to the audit job and any future jobs that don’t override it. Since the workflow only needs to read repository contents, we can set contents: read, which matches the minimal permissions CodeQL suggested.
Concretely, in .github/workflows/main.yml, add a permissions: block after the on: definition and before jobs:. No existing steps or functionality need to change; the workflow will still be triggered on push, run on ubuntu-latest, and execute the same npm commands, but with a GITHUB_TOKEN limited to reading repository contents. No imports, extra methods, or additional configuration files are required.
| @@ -2,6 +2,9 @@ | ||
|
|
||
| on: [push] | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| jobs: | ||
| audit: | ||
| name: Audit Project |
| import LoginType = Authentication.LoginType; | ||
|
|
||
| process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0' | ||
| process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0'; |
Check failure
Code scanning / CodeQL
Disabling certificate validation High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
In general, the fix is to stop globally disabling TLS certificate validation via NODE_TLS_REJECT_UNAUTHORIZED. Instead, allow Node.js to perform default certificate verification or, if a custom trust setup is required (for example, self-signed certs in a dev environment), configure trusted CAs or TLS options in a narrowly scoped manner without turning verification off.
For this specific file, the minimal, behavior-preserving fix that removes the insecure practice is to delete the line process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0';. The rest of the code will continue to function using Node.js’s default TLS behavior, relying on the certificates presented by KeeperEnvironment.DEV (or whatever host is used) being valid and trusted by the runtime environment. No other changes, imports, or definitions are needed, because this line is not required for program flow or logic—its only effect is weakening security. The change is localized to line 5 of examples/print-vault-node/src/index.ts.
| @@ -2,8 +2,6 @@ | ||
| import * as readline from 'readline'; | ||
| import LoginType = Authentication.LoginType; | ||
|
|
||
| process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0'; | ||
|
|
||
| const clientVersion = 'w16.4.0'; | ||
|
|
||
| async function printVault(username: string, password: string) { |
| } | ||
| keyBytesCache[keyId] = keyBytes | ||
| return keyBytes | ||
| keyBytesCache[keyId] = keyBytes; |
Check failure
Code scanning / CodeQL
Remote property injection High
| cryptoKeysCache[keyType][keyId] = key | ||
| return key | ||
| const key = await this.loadCryptoKey(keyId, keyType, storage); | ||
| cryptoKeysCache[keyType][keyId] = key; |
Check failure
Code scanning / CodeQL
Remote property injection High
| const { data, dataId, keyId, encryptionType } = task; | ||
| try { | ||
| const keyBytes = await platform.decrypt(data, keyId, encryptionType, keyStorage); | ||
| results[dataId] = keyBytes; |
Check failure
Code scanning / CodeQL
Remote property injection High
| keyCache[keyId] = keyBytes | ||
| return keyBytes | ||
| } | ||
| keyCache[keyId] = keyBytes; |
Check failure
Code scanning / CodeQL
Remote property injection High
| cryptoKeysCache[keyType][keyId] = key | ||
| return key | ||
| const key = await this.loadCryptoKey(keyId, keyType, storage); | ||
| cryptoKeysCache[keyType][keyId] = key; |
Check warning
Code scanning / CodeQL
Prototype-polluting assignment Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
In general, the fix is to ensure that the property name used for indexing cryptoKeysCache cannot be a magic prototype key (__proto__, constructor, prototype) or any unexpected string. The two common approaches are: (1) validate and restrict keyType to a known safe set of strings at runtime before using it as an object key, or (2) replace the plain object used as a cache with a prototype-less object or a Map so that even if an attacker supplies __proto__, it cannot affect Object.prototype.
For this code, the minimal, non–behaviour-changing fix is to add a runtime guard in decrypt (where the tainted encryptionType first enters this logic) that ensures it is one of the expected literal values ('cbc' | 'gcm' | 'rsa' | 'ecc'). If it is anything else, we throw an error before calling loadKey. This guarantees that loadKey and loadCryptoKey only ever receive safe keyType values, removing the possibility that cryptoKeysCache[keyType] could become cryptoKeysCache.__proto__. To further harden cryptoKeysCache itself against accidental pollution, we can also switch its declaration to a prototype-less object (created with Object.create(null)) in the snippet we control, but since that declaration is not shown, we will solve the issue solely via validation at the tainted entry point.
Concretely, in keeperapi/src/browser/platform.ts, inside static async decrypt(...), before the switch (encryptionType), introduce validation that checks encryptionType against the allowed set, and throws if it is anything else. The rest of the logic can remain unchanged, so existing functionality for known encryption types is not affected.
| @@ -432,6 +432,12 @@ | ||
| encryptionType: EncryptionType, | ||
| storage?: KeyStorage | ||
| ): Promise<Uint8Array> { | ||
| // Validate encryptionType to avoid prototype-pollution via unexpected keys | ||
| const allowedTypes: EncryptionType[] = ['cbc', 'gcm', 'rsa', 'ecc']; | ||
| if (!allowedTypes.includes(encryptionType)) { | ||
| throw Error('Unknown encryption type: ' + encryptionType); | ||
| } | ||
|
|
||
| switch (encryptionType) { | ||
| case 'cbc': { | ||
| const key = await this.loadKey(keyId, encryptionType, storage); | ||
| @@ -451,6 +457,7 @@ | ||
| return this.privateDecryptECWebCrypto(data, key); | ||
| } | ||
| default: | ||
| // This default should now be unreachable due to the validation above. | ||
| throw Error('Unknown encryption type: ' + encryptionType); | ||
| } | ||
| } |
These are the only real changes:
2494f53
2df3f2c
The rest is formatting from
npx prettier --write .The format commit (and others added to .git-blame-ignore-revs) can be ignored in the blame by running:
git config blame.ignoreRevsFile .git-blame-ignore-revs