fix(sap_concur): align the integration with SAP Concur's documented API - #6790
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview Auth and outbound calls in Proxy and upload routes: optional Tests: new coverage for token cache isolation, coalescing, geolocation/path validation, error extraction, and response-body read behavior. Reviewed by Cursor Bugbot for commit 0906e45. Configure here. |
Greptile SummaryThe PR aligns the SAP Concur integration with documented authentication, endpoint, payload, and response contracts.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the prior upload response-read issues are fixed, and the separately acknowledged shared-fetch limitation is intentionally accepted as outside this PR.
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/tools/sap_concur/upload/route.ts | Upload responses now preserve upstream error statuses on body-read failures while still rejecting incomplete successful responses; the previously reported fixes are present. |
| apps/sim/app/api/tools/sap_concur/proxy/route.ts | Adds equivalent status-sensitive response reading, forwarded response headers, redirect refusal, and clearer fetch errors. |
| apps/sim/app/api/tools/sap_concur/shared.ts | Hardens token caching and acquisition, validates Concur geolocation hosts, coalesces concurrent token requests, and expands documented error-shape handling. |
| apps/sim/app/api/tools/sap_concur/response-body.test.ts | Covers successful and unsuccessful body reads, empty bodies, read failures, and HTTP status boundaries for both response helpers. |
| apps/sim/blocks/blocks/sap_concur.ts | Aligns operation-specific block inputs and context options with the corresponding Concur tools. |
Reviews (8): Last reviewed commit: "fix(sap_concur): keep Concur's status wh..." | Re-trigger Greptile
Validated all 70 tools, the block, and both proxy routes against SAP's published API docs. Auth: - add the password and companyUuid to the token cache key so a request with the wrong password can no longer be served a cached token minted from someone else's - wire the documented company-level flow (username = company UUID, credtype = authtoken) so companyUuid actually scopes a token - expand the datacenter allowlist to the documented set (adds glz, apj1, usg, the impl hosts, and the www- twins) and drop the undocumented cn host; validate the returned geolocation by shape instead of membership - coalesce concurrent token fetches so a fan-out mints one token - forward Retry-After so 429 retries pace off Concur's own hint - handle the errorMessageList, SCIM detail, and legacy Error.Message shapes instead of falling through to a generic HTTP message - pin redirects and cap the response body Block: - collapse six contextType subBlocks that disagreed on their default, so a new block no longer seeds MANAGER for every operation - clamp contextType to each operation's documented set - stop requiring a userId and contextType that the default operation's tool does not accept, and scope the receipt fields to the upload ops - reach six params that had no subBlock, and pass userId on travel request updates so a stale value cannot impersonate Tools: - correct response shapes that resolved to undefined: budget headers, budget categories, allocations, receipts, SCIM nextCursor, and the delete endpoints that return a bare boolean - use the Travel Request Amount schema (currency, not currencyCode) - narrow the four XML-only travel tools to a documented string payload and request application/xml - surface real errors instead of a JSON parse failure when the proxy returns a non-JSON body - cap receipt uploads at the documented sizes before downloading Adds 106 tests covering the token cache, geolocation validation, path traversal, and error extraction.
Removing the `forwardId` subblock without a migration entry breaks deployed workflows that still carry a value under that key. It fed a `concur-forwardid` request header that is documented nowhere in Concur's Receipts v4 or Image v1 references, so it was never honored. There is no replacement subblock and the value is an opaque caller-chosen string rather than a secret, so it is dropped outright.
The upload route caught every error from the bounded response read and continued down the success path, so a size-limit breach or a stream failure surfaced as an upstream success with a null or header-only body. Concur returns Content-Length: 0 on a successful image-only upload, and readResponseTextWithLimit already returns an empty string for that without throwing, so dropping the catch keeps the legitimate empty-body case working while letting real read failures reach the route's handler.
de6f662 to
a48881a
Compare
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit a48881a. Configure here.
The password grant marked username required, so the company-level flow — which sends the company UUID as the token username and has no user login — could not be configured at all, even though the request schema and token fetch already accept companyUuid without a username. Username is now optional for that grant and the server-side check reports which of the two is missing. Relabels the password and companyUuid fields to say what they carry in the company flow. The shared body wand prompt also still described several payloads the way they looked before this branch: quick expenses in PascalCase rather than v4 camelCase, travel requests and expected expenses using currencyCode where the Request v4 Amount schema uses currency, the standard SCIM SearchRequest URN instead of Concur's, startIndex as a search parameter when it is unsupported, and a cash advance shape that does not match the documented request. A wand-generated body was therefore rejected for most of the create operations it covers.
|
@cursor review |
Removing the blanket catch from the upload read fixed one failure mode and introduced its inverse: a cap breach or stream error while reading a non-success body threw before the route reached the branch that preserves Concur's status, so an upstream 4xx surfaced as a Sim 500 and could trigger a retry the caller should not make. Both routes now split the two cases. On a success status the body is the result, so a read failure still propagates. On an error status the body only supplies the message, so a read failure resolves empty and the upstream status survives, with the message falling back to the generic HTTP-status form. Adds 21 tests covering both helpers over success, error, empty-body and boundary statuses; inverting the status check turns 14 of them red.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 0906e45. Configure here.
|
@cursor review |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 0906e45. Configure here.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 0906e45. Configure here.
Summary
companyUuidto the token cache key — a request with the wrong password could previously be served a cached token minted from someone else's correct one. Wired the documented company-level flow (username= company UUID,credtype=authtoken) socompanyUuidactually scopes a token instead of sitting inert. Expanded the datacenter allowlist to the documented set (addsglz,apj1,usg, the impl hosts and thewww-twins) and dropped the undocumentedcnhost; the returnedgeolocationis now validated by shape rather than membership, so GLZ andwww-tenants are no longer rejected.Retry-Afteris forwarded so 429 retries pace off Concur's own hint instead of blind backoff;errorMessageList, SCIMdetail, and the legacyError.Messageshapes are handled instead of falling through to a generic HTTP message; redirects are pinned and the response body capped.contextTypesubBlocks that disagreed on their default — a freshly dropped block was seedingMANAGERfor all 70 operations, including ones whose dropdown only offersTRAVELER.contextTypeis now clamped to each operation's documented set. The default operation no longer demands auserId/contextTypeits tool doesn't accept, receipt fields are scoped to the upload operations instead of being required on all 70, anduserIdis passed on travel-request updates so a stale value can't silently impersonate.undefined(budget headers, budget categories, allocations, receipts, SCIMnextCursor, and the deletes that return a bare boolean); used the Travel RequestAmountschema (currency, notcurrencyCode); narrowed the four XML-only travel tools to a documented string payload and made them requestapplication/xml; replaced a JSON parse crash with a real error when the proxy returns a non-JSON body; and capped receipt uploads at the documented sizes before downloading.Type of Change
Testing
Tested manually. Added 106 tests covering the token cache key, geolocation validation, path traversal, and error extraction — each verified to fail without its fix.
bun run check:audits(29/29),bun run lint, andtype-checkall pass; docs and tool metadata regenerated.Checklist