feat(cli): add authenticated OOXML commands by caio-pizzol · Pull Request #24 · superdoc/ooxml-dev

caio-pizzol · 2026-08-12T22:09:29Z

What changed

Add a private @ooxml-dev/cli package for specification search and schema lookup.
Add browser sign-in through the existing Clerk-backed OOXML service.
Add the research-ooxml skill so agents know which commands to use and how to combine schema and specification evidence.
Build and test the CLI in CI.

Why

Agents need a stable shell interface when they cannot connect to the MCP server directly. The skill uses OOXML commands such as ooxml element and ooxml attributes; MCP remains an internal transport detail.

The package stays private while we test it. It connects only to the production OOXML service. Tokens are stored as plain text in the user's application data directory, so secure OS storage is required before npm publishing.

Verified

bun run check
bun run cli:test (13 tests)
bun run --cwd apps/cli build
Existing web, MCP, migration, and PDF tests
Production Clerk sign-in, credential reuse in a new process, ooxml element w:p, and logout
Packed artifact installation and execution with Node 20

cubic-dev-ai

All reported issues were addressed across 20 files

_{Reply with feedback, questions, or to request a fix.

Re-trigger cubic}

qodo-code-review · 2026-08-12T22:17:52Z

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📜 Skill insights (0)

🔴 High

1. ~~Plaintext MCP endpoints allowed~~ ✓ 🐞

Description

mcpUrl() accepts a remote http:// endpoint and passes it directly to the authenticated
transport. When such an endpoint is configured, authentication traffic and any available bearer
credentials can traverse an unencrypted network connection instead of being rejected.

Code

apps/cli/src/constants.ts[R6-8]

+export function mcpUrl(): URL {
+	return new URL(process.env.OOXML_MCP_URL ?? DEFAULT_MCP_URL);
+}

Relevance

●●● Strong
Consistent with prior accepted URL safety hardening; likely to enforce HTTPS or loopback-only HTTP
for auth endpoints.
PR-#15

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The endpoint parser performs only syntactic URL construction, and newTransport() uses that URL
directly with the OAuth provider. By contrast, browser authorization URLs are explicitly restricted
to HTTPS or loopback HTTP, demonstrating that the equivalent transport-safety boundary is missing
for the MCP endpoint itself.

apps/cli/src/constants.ts[6-8]
apps/cli/src/mcp-client.ts[26-27]
apps/cli/src/browser-auth.ts[7-11]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The configurable MCP endpoint permits unencrypted HTTP on non-loopback hosts, exposing authenticated traffic to network interception.

## Issue Context
Apply the same HTTPS-or-loopback policy used for authorization URLs before constructing the MCP transport. Preserve HTTP support only for explicit loopback development hosts.

## Fix Focus Areas
- apps/cli/src/constants.ts[6-8]
- apps/cli/src/mcp-client.ts[26-27]
- apps/cli/src/browser-auth.ts[7-11]

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

2. Credentials cross MCP origins 🐞

Description

CliOAuthProvider ignores the OAuth context and returns one global token/client/discovery set even
when OOXML_MCP_URL selects a different server. Switching endpoints therefore reuses credentials
for the wrong origin, causing authentication failures and making the prior origin's bearer token
available to a transport connected to the newly configured endpoint.

Code

apps/cli/src/oauth-provider.ts[R60-61]

+	tokens(_context?: OAuthClientInformationContext): StoredOAuthTokens | undefined {
+		return this.credentials.tokens;

Relevance

●● Moderate
Requires credential-store redesign keyed by server/origin; security-relevant but larger behavioral
change without close precedent.
PR-#15

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
mcpUrl() allows the endpoint to change, and connectToMcp() attaches the same unscoped
CredentialStore provider to whichever serverUrl is selected. The provider then ignores
OAuthClientInformationContext for client information and tokens and persists all OAuth material in
the single credentials file.
apps/cli/src/constants.ts[6-8]
apps/cli/src/mcp-client.ts[26-33]
apps/cli/src/oauth-provider.ts[46-70]
apps/cli/src/credentials.ts[27-29]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
OAuth credentials are stored globally and reused after the configured MCP endpoint changes, creating cross-origin authentication confusion and potential bearer-token disclosure.

## Issue Context
Use the supplied OAuth context and the normalized MCP server origin to isolate client information, discovery state, verifiers, and tokens. Existing unscoped credentials should only be migrated to the default production origin or otherwise invalidated safely.

## Fix Focus Areas
- apps/cli/src/oauth-provider.ts[46-103]
- apps/cli/src/mcp-client.ts[30-33]
- apps/cli/src/credentials.ts[10-29]

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

View medium (2)

🟠 **Medium**

3. Credential lock held through 5-min browser login 🐞 ⭐

Description

connectToMcp() acquires the exclusive credentials.json lock via CredentialStore.open() before
starting the MCP connection and OAuth flow and holds it until the client/session closes, which can
include waiting up to CALLBACK_TIMEOUT_MS=5 minutes for the browser callback. As a result, any other
concurrent ooxml invocation (including ooxml logout or queries that only need to read existing
credentials) fails with "Another OOXML command is running" because competing processes only retry
the lock for a short window.

Code

apps/cli/src/mcp-client.ts[R32-52]

+	const credentials = await new CredentialStore().open();
+	try {
+		const provider = new CliOAuthProvider(redirectUrl, credentials);
+		await provider.load();
+
+		let client = newClient();
+		let transport = newTransport(serverUrl, provider);
+		try {
+			await client.connect(transport);
+		} catch (error) {
+			if (!(error instanceof UnauthorizedError)) throw error;
+			if (!options.allowBrowser) {
+				throw new Error("You are not signed in. Run `ooxml login` first.");
+			}
+
+			await authorizeInBrowser(provider, options.callbackPort, (params) =>
+				transport.finishAuth(params),
+			);
+			client = newClient();
+			transport = newTransport(serverUrl, provider);
+			await client.connect(transport);

Relevance

●●● Strong
Team has accepted multiple concurrency/locking reliability fixes; reducing long-held credential lock
fits that pattern.
PR-#20
PR-#17

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

In apps/cli/src/mcp-client.ts, connectToMcp() opens the CredentialStore lock (line 32) and only
releases it through the close() callback returned to the caller (lines 55–64) or on error (line
66), meaning the lock remains held across the MCP connection and subsequent operations. The OAuth
path awaits authorizeInBrowser (lines 47–49), which in apps/cli/src/browser-auth.ts awaits
callback.result and can block until the browser redirects back or the 5-minute
CALLBACK_TIMEOUT_MS elapses (lines 106–113, 133), extending the lock duration.
CredentialStore.open() in apps/cli/src/credentials.ts (lines 39–50) throws “Another OOXML
command is running...” when the lock cannot be acquired, and the CLI invokes credential operations
in other commands (e.g., logout in cli.ts lines 61–69), so those concurrent invocations fail
during the long-held lock window; the credential test further corroborates that a second session
cannot open until the first closes.

apps/cli/src/browser-auth.ts[106-113]
apps/cli/src/credentials.ts[39-50]
apps/cli/src/cli.ts[61-69]
apps/cli/src/credentials.ts[35-81]
apps/cli/src/mcp-client.ts[29-67]
apps/cli/src/browser-auth.ts[106-111]
tests/cli/credentials.test.ts[24-39]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The CLI holds an exclusive credential-file lock for the full duration of `connectToMcp()`/MCP usage and the interactive OAuth login, including potentially waiting up to 5 minutes for the browser callback. This unnecessarily serializes all CLI usage on the same machine and causes concurrent commands (including ones that only need to read credentials, such as queries or `ooxml logout`) to fail with “Another OOXML command is running” once their finite lock retry window expires.

## Issue Context
`connectToMcp()` opens a credential session/lock before attempting MCP connection and the OAuth browser flow and does not release it until the session/client closes, so the lock can be held across network connections, tool execution, and the browser callback wait. The lock is implemented with `proper-lockfile` in `apps/cli/src/credentials.ts`; keep credential reads/writes race-safe without holding the lock across long-running operations, and ensure short-lived read-modify-write updates do not overwrite refreshed tokens or other credential changes.

## Fix Focus Areas
- apps/cli/src/mcp-client.ts[29-68]
- apps/cli/src/credentials.ts[35-81]
- apps/cli/src/browser-auth.ts[116-139]
- tests/cli/credentials.test.ts[24-39]

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

4. ~~Invalid callback aborts login~~ ✓ 🐞

Description

waitForCallback closes the listener on the first /callback request before the OAuth state is
validated. A stale redirect or unrelated local request with missing/wrong state consumes the
listener, so the legitimate browser redirect cannot complete the login attempt.

Code

apps/cli/src/browser-auth.ts[R63-66]

+			settled = true;
+			clearTimeout(timeout);
+			server.close();
+			resolve(url.searchParams);

Relevance

●●● Strong
Team recently accepted auth-flow race hardening; this is a clear reliability bug in login callback
handling.
PR-#15

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The callback handler accepts every request whose path is /callback, marks the operation settled,
closes the server, and resolves its parameters. State validation only occurs afterward in
authorizeInBrowser, while the predictable default port makes stale or unrelated local requests
able to reach that listener.

apps/cli/src/browser-auth.ts[42-47]
apps/cli/src/browser-auth.ts[50-67]
apps/cli/src/constants.ts[3-4]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The first callback request settles and closes the loopback server even when its OAuth state is invalid, aborting the real login callback.

## Issue Context
Validate the expected state inside the callback handler before settling. Reject or ignore invalid requests while continuing to listen until a valid callback or timeout, and only show success after validation.

## Fix Focus Areas
- apps/cli/src/browser-auth.ts[42-47]
- apps/cli/src/browser-auth.ts[50-67]

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

Context

✅ Compliance rules (platform): 11 rules

Review mode: 🚀 Fast: This is a small, localized callback-state guard with a focused concurrency test, avoiding broader security or contract changes in this push.

Tip of the day

💡 Did you know, you can type 'qodo, fix this' on a finding and the fix lands right on your PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous review results

Review updated until commit b3cd81c

Results up to commit a084f84 🧠 Deep

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 📜 Skill insights (0)

🔴 High

1. ~~Plaintext MCP endpoints allowed~~ ✓ 🐞

Description

mcpUrl() accepts a remote http:// endpoint and passes it directly to the authenticated
transport. When such an endpoint is configured, authentication traffic and any available bearer
credentials can traverse an unencrypted network connection instead of being rejected.

Code

apps/cli/src/constants.ts[R6-8]

+export function mcpUrl(): URL {
+	return new URL(process.env.OOXML_MCP_URL ?? DEFAULT_MCP_URL);
+}

Relevance

●●● Strong
Consistent with prior accepted URL safety hardening; likely to enforce HTTPS or loopback-only HTTP
for auth endpoints.
PR-#15

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The endpoint parser performs only syntactic URL construction, and newTransport() uses that URL
directly with the OAuth provider. By contrast, browser authorization URLs are explicitly restricted
to HTTPS or loopback HTTP, demonstrating that the equivalent transport-safety boundary is missing
for the MCP endpoint itself.

apps/cli/src/constants.ts[6-8]
apps/cli/src/mcp-client.ts[26-27]
apps/cli/src/browser-auth.ts[7-11]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The configurable MCP endpoint permits unencrypted HTTP on non-loopback hosts, exposing authenticated traffic to network interception.

## Issue Context
Apply the same HTTPS-or-loopback policy used for authorization URLs before constructing the MCP transport. Preserve HTTP support only for explicit loopback development hosts.

## Fix Focus Areas
- apps/cli/src/constants.ts[6-8]
- apps/cli/src/mcp-client.ts[26-27]
- apps/cli/src/browser-auth.ts[7-11]

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

2. Credentials cross MCP origins 🐞

Description

CliOAuthProvider ignores the OAuth context and returns one global token/client/discovery set even
when OOXML_MCP_URL selects a different server. Switching endpoints therefore reuses credentials
for the wrong origin, causing authentication failures and making the prior origin's bearer token
available to a transport connected to the newly configured endpoint.

Code

apps/cli/src/oauth-provider.ts[R60-61]

+	tokens(_context?: OAuthClientInformationContext): StoredOAuthTokens | undefined {
+		return this.credentials.tokens;

Relevance

●● Moderate
Requires credential-store redesign keyed by server/origin; security-relevant but larger behavioral
change without close precedent.
PR-#15

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
mcpUrl() allows the endpoint to change, and connectToMcp() attaches the same unscoped
CredentialStore provider to whichever serverUrl is selected. The provider then ignores
OAuthClientInformationContext for client information and tokens and persists all OAuth material in
the single credentials file.
apps/cli/src/constants.ts[6-8]
apps/cli/src/mcp-client.ts[26-33]
apps/cli/src/oauth-provider.ts[46-70]
apps/cli/src/credentials.ts[27-29]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
OAuth credentials are stored globally and reused after the configured MCP endpoint changes, creating cross-origin authentication confusion and potential bearer-token disclosure.

## Issue Context
Use the supplied OAuth context and the normalized MCP server origin to isolate client information, discovery state, verifiers, and tokens. Existing unscoped credentials should only be migrated to the default production origin or otherwise invalidated safely.

## Fix Focus Areas
- apps/cli/src/oauth-provider.ts[46-103]
- apps/cli/src/mcp-client.ts[30-33]
- apps/cli/src/credentials.ts[10-29]

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

View medium (1)

🟠 **Medium**

3. ~~Invalid callback aborts login~~ ✓ 🐞

Description

waitForCallback closes the listener on the first /callback request before the OAuth state is
validated. A stale redirect or unrelated local request with missing/wrong state consumes the
listener, so the legitimate browser redirect cannot complete the login attempt.

Code

apps/cli/src/browser-auth.ts[R63-66]

+			settled = true;
+			clearTimeout(timeout);
+			server.close();
+			resolve(url.searchParams);

Relevance

●●● Strong
Team recently accepted auth-flow race hardening; this is a clear reliability bug in login callback
handling.
PR-#15

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The callback handler accepts every request whose path is /callback, marks the operation settled,
closes the server, and resolves its parameters. State validation only occurs afterward in
authorizeInBrowser, while the predictable default port makes stale or unrelated local requests
able to reach that listener.

apps/cli/src/browser-auth.ts[42-47]
apps/cli/src/browser-auth.ts[50-67]
apps/cli/src/constants.ts[3-4]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The first callback request settles and closes the loopback server even when its OAuth state is invalid, aborting the real login callback.

## Issue Context
Validate the expected state inside the callback handler before settling. Reject or ignore invalid requests while continuing to listen until a valid callback or timeout, and only show success after validation.

## Fix Focus Areas
- apps/cli/src/browser-auth.ts[42-47]
- apps/cli/src/browser-auth.ts[50-67]

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

Powered by Qodo

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a084f846b7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

caio-pizzol · 2026-08-12T22:42:33Z

Reviewed all three findings. The CLI now uses only the fixed HTTPS production endpoint, which also prevents credentials from crossing server origins. Invalid callbacks no longer consume the listener. Details and test evidence are in the inline threads.

cubic-dev-ai

All reported issues were addressed across 14 files (changes from recent commits).

_{Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic}

caiopizzol added 2 commits August 12, 2026 19:01

feat(cli): add authenticated OOXML commands

ea08f65

docs(cli): clarify commands and sign-in

a084f84

cubic-dev-ai Bot reviewed Aug 12, 2026

View reviewed changes

qodo-code-review Bot reviewed Aug 12, 2026

View reviewed changes

Comment thread apps/cli/src/oauth-provider.ts

Comment thread apps/cli/src/constants.ts Outdated

chatgpt-codex-connector Bot reviewed Aug 12, 2026

View reviewed changes

Comment thread apps/cli/src/oauth-provider.ts

Comment thread apps/cli/src/browser-auth.ts Outdated

caiopizzol added 2 commits August 12, 2026 19:37

fix(cli): protect sign-in and credentials

d06d571

fix(cli): show manual sign-in URL

4407a8e

cubic-dev-ai Bot reviewed Aug 12, 2026

View reviewed changes

Comment thread apps/cli/src/browser-auth.ts

fix(cli): handle duplicate auth callbacks

b3cd81c

caio-pizzol merged commit 4c37e42 into main Aug 13, 2026
2 checks passed

caio-pizzol deleted the caio/ooxml-cli branch August 13, 2026 01:08

Navigation Menu

Uh oh!

feat(cli): add authenticated OOXML commands - #24

feat(cli): add authenticated OOXML commands#24
caio-pizzol merged 5 commits into
mainfrom
caio/ooxml-cli

caio-pizzol commented Aug 12, 2026 •

edited

Loading

Uh oh!

cubic-dev-ai Bot left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qodo-code-review Bot commented Aug 12, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

Uh oh!

caio-pizzol commented Aug 12, 2026

Uh oh!

cubic-dev-ai Bot left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

caio-pizzol commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed

Why

Verified

Uh oh!

cubic-dev-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qodo-code-review Bot commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Previous review results

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

caio-pizzol commented Aug 12, 2026

Uh oh!

cubic-dev-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

caio-pizzol commented Aug 12, 2026 •

edited

Loading

cubic-dev-ai Bot left a comment •

edited

Loading

qodo-code-review Bot commented Aug 12, 2026 •

edited

Loading

cubic-dev-ai Bot left a comment •

edited

Loading