Conversation
Summary of ChangesHello @passcod, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's security by introducing a comprehensive framework for managing encrypted secrets. It provides developers with tools and mechanisms to store sensitive information, such as API keys and passwords, in both configuration files and the database in an encrypted format, thereby mitigating risks associated with plaintext storage. The implementation ensures that secrets are handled securely throughout their lifecycle, from generation to retrieval, using industry-standard cryptographic practices. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces a robust feature for handling encrypted secrets within configuration files and settings, utilizing the Web Crypto API with AES-GCM. The implementation includes CLI tools for key management and is accompanied by a comprehensive test suite for the cryptographic functions. While the core functionality is well-implemented, the review has identified a few significant issues that need attention. There is a critical bug in the Setting.ts model that prevents per-facility secrets from functioning as intended. Additionally, there's notable code duplication in the CLI command logic and the use of monkey-patching, which poses a maintainability risk. Addressing these points will greatly improve the quality and robustness of this new feature.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust mechanism for encrypting and storing secrets in configuration files and database settings using the Web Crypto API. The implementation is well-structured, with new crypto utilities, CLI commands for secret management, and comprehensive tests. The approach of using a server-specific key for config secrets and a pre-shared key for settings secrets is sound. My review focuses on a few areas for improvement, including fixing a bug that prevents encrypting empty strings, reducing code duplication, and improving type safety for better long-term maintainability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-implemented feature for handling encrypted secrets in both configuration and application settings. The use of the Web Crypto API with AES-GCM is a solid choice for security. The changes are extensive, touching everything from database models and API routes to CLI commands and the frontend UI. The code is generally of high quality, but I've identified a few areas for improvement, mainly concerning code duplication and a potential issue in the handling of the pre-shared key for settings encryption. My comments highlight these points with suggestions for refactoring.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust system for handling encrypted secrets in both configuration files and database settings, using the Web Crypto API with AES-GCM. The changes are extensive, touching on backend routes, database models, CLI commands, and frontend UI components to support this new functionality. Overall, the implementation is solid, with good security practices and comprehensive testing. I've identified a critical issue that could lead to data loss under specific circumstances, along with several medium-severity suggestions to improve code clarity, performance, and maintainability by reducing redundancy and refactoring complex logic. Addressing these points will further strengthen this already well-designed feature.
|
Android builds 📱
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive and well-implemented feature for managing encrypted secrets in both configuration files and database settings. It leverages the Web Crypto API for strong encryption and includes backend logic, CLI tooling, and a secure UI in the admin panel. The fallback mechanism for credentials is a thoughtful addition for smooth migration. The code is of high quality with thorough test coverage. I have a few suggestions to further improve maintainability by reducing code duplication and enhancing type safety, but overall this is an excellent contribution.
🍹
|
|
Note that the deploy is up and shows the UI, but Kubernetes-side is not yet configured to have a config secret and so on, so it won't "actually work" |
rohan-bes
left a comment
rohan-bes
left a comment
There was a problem hiding this comment.
Looks great @passcod! Awesome change.
Mostly questions/suggestions, just requesting changes around:
- Only making the password secret in DHIS2 integration settings/config
- The masking front-end logic, and whether we display the placeholder if a secret setting is empty
Also wasn't entirely sure why we need the patchReadSettings logic, but do feel its a a bit unexpected and might catch us out.
| try { | ||
| username = await this.context.settings.getSecret('integrations.dhis2.username'); | ||
| } catch { | ||
| // Settings secret not available, try config secret | ||
| try { | ||
| username = await getConfigSecret('integrations.dhis2.username'); | ||
| } catch { | ||
| // Config secret not available, fall back to plain config | ||
| username = config.integrations?.dhis2?.username || null; | ||
| } | ||
| } |
There was a problem hiding this comment.
In this specific instance might recommend just the password being encrypted. Nice to have the username in plaintext so we know it's set to the right user.
| it('should mask empty strings as valid secrets', () => { | ||
| const settings = { | ||
| emptySecret: '', | ||
| normalSetting: 'visible', | ||
| }; | ||
| const secretPaths = ['emptySecret']; | ||
|
|
||
| const masked = maskSecrets(settings, secretPaths); | ||
|
|
||
| expect(masked.emptySecret).toBe(SECRET_PLACEHOLDER); | ||
| expect(masked.normalSetting).toBe('visible'); | ||
| }); |
There was a problem hiding this comment.
Is this what we want? I might expect that we'd just display an empty string if the original secret setting is also empty.
Or at least, I'm wondering what happens when a user clears a secret setting? I think I'd expect that the field is cleared (it doesn't display the placeholder). Tho I'm not sure if we set the setting value to '' if the user clears the value...
There was a problem hiding this comment.
Empty secrets are still encrypted so we reveal nothing to an unauthorised observer. I'm not sure if there's some way to clear the secret in the UI, though, will look.
| description: 'Username for DHIS2 API authentication', | ||
| type: yup.string(), | ||
| defaultValue: '', | ||
| secret: true, |
There was a problem hiding this comment.
As mentioned, I think username can be plaintext 👍
| // Placeholder used by the server to indicate a secret exists but value is hidden | ||
| const SECRET_PLACEHOLDER = '••••••••'; |
There was a problem hiding this comment.
Worth moving this to constants?
| const isUnchangedFromDefault = useMemo(() => { | ||
| if (isSecret) { | ||
| // For secrets, we can't compare to default since value is hidden | ||
| return !secretEdited; | ||
| } |
There was a problem hiding this comment.
Could argue that secrets should never have a default value? Struggling to think of a use case where we want to hide a value but are happy with a global default
| edge="end" | ||
| size="small" | ||
| > | ||
| {showSecretValue ? <VisibilityOff /> : <Visibility />} |
There was a problem hiding this comment.
Perhaps I'm mmissing something here, but aren't we masking the secrets on the server-side of the api call? So even if we displayed the value of the secret it would just be the placeholder?
There was a problem hiding this comment.
I think I'm okay with not being able to view the secret setting values. Feels like it'd be hard to allow users to view in plaintext in a way that's still secure.
| /** | ||
| * Gets a decrypted secret from the settings. | ||
| * This method must be patched at runtime before use. | ||
| */ | ||
| async getSecret(_name: string): Promise<string> { | ||
| throw new Error('BUG: ReadSettings.getSecret must be patched before use'); | ||
| } |
There was a problem hiding this comment.
Why do we need to do this? What happens if the getSecret implementation from patchReadSettings is just here instead?
There was a problem hiding this comment.
The settings package can't depend on shared. It's ugly, an alternative would be to make a new package but would still need a way to inject/patch some context for the PSK
There was a problem hiding this comment.
Ah I get ya, that makes sense.
In that case, thoughts on injecting the getConfigSecret and decryptSecret functions in to the ReadSettings when initializing in the ApplicationContext for facility-server and central-server? Perhaps even wrap in a utility function called getSettingSecret a bit like what you've done for getConfigSecret?
That way we don't need any ReadSettings logic in the shared package, and it's a bit more clear what's going on.
| try { | ||
| password = await getConfigSecret('integrations.dhis2.password'); | ||
| } catch { | ||
| password = config.integrations?.dhis2?.password || null; | ||
| } |
There was a problem hiding this comment.
Thoughts on making this fallback logic generic in the getConfigSecret function? I could see it being convenient if it turns out that some deployments use secrets and others don't.
Just a suggestion tho
There was a problem hiding this comment.
Mixed mind, I think if it's in the utility it will entrench the ability to have plain text secrets in the config. Perhaps the solution is rather to just require that the secrets are encrypted and eat the manual upgrade cost once.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| import { addHooks } from './hooks'; | ||
| import { closeAllDatabases, openDatabase } from '@tamanu/database/services/database'; | ||
| import { patchReadSettings } from '@tamanu/shared/utils/patchReadSettings'; | ||
| import { log } from '@tamanu/shared/services/logging'; |
| import { closeAllDatabases, openDatabase } from '@tamanu/database/services/database'; | ||
| import { patchReadSettings } from '@tamanu/shared/utils/patchReadSettings'; | ||
| import { log } from '@tamanu/shared/services/logging'; | ||
| import { REPORT_DB_SCHEMAS } from '@tamanu/constants'; |
|
|
||
| import { closeAllDatabases, openDatabase } from '@tamanu/database/services/database'; | ||
| import { fakeUUID } from '@tamanu/utils/generateId'; | ||
| import { log } from '@tamanu/shared/services/logging'; |
| import { fakeUUID } from '@tamanu/utils/generateId'; | ||
| import { log } from '@tamanu/shared/services/logging'; | ||
| import { patchReadSettings } from '@tamanu/shared/utils/patchReadSettings'; | ||
| import { REPORT_DB_SCHEMAS } from '@tamanu/constants'; |
![review-hero[bot]](https://avatars.githubusercontent.com/in/2896273?s=60&v=4){kind=small}
| function flattenSettingsToEntries(obj, parentPath = '') { | ||
| const entries = []; | ||
|
|
||
| for (const [key, value] of Object.entries(obj)) { |
There was a problem hiding this comment.
[Design & Architecture] suggestion
flattenSettingsToEntries is a non-trivial recursive algorithm that has been placed inline in a route file. It operates purely on a settings object and has no dependency on HTTP context. It belongs in @tamanu/settings or at least in a shared util, both to keep the route file focused on HTTP concerns and to make the function independently testable.
| } | ||
|
|
||
| // Only save non-secret settings if there are any remaining leaf values | ||
| const remainingEntries = flattenSettingsToEntries(settings); |
There was a problem hiding this comment.
[Bugs & Correctness] critical
flattenSettingsToEntries(settings) is called unconditionally at line 165, but the guard at line 142 only runs when settings is a non-null object. If the caller sends no body, or settings is null/undefined/a primitive, Object.entries(settings) inside flattenSettingsToEntries will throw TypeError: Cannot convert undefined or null to object. The previous code passed settings directly to Setting.set which presumably handles such cases. Fix: wrap the call in the same guard, or add a standalone check: if (!settings || typeof settings !== 'object') { res.json({ code: 200 }); return; } before line 164.
| return pick(logEntry.get({ plain: true }), LOG_FIELDS); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
[Design & Architecture] suggestion
getDHIS2Credentials() fetches username and password independently through three nested try/catch fallback layers each. This means username could resolve from one source (e.g. DB secret) while password resolves from a different one (e.g. plain config), producing a mixed-source credential pair that is almost certainly wrong and very hard to debug. The fallback strategy should be applied once at the credential-pair level, not per-field — fetch both from the highest-priority source that has them, then fall back together.
| * 3. Fall back to plain config value | ||
| */ | ||
| async getDHIS2Credentials() { | ||
| let username = null; |
There was a problem hiding this comment.
[BES Requirements] suggestion
Two let variables reassigned through nested try/catch conditionals is an explicit antipattern in coding-rules.md ('let is a smell'). The identical fallback pattern for username and password should be extracted into a helper, e.g. async getCredentialWithFallback(settingPath) that tries settings secret → config secret → plain config in sequence. This would eliminate the duplication and the mutable lets.
[Bugs & Correctness] suggestion
getDHIS2Credentials swallows all exceptions from getSecret and getConfigSecret with empty catch blocks, including unexpected errors like corrupted key files, DB failures, or decryption errors on a tampered ciphertext. This means a deployment with a broken key file will silently fall back to plain-config credentials (or null) with no log or alert, making the failure invisible. The catch should at minimum distinguish 'not configured' from unexpected errors — e.g. check err.message or a specific error type — and log unexpected ones before falling through.
| // Try settings secret first | ||
| try { | ||
| username = await this.context.settings.getSecret('integrations.dhis2.username'); | ||
| } catch { |
There was a problem hiding this comment.
[Security] suggestion
The catch block for getSecret silently swallows ALL errors — including crypto failures like a missing key file, wrong PSK, or corrupted encrypted value. The fallback chain means that if the encryption infrastructure breaks (e.g. key file is deleted or PSK is wrong), the integration silently falls back to plaintext config values without any log warning. An operator who configured encrypted secrets would have no way to know their secrets are no longer being used. The outer catch should at minimum log a warning before falling back, and ideally should distinguish 'secret not configured' (expected, fall back) from 'decryption failed' (unexpected, warn loudly).
| @@ -0,0 +1,19 @@ | |||
| import { ReadSettings } from '@tamanu/settings'; | |||
There was a problem hiding this comment.
[Design & Architecture] suggestion
Monkey-patching ReadSettings.prototype.getSecret at startup is a fragile pattern. The @tamanu/settings package now contains a stub that says 'must be patched before use', creating an implicit runtime contract with no compile-time enforcement — any code path that calls getSecret before patchReadSettings() is invoked will fail silently until hit in production. A cleaner design would be to accept the decrypt function as a constructor parameter or via a factory (e.g. new ReadSettings({ getSecret: ... })), keeping the dependency explicit and testable without side effects.
| */ | ||
| export function patchReadSettings() { | ||
| ReadSettings.prototype.getSecret = async function (name) { | ||
| const encryptedValue = await this.get(name); |
There was a problem hiding this comment.
[Security] suggestion
getSecret retrieves the value and checks it's a non-empty string, but never calls isEncryptedSecret() before attempting to decrypt. If a setting path was previously stored as plaintext (before being marked secret: true in the schema), decryptSecret will throw 'Invalid encrypted secret format' — a confusing error that doesn't indicate the root cause. Adding an isEncryptedSecret guard would give a clearer error ('setting is not encrypted; re-save it via the admin UI to encrypt it') and prevents any confusion with legitimate non-secret string values at that path.
[Bugs & Correctness] suggestion
getConfigSecret('crypto.settingsPsk') reads and decrypts the PSK from disk on every call to getSecret. Because getSecret is called per-credential per integration run, this causes repeated key-file reads under normal operation. The PSK should be cached (e.g. a module-level promise) after the first successful read so subsequent calls don't incur repeated I/O.
| ['body']: SETTING_TYPES.LONG_TEXT, | ||
| }; | ||
|
|
||
| // Placeholder used by the server to indicate a secret exists but value is hidden |
There was a problem hiding this comment.
[BES Requirements] suggestion
SECRET_PLACEHOLDER is redefined locally as '••••••••' instead of importing it from @tamanu/settings where it is now exported. The frontend logic value === SECRET_PLACEHOLDER and isSecretPlaceholder comparisons will silently break if the server-side constant ever changes, causing secret fields to appear as user-edited when they're not, and potentially sending placeholder values to the server on save. Import the shared constant instead.
[Security] suggestion
SECRET_PLACEHOLDER ('••••••••') is hardcoded locally in the frontend rather than imported from @tamanu/settings where the canonical value is defined. If the value in @tamanu/settings is ever changed, the backend's placeholder check in the PUT /settings handler (entry.value !== SECRET_PLACEHOLDER) won't match what the frontend sends, causing the unchanged placeholder to be encrypted and stored as a new secret value — silently corrupting any secret the admin 'saves' without changing. Import the constant from the shared package.
[Design & Architecture] nitpick
SECRET_PLACEHOLDER = '••••••••' is re-defined locally here even though it is already exported from @tamanu/settings. Import it directly so the two values cannot drift apart: import { SECRET_PLACEHOLDER } from '@tamanu/settings';.
|
🦸 Review Hero Summary Nitpicks
Local fix prompt (copy to your coding agent)Fix these issues identified on the pull request. One commit per issue fixed.
getDHIS2Credentials swallows all exceptions from getSecret and getConfigSecret with empty catch blocks, including unexpected errors like corrupted key files, DB failures, or decryption errors on a tampered ciphertext. This means a deployment with a broken key file will silently fall back to plain-config credentials (or null) with no log or alert, making the failure invisible. The catch should at minimum distinguish 'not configured' from unexpected errors — e.g. check err.message or a specific error type — and log unexpected ones before falling through.
This file is byte-for-byte identical to
getConfigSecret('crypto.settingsPsk') reads and decrypts the PSK from disk on every call to getSecret. Because getSecret is called per-credential per integration run, this causes repeated key-file reads under normal operation. The PSK should be cached (e.g. a module-level promise) after the first successful read so subsequent calls don't incur repeated I/O.
SECRET_PLACEHOLDER ('••••••••') is hardcoded locally in the frontend rather than imported from @tamanu/settings where the canonical value is defined. If the value in @tamanu/settings is ever changed, the backend's placeholder check in the PUT /settings handler (entry.value !== SECRET_PLACEHOLDER) won't match what the frontend sends, causing the unchanged placeholder to be encrypted and stored as a new secret value — silently corrupting any secret the admin 'saves' without changing. Import the constant from the shared package.
|
|
Closing for now, need to rethink the architecture a bit. The concept is sound but the implementation is brittle. |
2 participants
Changes
Adds the ability to store secrets in config files, in local system facts, and in settings. Secrets are encrypted using the Web Crypto API, with a symmetric key. For settings, there's an additional layer so that all servers can share a key (as the secrets are encrypted once in the DB).
In config, it looks like this:
In settings and local system facts, it looks similar:
The unique thing with settings is the
settingsPskconfig secret. That's a shared key (PSK = Pre-Shared Key) common between all servers that can read secrets from settings. It's encrypted with the per-server key when it's stored in config. This key is required to be 32 bytes, encoded in hex, so it's sufficiently strong.To start with, the per-server key will be just a file stored separately from the config, but later we can improve that security if we need, for example by using a TPM on metal or in AWS on Nitro EC2 instances.
The format of encrypted secrets is
S1(just a version and a visual identifier that it's a secret), the IV, and the ciphertext. The algorithm used is AES-GCM.In the admin panel UI, secret values can't be read, only written.
As an example of a migration to secrets, we have the DHIS2 credentials, which are currently in config in plain text. This PR includes a change that will first look in settings, then for an encrypted value in config, and fall back to the plain value in config.
Deploys
Tests
Remember to...
🦸 Review Hero