Fix WSSecurityCert header placement when no SOAP headers exist#1489
Fix WSSecurityCert header placement when no SOAP headers exist#1489rubenaguadoc wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe pull request fixes a bug where ChangesSecurity Header Fallback and Validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/security/WSSecurityCert.js (2)
362-365: 💤 Low valueRedundant duplicate assertion —
endsWithcheck on line 362 is superseded by the index arithmetic on lines 364-365Both assertions verify that nothing follows
</soap:Envelope>. One is sufficient.🧹 Proposed cleanup
- assert.ok(requestXml.endsWith('</soap:Envelope>'), 'XML must end with </soap:Envelope>'); - const envelopeCloseIdx = requestXml.indexOf('</soap:Envelope>'); assert.strictEqual(envelopeCloseIdx + '</soap:Envelope>'.length, requestXml.length, 'No content should appear after </soap:Envelope>');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/security/WSSecurityCert.js` around lines 362 - 365, Remove the redundant endsWith assertion and keep the precise index-based check: delete the assert.ok(requestXml.endsWith('</soap:Envelope>'), ...) line and rely on the envelopeCloseIdx calculation with assert.strictEqual(envelopeCloseIdx + '</soap:Envelope>'.length, requestXml.length, ...) to ensure nothing follows the closing tag; this targets the assertions around the requestXml variable and the envelopeCloseIdx logic in the test.
349-351: ⚡ Quick winTest relies on synchronous
'request'event emission — correct but fragile
client.MyOperation({}, function () {})triggers_invoke, which callsthis.emit('request', xml, eid)synchronously (before the async HTTP call).requestXmlis therefore set when line 351 executes. This is correct today, but any future refactor that defers the emit (e.g. into a microtask) would silently break the assertion rather than produce a helpful failure. Consider wrapping the entire invocation in a promise resolved by the'request'event handler for robustness:🛡️ Suggested approach
- let requestXml; - client.on('request', function (xml) { - requestXml = xml; - }); - - client.MyOperation({}, function () {}); - - assert.ok(requestXml, 'request XML should have been captured'); + const requestXml = await new Promise((resolve) => { + client.once('request', resolve); + client.MyOperation({}, function () {}); + }); + + assert.ok(requestXml, 'request XML should have been captured');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/security/WSSecurityCert.js` around lines 349 - 351, The test assumes client.MyOperation triggers a synchronous 'request' event via _invoke's this.emit('request', xml, eid), which is fragile; change the test to await the 'request' event instead of relying on synchronous emission by wrapping the client.MyOperation call in a Promise that resolves in the client's 'request' event handler (capturing requestXml there) and then assert requestXml after that Promise resolves so the test remains robust if emission becomes asynchronous.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/security/WSSecurityCert.js`:
- Around line 362-365: Remove the redundant endsWith assertion and keep the
precise index-based check: delete the
assert.ok(requestXml.endsWith('</soap:Envelope>'), ...) line and rely on the
envelopeCloseIdx calculation with assert.strictEqual(envelopeCloseIdx +
'</soap:Envelope>'.length, requestXml.length, ...) to ensure nothing follows the
closing tag; this targets the assertions around the requestXml variable and the
envelopeCloseIdx logic in the test.
- Around line 349-351: The test assumes client.MyOperation triggers a
synchronous 'request' event via _invoke's this.emit('request', xml, eid), which
is fragile; change the test to await the 'request' event instead of relying on
synchronous emission by wrapping the client.MyOperation call in a Promise that
resolves in the client's 'request' event handler (capturing requestXml there)
and then assert requestXml after that Promise resolves so the test remains
robust if emission becomes asynchronous.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d202997-ab47-43e1-9bce-26e8023c7ad7
📒 Files selected for processing (2)
src/client.tstest/security/WSSecurityCert.js
1 participant
Fixes #1487
When using
WSSecurityCertwithout additional SOAP headers, the generated security header could be placed outside the SOAPEnvelope, resulting in invalid XML structure.This change ensures the security header is always inserted inside the SOAP
Envelope, regardless of whether other SOAP headers are present.Includes a test covering the scenario where no additional SOAP headers exist.
Summary by CodeRabbit
Bug Fixes
Tests