Skip to content

Fix OAuth signature generation in validateRestApiAccess#2

Closed
Mwalek wants to merge 4 commits intoGravityKit:mainfrom
Mwalek:fix/oauth-validation-headers
Closed

Fix OAuth signature generation in validateRestApiAccess#2
Mwalek wants to merge 4 commits intoGravityKit:mainfrom
Mwalek:fix/oauth-validation-headers

Conversation

@Mwalek
Copy link
Copy Markdown
Contributor

@Mwalek Mwalek commented Dec 30, 2025

Summary

  • Fix OAuth 1.0a signature generation in validateRestApiAccess function
  • Resolve "REST API validation failed: undefined" error when using HTTP connections

Problem

The validateRestApiAccess function was calling getAuthHeaders() without the required URL parameter for OAuth 1.0a signature generation. This caused authentication failures when using HTTP connections (which fall back to OAuth instead of Basic Auth).

Steps to Reproduce

  1. Clone and configure GravityMCP with HTTP base URL:

    git clone https://github.com/GravityKit/GravityMCP.git
    cd GravityMCP
    npm install
    cp .env.example .env
    # Configure .env with HTTP URL (e.g., http://localhost:31337)
  2. Add to Claude Code:

    claude mcp add gravitymcp --scope user -- node /path/to/GravityMCP/src/index.js
  3. Check MCP server status:

    claude mcp list
  4. Expected: gravitymcp shows "✓ Connected"

  5. Actual: gravitymcp shows "✗ Failed to connect" with error "REST API validation failed: undefined"

Solution

  • Get baseURL from httpClient.defaults.baseURL
  • Construct full URL for each endpoint being validated
  • Pass proper parameters (method, url, params) to getAuthHeaders()

Test plan

  • Verified fix with npm run check-env on HTTP localhost connection
  • All 3 endpoints (forms, entries, feeds) now validate successfully with OAuth
  • Confirmed claude mcp list shows gravitymcp as connected after fix

Summary by CodeRabbit

  • Bug Fixes

    • OAuth signing now uses the full request URL derived from the client's base URL, improving signature accuracy and ensuring authenticated API calls validate correctly.
    • Validation now surfaces an error when a base URL is unavailable, preventing ambiguous requests and improving endpoint reliability.
  • Tests

    • Test helpers now initialize a default mock client base URL for more consistent test behavior.

The validateRestApiAccess function was calling getAuthHeaders() without
the required URL parameter for OAuth 1.0a signature generation. This
caused authentication failures when using HTTP connections (which fall
back to OAuth instead of Basic Auth).

Changes:
- Get baseURL from httpClient.defaults.baseURL
- Construct full URL for each endpoint
- Pass proper parameters (method, url, params) to getAuthHeaders()

This fixes the "REST API validation failed: undefined" error when
connecting via HTTP/OAuth.
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Dec 30, 2025

Walkthrough

validateRestApiAccess now reads baseURL from httpClient.defaults.baseURL, constructs full endpoint URLs, and passes method, full URL, and query params to getAuthHeaders when generating OAuth headers for each endpoint.

Changes

Cohort / File(s) Summary
OAuth signature update
src/config/auth.js
validateRestApiAccess now derives baseURL from httpClient.defaults.baseURL, builds a fullUrl per endpoint, and calls getAuthHeaders('GET', fullUrl, { per_page: 1 }) instead of calling getAuthHeaders() with no args.
Test helper defaults
src/tests/helpers.js
MockHttpClient constructor now initializes this.defaults = { baseURL: 'https://test.example.com' }.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title 'Fix OAuth signature generation in validateRestApiAccess' directly and accurately summarizes the main change—fixing how OAuth headers are generated in the validateRestApiAccess function by passing proper URL parameters to getAuthHeaders().
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 958811a and 9b9247e.

📒 Files selected for processing (1)
  • src/config/auth.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/config/auth.js (2)
scripts/check-env.js (1)
  • httpClient (109-112)
src/tests/authentication.test.js (2)
  • headers (46-46)
  • headers (113-113)

Comment thread src/config/auth.js
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9b9247e and 75dccf1.

📒 Files selected for processing (1)
  • src/config/auth.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/config/auth.js (2)
scripts/check-env.js (2)
  • httpClient (109-112)
  • authManager (106-106)
src/tests/authentication.test.js (2)
  • headers (46-46)
  • headers (113-113)
🔇 Additional comments (1)
src/config/auth.js (1)

330-332: LGTM! OAuth signature generation fix is correct.

The fix properly constructs the full URL and passes it to getAuthHeaders along with the HTTP method and query parameters. This ensures OAuth1Handler can generate valid signatures for API requests.

The implementation is consistent with OAuth1Handler.testConnection (lines 159-160) and correctly addresses the authentication failures described in the PR objectives.

Comment thread src/config/auth.js
Mwalek and others added 2 commits December 30, 2025 13:52
Prevents TypeError when httpClient.defaults is undefined by using
optional chaining (httpClient?.defaults?.baseURL).
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/config/auth.js (1)

328-336: Core fix is correct — OAuth signatures now use the full URL.

Constructing fullUrl from baseURL + endpoint.path and passing method, fullUrl, and params to getAuthHeaders properly aligns the OAuth 1.0a signature base string with the actual request, fixing the authentication failure.

One minor robustness note: if baseURL ever ends with a trailing /, the concatenation would produce a double-slash (e.g., .../gf/v2//forms), causing an OAuth signature mismatch. Currently this is safe because callers strip trailing slashes, but a one-line normalization would make this resilient to future callers:

🛡️ Optional: normalize trailing slash
     const baseURL = httpClient?.defaults?.baseURL;

     if (!baseURL) {
       throw new Error('httpClient baseURL is not configured');
     }

     const results = [];
     for (const endpoint of endpoints) {
       try {
         // Generate proper OAuth headers with full URL for signature
-        const fullUrl = `${baseURL}${endpoint.path}`;
+        const fullUrl = `${baseURL.replace(/\/+$/, '')}${endpoint.path}`;
         const headers = authManager.getAuthHeaders('GET', fullUrl, { per_page: 1 });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/config/auth.js` around lines 328 - 336, The concatenation of baseURL and
endpoint.path can yield a double slash if baseURL ends with '/', so normalize
baseURL before building fullUrl: trim any trailing '/' (or ensure exactly one
'/' separator) prior to creating fullUrl used by getAuthHeaders and the
httpClient call; update the code around fullUrl, baseURL, endpoint.path, and
getAuthHeaders to perform this one-line normalization to make OAuth signature
generation resilient.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/config/auth.js`:
- Around line 320-325: The current change correctly guards against missing
defaults by using optional chaining to read baseURL (const baseURL =
httpClient?.defaults?.baseURL) and throws a clear error (throw new
Error('httpClient baseURL is not configured')) to avoid generating malformed
URLs; keep this defensive check in src/config/auth.js, ensure the
httpClient?.defaults?.baseURL expression is used wherever baseURL is required
(e.g., OAuth signature generation) and preserve the early throw to fail fast
when baseURL is absent.

---

Nitpick comments:
In `@src/config/auth.js`:
- Around line 328-336: The concatenation of baseURL and endpoint.path can yield
a double slash if baseURL ends with '/', so normalize baseURL before building
fullUrl: trim any trailing '/' (or ensure exactly one '/' separator) prior to
creating fullUrl used by getAuthHeaders and the httpClient call; update the code
around fullUrl, baseURL, endpoint.path, and getAuthHeaders to perform this
one-line normalization to make OAuth signature generation resilient.

@zackkatz
Copy link
Copy Markdown
Member

Closing — all changes from this PR were included in PR #3, which has been merged as part of v1.0.5. Thank you @Mwalek!

@zackkatz zackkatz closed this Feb 18, 2026
zackkatz added a commit that referenced this pull request Mar 20, 2026
Tests written first (22 regression tests), then code fixed:

- #2: Replace console.log with logger in 7 files (prevents stdout corruption)
- #4: Update mcp.json: remove phantom gf_submit_form, add 4 field ops, fix count
- #5: Add MCP tool annotations to all 27 tools (readOnlyHint, destructiveHint, etc.)
- #7: Strip _variant/_meta from validated fields before API payload
- #8: Let field operation errors propagate to wrapHandler (sets isError: true)
- #9: Fix name field sub-input IDs (.2=prefix not first, .3=first not prefix)
- #20: Remove deprecated crypto npm dependency (Node built-in)
- #21: Remove unused form-data dependency
- #23: Sync mcp.json version with package.json (1.4.0)
- #24: Fix feature filter key: supportsConditional → supportsConditionalLogic
- #25: Add ALLOW_DELETE to gf_delete_feed description

Test runner expanded from 7 to 10 suites. 234 tests, 100% pass rate.
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