Review fixes

[ScriptSmith]

No description provided.

[greptile-apps]

Greptile Summary

This PR addresses a batch of correctness and security fixes across the OAuth/command-security, JS runtime, and edit-tool layers. All changes are targeted, well-tested, and come with accurate documentation updates.

• Command-security model hardened: allowedCommands is restructured from a flat RegExp[] to a conjunction-of-groups RegExp[][]. buildSessionSecurity is extracted to its own file and fixed so that a per-client grant's patterns are ANDed with the global ceiling rather than replacing it — closing a path where a broad grant (e.g. .*) could silently widen the operator-configured restriction.
• Fail-closed scope-grants: scopesToGrant in the external JWT verifier now always returns a grant object; a token with no tools:* scopes yields { tools: [] }, which intersects to zero allowed tools instead of falling back to admin-level access. Bearer token comparisons across the codebase are upgraded to the hash-then-timingSafeEqual pattern.
• JS runtime timeout fix: vm.runInContext now receives the timeout option so synchronous busy-loops are interrupted at the first wrapping attempt, and a new isFatalEvalError predicate ensures timeout/abort errors propagate immediately rather than being retried across all wrapping strategies.

Confidence Score: 5/5

Safe to merge — all changes are targeted fixes with no regressions introduced.

Every changed code path is directly covered by new or updated tests. The security-model changes (command-group conjunction, fail-closed scope grants, timing-safe comparisons) are correctly implemented and represent improvements over the previous code. The JS timeout fix is bounded in its worst case and validated by a new test. No logic holes were found on review.

No files require special attention.

Important Files Changed

Filename	Overview
src/oauth/session-security.ts	New file: extracts buildSessionSecurity with the critical fix — command groups from global CLI and per-client grant are now ANDed, preventing a grant from widening the operator's ceiling.
src/security.ts	allowedCommands changed from RegExp[] to RegExp[][] (conjunction of disjunctions); validateCommand updated accordingly with clear JSDoc on empty-groups semantics.
src/utils.ts	Adds safeStrEqual — hash-then-timingSafeEqual pattern eliminates timing side-channels on bearer token comparison, including length leakage.
src/oauth/external-verifier.ts	scopesToGrant now always returns a grant (never null) in scope-grants mode; a token with no tools:* scopes yields { tools: [] }, implementing fail-closed behavior correctly.
src/tools/js.ts	Adds isFatalEvalError to propagate timeout/abort errors immediately through the evalAsync wrapping cascade instead of silently retrying, and passes { timeout: timeoutMs } to vm.runInContext to catch synchronous busy-loops.
src/tray/http-controller.ts	Removes buildSessionSecurity (moved to session-security.ts), uses safeStrEqual for tray auth, adds typeof guard for array Authorization headers, and correctly flattens allowedCommands for the consent-page display.
src/tools/edit.ts	Adds guard after fuzzy match to verify the matched slice round-trips through normalization, preventing silent mis-edits from positional drift.
tests/security.test.ts	New test file covering validateCommand conjunction semantics and buildSessionSecurity command narrowing scenarios.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Incoming Bearer Token] --> B{scopeGrants mode?}
    B -- No --> C[grant = null]
    B -- Yes --> D[scopesToGrant scopes]
    D --> E{Any tools:* scopes?}
    E -- Yes --> F["grant = { tools: [...] }"]
    E -- No --> G["grant = { tools: [] }\n(fail closed)"]

    C --> H[buildSessionSecurity global, null]
    F --> H
    G --> H

    H --> I{grant null?}
    I -- Yes --> J[Return global config\nminus onToolsChanged hook]
    I -- No --> K[Intersect tools\nglobal ∩ grant]
    K --> L[Filter paths\ngrant ⊆ global paths]
    L --> M[AND command groups\nglobal groups + grant group]
    M --> N[Per-session SecurityConfig]

    N --> O{validateCommand?}
    O --> P["For each group: command must\nmatch ≥1 pattern (AND of ORs)"]
    P -- Fails any group --> Q[Throw: Command not allowed]
    P -- Passes all groups --> R[Allow]

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[Incoming Bearer Token] --> B{scopeGrants mode?}
    B -- No --> C[grant = null]
    B -- Yes --> D[scopesToGrant scopes]
    D --> E{Any tools:* scopes?}
    E -- Yes --> F["grant = { tools: [...] }"]
    E -- No --> G["grant = { tools: [] }\n(fail closed)"]

    C --> H[buildSessionSecurity global, null]
    F --> H
    G --> H

    H --> I{grant null?}
    I -- Yes --> J[Return global config\nminus onToolsChanged hook]
    I -- No --> K[Intersect tools\nglobal ∩ grant]
    K --> L[Filter paths\ngrant ⊆ global paths]
    L --> M[AND command groups\nglobal groups + grant group]
    M --> N[Per-session SecurityConfig]

    N --> O{validateCommand?}
    O --> P["For each group: command must\nmatch ≥1 pattern (AND of ORs)"]
    P -- Fails any group --> Q[Throw: Command not allowed]
    P -- Passes all groups --> R[Allow]

Loading

_{Reviews (2): Last reviewed commit: "Review fixes" | Re-trigger Greptile}

[ScriptSmith]

@greptile-apps