fix(sap_concur): HMAC the token cache key and wire the sendback comment - #6794
Conversation
…asing The cache key hashed a user-chosen password with a bare SHA-256. The key never leaves the process, but a password is low-entropy enough to brute-force out of a plain digest if one ever reached a heap dump or a debug log, which is what CodeQL flags. Keying the digest with a server-side secret makes it useless without that secret. A password-hashing KDF would be the wrong tool here: this runs on every token fetch, and the goal is collision-free partitioning rather than verification of a stored credential. The body wand prompt also claimed every payload family is camelCase. Exchange rate uploads are the exception — they take a snake_case currency_sets array of from_crn_code, to_crn_code, start_date and rate — and that operation is in BODY_OPS, so the blanket claim produced bodies Concur rejects.
… block move_travel_request accepts a documented query comment that Concur applies to the sendback action, but the params branch never passed it and the block's only comment field is gated to create_report_comment, so the value was unreachable from the UI. Uses a dedicated sendbackComment subblock rather than widening the existing comment field: that one is required for create_report_comment while this is optional, so sharing an id would both clash on required-ness and let a value bleed between the two operations.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Token cache key in Move travel request ( Request Body wand prompt no longer claims all families are camelCase; it documents exchange rates as snake_case ( Reviewed by Cursor Bugbot for commit a21f854. Configure here. |
|
@cursor review |
Greptile SummaryThe PR secures SAP Concur token-cache keys with a server-secret HMAC, corrects exchange-rate prompt casing, and wires sendback comments through the block configuration.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; both previously reported issues are fixed in the current code.
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/tools/sap_concur/shared.ts | Replaces the bare cache-key digest with an HMAC and consolidates the function documentation into one accurate TSDoc block. |
| apps/sim/blocks/blocks/sap_concur.ts | Corrects exchange-rate prompt guidance and safely gates sendback-comment visibility and forwarding by operation and action. |
Reviews (2): Last reviewed commit: "fix(sap_concur): gate the sendback comme..." | Re-trigger Greptile
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 c1c3e69. Configure here.
The sendbackComment field was conditioned only on the operation, so it rendered for submit, approve, cancel and every other workflow action even though Concur applies the comment to sendback alone. It is now gated on the action as well, and the params branch only forwards it for sendback so a value retained from an earlier sendback cannot ride along once the field is hidden. Also folds the two consecutive TSDoc blocks left above tokenCacheKey into one. Only the nearest block binds to the declaration, so the separator and collision reasoning in the earlier block was detached.
|
@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 a21f854. Configure here.
Summary
Follow-up to #6790 — these two commits were pushed to that branch after it had already been merged, so they never landed. Cherry-picked onto current staging, unchanged.
tokenCacheKeyhashed a user-chosen password with a bare SHA-256, which CodeQL flagged (alert 479). The key never leaves the process, but a password is low-entropy enough to brute-force out of a plain digest if a key ever reached a heap dump or a debug log. It is nowcreateHmac('sha256', env.INTERNAL_API_SECRET)over the same tuple, so the digest is useless without the server secret. Deliberately not a password-hashing KDF: this runs on every token fetch and partitions a cache rather than verifying a stored credential, so bcrypt/argon2 would add latency for no gain. That reasoning is in TSDoc on the function.sap_concur_upload_exchange_ratesis inBODY_OPSand genuinely takes snake_case (currency_setsof{ from_crn_code, to_crn_code, start_date, rate }), so the blanket claim produced bodies Concur rejects.move_travel_requestaccepts a documented querycommentthat Concur applies to thesendbackaction, but the params branch never forwarded it and the block's onlycommentfield is gated tocreate_report_comment, so it was unreachable from the UI. Uses a dedicatedsendbackCommentsubblock — sharing the existing id would clash on required-ness and let values bleed between the two operations.Type of Change
Testing
Tested manually.
bun run check:audits(29/29),check-block-registry,lint,type-check, and the 127 SAP Concur tests all pass against current staging.Checklist