Skip to content

fix(core): validate broker sessions before sensitive operations - #125

Open
mauricemohr88-debug wants to merge 2 commits into
CALLE-AI:mainfrom
mauricemohr88-debug:fix/validate-broker-session-fields
Open

fix(core): validate broker sessions before sensitive operations#125
mauricemohr88-debug wants to merge 2 commits into
CALLE-AI:mainfrom
mauricemohr88-debug:fix/validate-broker-session-fields

Conversation

@mauricemohr88-debug

@mauricemohr88-debug mauricemohr88-debug commented Sep 10, 2026

Copy link
Copy Markdown

Malformed broker responses or cached pending logins could reach the cache, browser opener, request path, and session-secret header. This change validates the required fields before those operations and rejects unsafe persisted sessions before reuse.

Login URLs must use a configured broker or auth origin, with HTTPS required except for an exact configured loopback HTTP origin. Credentials, control characters, and overlong values are rejected. Session IDs are bounded opaque path segments, reject . and .., and are URL-encoded in status/exchange requests. Secrets are bounded printable ASCII suitable for the HTTP header. Direct normalizePendingSession(payload) calls retain the existing default CALL-E origin; an optional config supplies custom trusted origins, and an explicitly empty config fails closed.

The origin rule uses existing brokerBaseUrl/authBaseUrl configuration and preserves configured IPv4/IPv6 loopback development, following the transport direction of #47 without allowing remote brokers to redirect to arbitrary local services. Pending-session reuse from #122 remains supported and is covered by regression tests.

Validation:

  • Hostile response/cache regressions failed before the fix and pass afterward; tests cover cache writes, stderr/browser opening, request paths, and headers.
  • Full pnpm test, pnpm check, and pnpm pack:dry-run passed. The Windows-only cmd.exe case is skipped on macOS.
  • Python OAuth example: 8 tests passed with Python 3.12.14.
  • git diff --check passed; independent Codex review completed.

Tests used isolated local fixtures with private-file access and external network access blocked. Python dependencies were installed as wheels into a separate task directory. No real accounts or phone calls were used. Required hosted CI still needs maintainer approval and execution on the updated head; local results are not presented as a successful GitHub CI run.

Release decision: patch release recommended for @call-e/core; the existing patch changeset is updated. No package versions were changed. Prepared and independently reviewed with Codex AI assistance; no human QA is claimed.

@Ray-56 Ray-56 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Requesting changes on the exact current head (a1d014a).

[P1] Validate broker session fields for their security-sensitive sinks, not only for presence. requiredSessionField accepts every nonblank string unchanged. I verified this head accepts login_url: "javascript:alert(1)" and an OSC-bearing URL, a path-shaping session_id: "../exchange?x=", and session_secret: "ok\r\nX-Evil: 1". Those values are then cached; login_url is printed and passed to the OS URL opener, session_id is interpolated into broker request paths, and session_secret becomes an HTTP header. A compromised/misconfigured or user-selected broker can therefore trigger an arbitrary registered URL handler or terminal-control output, reshape follow-up requests, or turn header parsing into a denial of login. The new tests cover only missing/null/number/blank values and do not exercise any sink.

Please validate each field according to its use before writing the pending cache: parse login_url, require an allowed HTTP(S) scheme and the repository's intended trusted broker/auth origin policy, reject credentials/control characters and unreasonable lengths; constrain and bound the opaque session id and URL-encode it when building paths; reject control characters and unreasonable lengths in the header value. Add regressions proving hostile values are neither cached, printed/opened, nor sent in a request/header. Coordinate the trusted-origin behavior with #122 and the overlapping URL-hardening work in #47 rather than introducing a conflicting policy.

GitHub CI for this head is action_required with zero jobs. Because this changes authentication and token/session handling, the low-risk zero-job exception does not apply; a real required CI run must pass after the fix.

Release decision: Patch release recommended for @call-e/core; the included patch changeset is otherwise appropriate.

@mauricemohr88-debug mauricemohr88-debug changed the title fix(core): validate required broker session fields fix(core): validate broker sessions before sensitive operations Sep 11, 2026
@mauricemohr88-debug

Copy link
Copy Markdown
Author

Addressed the requested broker-field validation and regression coverage in the updated head. The trusted origins come from the existing broker/auth configuration; the HTTP loopback exception remains bound to an explicitly configured origin. This preserves the development direction of #47 and pending-session reuse from #122. Unsafe cached sessions, dot-only path IDs, terminal controls, and non-header-safe secrets are covered as well.

Full local tests, checks, package dry-runs, and all 8 Python example tests passed in isolation. The Windows-only test remains unverified locally. The patch changeset and PR description are updated.

@Ray-56, could you review the updated head and approve its required GitHub CI run? My contributor account has no upstream CI approval permission. I will address any actual job failures once the jobs can run; I am not treating the current zero-job state as a pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants