Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview Wires the integration into the product by registering 37 new Reviewed by Cursor Bugbot for commit 7fc1a33. Configure here. |
Greptile SummaryThis PR introduces the SAP S/4HANA integration with 37 tools covering Business Partners, Sales Orders, Products, Deliveries, and more, all routed through a single internal proxy that handles BTP UAA token caching, CSRF fetch+retry, and OData payload normalization.
Confidence Score: 3/5Not safe to merge until the SSRF vector in the proxy route is addressed. A single P1 security finding (SSRF via unvalidated baseUrl/tokenUrl) caps the score below 4. The remaining findings are P2. The 37 tool files and block config are well-structured and follow project conventions. apps/sim/app/api/tools/sap_s4hana/proxy/route.ts — SSRF and unbounded token cache; apps/sim/tools/sap_s4hana/utils.ts — parseJsonInput return-type unsafety.
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/tools/sap_s4hana/proxy/route.ts | Core proxy route: handles auth, CSRF, token caching, and OData forwarding. Contains an unvalidated baseUrl/tokenUrl SSRF vector (P1), an unbounded module-level token cache, and no fetch timeouts. |
| apps/sim/tools/sap_s4hana/utils.ts | Shared utility helpers for proxy body construction, OData query building, and JSON parsing. parseJsonInput returns undefined as unknown as T which undermines type safety for non-nullable callers. |
| apps/sim/tools/sap_s4hana/types.ts | Type definitions for all 37 SAP tools. Well-structured; SapBaseParams correctly makes all auth fields optional so each tool can compose them. |
| apps/sim/tools/sap_s4hana/odata_query.ts | Generic OData escape-hatch tool. normalizeQuery correctly handles JSON objects and query strings. Credentials marked user-only as required by policy. |
| apps/sim/blocks/blocks/sap_s4hana.ts | Block config for all 37 operations with conditional sub-block visibility. No issues found. |
| apps/sim/tools/sap_s4hana/create_sales_order.ts | Sales order creation with deep-insert items; body spread before fixed fields so user overrides are safely replaced. Credentials use user-only visibility. |
| apps/sim/tools/sap_s4hana/get_business_partner.ts | Representative get-entity tool. Uses quoteOdataKey for OData key escaping and user-only visibility for all credentials. Looks correct. |
Sequence Diagram
sequenceDiagram
participant Client as Sim Workflow Block
participant Proxy as /api/tools/sap_s4hana/proxy
participant TokenCache as MODULE TOKEN_CACHE
participant UAA as BTP UAA / OAuth Server
participant SAP as SAP S/4HANA OData API
Client->>Proxy: POST (tool params + credentials)
Proxy->>Proxy: checkInternalAuth()
Proxy->>Proxy: ProxyRequestSchema.parse()
alt OAuth flow
Proxy->>TokenCache: lookup(tokenUrl::clientId)
alt Cache miss / expired
Proxy->>UAA: POST tokenUrl (client_credentials)
UAA-->>Proxy: { access_token, expires_in }
Proxy->>TokenCache: store(key, token, expiresAt)
end
end
alt Write method (POST/PATCH/PUT/DELETE/MERGE)
Proxy->>SAP: GET /$metadata (X-CSRF-Token: Fetch)
SAP-->>Proxy: x-csrf-token + set-cookie
end
Proxy->>SAP: HTTP method + OData path + query + body
SAP-->>Proxy: OData JSON response
alt 403 + CSRF required
Proxy->>SAP: GET /$metadata (X-CSRF-Token: Fetch) [retry]
SAP-->>Proxy: refreshed csrf token
Proxy->>SAP: retry original request with new CSRF
SAP-->>Proxy: OData JSON response
end
Proxy->>Proxy: unwrapOdata(d.results / d)
Proxy-->>Client: { success, output: { status, data } }
Reviews (1): Last reviewed commit: "feat(integrations): SAP S/4HANA tools, b..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function resolveHost(req: ProxyRequest): string { | ||
| if (req.baseUrl) { | ||
| return req.baseUrl.replace(/\/+$/, '') | ||
| } | ||
| return `https://${req.subdomain}-api.s4hana.ondemand.com` | ||
| } | ||
|
|
||
| function buildOdataUrl(req: ProxyRequest, pathOverride?: string): string { | ||
| const host = resolveHost(req) | ||
| const servicePath = `/sap/opu/odata/sap/${req.service}` | ||
| const subPath = pathOverride ?? req.path | ||
| const normalized = subPath.startsWith('/') ? subPath : `/${subPath}` | ||
| const base = `${host}${servicePath}${normalized}` | ||
|
|
||
| if (!req.query || Object.keys(req.query).length === 0) { | ||
| return base | ||
| } | ||
| const search = new URLSearchParams() | ||
| for (const [key, value] of Object.entries(req.query)) { | ||
| if (value === undefined || value === null) continue | ||
| search.append(key, String(value)) | ||
| } | ||
| const queryString = search.toString() | ||
| if (!queryString) return base | ||
| return base.includes('?') ? `${base}&${queryString}` : `${base}?${queryString}` | ||
| } |
There was a problem hiding this comment.
baseUrl and tokenUrl
Both baseUrl (used in resolveHost) and tokenUrl (used in fetchAccessToken) are passed directly from the user-controlled request body to fetch() with no URL-scheme or host-allowlist validation. An authenticated Sim user can set baseUrl to an internal address (e.g., http://169.254.169.254/latest/meta-data/) and the proxy will faithfully fetch it and return the response — giving any platform user a server-side relay into the host's internal network or cloud metadata service. The same applies to tokenUrl, which receives a POST carrying the caller-supplied OAuth credentials.
At minimum, validate that both values start with https:// and consider a denylist of RFC-1918 / link-local ranges before issuing any outbound request.
| const TOKEN_CACHE = new Map<string, CachedToken>() | ||
| const TOKEN_SAFETY_WINDOW_MS = 60_000 |
There was a problem hiding this comment.
TOKEN_CACHE is a module-level Map, so in any serverless or multi-replica deployment each cold-start begins with an empty cache. More critically, tokens stored in one request's process are never cleaned up by the platform — in a long-running server process this map grows without bound (one entry per unique tokenUrl + clientId combination across all tenants). Consider capping the cache size or using an LRU strategy, and document that this cache is not shared across replicas.
| const response = await fetch(tokenUrl, { | ||
| method: 'POST', | ||
| headers: { | ||
| Authorization: `Basic ${basic}`, | ||
| 'Content-Type': 'application/x-www-form-urlencoded', | ||
| Accept: 'application/json', | ||
| }, | ||
| body: 'grant_type=client_credentials', | ||
| }) | ||
|
|
||
| if (!response.ok) { | ||
| const text = await response.text().catch(() => '') | ||
| logger.warn(`[${requestId}] Token fetch failed (${response.status}): ${text}`) | ||
| throw new Error(`SAP token request failed: HTTP ${response.status}`) | ||
| } | ||
|
|
||
| const data = (await response.json()) as { | ||
| access_token?: string | ||
| expires_in?: number | ||
| } | ||
|
|
||
| if (!data.access_token) { | ||
| throw new Error('SAP token response missing access_token') | ||
| } | ||
|
|
||
| const expiresInMs = (data.expires_in ?? 3600) * 1000 | ||
| TOKEN_CACHE.set(cacheKey, { | ||
| accessToken: data.access_token, | ||
| expiresAt: Date.now() + expiresInMs, | ||
| }) | ||
| return data.access_token | ||
| } | ||
|
|
||
| interface CsrfBundle { | ||
| token: string | ||
| cookie: string | ||
| } | ||
|
|
||
| function joinSetCookies(headers: Headers): string { | ||
| const cookies = | ||
| typeof (headers as { getSetCookie?: () => string[] }).getSetCookie === 'function' | ||
| ? (headers as { getSetCookie: () => string[] }).getSetCookie() | ||
| : (headers.get('set-cookie') ?? '').split(/,(?=[^ ;]+=)/) | ||
| return cookies | ||
| .map((c) => c.split(';')[0]?.trim()) | ||
| .filter(Boolean) | ||
| .join('; ') | ||
| } | ||
|
|
||
| function buildAuthHeader(req: ProxyRequest, accessToken: string | null): string { | ||
| if (req.authType === 'basic') { | ||
| const basic = Buffer.from(`${req.username}:${req.password}`).toString('base64') | ||
| return `Basic ${basic}` | ||
| } | ||
| return `Bearer ${accessToken}` | ||
| } | ||
|
|
||
| async function fetchCsrf( | ||
| req: ProxyRequest, | ||
| accessToken: string | null, | ||
| requestId: string | ||
| ): Promise<CsrfBundle | null> { | ||
| const url = buildOdataUrl(req, '/$metadata') | ||
| const response = await fetch(url, { | ||
| method: 'GET', |
There was a problem hiding this comment.
fetchAccessToken, fetchCsrf, and callOdata all call fetch() without a timeout or AbortSignal. A slow or unresponsive SAP host will hold the serverless function open for the platform's maximum timeout (typically 30–60 s), consuming execution quota and potentially blocking other concurrent requests. Pass signal: AbortSignal.timeout(30_000) (or similar) to each outbound fetch.
| export function parseJsonInput<T = unknown>(input: unknown, fieldName: string): T { | ||
| if (input === undefined || input === null || input === '') { | ||
| return undefined as unknown as T | ||
| } | ||
| if (typeof input === 'object') return input as T | ||
| if (typeof input !== 'string') { | ||
| throw new Error(`Invalid ${fieldName}: expected JSON object or string`) | ||
| } | ||
| try { | ||
| return JSON.parse(input) as T | ||
| } catch { | ||
| throw new Error(`Invalid ${fieldName}: must be valid JSON`) | ||
| } | ||
| } |
There was a problem hiding this comment.
parseJsonInput silently returns undefined typed as T
When input is undefined, null, or '', the function returns undefined as unknown as T. Callers that declare T as a non-nullable type (e.g., Record<string, unknown>) will receive undefined without a type error, and any property access on the result will throw at runtime. Either change the return type to T | undefined so callers must handle the absent case, or keep the cast but document the contract explicitly.
| export function parseJsonInput<T = unknown>(input: unknown, fieldName: string): T { | |
| if (input === undefined || input === null || input === '') { | |
| return undefined as unknown as T | |
| } | |
| if (typeof input === 'object') return input as T | |
| if (typeof input !== 'string') { | |
| throw new Error(`Invalid ${fieldName}: expected JSON object or string`) | |
| } | |
| try { | |
| return JSON.parse(input) as T | |
| } catch { | |
| throw new Error(`Invalid ${fieldName}: must be valid JSON`) | |
| } | |
| } | |
| export function parseJsonInput<T = unknown>(input: unknown, fieldName: string): T | undefined { |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7fc1a33. Configure here.
| password: true, | ||
| condition: { field: 'authType', value: 'oauth_client_credentials' }, | ||
| required: { field: 'authType', value: 'oauth_client_credentials' }, | ||
| }, |
There was a problem hiding this comment.
OAuth fields hidden for Cloud Public after deployment switch
Medium Severity
The clientId and clientSecret fields' visibility depends on authType equaling 'oauth_client_credentials', but the authType dropdown is hidden when deploymentType is 'cloud_public'. If a user switches from Cloud Private (selecting 'basic' auth) back to Cloud Public, the authType value may remain 'basic' since the dropdown is now hidden and won't reset, causing the required OAuth credential fields to disappear for the primary deployment mode.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 7fc1a33. Configure here.
| const queryString = search.toString() | ||
| if (!queryString) return base | ||
| return base.includes('?') ? `${base}&${queryString}` : `${base}?${queryString}` | ||
| } |
There was a problem hiding this comment.
No validation on service parameter allows path traversal
Medium Severity
The service parameter is interpolated directly into the URL path (/sap/opu/odata/sap/${req.service}) with only a minimum-length check. A value like ../../other_endpoint could escape the OData service prefix and reach unintended endpoints on the SAP host. The path parameter has the same issue. While this route requires internal auth, it still enables authenticated SSRF against the target SAP system.
Reviewed by Cursor Bugbot for commit 7fc1a33. Configure here.
1 participant
Summary
Type of Change
Testing
Tested manually
Checklist