Skip to content

PRO-1042: preserve opaque MCP credentials - #2

Closed
rares04 wants to merge 2 commits into
mainfrom
rares/pro-1042-mcp-credential-resolution
Closed

PRO-1042: preserve opaque MCP credentials#2
rares04 wants to merge 2 commits into
mainfrom
rares/pro-1042-mcp-credential-resolution

Conversation

@rares04

@rares04 rares04 commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Summary

  • prove dotted typed and non-dotted legacy credentials pass through unchanged from every supported source
  • document local CLI, environment, and .env setup without ever asking for a secret in a model conversation
  • preserve the existing source precedence and all credential issuance behavior

Verification

  • Full Jest suite: 9 suites, 91 tests passed.
  • Build passed.
  • Typecheck is not runnable because this JavaScript-only package tsconfig includes no TypeScript inputs.
  • Lint is not runnable because ESLint is not installed or declared by this package.

Base: c815499

@rares04
rares04 requested review from richard-lua August 29, 2026 13:03
richard-lua
richard-lua previously approved these changes Aug 29, 2026

@richard-lua richard-lua left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code review — LOW risk

Documentation- and test-only change: adds parametrized tests proving legacy (lk_...) and dotted typed (api_...) credentials pass through resolveApiKey and the Authorization header unchanged, and rewrites README/llms-install guidance to stop instructing users to paste secrets into a model conversation. No source logic changes; the new tests match the existing behavior in src/auth.mjs and src/api-client.mjs. Safe to merge.

The doc change to LUA_API_KEY: "${env:LUA_API_KEY}" and the "never paste a credential into an MCP request or model conversation" guidance are a genuine improvement to the install instructions' security posture.

Minor

  • tests/api-client.test.mjs:49 — The new test.each assigns process.env.LUA_API_KEY in the test body and depends on the suite afterEach to clean up. This works today but is slightly fragile versus the beforeEach convention used elsewhere. Optionally capture/restore the original value within the test.
  • tests/auth.test.mjs:153 — The typed fixture exercises ., _, - but nothing that starts with {, so the credentials-file tier's JSON-detection branch (raw.startsWith('{')) is not exercised for an opaque value. Consider adding a brace-leading fixture to lock in that such a credential is not silently JSON-parsed.

No security or correctness issues found.


PR Risk Reviewer — automated senior review of 6073f26 · risk: low · confidence: 0.90

@rares04

rares04 commented Aug 29, 2026

Copy link
Copy Markdown
Contributor Author

Review follow-up on the current head:\n- Removed the unsupported Cline environment interpolation example; Cline treats it literally. The copy-paste-safe config now relies on lua auth configure and the global credentials file.\n- Updated the new legacy fixtures to the actual supported non-dotted api_<32 hex> form.\n- Source precedence and runtime behavior remain unchanged; full Jest and build are green.\n\nCurrent head: b17bcb2.

@rares04

rares04 commented Aug 29, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #3, which carried the same credential/docs tests plus a real GitHub CI gate. #3 received fresh Richard and Stefan LOW approvals, passed test/build CI, and merged as b97a9ac. Closing this stale-review copy without merging.

@rares04 rares04 closed this Aug 29, 2026
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