Code Audit: 26 potential issue(s) found

Code Audit Report

All findings are reviewed for confidence before posting. Please verify each finding before acting on it.

Repository: unicity-astrid/sdk-js Findings: 26 issue(s) found — 7 high · 10 medium · 9 low

1. Potential TypeError when payload is undefined or null

Problem: The code accesses payload.length without verifying that payload is a defined object with a length property. If the host calls astridHookTrigger with payload as undefined or null, a TypeError will be thrown, crashing the component and preventing proper handling of the hook.

Suggested Fix: Add a guard to ensure payload is an object before accessing its length, e.g., const payloadSize = payload && typeof payload.length === 'number' ? payload.length : 0; and use payloadSize in the log.

2. Lifecycle hooks registered per instance causing duplicate records

Problem: Each of the @install, @upgrade, and @run method decorators registers the lifecycle hook inside an instance initializer (context.addInitializer). This initializer runs every time a capsule instance is created, so the same hook is recorded multiple times. Duplicate registrations can lead to incorrect runtime behavior (e.g., multiple install callbacks) and unnecessary performance overhead.

Suggested Fix: Move the registration logic to a static context that runs once per class, e.g., use a class decorator to collect method names or store a flag on the constructor to ensure registration occurs only on the first instance. Alternatively, check if the hook has already been recorded before calling recordInstall/recordUpgrade/recordRun.

3. bigint fields are not JSON‑serializable

Problem: The types.Usage interface defines input_tokens and output_tokens as bigint. When instances of this type are passed to JSON.stringify (common for inter‑process communication or network transport), bigint values cause a TypeError because JSON does not support the bigint type. This will lead to runtime crashes or failed message delivery wherever a Usage object is serialized.

Suggested Fix: Replace the bigint fields with a JSON‑compatible type such as string (and document that the value is a decimal string) or number if the range fits. Alternatively, implement a custom serializer/deserializer that converts bigint to string before JSON.stringify and back to bigint after parsing.

4. Potential Index Out of Bounds Error

Problem: The select function checks if the returned value is in the provided options array using the indexOf method. However, if the value is not found in the array, it will throw an error. This could potentially lead to an index out of bounds error if the value is not in the array.

Suggested Fix: Add a check to ensure the value is in the options array before attempting to access it.

5. Missing implementation of readdir function causing syntax error

Problem: The file ends with an orphaned character "e" and the comment describing the readdir API is not followed by an actual function implementation. This results in a syntax error and the readdir export is absent, breaking any code that imports it.

Suggested Fix: Remove the stray character, add the export async function readdir(path: string, options?: ReaddirOptions): Promise<string[] | Dirent[]> { ... } implementation (mirroring Node's fs.readdir behavior), and ensure the function is exported at the end of the module.

6. ReferenceError: sleepMs is not defined

Problem: The recv method calls sleepMs(50), but the module only imports hostSleepMs (aliased from ./time.js). Since sleepMs is not defined in this scope, invoking recv will throw a ReferenceError at runtime, breaking the blocking receive functionality.

Suggested Fix: Replace the call to sleepMs(50) with the imported function hostSleepMs(50) or import sleepMs directly. For example: hostSleepMs(50);

7. Missing length validation and upper bound enforcement for randomBytes

Problem: The randomBytes function claims to cap the output at 4096 bytes per call, but it does not enforce any validation on the length argument. Supplying a negative, non‑integer, or excessively large value will be passed directly to hostRandomBytes (converted to a BigInt), potentially causing host‑side memory exhaustion, errors, or undefined behavior. This violates the documented contract and opens a denial‑of‑service vector.

Suggested Fix: Validate the length argument before calling the host: ensure it is a finite positive integer, enforce length <= 4096, and throw a descriptive error if the check fails. Example:

export function randomBytes(length: number): Uint8Array {
  if (!Number.isInteger(length) || length <= 0) {
    throw SysError.api("randomBytes length must be a positive integer");
  }
  if (length > 4096) {
    throw SysError.api("randomBytes length exceeds maximum of 4096 bytes");
  }
  return callHost(`runtime.randomBytes(${length})`, () =>
    hostRandomBytes(BigInt(length)),
  );
}

8. ⚡ Repeated creation of TextDecoder and Uint8Array per request

Problem: Each invocation of the tool_execute_stub_call path creates a new TextDecoder instance and a new Uint8Array from the incoming payload. This adds unnecessary allocation overhead, especially if the stub is called frequently.

Suggested Fix: Instantiate a single TextDecoder (e.g., const decoder = new TextDecoder();) at module scope and reuse it. Also, avoid wrapping payload in new Uint8Array if it is already a Uint8Array; check its type before conversion.

9. Potential Secret Exposure

Problem: The secret function stores a secret via the kernel's SecretStore, but it does not validate the description parameter. This could potentially lead to sensitive information being stored in the description field.

Suggested Fix: Add validation for the description parameter to prevent potential secret exposure.

10. Potential information leakage in error messages

Problem: The extractWitError function builds an error message that includes the raw payload via safeStringify. If the payload contains sensitive data (e.g., user credentials, tokens), this data will be embedded in the thrown SysError message, which may be logged or exposed to callers, leading to unintended information disclosure.

Suggested Fix: Avoid embedding raw payload data in the error message. Instead, include only a generic description and store the payload separately on the SysError.payload field. For example, change the message construction to message = code; and rely on SysError.payload for detailed inspection by trusted code.

11. Async methods syncData and syncAll do not await host calls

Problem: Both syncData and syncAll are declared async but invoke callHost without await. If callHost returns a Promise (e.g., when the host operation is asynchronous), the functions will resolve immediately, potentially leading to unflushed data or race conditions.

Suggested Fix: Add await before the callHost calls in both methods, e.g., await callHost(..., () => this.#requireInner().syncData()); and similarly for syncAll.

12. Uncaught JSON parsing errors in FetchResponse.json

Problem: FetchResponse.json parses the response body with JSON.parse without a try/catch block. If the response body is not valid JSON, the method throws a raw exception, bypassing the SDK's error handling (SysError.json). This can cause uncaught errors in user code.

Suggested Fix: Wrap the JSON.parse call in a try/catch block and re‑throw a SysError.json (or a similar SDK‑specific error) to maintain consistent error handling, mirroring the implementation in Response.json.

13. Potential null value for displayName violates ResolvedUser type

Problem: The host may return null for displayName. The function forwards this value directly, but ResolvedUser.displayName is declared as string | undefined. Returning null breaks the type contract and can cause runtime errors in callers expecting undefined instead of null.

Suggested Fix: Normalize the value before returning, e.g., displayName: resp.displayName ?? undefined.

14. Exposes mutable internal array from runtimeInterceptors

Problem: The bindings function returns the array directly from runtimeInterceptors. If runtimeInterceptors provides a shared mutable array, callers of bindings can modify that array, unintentionally affecting the kernel's interceptor registry state and leading to hard‑to‑track bugs.

Suggested Fix: Return a shallow copy of the array, e.g., return [...runtimeInterceptors()]; to ensure callers cannot mutate the original internal data.

15. Zero timeout values are incorrectly rejected

Problem: The helper toHostTimeout throws an error for any timeoutMs <= 0, treating zero as invalid. However, pollAccept (and potentially other callers) use toHostTimeout(timeoutMs) ?? 0n to allow a zero timeout for non‑blocking behavior. Passing 0 will cause an exception instead of performing an immediate poll, leading to unexpected runtime errors.

Suggested Fix: Modify toHostTimeout to allow 0 as a valid timeout. Change the condition to if (!Number.isFinite(timeoutMs) || timeoutMs < 0) and ensure that 0 is converted to 0n without throwing.

16. Potential overflow when converting timeoutMs to BigInt

Problem: The wait and waitWithOutput methods convert the optional timeoutMs number to a BigInt using Math.max(0, Math.floor(timeoutMs)). If a caller provides a value larger than Number.MAX_SAFE_INTEGER, the intermediate number loses precision before being cast to BigInt, resulting in an incorrect timeout value or unexpected truncation.

Suggested Fix: Perform the conversion without losing precision by using BigInt(Math.trunc(timeoutMs)) after validating that timeoutMs is a finite integer within the safe range, or reject values that exceed Number.MAX_SAFE_INTEGER with a clear error.

17. sleepMs does not handle non-finite numbers (NaN/Infinity)

Problem: The function converts the input ms to a bigint by applying Math.max(0, Math.floor(ms)) and then BigInt. If ms is NaN or Infinity, Math.floor returns NaN, and BigInt(NaN) throws a runtime TypeError, causing the capsule to crash. This edge case is not guarded against, leading to unexpected failures.

Suggested Fix: Validate that ms is a finite number before processing. For example:

export function sleepMs(ms: number): void {
  if (!Number.isFinite(ms) || ms < 0) {
    throw new RangeError('sleepMs requires a finite non‑negative number');
  }
  const ns = BigInt(Math.floor(ms)) * 1_000_000n;
  callHost(`time.sleepMs(${ms})`, () => hostSleepNs(ns));
}

18. Potential undefined return value from check()

Problem: The function returns resp.allowed directly. If the host call fails to provide an allowed property (e.g., due to a malformed response or future API change), the function could return undefined instead of a boolean, violating its declared return type and possibly causing downstream type errors.

Suggested Fix: Validate the response before returning, e.g., return Boolean(resp && resp.allowed); or throw a clear error if the property is missing.

19. ⚡ Potential Performance Issue

Problem: The array function returns an array of strings, but it does not validate the length of the array. This could potentially lead to performance issues if the array is very large.

Suggested Fix: Add validation for the length of the array to prevent potential performance issues.

20. Reuse TextEncoder instance for better performance

Problem: The getBytes function creates a new TextEncoder on each call, which incurs unnecessary object allocation and garbage collection overhead. Reusing a single TextEncoder instance is more efficient, especially if getBytes is called frequently.

Suggested Fix: Define a module‑level constant const encoder = new TextEncoder(); and use encoder.encode(get(key)) inside getBytes.

21. ⚡ Repeated allocation of status text map

Problem: The httpStatusText function creates a new Record<number, string> map on every call, causing unnecessary allocations and garbage collection overhead for a static mapping.

Suggested Fix: Define the status-to-text map as a module‑level constant (e.g., const HTTP_STATUS_TEXT: Record<number, string> = { ... }) and have httpStatusText simply look up the code in that constant.

22. ⚡ Unnecessary object recreation in listLinks

Problem: The function maps each link to a new object with identical property names, adding overhead for large link sets. This extra allocation is unnecessary because the host already returns objects matching the Link interface.

Suggested Fix: Return the host result directly: return callHost(...); or cast the result to Link[] if needed.

23. Improper string escaping in log helper

Problem: The quote function only escapes double‑quote characters. Keys containing backslashes, newlines, or other control characters are not escaped, producing malformed log strings passed to callHost. While this does not affect the actual host calls, it can corrupt debugging output and may be exploited for log injection.

Suggested Fix: Escape backslashes and control characters (e.g., using JSON.stringify or a proper escaping routine). For example: return JSON.stringify(s);

24. Missing validation for limit parameter

Problem: listKeysPage forwards the limit argument directly to the host without checking that it is non‑negative. Supplying a negative value could cause the host to reject the request or behave unexpectedly, leading to runtime errors for capsule code.

Suggested Fix: Validate limit before calling the host, e.g., if (limit < 0) throw new SysError.invalidArgument('limit must be >= 0');

25. Potential performance overhead from JSON.stringify on large objects

Problem: The format function serializes every non‑string, non‑Error value using JSON.stringify, which can be expensive for large or deeply nested objects and may block the event loop. This overhead can degrade performance especially in high‑frequency logging scenarios.

Suggested Fix: Add a size or depth check before calling JSON.stringify, allow callers to pass pre‑formatted strings, or provide a lazy serialization option (e.g., defer JSON.stringify until the host actually needs the string) to avoid unnecessary work.

26. Assuming Symbol.dispose exists