Skip to content

fix(sap_concur): align the integration with SAP Concur's documented API - #6790

Merged
waleedlatif1 merged 5 commits into
stagingfrom
validate/sap-concur
Aug 17, 2026
Merged

fix(sap_concur): align the integration with SAP Concur's documented API#6790
waleedlatif1 merged 5 commits into
stagingfrom
validate/sap-concur

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Summary

  • Validated all 70 SAP Concur tools, the block, and both proxy routes against SAP's published API docs.
  • Auth: added the password and companyUuid to 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) so companyUuid actually scopes a token instead of sitting inert. Expanded the datacenter allowlist to the documented set (adds glz, apj1, usg, the impl hosts and the www- twins) and dropped the undocumented cn host; the returned geolocation is now validated by shape rather than membership, so GLZ and www- tenants are no longer rejected.
  • Auth hardening: concurrent token fetches are coalesced so a fan-out mints one token instead of N; Retry-After is forwarded so 429 retries pace off Concur's own hint instead of blind backoff; errorMessageList, SCIM detail, and the legacy Error.Message shapes are handled instead of falling through to a generic HTTP message; redirects are pinned and the response body capped.
  • Block: collapsed six contextType subBlocks that disagreed on their default — a freshly dropped block was seeding MANAGER for all 70 operations, including ones whose dropdown only offers TRAVELER. contextType is now clamped to each operation's documented set. The default operation no longer demands a userId/contextType its tool doesn't accept, receipt fields are scoped to the upload operations instead of being required on all 70, and userId is passed on travel-request updates so a stale value can't silently impersonate.
  • Tools: corrected response shapes that resolved to undefined (budget headers, budget categories, allocations, receipts, SCIM nextCursor, and the deletes that return a bare boolean); used the Travel Request Amount schema (currency, not currencyCode); narrowed the four XML-only travel tools to a documented string payload and made them request application/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

  • Bug fix

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, and type-check all pass; docs and tool metadata regenerated.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel

vercel Bot commented Aug 17, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Aug 17, 2026 11:22pm

Request Review

@cursor

cursor Bot commented Aug 17, 2026

Copy link
Copy Markdown

PR Summary

High Risk
Changes authentication token caching and OAuth grant handling—security-critical—and alter how all Concur API traffic is proxied; regressions could cause auth bypass, wrong tenant routing, or broken uploads/retries.

Overview
Aligns the SAP Concur integration docs and server-side proxy/upload routes with SAP’s published APIs, with a large refresh of sap_concur.mdx (auth, required fields, response shapes, XML-only travel endpoints, pagination, and workflow nuances).

Auth and outbound calls in shared.ts: token cache keys now include password, companyUuid, and credtype so wrong credentials cannot reuse another user’s cached token; company-level password grant (username = company UUID, credtype=authtoken) is wired; datacenter allowlist and geolocation validation are expanded/relaxed per SAP guidance; concurrent token mints are single-flighted; token error bodies are truncated/redacted; extractSapConcurError covers Budget/SCIM/legacy envelopes; Retry-After, Location, and Link are forwarded; redirects are refused (maxRedirects: 0) and responses are size-capped.

Proxy and upload routes: optional Accept (e.g. application/xml); per-request concur-correlationid UUIDs; error-status body reads that preserve upstream 4xx; clampErrorStatus for odd 3xx; receipt uploads enforce Concur size limits before download and validate MIME types; removed upload forwardId header wiring.

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-apps

greptile-apps Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR aligns the SAP Concur integration with documented authentication, endpoint, payload, and response contracts.

  • Expands and hardens token acquisition, caching, geolocation validation, redirect handling, and error extraction.
  • Corrects block inputs, operation-specific context values, tool request formats, and generated output schemas.
  • Splits response-read handling so successful responses propagate read failures while unsuccessful responses preserve Concur’s status when only the error body cannot be read.
  • Adds upload limits and focused tests for authentication, validation, error extraction, and response-body behavior.

Confidence Score: 5/5

The 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.

Important Files Changed

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

Comment thread apps/sim/app/api/tools/sap_concur/upload/route.ts Outdated
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.
Comment thread apps/sim/blocks/blocks/sap_concur.ts
Comment thread apps/sim/blocks/blocks/sap_concur.ts Outdated
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot 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.

✅ 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.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/app/api/tools/sap_concur/upload/route.ts Outdated
Comment thread apps/sim/blocks/blocks/sap_concur.ts
Comment thread apps/sim/app/api/tools/sap_concur/shared.ts Dismissed
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.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot 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.

✅ 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.

@waleedlatif1
waleedlatif1 merged commit 38075ad into staging Aug 17, 2026
24 checks passed
@waleedlatif1
waleedlatif1 deleted the validate/sap-concur branch August 17, 2026 23:27
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/app/api/tools/sap_concur/upload/route.ts
Comment thread apps/sim/app/api/tools/sap_concur/shared.ts
Comment thread apps/sim/blocks/blocks/sap_concur.ts
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot 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.

✅ 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.

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@cursor cursor Bot 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.

✅ 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.

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