fix: harden synapse config perms and remove plaintext bridge tokens#60
fix: harden synapse config perms and remove plaintext bridge tokens#60altantutar wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR implements filesystem hardening and moves bridge tokens from plaintext config to environment-only storage. A new fs-security module provides utilities to enforce restrictive permissions (0o700 for directories, 0o600 for files) across config storage. Configuration modules are refactored to strip sensitive fields from persisted JSON and restore them from environment variables or in-memory storage. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Config as Bridge Config Module
participant File as FileSystem
participant Secrets as SessionSecrets (Memory)
participant Env as Environment Variables
App->>Config: getBridgeConfig()
Config->>File: loadBridgeFileConfig(bridge.json)
File-->>Config: {fileConfig, legacySecrets, hadLegacySecrets}
alt Legacy Secrets Present
Config->>Secrets: recordSessionSecrets(legacySecrets)
Secrets-->>Config: sessionSecrets updated
Config->>Config: stripSecrets(fileConfig)
Config->>File: writePrivateTextFile(bridge.json, sanitized)
File-->>Config: written with 0o600
end
Config->>Config: merge DEFAULT_CONFIG + fileConfig
Config->>Env: check GATEWAY_AUTH_TOKEN, IRONCLAW_TOKEN
Env-->>Config: env values (if set)
Config->>Secrets: restore sessionSecrets if no env var
Secrets-->>Config: in-memory secrets applied
Config-->>App: merged BridgeConfig with restored secrets
sequenceDiagram
participant App as Application
participant Config as Bridge Config Module
participant Secrets as SessionSecrets (Memory)
participant File as FileSystem
App->>Config: saveBridgeConfig(partialConfig)
Config->>File: loadBridgeFileConfig(bridge.json)
File-->>Config: {fileConfig, legacySecrets}
Config->>Secrets: recordSessionSecrets(legacySecrets)
Secrets-->>Config: sessionSecrets accumulated
Config->>Config: stripSecrets(partialConfig)
Config->>Config: merge sanitized with existing fileConfig
Config->>File: ensurePrivateDirectory(~/.synapse)
File-->>Config: directory created with 0o700
Config->>File: writePrivateTextFile(bridge.json, mergedConfig)
File-->>Config: written with 0o600
Config-->>App: config persisted (secrets excluded)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello @altantutar, 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 security posture of the application by preventing the persistence of sensitive bridge authentication tokens in configuration files. It introduces a robust file system security layer to ensure that configuration directories and files have appropriately restrictive permissions, protecting user data from unauthorized access. Additionally, it includes a migration path for existing installations to remove legacy plaintext tokens, streamlining the transition to a more secure configuration management approach. Highlights
Changelog
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 significantly improves the security of the application by removing plaintext bridge tokens from configuration files and hardening file permissions for configuration artifacts. Secrets are now handled via environment variables, and a one-time migration is in place to remove legacy tokens from existing bridge.json files. The permissions for the ~/.synapse directory and its configuration files are now restricted to the owner (0700 for the directory, 0600 for files). The changes are well-supported by new and updated tests, and the user-facing setup wizards have been updated to reflect the new secret handling process. Overall, this is a great set of changes that enhances the security posture of the application. I have one suggestion to further improve the security hardening.
| if (!existsSync(SYNAPSE_DIR)) { | ||
| mkdirSync(SYNAPSE_DIR, { recursive: true }); | ||
| } | ||
| ensurePrivateDirectory(SYNAPSE_DIR); |
There was a problem hiding this comment.
Great job on hardening the permissions for the Synapse directory. To further improve security, you should also consider hardening the permissions of the SQLite database file itself. The directory is now 0700, but the database file will be created with default permissions (subject to umask). Since the database can contain sensitive user activity data, it should be protected with 0600 permissions, just like the configuration files.
You could use the new hardenFilePermissions function from fs-security.ts for this. A good place to add this would be in getSynapseDb() right after the database object is created:
import { ensurePrivateDirectory, hardenFilePermissions } from './fs-security.js';
// ...
export function getSynapseDb(): Database {
ensureSynapseDir();
const db = new Database(SYNAPSE_DB_PATH);
hardenFilePermissions(SYNAPSE_DB_PATH); // Harden DB file perms
initializeTables(db);
return db;
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/bridge/__tests__/config.test.ts (1)
129-151:⚠️ Potential issue | 🟡 MinorIsolate env overrides to avoid flaky token assertions.
If
GATEWAY_AUTH_TOKEN/IRONCLAW_TOKENare set in the environment, these tests will fail becausegetBridgeConfig()prioritizes env vars. Clear and restore them within the tests.🛠️ Suggested fix
test('saveBridgeConfig partial update preserves existing config fields', () => { saveBridgeConfig({ enabled: true, openclawEnabled: true, toolsEnabled: true, apiPort: 49001, gatewayAuthToken: 'do-not-persist', }); // Simulate `synapse bridge disable` saveBridgeConfig({ enabled: false }); - const config = getBridgeConfig(); + const prevGateway = process.env.GATEWAY_AUTH_TOKEN; + const prevIronclaw = process.env.IRONCLAW_TOKEN; + delete process.env.GATEWAY_AUTH_TOKEN; + delete process.env.IRONCLAW_TOKEN; + const config = getBridgeConfig(); + if (prevGateway !== undefined) process.env.GATEWAY_AUTH_TOKEN = prevGateway; + if (prevIronclaw !== undefined) process.env.IRONCLAW_TOKEN = prevIronclaw; expect(config.enabled).toBe(false); expect(config.openclawEnabled).toBe(true); expect(config.toolsEnabled).toBe(true); expect(config.apiPort).toBe(49001); expect(config.gatewayAuthToken).toBeUndefined(); const raw = readFileSync(CONFIG_PATH, 'utf-8'); const parsed = JSON.parse(raw); expect(parsed.gatewayAuthToken).toBeUndefined(); }); test('legacy plaintext token fields are removed from bridge.json on read', () => { const legacyConfig = { enabled: true, gatewayAuthToken: 'legacy-gateway-token', ironclawToken: 'legacy-ironclaw-token', apiPort: 49999, }; writeFileSync(CONFIG_PATH, JSON.stringify(legacyConfig), 'utf-8'); - const config = getBridgeConfig(); + const prevGateway = process.env.GATEWAY_AUTH_TOKEN; + const prevIronclaw = process.env.IRONCLAW_TOKEN; + delete process.env.GATEWAY_AUTH_TOKEN; + delete process.env.IRONCLAW_TOKEN; + const config = getBridgeConfig(); + if (prevGateway !== undefined) process.env.GATEWAY_AUTH_TOKEN = prevGateway; + if (prevIronclaw !== undefined) process.env.IRONCLAW_TOKEN = prevIronclaw; expect(config.gatewayAuthToken).toBe('legacy-gateway-token'); expect(config.ironclawToken).toBe('legacy-ironclaw-token'); expect(config.apiPort).toBe(49999);Also applies to: 185-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bridge/__tests__/config.test.ts` around lines 129 - 151, The test relies on getBridgeConfig() which reads GATEWAY_AUTH_TOKEN / IRONCLAW_TOKEN from process.env, causing flakiness; modify the test(s) that call saveBridgeConfig/getBridgeConfig (including the block around lines 185-203) to save the original process.env.GATEWAY_AUTH_TOKEN and process.env.IRONCLAW_TOKEN, delete those env entries before running the assertions, and restore the original values in a finally/after step so the environment is isolated for the test and restored afterwards.src/config.ts (1)
77-83:⚠️ Potential issue | 🟠 MajorHarden the config directory on read as well.
getConfig()only hardens the file. If the directory already exists with permissive mode, it stays open unlesssaveConfig()or doctor runs. It’s safer to ensure the directory permissions whenever reading.🔒 Suggested fix
export function getConfig(): SynapseConfig { if (!existsSync(CONFIG_PATH)) { return DEFAULT_CONFIG; } + ensurePrivateDirectory(CONFIG_DIR); hardenFilePermissions(CONFIG_PATH);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.ts` around lines 77 - 83, getConfig() currently only calls hardenFilePermissions(CONFIG_PATH) for the file, leaving the containing directory potentially permissive; update getConfig() to also harden the config directory on read by invoking the directory-permissions hardening (e.g., call hardenFilePermissions(path.dirname(CONFIG_PATH)) or add/rename to a hardenDirPermissions utility) before returning DEFAULT_CONFIG or reading the file so directory permissions are enforced whenever the config is accessed (similar to how saveConfig() or doctor would do).src/bridge/wizard.ts (1)
149-173:⚠️ Potential issue | 🟡 MinorUse env vars when deciding to warn about unauthenticated 0.0.0.0 binding.
Line 171 only checks the prompt value; if
GATEWAY_AUTH_TOKENis already set in the environment, the warning is a false positive. Consider includingprocess.env.GATEWAY_AUTH_TOKENin the check.🛠️ Suggested fix
- if (apiHost === '0.0.0.0' && !openclawAdvanced.gatewayAuthToken) { + const hasGatewayToken = + Boolean(openclawAdvanced.gatewayAuthToken || process.env.GATEWAY_AUTH_TOKEN); + if (apiHost === '0.0.0.0' && !hasGatewayToken) { p.log.warn('API server will be network-accessible without authentication.\n Set a gateway auth token to secure it.'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bridge/wizard.ts` around lines 149 - 173, The warning about an unauthenticated 0.0.0.0 binding currently checks only openclawAdvanced.gatewayAuthToken; update the logic to consider the environment variable as well by treating process.env.GATEWAY_AUTH_TOKEN as a valid token when deciding whether to warn. In practice, ensure gatewayAuthToken is set from openclawAdvanced.gatewayAuthToken or process.env.GATEWAY_AUTH_TOKEN before the check (references: openclawAdvanced, gatewayAuthToken, apiHost, process.env.GATEWAY_AUTH_TOKEN) and change the conditional to only warn when apiHost === '0.0.0.0' && !gatewayAuthToken (or && !process.env.GATEWAY_AUTH_TOKEN if you prefer to check env directly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/config.test.ts`:
- Around line 24-30: The test teardown in afterAll (using originalConfig,
CONFIG_DIR, CONFIG_PATH) can leave the restored config file world-readable;
update the restore path to ensure secure permissions by either passing a mode
when writing (e.g., writeFileSync(CONFIG_PATH, originalConfig, { encoding:
'utf-8', mode: 0o600 })) and/or calling chmodSync(CONFIG_PATH, 0o600)
immediately after writeFileSync, and also ensure the else branch that leaves an
existing CONFIG_PATH preserves/sets secure permissions via
chmodSync(CONFIG_PATH, 0o600).
In `@src/bridge/__tests__/api-client.test.ts`:
- Around line 55-62: The test mutates process.env.GATEWAY_AUTH_TOKEN without
preserving existing state—capture the original value before setting (e.g., const
original = process.env.GATEWAY_AUTH_TOKEN), set process.env.GATEWAY_AUTH_TOKEN =
'abc123' for the test, then in the finally block restore it (if original is
undefined delete the env var, otherwise restore the original string); apply this
change around the getHeaders() assertions in the api-client.test.ts test that
references getHeaders to avoid clobbering a developer's environment or causing
order-dependent tests.
---
Outside diff comments:
In `@src/bridge/__tests__/config.test.ts`:
- Around line 129-151: The test relies on getBridgeConfig() which reads
GATEWAY_AUTH_TOKEN / IRONCLAW_TOKEN from process.env, causing flakiness; modify
the test(s) that call saveBridgeConfig/getBridgeConfig (including the block
around lines 185-203) to save the original process.env.GATEWAY_AUTH_TOKEN and
process.env.IRONCLAW_TOKEN, delete those env entries before running the
assertions, and restore the original values in a finally/after step so the
environment is isolated for the test and restored afterwards.
In `@src/bridge/wizard.ts`:
- Around line 149-173: The warning about an unauthenticated 0.0.0.0 binding
currently checks only openclawAdvanced.gatewayAuthToken; update the logic to
consider the environment variable as well by treating
process.env.GATEWAY_AUTH_TOKEN as a valid token when deciding whether to warn.
In practice, ensure gatewayAuthToken is set from
openclawAdvanced.gatewayAuthToken or process.env.GATEWAY_AUTH_TOKEN before the
check (references: openclawAdvanced, gatewayAuthToken, apiHost,
process.env.GATEWAY_AUTH_TOKEN) and change the conditional to only warn when
apiHost === '0.0.0.0' && !gatewayAuthToken (or &&
!process.env.GATEWAY_AUTH_TOKEN if you prefer to check env directly).
In `@src/config.ts`:
- Around line 77-83: getConfig() currently only calls
hardenFilePermissions(CONFIG_PATH) for the file, leaving the containing
directory potentially permissive; update getConfig() to also harden the config
directory on read by invoking the directory-permissions hardening (e.g., call
hardenFilePermissions(path.dirname(CONFIG_PATH)) or add/rename to a
hardenDirPermissions utility) before returning DEFAULT_CONFIG or reading the
file so directory permissions are enforced whenever the config is accessed
(similar to how saveConfig() or doctor would do).
| afterAll(() => { | ||
| if (originalConfig !== null) { | ||
| mkdirSync(CONFIG_DIR, { recursive: true }); | ||
| writeFileSync(CONFIG_PATH, originalConfig, 'utf-8'); | ||
| } else if (existsSync(CONFIG_PATH)) { | ||
| rmSync(CONFIG_PATH); | ||
| } |
There was a problem hiding this comment.
Restore secure permissions when writing back the original config.
writeFileSync may recreate the file with a default umask (often 0644), which can leave the user’s real config less secure after tests.
🛠️ Suggested fix
afterAll(() => {
if (originalConfig !== null) {
mkdirSync(CONFIG_DIR, { recursive: true });
writeFileSync(CONFIG_PATH, originalConfig, 'utf-8');
+ if (POSIX_PERMISSIONS_SUPPORTED) {
+ chmodSync(CONFIG_PATH, 0o600);
+ }
} else if (existsSync(CONFIG_PATH)) {
rmSync(CONFIG_PATH);
}
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/config.test.ts` around lines 24 - 30, The test teardown in
afterAll (using originalConfig, CONFIG_DIR, CONFIG_PATH) can leave the restored
config file world-readable; update the restore path to ensure secure permissions
by either passing a mode when writing (e.g., writeFileSync(CONFIG_PATH,
originalConfig, { encoding: 'utf-8', mode: 0o600 })) and/or calling
chmodSync(CONFIG_PATH, 0o600) immediately after writeFileSync, and also ensure
the else branch that leaves an existing CONFIG_PATH preserves/sets secure
permissions via chmodSync(CONFIG_PATH, 0o600).
| process.env.GATEWAY_AUTH_TOKEN = 'abc123'; | ||
| try { | ||
| const headers = getHeaders(); | ||
| expect(headers.Authorization).toBe('Bearer abc123'); | ||
| expect(headers['Content-Type']).toBe('application/json'); | ||
| } finally { | ||
| delete process.env.GATEWAY_AUTH_TOKEN; | ||
| } |
There was a problem hiding this comment.
Preserve any pre-existing env var to avoid test side effects.
Deleting the env var unconditionally can clobber a developer’s existing GATEWAY_AUTH_TOKEN and make tests order-dependent.
🛠️ Suggested fix
- process.env.GATEWAY_AUTH_TOKEN = 'abc123';
+ const previousToken = process.env.GATEWAY_AUTH_TOKEN;
+ process.env.GATEWAY_AUTH_TOKEN = 'abc123';
try {
const headers = getHeaders();
expect(headers.Authorization).toBe('Bearer abc123');
expect(headers['Content-Type']).toBe('application/json');
} finally {
- delete process.env.GATEWAY_AUTH_TOKEN;
+ if (previousToken === undefined) {
+ delete process.env.GATEWAY_AUTH_TOKEN;
+ } else {
+ process.env.GATEWAY_AUTH_TOKEN = previousToken;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| process.env.GATEWAY_AUTH_TOKEN = 'abc123'; | |
| try { | |
| const headers = getHeaders(); | |
| expect(headers.Authorization).toBe('Bearer abc123'); | |
| expect(headers['Content-Type']).toBe('application/json'); | |
| } finally { | |
| delete process.env.GATEWAY_AUTH_TOKEN; | |
| } | |
| const previousToken = process.env.GATEWAY_AUTH_TOKEN; | |
| process.env.GATEWAY_AUTH_TOKEN = 'abc123'; | |
| try { | |
| const headers = getHeaders(); | |
| expect(headers.Authorization).toBe('Bearer abc123'); | |
| expect(headers['Content-Type']).toBe('application/json'); | |
| } finally { | |
| if (previousToken === undefined) { | |
| delete process.env.GATEWAY_AUTH_TOKEN; | |
| } else { | |
| process.env.GATEWAY_AUTH_TOKEN = previousToken; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bridge/__tests__/api-client.test.ts` around lines 55 - 62, The test
mutates process.env.GATEWAY_AUTH_TOKEN without preserving existing state—capture
the original value before setting (e.g., const original =
process.env.GATEWAY_AUTH_TOKEN), set process.env.GATEWAY_AUTH_TOKEN = 'abc123'
for the test, then in the finally block restore it (if original is undefined
delete the env var, otherwise restore the original string); apply this change
around the getHeaders() assertions in the api-client.test.ts test that
references getHeaders to avoid clobbering a developer's environment or causing
order-dependent tests.
| import { homedir } from 'os'; | ||
| import { getConfig, saveConfig } from '../config.js'; | ||
|
|
||
| const CONFIG_DIR = join(homedir(), '.synapse'); |
There was a problem hiding this comment.
These tests are touching the real ~/.synapse/config.json path in the user home directory. That makes them stateful/flaky and risks modifying user config if the run is interrupted before afterAll cleanup.\n\nCan we isolate with a temp HOME (or injectable config path) per test run? That would keep permission checks valid while avoiding side effects on real local config.
| import { homedir } from 'os'; | ||
| import { getBridgeConfig, saveBridgeConfig } from '../config.js'; | ||
|
|
||
| const CONFIG_DIR = join(homedir(), '.synapse'); |
There was a problem hiding this comment.
Same isolation issue here: this suite writes to the real ~/.synapse/bridge.json. If tests fail/abort, local bridge config can be left modified, and parallel runs can interfere with each other.\n\nPlease consider temp HOME / temporary config root injection to make this deterministic and side-effect free.
2 participants
Summary
~/.synapse/bridge.jsonby default (env-only forGATEWAY_AUTH_TOKEN/IRONCLAW_TOKEN)bridge.jsonon read~/.synapseconfig artifacts (dir0700, files0600)Validation
bunx tsc --noEmitbun testCloses #46
Summary by CodeRabbit
Security Enhancements
Feature Changes
Improvements
Tests