Skip to content

fix: strip trigger variable brackets in one step - #1138

Merged
Klakurka merged 2 commits into
masterfrom
cursor/fix-trigger-variable-sanitization-2e84
Oct 1, 2026
Merged

Klakurka merged 2 commits into
masterfrom
cursor/fix-trigger-variable-sanitization-2e84

Conversation

@Klakurka

@Klakurka Klakurka commented Oct 1, 2026 •

Copy link
Copy Markdown
Member

Related to https://github.com/PayButton/paybutton-server/security/code-scanning/3
Related to https://github.com/PayButton/paybutton-server/security/code-scanning/6
Related to https://github.com/PayButton/paybutton-server/security/code-scanning/7
Related to https://github.com/PayButton/paybutton-server/security/code-scanning/8

Description

CodeQL js/incomplete-sanitization reported two findings on the same line in the trigger signature payload: replace('<', '') and replace('>', ''). Each call removes only the first match.

Trigger variables are always a single <name> token from TRIGGER_POST_VARIABLES. The key is now the slice between the first and last character, so both brackets are removed together. One change clears both findings.

The GitHub token for this run cannot read code-scanning alert bodies, so this is one of the fixes from a local CodeQL code-scanning run of the same default query suite.

Test plan

  • npx ts-node -O '{"module":"commonjs"}' node_modules/jest/bin/jest.js tests/unittests/validators.test.ts --forceExit
  • Existing signature payload cases still match the expected strings (<amount> → amount, and the multi-variable payloads).
  • Re-ran the CodeQL query; both findings are gone.
Open in Web Open in Cursor 

Summary by CodeRabbit

  • Bug Fixes
    • Improved parsing of placeholder names in signature tokens, so names are extracted consistently when processing signature data. This avoids unintended changes to names containing angle brackets and helps ensure the correct placeholders are recognized.

replace() only removed the first angle bracket, so a token with more than
one bracket would keep the rest. Trigger variables are a single <name>
pair, and the key is now taken from the characters between those brackets.

Co-authored-by: David <Klakurka@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 47 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: c5ab12ed-b7f7-4184-8298-14819a73d628

📥 Commits

Reviewing files that changed from the base of the PR and between d356915 and 1afe114.

📒 Files selected for processing (1)
  • utils/validators.ts

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: cabf5d19-d317-41c4-a752-82f6aa688801

📥 Commits

Reviewing files that changed from the base of the PR and between 525622e and d356915.

📒 Files selected for processing (1)
  • utils/validators.ts

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

getSignaturePayload now derives parameter keys by removing the first and last characters of each matched token. Its documentation describes tokens as single <name> pairs.

Changes

Signature payload parsing

Layer / File(s) Summary
Token key extraction
utils/validators.ts
getSignaturePayload uses slicing to remove each token’s first and last characters when deriving its parameter key. The documentation specifies a single <name> pair.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to d3569

The token-key change preserves the existing signature payload behavior for supported tokens, so no merge-blocking issue is identified.

Architecture Summary

Architecture risk: 🔵 Low · up to d3569

The change affects 1 system.

Changed systems: utils

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — utils (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in utils/validators.ts: getSignaturePayload now documents the single-pair token format and derives each parameter key by slicing off the token’s first and last characters instead of replacing angle brackets.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: stripping trigger-variable brackets in one step. It is concise and specific.
Description check ✅ Passed The description includes the required Related to, Description, and Test plan sections. It explains the CodeQL findings, the implementation change, and the reported validation steps.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

@Klakurka
Klakurka marked this pull request as ready for review October 1, 2026 17:08
@Klakurka Klakurka self-assigned this Oct 1, 2026
@Klakurka Klakurka added the bug Something isn't working label Oct 1, 2026
@Klakurka Klakurka added this to the Phase 3 milestone Oct 1, 2026
@Klakurka
Klakurka requested a balanced review from Copilot October 1, 2026 17:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The focused change preserves existing payload behavior and matches the established token definitions.

Review effort: Balanced
Findings: None

What changed in this PR

Simplifies trigger-variable key extraction while resolving CodeQL incomplete-sanitization findings.

Changes:

  • Replaces chained bracket removal with deterministic token slicing.
  • Documents the expected <name> token format.
File Description
utils/​validators.ts Extracts trigger keys by removing the first and last characters.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@Klakurka
Klakurka merged commit 0ee5fc3 into master Oct 1, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants