Skip to content

Fix null export (#69) and stale export after clear-retrace (#106)#108

Merged
khlr merged 1 commit into
simplesamlphp:mainfrom
kellenmurphy:fix/null-export
May 31, 2026
Merged

Fix null export (#69) and stale export after clear-retrace (#106)#108
khlr merged 1 commit into
simplesamlphp:mainfrom
kellenmurphy:fix/null-export

Conversation

@kellenmurphy

Copy link
Copy Markdown
Contributor

This fixes two bugs that both cause the export to silently fail.

The first (related to #69) happens when a request is blocked, cancelled, or interrupted before a response arrives. getResponse() returns undefined, which gets passed to createFromJSON. Inside there, JSON.stringify(undefined) doesn't produce a string — it returns the JS value undefined — so JSON.parse throws a SyntaxError. The exception goes uncaught, ui.exportResult is never updated, and the downloaded file contains just null. The fix is a one-line null guard in enrichWithResponse in SAMLTraceIO.js.

The second (related to #106) happens when a request entry has isVisible set but parsed was never populated — this can occur when onHeadersReceived fires before onBeforeSendHeaders in a clear-and-retrace scenario. The export filter maps req.parsed across all entries, producing undefined in the array, which crashes createFromJSON and leaves ui.exportResult stale. The fix is .filter(Boolean) after the .map call in exportDialog.js.

Both fixes are covered by Jest unit tests and Playwright end-to-end tests. The Playwright suite lives on the playwright-testing branch at https://github.com/kellenmurphy/SAML-tracer/tree/playwright-testing — see E2E_TESTS.md there for context and instructions on running the tests. Each bug test was confirmed to fail on unfixed code and pass with the fix applied.

Adds Jest test suite and CI workflow. Guards enrichWithResponse against
null/undefined from getResponse() (simplesamlphp#69), and filters undefined parsed
entries from the export request list (simplesamlphp#106).
Comment thread .github/workflows/test.yml
Comment thread .github/workflows/test.yml
Comment thread .github/workflows/test.yml
@tvdijen tvdijen requested a review from khlr May 29, 2026 15:46
@tvdijen

tvdijen commented May 29, 2026

Copy link
Copy Markdown
Member

@khlr I think if you agree with the PR, we should merge and release a new version

kellenmurphy added a commit to kellenmurphy/SAML-tracer that referenced this pull request May 29, 2026
actions/checkout and actions/setup-node both updated to v6; node-version bumped from 20 to 24 per @tvdijen's inline review comments on PR simplesamlphp#108.
@kellenmurphy

Copy link
Copy Markdown
Contributor Author

Done, @tvdijen — bumped checkout, setup-node, and node-version per your suggestions. CI is green.

@khlr khlr 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.

Thank you very much for this excellent contribution, Kellen! The fixes are so 'simple' – it's just crazy. But it's great that you found them.

I'm a huge fan of automated testing. Hence I'm delighted that, as well as carrying out the actual tests, you’ve also put the necessary infrastructure in place.
I’d actually thought about writing tests for SAML-tracer once before. In the end, though, my lack of experience with JavaScript testing and GitHub Actions put me off. It’s great that everything needed is now in place so we can easily add more tests in future!

@khlr khlr merged commit 42f4647 into simplesamlphp:main May 31, 2026
1 check passed
@khlr

khlr commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

I just published release 1.9.3 to AMO and CWS. After the store-reviews, the new version will be available in the respective store. Thanks again, Kellen!

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.

3 participants