Skip to content

Implement config saving logic in gateway server #5

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.

Sign up for GitHub

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

Open
Ayush-Vish wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Implement config saving logic in gateway server #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions DESIGN_CHOICES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# Design Choices

## Configuration Persistence
- **Choice**: Persist `ScholarConfig` to `scholar.json` in the project root.
- **Rationale**: The CLI `init` command description explicitly mentions initializing `scholar.json`. Saving it in the project root ensures it's easily discoverable and follows standard Node.js practices for local configuration.
- **Implementation**: Used `node:fs/promises`'s `writeFile` in the `GatewayServer`'s `init_config` request handler. This ensures that the response and event broadcast only happen after a successful write, maintaining consistency.

## Error Handling in WebSocket Requests
- **Choice**: Use `.then().catch()` for asynchronous file operations within the request handler.
- **Rationale**: This prevents the server from hanging if an error occurs during file I/O and allows sending a meaningful error response to the client.

Copilot AI Apr 2, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rationale says using .then().catch() "prevents the server from hanging" during file I/O. The server/event loop won't inherently hang with async/await either; the key benefit here is ensuring errors are handled and a response is sent back. Consider rewording this to focus on handled rejections / returning an error response rather than implying .then().catch() prevents hangs.

Suggested change
- **Rationale**: This prevents the server from hanging if an error occurs during file I/O and allows sending a meaningful error response to the client.
- **Rationale**: This ensures that promise rejections from file I/O are explicitly handled so the server can always send a meaningful error response to the client instead of leaving the request unresolved.

Copilot uses AI. Check for mistakes.
22 changes: 17 additions & 5 deletions src/daemon/gateway/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { WebSocketServer, WebSocket } from 'ws';
import { writeFile } from 'node:fs/promises';
import { createResponse, createEvent } from '../../core/protocol.js';
import type { ProtocolMessage, RequestMessage } from '../../core/protocol.js';

Expand Down Expand Up @@ -49,11 +50,16 @@ export class GatewayServer {
break;
case 'init_config':
console.log('Received config init payload:', request.payload);
// Implementation for saving the config logic would go here
ws.send(JSON.stringify(createResponse(request.id, true, { status: 'saved' })));

// Broadcast an event to all connected clients that config changed
this.broadcastEvent('config_updated', { version: request.payload?.version || '1.0' });
this.saveConfig(request.payload)
.then(() => {
ws.send(JSON.stringify(createResponse(request.id, true, { status: 'saved' })));
// Broadcast an event to all connected clients that config changed
this.broadcastEvent('config_updated', { version: request.payload?.version || '1.0' });
Comment on lines 51 to +57

Copilot AI Apr 2, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

request.payload is optional per RequestMessage, but saveConfig assumes it can always be stringified and written. If payload is missing/invalid, JSON.stringify can produce undefined and writeFile will throw a TypeError. Consider validating request.payload (and ideally its shape) before calling saveConfig, and return a clear client-facing error like "init_config requires a config payload".

Copilot uses AI. Check for mistakes.
})
.catch((err) => {
console.error('Failed to save config:', err);
ws.send(JSON.stringify(createResponse(request.id, false, undefined, `Failed to save config: ${err.message}`)));
});
Comment on lines 51 to +62

@qodo-code-review qodo-code-review Bot Apr 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Unauthenticated config overwrite 🐞 Bug ⛨ Security

init_config persists client-provided payloads to scholar.json without any
authentication/authorization, so any connected client can overwrite local configuration (including
secret fields like API keys/tokens). This enables unauthorized configuration tampering via the
Gateway WebSocket.
Agent Prompt 
## Issue description
`init_config` writes arbitrary client-provided data to `scholar.json` without any authentication/authorization. Any connected WebSocket client can therefore persist attacker-controlled config (including credentials), which is a security vulnerability.

## Issue Context
The Gateway currently accepts connections and processes requests without identifying the caller. The `ScholarConfig` schema includes sensitive fields (API keys/tokens), increasing the impact of an unauthorized overwrite.

## Fix Focus Areas
- src/daemon/gateway/server.ts[10-33]
- src/daemon/gateway/server.ts[44-67]
- src/daemon/gateway/server.ts[83-87]

## Suggested fix (implementation guidance)
- Restrict exposure by binding the server to loopback only (e.g., set `host: '127.0.0.1'` / `localhost`) and/or validating the remote address.
- Add an authentication mechanism for mutating commands (at minimum `init_config`), e.g.:
  - Require a pre-shared token from env (`SCHOLAR_DAEMON_TOKEN`) and validate it in the request payload/headers before calling `saveConfig`.
  - Optionally enforce an Origin/Host allowlist if used from browsers.
- Validate that `request.payload` conforms to `ScholarConfig` before persisting.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment on lines +53 to +62

@qodo-code-review qodo-code-review Bot Apr 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

2. Send on closed socket 🐞 Bug ☼ Reliability

The init_config handler calls ws.send() after an async file write without checking
ws.readyState, so a client disconnect during the write can make ws.send() throw. The error-path
ws.send() in .catch() is also unguarded, which can lead to an unhandled rejection and
potentially crash the server.
Agent Prompt 
## Issue description
`ws.send()` is called in the `init_config` `.then()` and `.catch()` without verifying the socket is still open. Because the send happens after an awaited file write, the client may have disconnected, causing `ws.send()` to throw; the `.catch()` send is currently unprotected by any outer error handler.

## Issue Context
`broadcastEvent()` already guards sends with `client.readyState === WebSocket.OPEN`, but `init_config` responses do not.

## Fix Focus Areas
- src/daemon/gateway/server.ts[51-63]
- src/daemon/gateway/server.ts[69-77]

## Suggested fix (implementation guidance)
- Before calling `ws.send`, check `ws.readyState === WebSocket.OPEN`.
- Wrap `ws.send` calls in `try/catch`, or use the callback form `ws.send(data, (err) => ...)` and log errors.
- Consider centralizing send logic into a helper like `safeSend(ws, msg)` to avoid repeating this pattern.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment on lines +53 to +62

Copilot AI Apr 2, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the async init_config flow, ws.send(...) is called inside the Promise callbacks without checking ws.readyState. If the client disconnects before the write completes, ws.send can throw, and the .catch handler may also throw when trying to send the error response. Consider guarding sends with if (ws.readyState === WebSocket.OPEN) (similar to broadcastEvent) or wrapping sends in a try/catch.

Copilot uses AI. Check for mistakes.
break;
default:
ws.send(JSON.stringify(createResponse(request.id, false, undefined, 'Unknown command')));
Expand All @@ -73,6 +79,12 @@ export class GatewayServer {
public stop() {
this.wss.close();
}

private async saveConfig(config: any) {
const configPath = './scholar.json';
await writeFile(configPath, JSON.stringify(config, null, 4), 'utf-8');
console.log(`Config saved to ${configPath}`);
Comment on lines +83 to +86

Copilot AI Apr 2, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

configPath is a relative path, so the daemon will write scholar.json relative to the process working directory, which may not be the project root (e.g., when started via a service manager). Consider resolving an absolute path for the intended root (e.g., via path.resolve(...) from a known base) to match the stated behavior.

Copilot uses AI. Check for mistakes.
Comment on lines +84 to +86

Copilot AI Apr 2, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Writing directly to scholar.json with writeFile risks leaving a partially-written/truncated config if the process crashes mid-write. For a config file that controls startup/runtime behavior, consider an atomic write strategy (write to a temp file in the same directory, then rename it over the target).

Copilot uses AI. Check for mistakes.
}
}

// Support running the daemon directly
Expand Down
Loading