Skip to content

fix(iast): fix detached parenthesis in IAST rewrite of optional chains inside conditionals - #203

Draft
CarlesDD wants to merge 4 commits into
mainfrom
ccapell/APPSEC-69708/fix-iast-if-optchain-detached-parent
Draft

fix(iast): fix detached parenthesis in IAST rewrite of optional chains inside conditionals#203
CarlesDD wants to merge 4 commits into
mainfrom
ccapell/APPSEC-69708/fix-iast-if-optchain-detached-parent

Conversation

@CarlesDD

@CarlesDD CarlesDD commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Fixes if conditions with optional-call chains ending in a CSI method (e.g. if (value?.toLowerCase()?.includes('needle'))): the rewriter closed the parenthesis before the final call, detaching it from its receiver and making it throw at runtime. Fixed by visiting if_stmt.test via visit_mut_with instead of visit_mut_children_with, so the top-level optional-chain transform applies as it already does for while/return/ternaries.

Also removes the now-redundant visit_mut_if_stmt override entirely, relying on swc's default IfStmt traversal. This fixes a second bug where a braceless else branch was never instrumented at all.

Motivation

Reported in DataDog/dd-trace-js#9836. With IAST enabled, this rewrites valid code into code that throws on every execution, silently, since it only shows up at runtime.

Additional Notes

The custom visitor override for if statements predated this fix and only handled test/cons, never alt, causing the second bug above. It has been removed in favor of the default traversal.

Describe how to test your changes

Reproduced the issue's exact repro with this repo's Rewriter against dd-trace-js's csiMethods. Before: throws for both matching and undefined input. After: returns "hit"/"miss" correctly, and generated code closes the parenthesis after .includes('needle').

While investigating, found and fixed a second bug in the same visitor: a braceless else branch (e.g. if (x) fn(); else a?.trim();) was never instrumented, since the old override never visited if_stmt.alt. Refactored by removing the special-case override entirely and relying on swc's default IfStmt traversal, which correctly visits test, cons, and alt.

Added regression tests for both cases. Full unit suite (npm test) passes, 357/357; integration suite (npm run test:integration) passes, 6/6; cargo clippy --workspace -- -D warnings clean.

Checklist

  • The CHANGELOG.md has been updated
  • Unit tests have been updated and pass
  • If known, an appropriate milestone has been selected

IlyasShabi
IlyasShabi previously approved these changes Aug 19, 2026
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