Refactor radius-proxy into standalone ts-radius-client

[essinghigh]

Extracted the RADIUS client implementation into a standalone TypeScript library.

• Moved core logic to src/protocol.ts and src/client.ts.
• Removed all web/proxy related code.
• Implemented RadiusClient class for managing failover and health checks.
• Refactored to use passed configuration object instead of global env vars.
• Added tests using bun test.
• Configured project for direct GitHub installation via Bun.

---

PR created automatically by Jules for task 16941793293977832270 started by @essinghigh

[google-labs-jules]

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.

---

For security, I will only act on instructions from the user who triggered this task.

[opencode-agent]

Code Review Summary

Critical Issues

1. Missing type dependencies (package.json:13-14): No @types/node dependency. This is required for Node.js globals (Buffer, setTimeout, dgram, etc.) and will cause runtime errors.

2. verbatimModuleSyntax violations: Type imports must use import type syntax:

   • src/client.ts:2 - RadiusConfig, RadiusResult, Logger
   • src/protocol.ts:3 - Logger, RadiusResult, RadiusProtocolOptions
   • tests/client.test.ts:3 - RadiusConfig, RadiusResult

3. tsconfig.json missing Node.js types: Should add "types": ["node"] or include "DOM" in lib for console support.

Logic Bugs

4. src/client.ts:203 - Incorrect failure counting: When health probe receives Access-Reject (expected with invalid creds), consecutiveFailures is incremented. This is semantically wrong - invalid credentials are expected to fail, so they shouldn't count toward failure tracking.

5. src/client.ts:97-100 - Race condition: fastFailoverSequence checks if (this.inProgress) before setting it to true, creating a race window where concurrent calls could both proceed.

6. src/client.ts:150 - Public failover() doesn't check inProgress: Unlike fastFailoverSequence, the public failover() method can be called concurrently, potentially causing issues.

Security Concerns

7. src/client.ts:169-170 - Hardcoded default credentials: Default health check credentials (grafana_dummy_user/dummy_password) are hardcoded. These should be required or explicitly documented as insecure defaults.

8. src/protocol.ts:98 - Empty secret handling: When secret is empty/undefined, response authenticator verification uses empty buffer, potentially allowing spoofed responses.

Suggested Fixes

// src/client.ts:169-170 - Make these required or throw
const hcUser = this.config.healthCheckUser;
const hcPass = this.config.healthCheckPassword;
if (!hcUser || !hcPass) throw new Error('Health check credentials required');

// src/client.ts:203 - Don't count expected rejections as failures  
// Remove: entry.consecutiveFailures++;

// src/client.ts:97 - Set inProgress before the check
private async fastFailoverSequence(): Promise<string | null> {
  if (this.inProgress) return this.activeHost;
  this.inProgress = true;  // Move here
  // ...
}

// package.json - Add @types/node
"@types/node": "^20"

github run

[opencode-agent]

Code Review Summary

Verified Issues

1. src/protocol.ts:98 - Weak empty secret handling: When secret is empty/undefined, Buffer.from(secret || "", "utf8") uses an empty buffer, severely weakening response authenticator verification. This allows trivial spoofing of RADIUS responses.

2. src/client.ts:178-179 - Hardcoded default credentials: Defaults grafana_dummy_user/dummy_password are documented in types but still a security concern for production use. Consider throwing if not explicitly provided.

3. src/client.ts:100-102 - Minor race condition: The check if (this.inProgress) happens before setting this.inProgress = true, creating a theoretical race window. While minimal in practice, a mutex pattern would be safer.

Issues from Original Review That Are Actually OK

• @types/node: Present in devDependencies (package.json:15)
• verbatimModuleSyntax: Type imports are correct in all files
• failover() inProgress check: Present at client.ts:154
• Health check failure counting: Already correctly handled at client.ts:210-215

Minor Suggestions

• tsconfig.json: Consider adding "types": ["bun-types"] or "node" for broader compatibility
• src/client.ts:22-24: The secret validation throws but RadiusProtocolOptions.secret in types.ts is not marked as required (secret: string vs secret?: string). Either make it optional in the type or remove the runtime check.

Summary

The refactoring is solid overall. The main actionable items are the empty secret handling (security) and the hardcoded credentials (should required or documented as be insecure defaults).

github run

[opencode-agent]

Code Review Summary

Critical Issues

1. src/types.ts:40-53 - Type/runtime mismatch for health check credentials: The type definition shows defaults in comments (default: grafana_dummy_user) but the constructor (client.ts:25-27) throws if they're missing. Either remove the comments or make them required in the type.

2. src/client.ts:100 - Unsafe fallback chain: return this.hosts[0] || this.config.host - if both are undefined, returns undefined. Should throw or return null.

3. src/client.ts:78-80 - Config reload doesn't clear removed hosts: When config changes, old hosts remain in health Map. Could cause memory leak if hosts are frequently reconfigured.

Logic Bugs

4. src/client.ts:128-133 - Health check interval has minimum of 5000ms: The Math.max(5000, intervalMs) means intervals under 5 seconds are silently increased. Users expecting fast health checks will be surprised.

Security Concerns

5. src/protocol.ts:101 - Empty secret fallback: While protocol.ts:16-18 throws for empty secret, the code at line 101 still uses secret || "" as a fallback. This is defensive but could mask issues.

Code Quality

6. tsconfig.json:7 - Unnecessary jsx option: Since this is a library with no React components, "jsx": "react-jsx" is unnecessary and increases build complexity.

7. src/client.ts:17-18 - Type inference for interval handle: ReturnType<typeof setInterval> works but Bun.Timer or just any would be simpler for Bun-first code.

Minor Issues

8. tests/client.test.ts:97-98 - Magic number: await new Promise(r => setTimeout(r, 50)) - the timeout value should be a constant matching the test config.

9. No index export: The src/index.ts only exports from ./client, but users may expect ./types or ./protocol to be available.

Positive Observations

• Proper use of verbatimModuleSyntax with type imports
• Good error handling in protocol layer
• Health check correctly treats Access-Reject as success (server is alive)
• Failover logic properly excludes current active host
• Constructor validates required fields

Recommended Priority Fixes

1. Fix the unsafe host fallback in client.ts:100
2. Update tsconfig to remove unnecessary jsx option
3. Either document or remove the "default" comments in types.ts since they're not actually defaults

github run

[opencode-agent]

Code Review Summary

Critical Issues

1. README/Code Mismatch (docs vs implementation):

   • README.md:65 documents healthCheckUser default as grafana_dummy_user
   • client.ts:25-27 throws if healthCheckUser or healthCheckPassword are missing
   • The config table at README.md:65 is missing the healthCheckPassword field entirely
   • Fix: Update README to reflect required fields, or make them optional with defaults in code

2. Incorrect Installation Command:

   • README.md:19 shows bun add github:essinghigh-org/ts-radius-client
   • The repository is essinghigh-org/ts-radius, not ts-radius-client
   • Fix: Change to bun add github:essinghigh-org/ts-radius

Minor Issues

3. Race condition in failover (low risk):

   • client.ts:117-119 - The check if (this.inProgress) happens before setting this.inProgress = true
   • Concurrent calls could both pass the check in theory, though unlikely in practice
   • Suggested: Set inProgress = true before the check

4. tsconfig.json - Bun types:

   • No "types": ["bun-types"] specified, which could cause issues with console.debug in types.ts:10
   • Works with Bun but could fail in other environments

5. package.json - Missing description fields:

   • Missing repository, keywords, author fields for npm publishing

Code Quality (Good)

• Proper verbatimModuleSyntax usage with type imports
• Good error handling in protocol layer
• Health check correctly treats Access-Reject as success
• Failover logic properly excludes current active host
• Constructor validates required fields appropriately
• Attribute extraction handles both regular attributes and VSAs correctly
• Message-Authenticator is properly computed and inserted

Summary

The core implementation is solid. The main issues are documentation mismatches and the incorrect installation URL in the README.

github run

[opencode-agent]

Code Review Summary

Critical Issues

1. Race condition in failover sequence (src/client.ts:117-118)

if (this.inProgress) return this.activeHost;
this.inProgress = true;

The check happens before setting inProgress, allowing concurrent calls to both pass the check. Fix: Move assignment before the check.

2. Missing export for protocol layer (src/index.ts)
Users cannot access radiusAuthenticate directly for custom use cases. Consider exporting from ./protocol as well.

Logic Bugs

3. Health check interval validation missing (src/client.ts:143)
The code uses healthCheckIntervalMs directly without enforcing a minimum. Users expecting fast health checks (< 5s) will be surprised by the behavior.

4. Test mock doesn't handle Access-Reject (tests/client.test.ts:10-16)
The mock always returns { ok: true } when host is responsive. It doesn't test the important case where health probes receive Access-Reject (which should count as success since server is alive).

Security Concerns

5. ConsoleLogger uses console.debug (src/types.ts:10)
console.debug is not available in Node.js by default without --inspect flag. Should use console.log for broader compatibility.

Code Quality

6. Unnecessary tsconfig options (tsconfig.json:8)

• allowJs: true - unnecessary for pure TS library
• paths with @/* alias - defined but never used

7. Type/runtime mismatch (src/types.ts:50-52)
The comments say "(Required)" but the property is not marked with ?: string vs string. The implementation correctly throws if missing, but the type could be clearer.

Minor Issues

8. Missing error propagation (src/client.ts:64-65)
onAuthTimeout() errors are caught and logged but not surfaced to caller. Consider emitting events or using a callback.

---

The core implementation is solid. Priority fixes: #1 (race condition), #4 (test coverage), #5 (ConsoleLogger compatibility).

github run

[opencode-agent]

Code Review Summary

Overall Assessment

The refactoring is well-executed. Core RADIUS protocol implementation is solid, failover logic is sound, and health checks correctly treat Access-Reject as "server alive."

Verified - No Issues

• Type imports: Correctly use import type syntax (client.ts:2, protocol.ts:3, types.ts:1-6)
• Health check logic: Properly treats Access-Reject as success (protocol.ts:227-233)
• Authenticator verification: Correctly implemented per RFC2865 (protocol.ts:90-108)
• Failover with inProgress check: Present at client.ts:171
• README/Code alignment: Both document healthCheckUser/healthCheckPassword as required
• tsconfig: Properly configured for Bun with bun-types

Minor Issues Found

1. Race condition (low risk) - client.ts:117-118

if (this.inProgress) return this.activeHost;
this.inProgress = true;

The check happens before assignment. While unlikely in practice due to JS single-threaded nature, moving the assignment before the check would be safer:

this.inProgress = true;
if (this.inProgress) return this.activeHost; // always false on first pass

2. Test timing assumption - client.test.ts:106

await new Promise(r => setTimeout(r, config.healthCheckTimeoutMs! + 50));

Using a magic number (50ms buffer). Consider extracting as a constant.

3. Error swallowing - client.ts:64-65

this.onAuthTimeout().catch(e => this.logger.warn('[radius-client] onAuthTimeout error', e));

Background failover errors are logged but not surfaced. Consider emitting events for monitoring.

Positive Observations

• Clean separation of protocol layer from client logic
• Good attribute parsing with VSA support
• Message-Authenticator properly computed before send
• Constructor validates required fields appropriately
• Health Map properly cleans up removed hosts (client.ts:93-97)

Recommendation

The code is production-ready. The minor race condition (#1) could be fixed, but it's low priority since JavaScript's event loop makes it theoretical rather than practical.

github run