SEC-2309: sync fork with upstream (resolves form-data CVE) - #2
Open
himynameisang wants to merge 37 commits into
Open
SEC-2309: sync fork with upstream (resolves form-data CVE)#2himynameisang wants to merge 37 commits into
himynameisang wants to merge 37 commits into
Conversation
I've implemented new ways to create, read, update, and delete questions (cards) and dashboards by interacting with the Metabase API. This includes input validation, error handling, and logging for these new operations, consistent with the existing server structure. Unit tests using Jest have been added for all new CRUD handlers. NOTE: I could not run the tests due to an environment-specific npm install failure (E401 Unauthorized for Sonatype Nexus). You should run the tests manually after resolving the environment issue.
This commit introduces API key authentication as the preferred method for the Metabase MCP server, enhancing security and flexibility.
Changes include:
1. **Core Logic (`src/index.ts`):**
* Added support for a new `METABASE_API_KEY` environment variable.
* The server now attempts to authenticate using the API key if provided.
* If `METABASE_API_KEY` is not found, it falls back to the existing username/password authentication (`METABASE_USERNAME`, `METABASE_PASSWORD`).
* Updated startup checks to ensure that either an API key or username/password credentials are provided.
* Included logging to indicate the active authentication method.
2. **Documentation (`README.md`):**
* Updated the "Configuration" section to include `METABASE_API_KEY` and explain the new authentication hierarchy.
* Revised examples in "Configuration" and "Installation" (for `claude_desktop_config.json`) to reflect the API key option.
* Added a comprehensive "Testing" section with manual test cases for:
* API key authentication.
* Username/password fallback.
* Authentication failure scenarios (invalid key, invalid credentials, missing credentials).
This change allows you to connect to Metabase using API keys, which is often a more secure and manageable approach than using direct username/password credentials, especially in automated or production environments.
feat: Implement API key authentication for Metabase integration
feat: Add CRUD operations for Metabase questions and dashboards
This commit addresses a Severity.HIGH severity npm-CWE-918 detected by npm_audit. CWE: CWE-918 Fix details: - Version 1.8.2 is the first version after the vulnerability (GHSA-jr5f-v2jv-69x6) - Uses the caret (^) to maintain semantic versioning compatibility - Directly addresses the high-severity vulnerability - Minimal version bump to ensure security fix with lowest risk of breaking changes --- Generated by Waclaude Security Scanner https://waclaude.com
…-20250623-030557 🔴 Fix HIGH severity npm-CWE-918
…ignore-fix docs(readme):add installation oneliner and add mising gitignore
…cumentation - Add get_card tool to retrieve a single card by ID with full details - Enhance update_card description with comprehensive documentation for template-tags configuration (dropdowns, filters, variables) - Include detailed schema for dataset_query.native.template-tags - Add various dashboard and collection management tools
- Add create_user, update_user, disable_user, get_user tools - Add remove_user_from_group tool - Upgrade @modelcontextprotocol/sdk from 0.6.1 to 1.25.2 - Add 30-second axios timeout to prevent hanging requests
feat: add get_card tool and enhance update_card with template-tags documentation
…ers-and-dashboard-cards Fix execute_card parameters normalisation and dashboard cards parsing
Add MseeP.ai badge
Optimize output for more efficient context usage
- Add installation instructions for Claude Code CLI - Include troubleshooting section for common MCP issues - Provide example configuration for ~/.claude.json
- Change package name from metabase-server to scoped @imlewc/metabase-server - Update package description for better npm discoverability - Remove private flag to allow publishing
- Update formatDashboardCards function to accept an optional tabs parameter for improved output. - Include dashboard tab information in the formatted card list. - Add new list_dashboard_tabs functionality to retrieve and display dashboard tabs. - Modify MetabaseServer methods to handle dashboard_tab_id for card management.
update_dashboard_cards inputSchema was missing `items: {}` in `properties.parameter_mappings.items.properties.target`
LLMs with stricter tool use schema validation (like openai GPT) were failing due to this.
Add missing items spec to update_dashboard_cards prop array
feat: enhance dashboard card formatting and add tab support
…url-links-for-llm feat: return related Metabase URL links in CRUD responses
- Bumps form-data 4.0.2 → 4.0.6 via npm update (resolves ORCA-8368351) - Switches Dockerfile from npm install to npm ci so future builds fail fast if the lock file is ever missing or inconsistent Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
| Infrastructure as Code | View in Orca | ||
| SAST | View in Orca | ||
| Secrets | View in Orca | ||
| Vulnerabilities | View in Orca |
There was a problem hiding this comment.
Pull request overview
This PR addresses a security finding by updating the transitive form-data dependency to a patched release and tightening Docker build reproducibility by using npm ci instead of npm install.
Changes:
- Updated
form-datafrom4.0.2to4.0.6inpackage-lock.json(including associated dependency updates likehasownandmime-types). - Changed the Docker build dependency install step from
npm installtonpm cito enforce lockfile consistency during image builds.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| package-lock.json | Bumps form-data to 4.0.6 (and updates related transitive deps) to address CVE-2025-7501. |
| Dockerfile | Switches dependency installation to npm ci to ensure deterministic installs and fail fast on lockfile issues. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Adds overrides.form-data >=4.0.5 to package.json for consistency with zendesk-mcp-server and to guard against future npm update runs re-resolving below the patched floor. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Bounds the major version to avoid resolving into a future 5.x release. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Cleaner fix: bump axios ^1.8.1 → ^1.14.0 so form-data 4.0.6 flows in via axios's own ^4.0.5 constraint. Removes the overrides block. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…-data # Conflicts: # .gitignore # package-lock.json # package.json
Keeps axios at ^1.14.0 (this branch's earlier fix) rather than upstream's ^1.8.2 — axios 1.8.2 only requires form-data ^4.0.0, which still covers vulnerable versions, whereas 1.14.0 requires ^4.0.5. form-data resolves to 4.0.6. Regenerated against the public npm registry so npm ci works in CI (same issue hit on the salesforce fork sync). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
src/index.ts:1004
execute_querysendstemplate_tags(underscore) in the dataset request body, but elsewhere in this file (tool descriptions / update_card schema) the Metabase field is documented astemplate-tags(hyphenated). If Metabase expects the hyphenated key, template variable support will break.
type: "native",
native: {
query: query,
template_tags: {}
},
- index.ts: add_card_to_dashboard/remove_card_from_dashboard only read data.dashcards, missing the ordered_cards fallback already used in get_dashboard_cards. On a Metabase API that returns ordered_cards, both would see an empty existing-cards list and silently wipe every other card on the dashboard when PUTting the update. - response-logger.ts: gate full request/response logging behind METABASE_MCP_LOG_RESPONSES=true (off by default) — it was writing raw SQL text and query results to disk on every call unconditionally - index.ts: validate list_cards' `f` filter against the documented enum and URL-encode it, instead of interpolating it unvalidated - index.ts: execute_card now rejects non-array/non-empty-object `parameters` with InvalidParams instead of silently wrapping a scalar into a single-element array - package.json: test script's `test/**/*.test.js` glob doesn't expand under sh, so tests were silently not running at all — use node --test's built-in directory discovery instead - formatters.ts: fix the logged-response path in the user-facing message (was pointing at a path the logger doesn't actually write to) - README.md: fix the Claude Desktop config example — it had // comments which made it invalid JSON despite being meant for copy/paste Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://simondata.atlassian.net/browse/SECURITY-2309
Summary
This fork was 30 commits behind
imlewc/metabase-serverupstream. Rather than carrying a standalone dependency bump on stale code, this syncs the fork:upstream/main(30 commits) — conflicts in.gitignore,package.json(name/description,axiosversion), andpackage-lock.json, all resolved.axiosat^1.14.0(this branch's existing fix) rather than reverting to upstream's^1.8.2— axios 1.8.2 only requiresform-data: ^4.0.0, which still covers the vulnerable range; 1.14.0 requires^4.0.5. So the upstream sync alone would NOT have resolved this CVE (unlike the similar sync done formcp-server-salesforce/SECURITY-2312) — the axios bump still needed to survive the merge.form-dataresolves to 4.0.6, resolving ORCA-8368351 (CVE-2025-7501).package-lock.jsonagainst the public npm registry (not Radico's internal Artifactory proxy) so it resolves consistently regardless of local.npmrcconfig.Dockerfile's earliernpm install→npm cifix (fail fast on lockfile drift) survived the merge untouched.This also brings in upstream work this fork was missing: new response formatters (
src/formatters.ts), a response logger, an@modelcontextprotocol/sdkbump (^0.6.1→^1.25.2), and test coverage (test/formatters.test.js).Blast radius
Contained: dependency versions + new upstream source files (additive formatters/logging, no Radico-specific logic changed beyond the package name and axios version conflict resolutions).
Testing
npm run build— cleannode --test test/formatters.test.js— 4/4 passing (upstream's ownnpm testscript has a pre-existing glob bug —test/**/*.test.jsdoesn't expand undersh, reproduced identically on unsyncedupstream/main, unrelated to this change)npm audit— 0 vulnerabilitiesRisk
This is moderately risky, but I'm feeling confident — it's a large diff by line count, but it's almost entirely upstream code, plus a few small, well-understood conflict resolutions on our side.
Reviews
Adding new dependencies
Note: no new dependencies added.
@modelcontextprotocol/sdkbumped to the version upstream already adopted;axiosstays at this branch's existing^1.14.0(higher than upstream's^1.8.2, intentionally, per above);form-datais a transitive bump that comes along with it.